Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test UI popup window with TestFX (ReplaceStringView.java) #7654

Closed
wants to merge 1 commit into from
Closed

Test UI popup window with TestFX (ReplaceStringView.java) #7654

wants to merge 1 commit into from

Conversation

ningxie1991
Copy link
Contributor

@ningxie1991 ningxie1991 commented Apr 20, 2021

This pull requests relates to #6089, which is testing the UI using TestFX as suggested by @koppor. It also contributes to #6207 for adding more tests to the project. Test added:

ReplaceStringViewTest.java

This test uses TestFX to mock the ReplaceStringView and its fxml, brings up the dialogPane and executes a series of actions using FxRobot on the UI to verify the replace string action. However, it requires the Globals.prefs and Globals.fileUpdateMonitor variables to not be null, which implies a refactoring on the BaseDialog.java class and all its dependencies. Right now I initialized the Globals.prefs in the test to prove the concept that TestFX works.

Video of the test:
https://drive.google.com/file/d/1YTdLYF1SKNyjSvR9GRcz4Lv4S_yYzXcO/view?usp=sharing

Screenshot of the test:
image

  • Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
  • Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (for UI changes)
  • Checked documentation: Is the information available and up to date? If not created an issue at https://github.com/JabRef/user-documentation/issues or, even better, submitted a pull request to the documentation repository.

This is a TestFX test for the GUI popup window "Find and Replace"
@Override
public void start (Stage stage) throws Exception {
// Globals.prefs = JabRefPreferences.getInstance();
// Globals.startBackgroundTasks();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are the lines of code that helped my test to run, because it needs Globals.prefs and Globals.fileUpdateMonitor to not be null.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just an idea, can you try if Globals.prefs = mock(JabRefPreferences.class) works?
for Globals.fileUpdateMonitor you can try to use the DummyFileUpdateMonitor

Copy link
Contributor Author

@ningxie1991 ningxie1991 Apr 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Siedlerchr Yes it would work if we mock it up but I think the point is that we should not assign new value to Globals.prefs like this "Globals.prefs = ...." in the test. Because it's not like creating an instance of the object Globals and mock its prefs, for example, Globals globals = mock(Globals.class); globals.prefs = mock(JabRefPreferences.class);, but now we are directly setting the value of this global variable which is not ideal I think.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JabRef/developers anyone an idea how to either extract the globals or how to test it?

@tobiasdiez
Copy link
Member

In my opinion, it should suffices to test the view model. Then you don't need to worry about the whole gui setup, and it's more flexible. Using testfx should only be necessary in very rare instances (for example, to test complex actions that happen on focus or focus loss).

@ningxie1991 ningxie1991 marked this pull request as draft May 4, 2021 08:32
@koppor
Copy link
Member

koppor commented May 17, 2021

This PR showed that UI testing is really hard. - We should try to work on tests on components where code will be refactored (and the functionality should be kept) - or the functionality will be changed without breaking other desired functionality.

Since the JabRef team did not provice guidance for concrete components which sould be rewritten, finding an "awesome" component to write tests for is hard.

Maybe, the whole work on UI testing should be suspended and non-UI features should be worked on.

@koppor koppor closed this May 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants