Fix GUI tests

[koppor]

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.prefs at more places. Currently, the issue is in MainTable.java:

        importHandler = new ImportHandler(
                frame.getDialogService(), database, externalFileTypes,
                Globals.prefs.getFilePreferences(),
                Globals.prefs.getImportFormatPreferences(),
                Globals.prefs.getUpdateFieldPreferences(),
                Globals.getFileUpdateMonitor(),
                undoManager,
                Globals.stateManager);

I would propose to convert the constructor of MainTable (and BasePanel) 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 off Globals.

Is #1579 the right issue, where we discussed Globals.prefs? I think, the comment at #1593 (comment) summarizes the discussion.

[tobiasdiez]

+1 for extracting Globals.prefs and mocking the relevant data. I don't think a builder pattern is necessary as the relevant classes are rarely initialized and all arguments need to be present anyway.

[koppor]

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> .

[tobiasdiez]

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.

[koppor]

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.