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.

Sign up for GitHub

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

Jump to bottom

Add simple gui test #3399

Merged
merged 6 commits into from
Jan 1, 2018
Merged

Add simple gui test #3399

merged 6 commits into from
Jan 1, 2018

Conversation

tobiasdiez
Copy link
Member

This PR adds a simple GUI test for the source editor, which I used to track down the index of out bounds exception. It uses TestFX, which seems to be a good framework for javafx gui tests. For example, it is possible to run the unit tests in a headless method - thus allowing it to run on travis. For me, the biggest advantage is that you can isolate different parts of the system (i.e. only add the source tab) and ignore the rest of the application. Thus you can write "unit tests for ui".

Disclaimer: I will not invest any more work in these gui tests. But since some of you were quite in favor of having a gui test suite, I thought I share with you what I have so far.

  • Change in CHANGELOG.md described
  • Tests created for changes
  • Screenshots added (for bigger UI changes)
  • Manually tested changed features in running JabRef
  • Check documentation status (Issue created for outdated help page at help.jabref.org?)
  • If you changed the localization: Did you run gradle localizationUpdate?

@tobiasdiez tobiasdiez requested a review from koppor November 4, 2017 03:53
Siedlerchr
Siedlerchr approved these changes Nov 4, 2017
View reviewed changes
Copy link
Member

@Siedlerchr Siedlerchr left a comment

Choose a reason for hiding this comment

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

Looks good to me! Interesting way. What if I execute it locally, will it be visble or headless as well?
Please mark it with GUI Test category

@Siedlerchr
Copy link
Member

And please have a look at the errors, you need a BasePanel

@koppor
Copy link
Member

koppor commented Nov 4, 2017

I think, it does not need any XServer running etc? Thus, I lean towards not including it in our existing GUI tests, but eitehr keep it in the main testing block or create a new category javafxguitests.

koppor
koppor approved these changes Nov 4, 2017
View reviewed changes
Copy link
Member

@koppor koppor left a comment

Choose a reason for hiding this comment

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

LGTM. Also using + for the minr versions is OK for me. With the hope, that the testfx-guys really use semantic versioning ^^.

LinusDietz
LinusDietz requested changes Dec 22, 2017
View reviewed changes
src/test/java/org/jabref/gui/entryeditor/EntryEditorTest.java Outdated
public void start(Stage stage) throws Exception {
area = new CodeArea();
area.appendText("some example\n text to go here\n across a couple of \n lines....");
sourceTab = new SourceTab(new BibDatabaseContext(), new LatexFieldFormatterPreferences());
Copy link
Member

Choose a reason for hiding this comment

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

The Argument CountingUndoManager is missing in the signature of this test. I have tried to add it by simply adding new CountingUndoManager(), but then the test fails. Can you have a look at it?

@LinusDietz LinusDietz changed the title Add simple gui test [WIP] Add simple gui test Dec 22, 2017
@tobiasdiez tobiasdiez changed the title [WIP] Add simple gui test Add simple gui test Dec 23, 2017
@tobiasdiez tobiasdiez added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Dec 23, 2017
@tobiasdiez
Copy link
Member Author

It now compiles. However, I need to use the preferences in the test and thus had to add an exception to the architecture tests. Since this is often a critical area of discussion, I would like to have another reviewers' ok before merging. Thanks.

Siedlerchr
Siedlerchr reviewed Dec 23, 2017
View reviewed changes
src/main/java/org/jabref/gui/entryeditor/SourceTab.java
@@ -71,7 +73,7 @@ private static String getSourceString(BibEntry entry, BibDatabaseMode type, Late
private CodeArea createSourceEditor() {
CodeArea codeArea = new CodeArea();
codeArea.setWrapText(true);
codeArea.lookup(".styled-text-area").setStyle("-fx-font-size: " + Globals.prefs.getFontSizeFX() + "pt;");
codeArea.lookup(".styled-text-area").setStyle("-fx-font-size: " + preferences.getFontSizeFX() + "pt;");
Copy link
Member

Choose a reason for hiding this comment

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

Why not pass the font size directly as int parameter? Would also work for test

Copy link
Member Author

Choose a reason for hiding this comment

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

That would work but there are 2 other accesses to the preferences. Thus 3 new constructor arguments just to get rid of the preference class seems a bit too much for me.

@tobiasdiez tobiasdiez merged commit add96db into master Jan 1, 2018
@tobiasdiez tobiasdiez deleted the guiTests branch January 1, 2018 14:18
@tobiasdiez
Copy link
Member Author

As before, I need this PR to continue working on the javafx migration of the maintable. Thus I merge this now but, of course, implement further feedback on how to improve the code. (For some reason the build on travis does not started for the merge commit...but it was green before..so hopefully the build still passes).

Siedlerchr added a commit that referenced this pull request Jan 2, 2018
@Siedlerchr
Merge remote-tracking branch 'upstream/master' into selectFilesDlg
d4aed9b 
* upstream/master:
  Remove dependency to jgoodies-looks (#3458)
  Add simple gui test (#3399)
  Library properties: no change but.. (issue #3562) (#3579)
  Initialize previe before MainPanel to prevent NPEs on delete Fixes #3584
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Siedlerchr Siedlerchr Siedlerchr approved these changes

@LinusDietz LinusDietz LinusDietz requested changes

@koppor koppor koppor approved these changes

Assignees
No one assigned
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tobiasdiez @Siedlerchr @koppor @LinusDietz