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

Gui testing #507

Merged
merged 13 commits into from
Mar 15, 2016
Merged

Gui testing #507

merged 13 commits into from
Mar 15, 2016

Conversation

simonharrer
Copy link
Contributor

Adds the ability to do GUI tests automatically.

With just 80 lines of testing code, I was able to raise the code coverage by 15%. And I detected two NullPointerExceptions (still to be fixed) plus three places where Swing classes were interacted with outside of the EDT.

This change allows to model bug reports as test cases on a GUI level.

What has been done

  • Tests fail when an uncaught Exception occurs on the EDT
  • Raise test coverageby 15%
  • GUI tests run on CircleCI
  • Fix language to english for all (GUI) tests
  • somehow only run the tests on CircleCI?
  • maybe use headless mode @matthiasgeiger - NOT POSSIBLE WITH THE USED API
  • make it work with Gradle sync in Intellij so that one can edit the tests and run them from within the IDE
  • make tests work on travis-ci
  • enable tests on circle-ci as well

@lenhard
Copy link
Member

lenhard commented Dec 14, 2015

Wow, if that works, then it's terrific!

@simonharrer
Copy link
Contributor Author

It does work already. :) The failing build is a sign that the remaining two null pointer exceptions need to be fixed.

@stefan-kolb
Copy link
Member

Nice work 👍

@matthiasgeiger
Copy link
Member

As already said today: 👍

Two things:

  • running those GUI checks (naturally) opens the GUI - so running "all tests" before a commit gets quite unnerving - I'm not sure how to deal with this... running the tests only at circle ci?
  • the current tests seem to be dependent on the chosen language and therefore fail if another language is set:
    org.assertj.swing.exception.ComponentLookupException: Unable to find component using matcher org.assertj.swing.driver.JMenuItemMatcher[label='File|New database']. - a solution might be to set the prefs for the tests accordingly

@stefan-kolb
Copy link
Member

Aren't there any test frameworks that run in headless mode?

@simonharrer
Copy link
Contributor Author

I have not found any library that is still maintained and support this.

@matthiasgeiger
Copy link
Member

There are some indications in the documentation that this might be possible with assertJ Swing:
See http://joel-costigliola.github.io/assertj/assertj-swing-lookup.html -> "Scope of component lookups" and
http://joel-costigliola.github.io/assertj/swing/api/org/assertj/swing/core/ComponentLookupScope.html

@simonharrer
Copy link
Contributor Author

@matthiasgeiger hm, can't find any of these indications.

@matthiasgeiger
Copy link
Member

http://joel-costigliola.github.io/assertj/swing/api/org/assertj/swing/core/ComponentLookupScope.html#ALL - if a UI component can be found without being shown it can be tested without being shown?

@simonharrer
Copy link
Contributor Author

No. If you test without the robot, then you have a headless mode. But I think you cannot test meaningful unit tests without the robot.

@koppor
Copy link
Member

koppor commented Dec 16, 2015

For the record: Screenscraping UI test framework: http://www.sikulix.com/

@tobiasdiez
Copy link
Member

I'm not sure if I like these kind of tests or not.
On the one hand it is of course good to increase the test coverage. Hence, the more tests the better. However, these 15% increase actually don't say anything since the tests are quite weak (there are no assertion statements??). For example, the folder "SpecialFields" went from 2% to 50%. Seeing these numbers, I would actually expect that the logic in these classes is relatively well-tested. But this is not the case. So the increase in code coverage actually says nothing and can be quite misleading.

Additionally, these kind of tests are rather fragile since they fail in case of trivial changes (like renaming the corresponding button).

Personally, I would wait with GUI tests until after the migration to JavaFX and until the GUI is relatively stable. For the moment, there are enough untested logic classes which should be covered first.

@simonharrer
Copy link
Contributor Author

The thing is: with these tests we can easily spot some major issues which are hard to detect without the tests, e.g., that Swing classes are accessed outside the EDT thread or that exceptions occur upon clicking some buttons. And with only the 80 lines of code, five issues have been unearthed. Hence, the effort/benefit ratio is actually quite good.

@koppor
Copy link
Member

