8304922: [testbug] SliderTooltipNPETest fails on Linux #1119

karthikpandelu · 2023-04-28T12:06:51Z

The test was failing on the first run and then it was not failing. Cause for the failure looked to be the UI state not getting updated. Hence added call to Toolkit firePulse method.

Able to run the test consistently without failure in Linux after the fix.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8304922: [testbug] SliderTooltipNPETest fails on Linux (Bug - P4)

Reviewers

Kevin Rushforth (@kevinrushforth - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1119/head:pull/1119
$ git checkout pull/1119

Update a local copy of the PR:
$ git checkout pull/1119
$ git pull https://git.openjdk.org/jfx.git pull/1119/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1119

View PR using the GUI difftool:
$ git pr show -t 1119

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1119.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2023-04-28T12:07:23Z

👋 Welcome back kpk! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

mlbridge · 2023-04-28T12:14:29Z

Webrevs

kevinrushforth

It doesn't work for me.

kevinrushforth · 2023-04-28T13:04:44Z

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

@@ -94,6 +94,7 @@ private void dragSliderAfterTooltipDisplayed(int dragDistance) throws Exception
                                slider.getLayoutX() + slider.getLayoutBounds().getWidth()/2),
                            (int)(scene.getWindow().getY() + scene.getY() +
                                slider.getLayoutY() + slider.getLayoutBounds().getHeight()/2));
+            Toolkit.getToolkit().firePulse();


A system test should not need to use firePulse, and I can't think why this would help. In any case, I still see the failure on my Ubuntu 22.04 VM.

Oh Okay! Let me check on that again.
Looks like the failure is not consistent in all the Linux systems. Even without the fix it was failing only on the first run for me in VM as well as in Ubuntu workstation.

karthikpandelu · 2023-05-11T12:04:50Z

Since consumeAutoHidingEvents property value is asserted in the test, I'm suspecting that it is getting asserted before the mouse moves away from the slider and consumeAutoHidingEvents property value is reset to original value.
Since the test is not failing consistently for me in Ubuntu, I believe possibility of product issue is less. Hence added a latch and small delay to make sure that the property value is asserted only after the mouse is moved.
@kevinrushforth could you please check this fix.
I have ran the test in Ubuntu 22.04 VM multiple times and test didn't fail.

bridgekeeper · 2023-06-08T17:45:53Z

@karthikpandelu This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

karthikpandelu · 2023-06-12T05:39:40Z

@kevinrushforth could you please check the updated fix whenever possible?

kevinrushforth

It still fails the first time I run it. I ran it on a physical machine, and I now know why. I can make it fail every time by moving the mouse away from the center of the screen (so that it is outside of where the window will appear).

The problem is that the tooltip will not show up on Linux if you directly move the cursor with Robot to be over the control without it ever having been somewhere else in the window first. So the only reason it works on subsequent runs is that the mouse is left in a position such that it will be over the window when the test is run the next time.

In general, it's also a good idea to click in the window to ensure that it is activated. The only other robot-based test that shows a Tooltip first moves the mouse to the upper-left corner of the scene and then does a mouse click. Adding that code to your patch causes it to work.

So I recommend adding the following to the test (in addition to the changes you already made, which look good).

