Fix GUI tests#5781
Merged
Merged
Conversation
76b159e to
674cb6e
Compare
Member
|
+1 for extracting |
tobiasdiez
reviewed
Dec 26, 2019
Member
Author
|
I'm not sure whether we should continue with it like that. Should we try
other solutions such as Eclipse Jubula - which are independent from the
concrete code. I see that separate preferences for model and logic makes
sense. For GUI, it is *much* effort... Is it really worth the effort? Do we
need these kinds of tests? Would it make more sense to do black box testing?
Maybe, we should let this task (enable testing by TestFX) be done by
students in a training next year? Seems to be much more work than expected.
I wanted to come up with a quick solution to get it running to have our
tests green.
Tobias Diez <notifications@github.com> schrieb am Do., 26. Dez. 2019, 09:45:
… ***@***.**** commented on this pull request.
------------------------------
In src/main/java/org/jabref/gui/BasePanel.java
<#5781 (comment)>:
> @@ -145,8 +154,8 @@
private Optional<DatabaseChangeMonitor> changeMonitor = Optional.empty();
private JabRefExecutorService executorService;
- public BasePanel(JabRefFrame frame, BasePanelPreferences preferences, BibDatabaseContext bibDatabaseContext, ExternalFileTypes externalFileTypes) {
- this.preferences = Objects.requireNonNull(preferences);
+ public BasePanel(JabRefFrame frame, BasePanelPreferences basePanelPreferences, BibDatabaseContext bibDatabaseContext, ExternalFileTypes externalFileTypes, GroupViewMode groupViewMode, FilePreferences filePreferences, ImportFormatPreferences importFormatPreferences, UpdateFieldPreferences updateFieldPreferences, FileUpdateMonitor fileUpdateMonitor, StateManager stateManager, Consumer<ColumnPreferences> updateColumnPreferences, Supplier<TimestampPreferences> timestampPreferencesSupplier) {
I think it's better to pass a PreferenceService if you need to store
preference options (or get the current preferences at a later moment).
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#5781?email_source=notifications&email_token=AAKNU7TWQXUO7ZOYXCOOV3LQ2RVLJA5CNFSM4J6N67D2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCQG7HUI#pullrequestreview-336458705>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAKNU7WSB7IVC7GUV3ZZO5LQ2RVLJANCNFSM4J6N67DQ>
.
|
Member
|
I think GUI tests would be nice to have. It's a good idea to let students extend the code coverage in a forthcoming project (similarly to what we did with the fetcher test suite). However, we should provide a few basic tests that serve as an orientation. With MVVM there shouldn't be any need for UI tests in the traditional sense (TestFX or Jubala or whatever) as one can simply write unit tests for the view model classes. Moreover, the current GUI tests don't add much value in my opinion. For me it would be fine to remove all GUI tests that we have currently. |
Member
Author
|
The whole "mess" is only for the BasePanelTest - to fix the other tests was easy. I removed all the "improvements" and made only cosmetic changes. |
tobiasdiez
approved these changes
Jan 2, 2020
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #5602, refs #2768.
Wanted to know why our GUI tests are broken. The reason seems to be that we don't pass the preferences objects directly around, but use
Globals.prefsat more places. Currently, the issue is inMainTable.java:I would propose to convert the constructor of
MainTable(andBasePanel) to a builder pattern (because we have more than three arguments) and add passing of these preferences.Alternatively, we can mock
Globals.prefs.GetFilePreferences()etc. This is also OK for me as I accept global objects. However, we had a long discussion that we should get rid offGlobals.Is #1579 the right issue, where we discussed
Globals.prefs? I think, the comment at #1593 (comment) summarizes the discussion.