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

Split dock unit test #1996

Merged

Conversation

lorenzo-gomez-windhover
Copy link
Contributor

Hi there,

Hope you are all doing well.

These unit tests use TestFX's Robot API to test the behavior of DockPane and SplitDock classes. Specifically these unit tests test the behavior discussed in #1986 and #1991.

I know GUI testing can be tricky because it might very well behave differently on different platforms, but I tried my best to write these tests in a platform-independent way. By all means try it in other platforms as I ran the test only on Ubuntu 18.04.5 LTS. It is possible I missed something, but hopefully these tests are reliable enough that we can trust them when it comes to GUI testing.

Thanks
Lorenzo

@kasemir
Copy link
Collaborator

kasemir commented Aug 27, 2021

@shroffk Is there a special naming convention for tests that we should use so that GUI tests can be distinguished from "plain" tests? I'm afraid we'll sooner or later need a way to skip running GUI tests.

@georgweiss
Copy link
Collaborator

Running mvn clean install on Mac will hang the build at SplitDockTest.
Running the test from IntelliJ succeeds, but there are exceptions:

WARNING: Cannot set application icon java.lang.NullPointerException at java.base/java.io.File.<init>(File.java:276) at org.phoebus.framework.workbench.Locations.install(Locations.java:83) at org.phoebus.ui.docking.DockStage.getLogo(DockStage.java:79) at org.phoebus.ui.docking.DockStage.configureStage(DockStage.java:146) at org.phoebus.ui.docking.SplitDockTest.start(SplitDockTest.java:44) at org.testfx.framework.junit.ApplicationAdapter.start(ApplicationAdapter.java:40) at org.testfx.toolkit.impl.ApplicationServiceImpl.lambda$start$0(ApplicationServiceImpl.java:51) at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264) at com.sun.javafx.application.PlatformImpl.lambda$runLater$10(PlatformImpl.java:447) at java.base/java.security.AccessController.doPrivileged(Native Method) at com.sun.javafx.application.PlatformImpl.lambda$runLater$11(PlatformImpl.java:446) at com.sun.glass.ui.InvokeLaterDispatcher$Future.run(InvokeLaterDispatcher.java:96)

@kasemir
Copy link
Collaborator

kasemir commented Sep 4, 2021

Again I think we need a naming convention or related mechanism to distinguish UI tests.
In #314, Kunal suggested ...UI for "interface tests using testFX".
Maybe rename SplitDockTest into SplitDockUI so it's not automatically invoked by the headless build which has problems executing UI tests?

@lorenzo-gomez-windhover
Copy link
Contributor Author

Maybe rename SplitDockTest into SplitDockUI so it's not automatically invoked by the headless build which has problems executing UI tests?

@kasemir So does the CI ignore any tests that end in *UI? If that is the case, I can always rename the test.

@kasemir
Copy link
Collaborator

kasemir commented Sep 7, 2021

Better ask anyone but me about maven...
In #314, Kunal suggested ..UI for UI tests, and he also mentioned using surefire for running tests.
As I read the surefire doc, https://maven.apache.org/surefire/maven-surefire-plugin/examples/inclusion-exclusion.html, these are automatically executed:

By default, the Surefire Plugin will automatically include all test classes with the following wildcard patterns:
"**/Test*.java" - includes all of its subdirectories and all Java filenames that start with "Test".
"**/*Test.java" - includes all of its subdirectories and all Java filenames that end with "Test".
"**/*Tests.java" - includes all of its subdirectories and all Java filenames that end with "Tests".
"**/*TestCase.java" - includes all of its subdirectories and all Java filenames that end with "TestCase".

I assume that was part of the reason for suggesting ..UI for UI tests, so by default they would be skipped.

@lorenzo-gomez-windhover
Copy link
Contributor Author

I renamed my tests to DockPaneTestUI and SplitDockTestUI and they are skipped.

@shroffk
Copy link
Member

shroffk commented Sep 8, 2021

@kasemir thank you for digging through the issues and finding that.
I think we should move this to read the docs... I am thinking a section under the "for developers".
I know in the past we have had divergence in opinions about code styles, but despite that we have a lot of agreement... like class names, test names, organization of modules, design patterns etc... which we should start documenting and leave out the things we don't yet have consensus on.

@shroffk
Copy link
Member

shroffk commented Sep 8, 2021

@lorenzo-gomez-windhover we tend to have monthly meeting to share new developments or issues, if you are interested in attending let me know.

@lorenzo-gomez-windhover
Copy link
Contributor Author

@shroffk Yes, I I'd love to! How can I join the meeting?

@shroffk
Copy link
Member

shroffk commented Sep 13, 2021

Can this PR be merged.

@lorenzo-gomez-windhover we meet using google hangouts, I will add you to the next meeting :)

@georgweiss
Copy link
Collaborator

I do not mind as long as Maven builds are fine.

@georgweiss georgweiss merged commit 31a8713 into ControlSystemStudio:master Sep 15, 2021
@shroffk
Copy link
Member

shroffk commented Oct 5, 2021

@lorenzo-gomez-windhover I do not have an email address to forward the monthly meeting invitation.
You can email me at [email protected] and I will add you to the meeting list.

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