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

[WIP] (refactor) Use Java-version-independent adapter for internal API #429

Merged
merged 11 commits into from Sep 9, 2017

Conversation

JordanMartinez
Copy link
Contributor

@JordanMartinez JordanMartinez commented Sep 4, 2017

There's a number of issues with the current state of this PR only because I'm not yet familiar with how to set up some things with Java 9 or integrate it into this project's somewhat complex build system.

  • Tests don't work correctly due to not having access to things
  • Javadoc warns of bad javadoc (using & instead & and > instead of >)
  • CI files need to be updated

At least that was the issues it raised. I changed something in the build file and now it doesn't work due to

* What went wrong:
Execution failed for task ':testfx-internal-java9:javadoc'.
> Could not write to file 
    'TestFX/subprojects/testfx-internal-java9/build/tmp/javadoc/javadoc.options'.

@JordanMartinez
Copy link
Contributor Author

JordanMartinez commented Sep 4, 2017

@brcolow Here's your comparison to #425

@JordanMartinez JordanMartinez changed the title [WIP] (refactor) Use Java version independent adapter for internal API [WIP] (refactor) Use Java-version-independent adapter for internal API Sep 4, 2017
@brcolow
Copy link
Collaborator

brcolow commented Sep 5, 2017

Just trying to see if we can get the build to work, can you try changing the _JAVA_OPTIONS for .travis.yml for the JDK9 build to:

- _JAVA_OPTIONS="--add-opens=java.base/java.util=ALL-UNNAMED --add-exports=javafx.graphics/com.sun.glass.ui=ALL-UNNAMED"

Also the Window iterator order returned by the Java 9 interface implementation should be reversed. That should make the build pass, but we will see.

@JordanMartinez
Copy link
Contributor Author

can you try changing the _JAVA_OPTIONS for .travis.yml for the JDK9 build to:

It's already that. I rebased this on top of the latest commit you pushed that had the change.

Looks like this comment shows how to fix a related issue. I'm not sure if that will be the same for us or not. Thoughts?

Also the Window iterator order returned by the Java 9 interface implementation should be reversed.

Ah, didn't know that.

@JordanMartinez
Copy link
Contributor Author

This is what is written in the javadoc.options file

-Xdoclint:none '-quiet'
-author 
-charSet 'UTF-8'
-d '/home/jordan/Git Projects/TestFX/subprojects/testfx-internal-java9/build/docs/javadoc'
-doctitle 'TestFX Internal Java 9 4.0.7-alpha API'
-encoding 'UTF-8'
-footer 'Copyright © 2013-2016 The TestFX Contributors. All rights reserved.'

@JordanMartinez
Copy link
Contributor Author

JordanMartinez commented Sep 5, 2017

Turns out the javadoc task was failing because the given URL to Java 9 javadoc links weren't correct. Once these were updated, it would build.... until it got to the test classes ;-) So, I added read access and it works now. However, there are still the 2 flaky WindowFinderImplTest that prevented me from adding read access to JUnit 4/5. (This may be due to the windows list not being reversed yet)

@brcolow
Copy link
Collaborator

brcolow commented Sep 6, 2017

We must change the logging verbosity in ./ci/script.sh:

# Increase build log verbosity for JDK 9.
if [ "$TRAVIS_JDK_VERSION" == "oraclejdk9" ]; then
  ./gradlew build --debug --stacktrace
else

Just remove the conditional and have them both build with --info.

@brcolow
Copy link
Collaborator

brcolow commented Sep 6, 2017

Not really related to this PR but just noticed it because of you posting the contents of javadoc.options, might as well change the javadoc.footer in gradle.properties:

javadocFooter = Copyright © 2013-2016 The TestFX Contributors. All rights reserved.

To 2017. It would be nice to use a templating feature so that the current year is always used, but that's probably more trouble than it's worth.

@JordanMartinez
Copy link
Contributor Author

It built, but it's failing on 2 tests (same thing happened on my on local machine). If I reverse the iterator, via

List<Window> windows = Window.getWindows();
Collections.reverse(windows);
return windows.iterator();

then 19 tests fail. So I don't think the reverse part is the issue, but correct me if I'm wrong.

@JordanMartinez
Copy link
Contributor Author

First failure

org.testfx.service.finder.impl.WindowFinderImplTest > window_windowIndex FAILED
    java.lang.AssertionError: 
    Expected: is <javafx.stage.Stage@23359ada>
         but: was <javafx.stage.Stage@372bce1d>
        at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
        at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:8)
        at org.testfx.service.finder.impl.WindowFinderImplTest.window_windowIndex(WindowFinderImplTest.java:162)

Second failure

org.testfx.service.finder.impl.WindowFinderImplTest > targetWindow_windowIndex FAILED
    java.lang.AssertionError: 
    Expected: is <javafx.stage.Stage@23359ada>
         but: was <javafx.stage.Stage@372bce1d>
        at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
        at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:8)
        at org.testfx.service.finder.impl.WindowFinderImplTest.targetWindow_windowIndex(WindowFinderImplTest.java:137)

@JordanMartinez
Copy link
Contributor Author

JordanMartinez commented Sep 6, 2017

Actually, the reverse thing fails to build because reverse can't be called on an unmodifiable collection. However, once that's fixed, the above two tests still fail:

First failure on my machine

org.testfx.service.finder.impl.WindowFinderImplTest > window_windowIndex STANDARD_OUT
    Last target window is: null
    windows = [
        javafx.stage.Stage@16fdccba, 
        javafx.stage.Stage@50e43cf2, 
        javafx.stage.Stage@66ac14f0, 
        javafx.stage.Stage@daa41e4, 
        javafx.stage.Stage@380c429b, 
        javafx.stage.Stage@497eeac4
    ]

org.testfx.service.finder.impl.WindowFinderImplTest > window_windowIndex FAILED
    java.lang.AssertionError: 
    Expected: is <javafx.stage.Stage@66ac14f0>
         but: was <javafx.stage.Stage@50e43cf2>
        at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
        at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:8)
        at org.testfx.service.finder.impl.WindowFinderImplTest.window_windowIndex(WindowFinderImplTest.java:162)

Second failure on my machine

org.testfx.service.finder.impl.WindowFinderImplTest > targetWindow_windowIndex STANDARD_OUT
    Last target window is: null
    windows = [
        javafx.stage.Stage@16fdccba, 
        javafx.stage.Stage@50e43cf2, 
        javafx.stage.Stage@66ac14f0, 
        javafx.stage.Stage@daa41e4, 
        javafx.stage.Stage@380c429b, 
        javafx.stage.Stage@497eeac4
    ]

org.testfx.service.finder.impl.WindowFinderImplTest > targetWindow_windowIndex FAILED
    java.lang.AssertionError: 
    Expected: is <javafx.stage.Stage@66ac14f0>
         but: was <javafx.stage.Stage@50e43cf2>
        at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
        at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:8)
        at org.testfx.service.finder.impl.WindowFinderImplTest.targetWindow_windowIndex(WindowFinderImplTest.java:137)

@brcolow
Copy link
Collaborator

brcolow commented Sep 6, 2017

Sorry I was mis-remembering a bit. To get those tests to pass I changed:

     @SuppressWarnings("deprecation")
    private List<Window> fetchWindowsInQueue() {
        List<Window> windows = Lists.newArrayList(Window.impl_getWindows());
        return ImmutableList.copyOf(Lists.reverse(windows));
    }

To:

 private List<Window> fetchWindowsInQueue() {
        return ImmutableList.copyOf(Lists.newArrayList(Window.getWindows()));
    }

For the JDK 9 implementation. In other words I removed reversal.

@JordanMartinez
Copy link
Contributor Author

Still fails:

org.testfx.service.finder.impl.WindowFinderImplTest > window_windowIndex STANDARD_OUT
    Last target window is: null
    windows = [javafx.stage.Stage@347b170d, javafx.stage.Stage@6fe8dcec, javafx.stage.Stage@29e55e60, javafx.stage.Stage@3ff5778b, javafx.stage.Stage@42f630af, javafx.stage.Stage@6b508ba1]

org.testfx.service.finder.impl.WindowFinderImplTest > window_windowIndex FAILED
    java.lang.AssertionError: 
    Expected: is <javafx.stage.Stage@29e55e60>
         but: was <javafx.stage.Stage@6fe8dcec>
        at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
        at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:8)
        at org.testfx.service.finder.impl.WindowFinderImplTest.window_windowIndex(WindowFinderImplTest.java:162)

org.testfx.service.finder.impl.WindowFinderImplTest > targetWindow_window PASSED

org.testfx.service.finder.impl.WindowFinderImplTest > window_scene PASSED

org.testfx.service.finder.impl.WindowFinderImplTest > targetWindow_windowIndex STANDARD_OUT
    Last target window is: null
    windows = [javafx.stage.Stage@347b170d, javafx.stage.Stage@6fe8dcec, javafx.stage.Stage@29e55e60, javafx.stage.Stage@3ff5778b, javafx.stage.Stage@42f630af, javafx.stage.Stage@6b508ba1]

org.testfx.service.finder.impl.WindowFinderImplTest > targetWindow_windowIndex FAILED
    java.lang.AssertionError: 
    Expected: is <javafx.stage.Stage@29e55e60>
         but: was <javafx.stage.Stage@6fe8dcec>
        at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
        at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:8)
        at org.testfx.service.finder.impl.WindowFinderImplTest.targetWindow_windowIndex(WindowFinderImplTest.java:137)

@JordanMartinez
Copy link
Contributor Author

Also, since targetWindow is null in the following code:

private List<Window> orderWindowsByProximityTo(Window targetWindow,
                                                   List<Window> windows) {
        List<Window> copy = new ArrayList<>(windows);
        copy.sort(Comparator.comparingInt(w -> calculateWindowProximityTo(targetWindow, w)));
        return Collections.unmodifiableList(copy);
    }

    private int calculateWindowProximityTo(Window targetWindow,
                                           Window window) {
        if (window == targetWindow) {
            return 0;
        }
        if (isOwnerOf(window, targetWindow)) {
            return 1;
        }
        return 2;
    }

it will return 2, not 0 or 1. Is that supposed to happen? I thought comparator returns -1, 0, or 1, not 2.

@brcolow
Copy link
Collaborator

brcolow commented Sep 6, 2017

It is okay for Comparator to return any negative, zero, or positive value as long as it is an equivalence relation which would take some thought to investigate if our code is one or not. I will investigate the test failures in more detail tomorrow.

@@ -138,7 +140,7 @@ public Window window(Node node) {

@SuppressWarnings("deprecation")
private List<Window> fetchWindowsInQueue() {
List<Window> windows = Lists.newArrayList(Window.impl_getWindows());
List<Window> windows = Lists.newArrayList(getWindows());
return ImmutableList.copyOf(Lists.reverse(windows));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove the reversal here, and let the JDK 8 implementation return it in reverse, and the JDK 9 implementation not reversed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, looks like this is another method to add to JavaVersionAdapter.

Also, when I did remove the reversal and ran the tests solo, I believe they worked fine. When I run it in context of an entire gradle build, that's when it fails. So, it might be more than just that going on here.

@@ -250,7 +252,7 @@ private static boolean matchesNodeMatcher(Node node,

@SuppressWarnings("deprecation")
private static boolean isNodeVisible(Node node) {
if (!node.isVisible() || !node.impl_isTreeVisible()) {
if (isNotVisible(node)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I did the same thing in my PR, don't really know what the implications are of removing the isTreeVisible() condition but there is no way to check that in Java 9 so I don't think it should be relied upon for anything. Would maybe be a candidate for the JavaFX devs to move into the public API but that is outside our control.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd guess it only matters if one is using a Tree-related control, a component which I'm not sure we test for in TestFX or not. For all we know, they could have implemented whatever code that was into the Tree node itself... Otherwise, yes, outside of our control

@JordanMartinez
Copy link
Contributor Author

When I looked at the output, there were 6 windows stored in Window.getWindows(). There's only 5 in the test class, so I knew the extra window, which comes at index 1, was coming from another test. I added a change listener to Window.getWindows() and printed all windows whenever a new one was added.

Turns out BoundsQueryUtilsTest doesn't clean up its stage after its tests run. The build has progressed further, but it still fails due to check style running on the internal9 subproject. Error message below:

> Task :testfx-internal-java9:checkstyleMain
Putting task artifact state for task ':testfx-internal-java9:checkstyleMain' into context took 0.01 secs.
Executing task ':testfx-internal-java9:checkstyleMain' (up-to-date check took 0.005 secs) due to:
  No history is available.
[ant:checkstyle] Running Checkstyle 7.6.1 on 2 files

:testfx-internal-java9:checkstyleMain (Thread[Task worker for ':' Thread 3,5,main]) completed. Took 0.128 secs.

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':testfx-internal-java9:checkstyleMain'.
> Unable to process files: [
    TestFX/subprojects/testfx-internal-java9/src/main/java/
        module-info.java,
    TestFX/subprojects/testfx-internal-java9/src/main/java/
        org/testfx/service/adapter/JavaVersionAdapter.java
]

* Try:
Run with --stacktrace option to get the stack trace. Run with --debug option to get more log output.

* Get more help at https://help.gradle.org

@JordanMartinez
Copy link
Contributor Author

K, now it gets to JUnit 5 and fails with:

  JUnit Jupiter:ApplicationStartTest:should_click_on_button()
    MethodSource [className = 'org.testfx.cases.acceptance.ApplicationStartTest', methodName = 'should_click_on_button', methodParameterTypes = '']
    => java.lang.AssertionError: 
Expected: Node has text "clicked!"
     but: was <Button@5af7e405[styleClass=button]'click me!'>

Test run finished after 2808 ms
[         5 containers found      ]
[         1 containers skipped    ]
[         4 containers started    ]
[         0 containers aborted    ]
[         4 containers successful ]
[         0 containers failed     ]
[        10 tests found           ]
[         4 tests skipped         ]
[         6 tests started         ]
[         0 tests aborted         ]
[         5 tests successful      ]
[         1 tests failed          ]

:testfx-junit5:junitPlatformTest (Thread[Task worker for ':' Thread 5,5,main]) completed. Took 3.895 secs.

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':testfx-junit5:junitPlatformTest'.
> Process 'command '/usr/lib/jvm/java-9-oracle/bin/java'' finished with non-zero exit value 1

* Try:
Run with --stacktrace option to get the stack trace. Run with --debug option to get more log output.

* Get more help at https://help.gradle.org

When I ran it separately, it worked fine, so I think there's an issue somewhere else that causes the problem.

@JordanMartinez
Copy link
Contributor Author

When I ran the full build, it seemed like there is one test in the JUnit5 tests where the mouse is supposed to move towards a button and click on it, but that did not happen. Even the fired events below show that its clicking on the stack pane, not the button:

Failures (1):
  JUnit Jupiter:ApplicationStartTest:should_click_on_button()
    MethodSource [className = 'org.testfx.cases.acceptance.ApplicationStartTest', methodName = 'should_click_on_button', methodParameterTypes = '']
    => java.lang.AssertionError: 
Expected: Node has text "clicked!"
     but: was <Button@20be827a[styleClass=button]'click me!'>

   Context:
      Test image saved at:
         TestFX/subprojects/testfx-junit5/testfx-test - 0.png
      Keys pressed at test failure:
      Mouse Buttons pressed at test failure:
      Fired events since test began:
         WindowEvent [source = javafx.stage.Stage@5db6b9cd, target = javafx.stage.Stage@5db6b9cd, eventType = WINDOW_SHOWING, consumed = false]
         WindowEvent [source = javafx.stage.Stage@5db6b9cd, target = javafx.stage.Stage@5db6b9cd, eventType = WINDOW_SHOWN, consumed = false]
         MouseEvent [source = javafx.stage.Stage@5db6b9cd, target = javafx.scene.Scene@210ab13f, eventType = MOUSE_ENTERED_TARGET, consumed = false, x = 49.0, y = 27.0, z = 0.0, button = NONE, pickResult = PickResult [node = StackPane@58f672b1[styleClass=root], point = Point3D [x = 49.0, y = 27.0, z = 0.0], distance = 186.60254037844388]
         MouseEvent [source = javafx.stage.Stage@5db6b9cd, target = StackPane@58f672b1[styleClass=root], eventType = MOUSE_ENTERED_TARGET, consumed = false, x = 49.0, y = 27.0, z = 0.0, button = NONE, pickResult = PickResult [node = StackPane@58f672b1[styleClass=root], point = Point3D [x = 49.0, y = 27.0, z = 0.0], distance = 186.60254037844388]
         MouseEvent [source = javafx.stage.Stage@5db6b9cd, target = StackPane@58f672b1[styleClass=root], eventType = MOUSE_MOVED, consumed = false, x = 49.0, y = 27.0, z = 0.0, button = NONE, pickResult = PickResult [node = StackPane@58f672b1[styleClass=root], point = Point3D [x = 49.0, y = 27.0, z = 0.0], distance = 186.60254037844388]
         MouseEvent [source = javafx.stage.Stage@5db6b9cd, target = StackPane@58f672b1[styleClass=root], eventType = MOUSE_MOVED, consumed = false, x = 49.0, y = 27.0, z = 0.0, button = NONE, pickResult = PickResult [node = StackPane@58f672b1[styleClass=root], point = Point3D [x = 49.0, y = 27.0, z = 0.0], distance = 186.60254037844388]
         MouseEvent [source = javafx.stage.Stage@5db6b9cd, target = StackPane@58f672b1[styleClass=root], eventType = MOUSE_MOVED, consumed = false, x = 49.0, y = 27.0, z = 0.0, button = NONE, pickResult = PickResult [node = StackPane@58f672b1[styleClass=root], point = Point3D [x = 49.0, y = 27.0, z = 0.0], distance = 186.60254037844388]
         MouseEvent [source = javafx.stage.Stage@5db6b9cd, target = StackPane@58f672b1[styleClass=root], eventType = MOUSE_PRESSED, consumed = false, x = 49.0, y = 27.0, z = 0.0, button = PRIMARY, primaryButtonDown, pickResult = PickResult [node = StackPane@58f672b1[styleClass=root], point = Point3D [x = 49.0, y = 27.0, z = 0.0], distance = 186.60254037844388]
         MouseEvent [source = javafx.stage.Stage@5db6b9cd, target = StackPane@58f672b1[styleClass=root], eventType = MOUSE_RELEASED, consumed = false, x = 49.0, y = 27.0, z = 0.0, button = PRIMARY, pickResult = PickResult [node = StackPane@58f672b1[styleClass=root], point = Point3D [x = 49.0, y = 27.0, z = 0.0], distance = 186.60254037844388]
         MouseEvent [source = javafx.stage.Stage@5db6b9cd, target = StackPane@58f672b1[styleClass=root], eventType = MOUSE_CLICKED, consumed = false, x = 49.0, y = 27.0, z = 0.0, button = PRIMARY, pickResult = PickResult [node = StackPane@58f672b1[styleClass=root], point = Point3D [x = 49.0, y = 27.0, z = 0.0], distance = 186.60254037844388]

Test run finished after 3060 ms
[         5 containers found      ]
[         1 containers skipped    ]
[         4 containers started    ]
[         0 containers aborted    ]
[         4 containers successful ]
[         0 containers failed     ]
[        10 tests found           ]
[         4 tests skipped         ]
[         6 tests started         ]
[         0 tests aborted         ]
[         5 tests successful      ]
[         1 tests failed          ]


:testfx-junit5:junitPlatformTest (Thread[Task worker for ':' Thread 2,5,main]) completed. Took 3.897 secs.

@JordanMartinez
Copy link
Contributor Author

JordanMartinez commented Sep 8, 2017

Everything passed except the flaky test on one of the Windows' builds. What else needs to be done to clean this up? How should I exclude module-info.java from the check style task?

@JordanMartinez
Copy link
Contributor Author

I think this should be merged once the non-flaky tests pass. The findbugs task doesn't appear to work with Java 9 yet (maybe it requires a different version?). Since it needs further investigation and since the build files should be cleaned up later, it's outside the scope of this PR.

@brcolow brcolow merged commit 2fb6690 into TestFX:master Sep 9, 2017
@brcolow
Copy link
Collaborator

brcolow commented Sep 9, 2017

Great work @JordanMartinez - thanks so much.

@JordanMartinez
Copy link
Contributor Author

Woot!

@JordanMartinez
Copy link
Contributor Author

Just FYI. Someone extended the experimental Java 9 plugin for Gradle and started work on what they call Chainsaw.

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.

None yet

2 participants