diff --git a/tests/system/src/test/java/test/robot/javafx/scene/SliderTooltipNPETest.java b/tests/system/src/test/java/test/robot/javafx/scene/SliderTooltipNPETest.java
--- a/tests/system/src/test/java/test/robot/javafx/scene/SliderTooltipNPETest.java
+++ b/tests/system/src/test/java/test/robot/javafx/scene/SliderTooltipNPETest.java
@@ -88,6 +88,13 @@ public class SliderTooltipNPETest {
     }
 
     private void dragSliderAfterTooltipDisplayed(int dragDistance) throws Exception {
+        Util.runAndWait(() -> {
+            // Click somewhere in the Stage to ensure that it is active
+            robot.mouseMove((int)(scene.getWindow().getX() + scene.getX()),
+                            (int)(scene.getWindow().getY() + scene.getY()));
+            robot.mouseClick(MouseButton.PRIMARY);
+        });
+
         Thread.sleep(1000); // Wait for slider to layout
 
         Util.runAndWait(() -> {

karthikpandelu · 2023-06-13T05:48:22Z

Thanks for the suggestion @kevinrushforth. Made changes according to your comments. Please check.

openjdk · 2023-06-13T13:03:18Z

@karthikpandelu This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8304922: [testbug] SliderTooltipNPETest fails on Linux

Reviewed-by: kcr

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 42 new commits pushed to the master branch:

614dc55: 8306021: Add event handler management to EventTarget
bd24fc7: 8309508: Possible memory leak in JPEG image loader
9913b23: 8306648: Update the JavaDocs to show the NEW section and DEPRECATED versions
9ad0e90: 8309470: Potential performance improvements in VirtualFlow
6a159e9: 8194704: Text/TextFlow hitTest() javadoc
883c4e0: 8309001: Allow building JavaFX on Linux/riscv64
17ed2e7: 8307538: Memory leak in TreeTableView when calling refresh
05548ac: 8301312: Create implementation of NSAccessibilityButton protocol
10f41b7: 8293836: Rendering performance degradation at bottom of TableView with many rows
1a0f6c7: 8306447: Adding an element to a long existing list may cause the first visible element to jump
... and 32 more: https://git.openjdk.org/jfx/compare/52d32c09b34368f83dc761776bef410c0d1e991f...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

karthikpandelu · 2023-06-13T13:32:47Z

/integrate

openjdk · 2023-06-13T13:33:40Z

Going to push as commit 72be85e.
Since your change was applied there have been 42 commits pushed to the master branch:

614dc55: 8306021: Add event handler management to EventTarget
bd24fc7: 8309508: Possible memory leak in JPEG image loader
9913b23: 8306648: Update the JavaDocs to show the NEW section and DEPRECATED versions
9ad0e90: 8309470: Potential performance improvements in VirtualFlow
6a159e9: 8194704: Text/TextFlow hitTest() javadoc
883c4e0: 8309001: Allow building JavaFX on Linux/riscv64
17ed2e7: 8307538: Memory leak in TreeTableView when calling refresh
05548ac: 8301312: Create implementation of NSAccessibilityButton protocol
10f41b7: 8293836: Rendering performance degradation at bottom of TableView with many rows
1a0f6c7: 8306447: Adding an element to a long existing list may cause the first visible element to jump
... and 32 more: https://git.openjdk.org/jfx/compare/52d32c09b34368f83dc761776bef410c0d1e991f...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-06-13T13:33:54Z

@karthikpandelu Pushed as commit 72be85e.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Adding firePulse call to update UI state and display tooltip

53a008e

openjdk bot added the rfr Ready for review label Apr 28, 2023

kevinrushforth reviewed Apr 28, 2023

View reviewed changes

Add latch for tooltip hide and mouse move events

1033f25

karthikpandelu mentioned this pull request May 18, 2023

8304831: TextFlow.hitTest.insertionIndex incorrect with surrogate pairs #1091

Closed

3 tasks

kevinrushforth self-requested a review May 22, 2023 21:19

kevinrushforth requested changes Jun 12, 2023

View reviewed changes

Stabilize test in Linux

08d08a7

kevinrushforth approved these changes Jun 13, 2023

View reviewed changes

openjdk bot added the ready Ready to be integrated label Jun 13, 2023

openjdk bot added the integrated Pull request has been integrated label Jun 13, 2023

openjdk bot closed this Jun 13, 2023

openjdk bot removed ready Ready to be integrated rfr Ready for review labels Jun 13, 2023

karthikpandelu deleted the slider_test_fix branch June 13, 2023 13:47

8304922: [testbug] SliderTooltipNPETest fails on Linux #1119

8304922: [testbug] SliderTooltipNPETest fails on Linux #1119

karthikpandelu commented Apr 28, 2023 •

edited by openjdk bot

bridgekeeper bot commented Apr 28, 2023

mlbridge bot commented Apr 28, 2023 •

edited

kevinrushforth left a comment

kevinrushforth Apr 28, 2023 •

edited

karthikpandelu Apr 28, 2023

karthikpandelu commented May 11, 2023

bridgekeeper bot commented Jun 8, 2023

karthikpandelu commented Jun 12, 2023

kevinrushforth left a comment

karthikpandelu commented Jun 13, 2023

openjdk bot commented Jun 13, 2023

karthikpandelu commented Jun 13, 2023

openjdk bot commented Jun 13, 2023

openjdk bot commented Jun 13, 2023

8304922: [testbug] SliderTooltipNPETest fails on Linux #1119

8304922: [testbug] SliderTooltipNPETest fails on Linux #1119

Conversation

karthikpandelu commented Apr 28, 2023 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Apr 28, 2023

mlbridge bot commented Apr 28, 2023 • edited

Webrevs

kevinrushforth left a comment

Choose a reason for hiding this comment

kevinrushforth Apr 28, 2023 • edited

Choose a reason for hiding this comment

karthikpandelu Apr 28, 2023

Choose a reason for hiding this comment

karthikpandelu commented May 11, 2023

bridgekeeper bot commented Jun 8, 2023

karthikpandelu commented Jun 12, 2023

kevinrushforth left a comment

Choose a reason for hiding this comment

karthikpandelu commented Jun 13, 2023

openjdk bot commented Jun 13, 2023

karthikpandelu commented Jun 13, 2023

openjdk bot commented Jun 13, 2023

openjdk bot commented Jun 13, 2023

karthikpandelu commented Apr 28, 2023 •

edited by openjdk bot

mlbridge bot commented Apr 28, 2023 •

edited

kevinrushforth Apr 28, 2023 •

edited