koppor commented Jan 29, 2016

I really want to get this through. We could add a gradle build switch disabling the GUI testing. I want to have this in ensure that (when possible) the answer of a FAQ is backed by a test case. This will improve the documentation very much.

@simonharrer simonharrer force-pushed the gui-testing branch 2 times, most recently from 2cef1fe to 9d3c842 Compare January 31, 2016 21:47
@oscargus
Copy link
Contributor

oscargus commented Feb 2, 2016

Looks good to go! The Mime-test seems to be failing every now and then...

Integration tests are now separated from the normal unit tests and have to be called manually.
@simonharrer
Copy link
Contributor Author

The thing is, locally, they work. On the CI server, it somehow fails cause of timeouts as some windows may open not quick enough. I tried to increase the timeouts I could increase, but it did not solve the problem.

I am leaning towards closing this PR, and just live with the fact that we cannot test our GUI. What is the opinion of the other @JabRef/developers ?

@simonharrer simonharrer added status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers and removed status: devcall labels Feb 24, 2016
@lenhard
Copy link
Member

lenhard commented Feb 26, 2016

So, the tests are now working on travis and on circleci. The original problems were coding errors in one of the tests (which is still not working properly, but is ignored now).

As for getting the tests to work within IntelliJ, I am really at a loss. Why does the IDE receive no test events when executing the test class? Any hints @simonharrer

Once we get that working there are still a few hurdles to jump. We need to find a proper syntax to test newly opened dialog windows. After that, there are practically hundreds of test we could write.

@koppor
Copy link
Member

koppor commented Mar 11, 2016

Is the "proper syntax to test newly opened dialog windows" a show stopper? Besides that, is IntelliJ support the only other show stopper?

No issues (at first sight) with Eclipse.

grabbed_20160311-192545

@simonharrer simonharrer changed the title [WIP] Gui testing Gui testing Mar 15, 2016
@simonharrer
Copy link
Contributor Author

We can test a lot with this at the moment already. I vote to merge this in and work on that when bugs arrive.

@tobiasdiez
Copy link
Member

👍 for merge if these GUI tests are not taken into account for the general test coverage metric.

@matthiasgeiger
Copy link
Member

It would be nice, if they would increase the calculated coverage ;-)

This does not yet work, however I'll merge this in!

matthiasgeiger added a commit that referenced this pull request Mar 15, 2016
@matthiasgeiger matthiasgeiger merged commit cda74be into master Mar 15, 2016
@matthiasgeiger matthiasgeiger deleted the gui-testing branch March 15, 2016 13:47
@Braunch
Copy link
Contributor

Braunch commented May 4, 2016

In my opinion ui testing is a pretty good thing. On the other hand @tobiasdiez also has a point. When we are changing from swing to javafx, all the effort will be for nothing. So I thought about two options that we have, if we want to do ui testing anyway (and I do).

The first would be to use sikulix and screenscraping to do the tests independent from the ui-framework.
The thing is, when doing the conversion to JavaFX the layout might also change, but maybe using new screenshots is easier and faster then new tests.

The second solution is to try writing test interfaces that represent the use-case-flow without doing anything to the ui and in parallel write assertJ tests that use the interfaces. When converting the ui to javafx we could then still use the interfaces and only need to change the ui controlling framework.

When we finished writing the assertJ test for #988 , @bruehldev and I will have a look at sikulix and determine how much effort it is to write tests with it, so we can estimate what solution is best suited for the conversion. Any opinions on my suggestions?

@Siedlerchr
Copy link
Member

Siedlerchr commented May 5, 2016

@Braunch Completely agree with you. We should not focus on creating so much gui tests.
Regarding JavaFX tests, there is this (older) blog posts about different GUI Testing Frameworks for JavaFX: http://qaware.blogspot.de/2015/03/gui-tests-for-javafx.html

@tobiasdiez
Copy link
Member

I also wouldn't worry too much about GUI tests. For JavaFX the databinding mechanism makes it relatively easy to test the ui without actually running the application (this more or less corresponds to the test interfaces you are speaking about).

@Braunch Braunch mentioned this pull request May 7, 2016
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants