JDK-8207369 Add more robot tests. #215

brcolow · 2018-09-24T04:57:28Z

(Partially? at least a step in the right direction...) resolves robot API follow-up issue JDK-8207369.

kevinrushforth

This looks like the right set of tests. I get several failures, though (I only tested Linux).

test.robot.javafx.scene.RobotTest > testKeyRelease FAILED
    org.junit.ComparisonFailure: letter 'a' should be RELEASED by Robot expected:<[a]> but was:<[]>
        at org.junit.Assert.assertEquals(Assert.java:123)
        at test.robot.javafx.scene.RobotTest.testKeyboard(RobotTest.java:190)
        at test.robot.javafx.scene.RobotTest.testKeyRelease(RobotTest.java:136)

test.robot.javafx.scene.RobotTest > testMouseReleasePrimary FAILED
    java.lang.AssertionError: Timeout while waiting for button.onMouseRELEASED().
        at org.junit.Assert.fail(Assert.java:91)
        at test.robot.javafx.scene.RobotTest.waitForLatch(RobotTest.java:728)
        at test.robot.javafx.scene.RobotTest.testMouseAction(RobotTest.java:417)
        at test.robot.javafx.scene.RobotTest.testMouseReleasePrimary(RobotTest.java:311)

test.robot.javafx.scene.RobotTest > testMouseReleaseSecondary FAILED
    java.lang.AssertionError: Timeout while waiting for button.onMouseRELEASED().
        at org.junit.Assert.fail(Assert.java:91)
        at test.robot.javafx.scene.RobotTest.waitForLatch(RobotTest.java:728)
        at test.robot.javafx.scene.RobotTest.testMouseAction(RobotTest.java:417)
        at test.robot.javafx.scene.RobotTest.testMouseReleaseSecondary(RobotTest.java:316)

test.robot.javafx.scene.RobotTest > testMouseRelaseMiddle FAILED
    java.lang.AssertionError: Timeout while waiting for button.onMouseRELEASED().
        at org.junit.Assert.fail(Assert.java:91)
        at test.robot.javafx.scene.RobotTest.waitForLatch(RobotTest.java:728)
        at test.robot.javafx.scene.RobotTest.testMouseAction(RobotTest.java:417)
        at test.robot.javafx.scene.RobotTest.testMouseRelaseMiddle(RobotTest.java:321)

test.robot.javafx.scene.RobotTest > testMouseWheelPositiveAmount FAILED
    org.junit.ComparisonFailure: mouse wheel should be scrolled 5 vertical units by Robot expected:<Scrolled [5]> but was:<Scrolled [-40.0]>
        at org.junit.Assert.assertEquals(Assert.java:123)
        at test.robot.javafx.scene.RobotTest.testMouseWheel(RobotTest.java:525)
        at test.robot.javafx.scene.RobotTest.testMouseWheelPositiveAmount(RobotTest.java:490)

test.robot.javafx.scene.RobotTest > testMouseWheelNegativeAmount FAILED
    org.junit.ComparisonFailure: mouse wheel should be scrolled -5 vertical units by Robot expected:<Scrolled [-5]> but was:<Scrolled [40.0]>
        at org.junit.Assert.assertEquals(Assert.java:123)
        at test.robot.javafx.scene.RobotTest.testMouseWheel(RobotTest.java:525)
        at test.robot.javafx.scene.RobotTest.testMouseWheelNegativeAmount(RobotTest.java:495)

tests/system/src/test/java/test/robot/javafx/scene/RobotTest.java

brcolow · 2018-11-26T08:44:25Z

I think the failures (besides the mouse scroll amount which seems to be hardcoded to a value of (-)40 on Linux) are happening because you can't release "in a vacuum" without a preceding press. I will remove the keyboard and mouse tests that only release, but will keep the press only and press and release tests.

brcolow · 2018-11-26T23:50:33Z

The mouse scroll wheel failure is kind of interesting. Basically the GtkRobot sends multiple fake mouse buttons presses (4 for scroll up, 5 for scroll down), one for each int of the wheelAmt. Therefore I think what I can do is add a sort of timed collection to accumulate all scroll events sent to the Button and then add them up. I may still need to mess with the multiplier though on a system-dependent basis which is going to be ugly. The reason for this is that the JavaFX event handler will get 5 scroll events each with a scroll amount of (+/-) 40 for a total of 200. Then one would need to divide by the multiplier to match the passed-in wheelAmt.

Edit: Actually I may be able to use event.getTotalDeltaY...investigating that now.

brcolow · 2018-11-27T07:00:22Z

For some reason event.getTotalDeltaY() reports 0. But I was able to hack something together that passes on Windows and Linux. Still need to test macOS.

brcolow · 2018-11-28T04:12:23Z

I finally was able to test and fix the new tests on macOS. I have now confirmed that the new tests pass locally for me on all 3 operating systems. I am not really happy with the approach I took in order to get a unified scroll amount between platforms but wasn't sure of an alternative approach.

brcolow · 2018-11-30T06:54:18Z

The new drag tests pass locally for me on Windows but seem to fail in a Linux virtual machine but I can't tell if that's because it's really slow or because there is an actual problem. Do you have a local Linux install you could test them on?

kevinrushforth · 2018-11-30T13:18:16Z

I can test them later this morning when I get into the office.

kevinrushforth · 2018-11-30T22:43:10Z

The tests all PASS on my Linux system (a physical machine running Ubuntu 18.04). I'll run on Windows and Mac and then finish my review.

I don't have a good answer to your question about the platform differences regarding the scrolling multiplier (-1 on Mac versus 40 on Windows and Linux). The negative versus positive might be related to the "natural scrolling order" on Mac, which is intentionally backwards by default from the other platforms. However, a difference of 40x in magnitude points to something wrong in the implementation.

kevinrushforth

I ran on 4 different systems: Linux, Mac (Retina display), Windows 10, Windows 7. With 125% scaling on Windows (which is what I run by default), the scroll tests fail with a timeout, but they pass if I set the scale to 100%. I didn't look at the code to see what the problem might be, but if there isn't an an obvious solution, one possible fix is to either skip those tests on Windows if the scale is not 1 or else force it to 1.

I also left a comment in the code about the platform-specific scroll scale.

kevinrushforth · 2018-11-30T23:01:20Z

tests/system/src/test/java/test/robot/javafx/scene/RobotTest.java

        CountDownLatch onScrollLatch = new CountDownLatch(1);
        CountDownLatch setSceneLatch = new CountDownLatch(1);
        Button button = new Button("Scroll me");
        InvalidationListener invalidationListener = observable -> setSceneLatch.countDown();
+        int[] totalScroll = new int[]{0};
+        long[] firstScrollMillis = new long[]{0};
+        int scrollMultiplier = System.getProperty("os.name").toLowerCase().startsWith("mac") ? -1 : 40;


Can you add a comment here? This suggests that there might be an issue with scrolling in the Mac robot implementation. We might want to file a follow-up issue to investigate.

brcolow · 2018-12-01T00:22:36Z

I see why it is failing on non 1.0 scales, I will add a commit as soon as I confirm it's fixed (need to multiply event.getDeltaY() by Screen.getPrimary().getOutputScaleY(). I will also add the comment you mentioned.

brcolow · 2018-12-01T00:42:30Z

Added a commit which addresses both issues.

kevinrushforth · 2018-12-01T14:10:43Z

tests/system/src/test/java/test/robot/javafx/scene/RobotTest.java

@@ -474,10 +474,12 @@ private static void testMouseWheel(int amount) {
        InvalidationListener invalidationListener = observable -> setSceneLatch.countDown();
        int[] totalScroll = new int[]{0};
        long[] firstScrollMillis = new long[]{0};
+        // The scroll wheel amount is multiplied by 40 on Linux and Windows, but not on macOS. The
+        // directions are also reversed.


I filed JDK-8214580 to investigate this. Can you add to this comment that the difference in behavior is unexpected and might be a bug? Something like:

// This difference in behavior is unexplained and might be a bug. See JDK-8214580.

kevinrushforth · 2018-12-01T14:11:28Z

tests/system/src/test/java/test/robot/javafx/scene/RobotTest.java

        int scrollMultiplier = System.getProperty("os.name").toLowerCase().startsWith("mac") ? -1 : 40;
        Util.runAndWait(() -> {
            button.setOnScroll(event -> {
-                totalScroll[0] += event.getDeltaY();
+                totalScroll[0] += event.getDeltaY() * Screen.getPrimary().getOutputScaleY();


Looks good. I'll test it on Monday.

kevinrushforth · 2018-12-03T17:19:00Z

tests/system/src/test/java/test/robot/javafx/scene/RobotTest.java

        int scrollMultiplier = System.getProperty("os.name").toLowerCase().startsWith("mac") ? -1 : 40;
        Util.runAndWait(() -> {
            button.setOnScroll(event -> {
-                totalScroll[0] += event.getDeltaY();
+                totalScroll[0] += event.getDeltaY() * Screen.getPrimary().getOutputScaleY();


Multiplying by the output scale works correctly on Windows. It doesn't work on a Mac with a retina display, though. The test now fails for me. This suggests another platform difference between how Robot::mouseWheel works on Mac versus the other platforms.

Do you think we should skip the test on Mac for now and un-skip it as part of JDK-8214580?

I was starting to think the same thing. That would be fine with me. Please update JDK-8214580 with the additional information.

And then you can add an assumeTrue(!isMac) at the top of this test with the JBS bug ID in the comment.

Okay, I added a new commit and updated the bug.

kevinrushforth · 2018-12-03T23:17:16Z

That looks good. One quick thought: for consistency, the following could also use PlatformUtil.isMac() now that the test imports it:

    int scrollMultiplier = System.getProperty("os.name").toLowerCase().startsWith("mac") ? -1 : 40;

I'll do a quick final test on Mac and Windows and then I think we can get this in.

kevinrushforth · 2018-12-06T01:53:25Z

Btw, this would have been easier for me to review without the squash / force-push

brcolow · 2018-12-06T02:22:35Z

Ah, sorry about that. Will keep that in mind going forward.

kevinrushforth

Looks good. I'll merge it in shortly.

eugener added the enhancement New feature or request label Sep 25, 2018

kevinrushforth reviewed Nov 21, 2018

View reviewed changes

tests/system/src/test/java/test/robot/javafx/scene/RobotTest.java Show resolved Hide resolved

brcolow force-pushed the JDK-8207369 branch from 503421a to 46c1791 Compare November 21, 2018 21:14

brcolow force-pushed the JDK-8207369 branch from 993f4fa to 78c2964 Compare November 28, 2018 21:20

brcolow mentioned this pull request Nov 30, 2018

JDK-8090930: Support mouse forward/back buttons. #173

Merged

kevinrushforth reviewed Nov 30, 2018

View reviewed changes

kevinrushforth reviewed Dec 1, 2018

View reviewed changes

kevinrushforth reviewed Dec 3, 2018

View reviewed changes

JDK-8207369 Add more robot tests.

e09b032

brcolow force-pushed the JDK-8207369 branch from b1fece7 to e09b032 Compare December 5, 2018 22:49

kevinrushforth approved these changes Dec 6, 2018

View reviewed changes

kevinrushforth merged commit 1703d45 into javafxports:develop Dec 6, 2018

brcolow deleted the JDK-8207369 branch December 6, 2018 05:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JDK-8207369 Add more robot tests. #215

JDK-8207369 Add more robot tests. #215

brcolow commented Sep 24, 2018 •

edited

Loading

kevinrushforth left a comment

brcolow commented Nov 26, 2018 •

edited

Loading

brcolow commented Nov 26, 2018 •

edited

Loading

brcolow commented Nov 27, 2018

brcolow commented Nov 28, 2018 •

edited

Loading

brcolow commented Nov 30, 2018

kevinrushforth commented Nov 30, 2018

kevinrushforth commented Nov 30, 2018

kevinrushforth left a comment

kevinrushforth Nov 30, 2018

brcolow commented Dec 1, 2018

brcolow commented Dec 1, 2018

kevinrushforth Dec 1, 2018

kevinrushforth Dec 1, 2018

kevinrushforth Dec 3, 2018

brcolow Dec 3, 2018

kevinrushforth Dec 3, 2018 •

edited

Loading

brcolow Dec 3, 2018

kevinrushforth commented Dec 3, 2018

kevinrushforth commented Dec 6, 2018

brcolow commented Dec 6, 2018

kevinrushforth left a comment

JDK-8207369 Add more robot tests. #215

JDK-8207369 Add more robot tests. #215

Conversation

brcolow commented Sep 24, 2018 • edited Loading

kevinrushforth left a comment

Choose a reason for hiding this comment

brcolow commented Nov 26, 2018 • edited Loading

brcolow commented Nov 26, 2018 • edited Loading

brcolow commented Nov 27, 2018

brcolow commented Nov 28, 2018 • edited Loading

brcolow commented Nov 30, 2018

kevinrushforth commented Nov 30, 2018

kevinrushforth commented Nov 30, 2018

kevinrushforth left a comment

Choose a reason for hiding this comment

kevinrushforth Nov 30, 2018

Choose a reason for hiding this comment

brcolow commented Dec 1, 2018

brcolow commented Dec 1, 2018

kevinrushforth Dec 1, 2018

Choose a reason for hiding this comment

kevinrushforth Dec 1, 2018

Choose a reason for hiding this comment

kevinrushforth Dec 3, 2018

Choose a reason for hiding this comment

brcolow Dec 3, 2018

Choose a reason for hiding this comment

kevinrushforth Dec 3, 2018 • edited Loading

Choose a reason for hiding this comment

brcolow Dec 3, 2018

Choose a reason for hiding this comment

kevinrushforth commented Dec 3, 2018

kevinrushforth commented Dec 6, 2018

brcolow commented Dec 6, 2018

kevinrushforth left a comment

Choose a reason for hiding this comment

brcolow commented Sep 24, 2018 •

edited

Loading

brcolow commented Nov 26, 2018 •

edited

Loading

brcolow commented Nov 26, 2018 •

edited

Loading

brcolow commented Nov 28, 2018 •

edited

Loading

kevinrushforth Dec 3, 2018 •

edited

Loading