8351357: Add canary system test checking if Stage receives focus on show #1804
Conversation
This test is used as a canary to make sure Stage has focus after showing on screen. Failing this test means other tests might also fail.
|
👋 Welcome back lkostyra! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
Webrevs
|
lukostyra
changed the title
|
Renamed the issue title to better reflect what the change actually does |
There was a problem hiding this comment.
The test looks good and runs as expected on my Windows 11 system. I left two related questions about the delivery of the event. If the key press event might not be delivered synchronously from the robot key press call, then the test could fail spuriously. You might consider a couple changes to make the test more robust.
| Util.runAndWait(() -> { | ||
| robot.keyPress(KeyCode.A); | ||
| }); | ||
| assertTrue(receivedEvent, "Expected key press has NOT been received! Stage did not have focus after showing. Some tests might fail because of this." + |
There was a problem hiding this comment.
Do you need to sleep before the assertion to ensure that the event has been delivered?
There was a problem hiding this comment.
Perhaps we ought to have a utility (latch?) to ensure the sequence?
There was a problem hiding this comment.
A delay of some sort is necessary to give the event loop time to process the event.
There was a problem hiding this comment.
Right. I think Andy was suggesting a latch (with a timeout) that would be triggered (counted down to 0) when the event was received. This would also remove the concern I raised about the flag not being volatile.
There was a problem hiding this comment.
With a latch I think we don't even need the boolean value (we can simply wait for latch to trigger) so I removed it
tests/system/src/test/java/test/robot/javafx/stage/StageFocusTest.java
Outdated
Show resolved
Hide resolved
| @Test | ||
| public void testStageHasFocusAfterShow() { | ||
| Util.sleep(250); | ||
| Util.runAndWait(() -> { |
There was a problem hiding this comment.
If the stage isn't focused after the sleep the robot's key press might go to some other app entirely. You might want to add an assert after the sleep checking that stage.isFocused() returns true.
There was a problem hiding this comment.
I checked this and it actually is a bug, isFocused() returns true in this case (when the Stage is not focused). We don't capture the failure of SetForegroundWindow(), so JFX assumes we are in focus. It doesn't necessarily change the outcome of the test, the key press check will then fail, but it would be good for the flag to also reflect that.
I'll add the assertion and file a separate issue for that.
There was a problem hiding this comment.
I wonder if we could paint the window in some unusual color and check the screen pixels to make sure the window is on top, in addition to being focused?
| // check if window is on top | ||
| Util.runAndWait(() -> { | ||
| Color color = getColor(STAGE_SIZE / 2, STAGE_SIZE / 2); | ||
| assertColorEquals(Color.LIGHTGREEN, color, TOLERANCE); |
There was a problem hiding this comment.
I wonder if this implementation is a reliable test: the stage in question may be overlapped by another window somewhere in the corner, right?
What would be a reliable test? Strictly speaking, we must check every pixel in the scene, though I wonder if checking each pixel in a grid (maybe 20 x 20, since we don't expect any reasonable window to be less than that) should be enough?
There was a problem hiding this comment.
The most reliable way would be to probably scan the whole grid, I agree... I could change this - the stage is not that big after all. Maybe even turn the stage to undecorated to make this more reliable across platforms. I expect the very edge of the Stage to be not exactly the same color, but if we leave ex. 1px margin around the edges it should still be fine. I'll play around with this and update soon.
There was a problem hiding this comment.
I thought about that too - but why wouldn't the client area, the scene itself, be of different color? The decorations and possible effects should be outside of the stage, shouldn't they?
There was a problem hiding this comment.
Turning the scene to undecorated would just make the math easier, without having to count in the window decorations which almost certainly would be of different color.
As for the margin thing, I had to do a bit of history searching and I came back with this old PR: #1242
I think when writing the above my blurry memory recalled one of the attempts at solving this which involved just skipping the 1px margin of the Stage on HiDPI systems. I agree, it should be fine just going through the entire Stage.
There was a problem hiding this comment.
Actually a small correction, using VisualTestBase already assumes the Stage we get is undecorated. So no changes need to be made there.
| theStage.addEventHandler(WindowEvent.WINDOW_SHOWN, e -> { | ||
| Platform.runLater(() -> { | ||
| theStage.setX(STAGE_X); | ||
| theStage.setY(STAGE_Y); |
There was a problem hiding this comment.
setting Stage (x,y) coordinates in runLater makes the window jump.
it's probably better to move LL98-99 before runLater, to L96
with the robot disabled, the test fails with expected
org.opentest4j.AssertionFailedError: Event received latch timed out! Stage most probably did not have focus after showing. Some tests might fail because of this. If that is the case, try re-running the tests with '--no-daemon' flag in Gradle. ==> expected: <true> but was: <false>
at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
tests/system/src/test/java/test/robot/javafx/stage/StageFocusTest.java
Outdated
Show resolved
Hide resolved
tests/system/src/test/java/test/robot/javafx/stage/StageFocusTest.java
Outdated
Show resolved
Hide resolved
|
I'm having an issue with this test. I can't get it to fail on my Windows 11 box but I can get other tests to fail due to focus issues. I'm working on a system keyboard test (see attached patch for a pared down version). On my Windows 11 box the focus does not go to the stage unless I invoke the Robot mouse-click workaround. If I don't use that workaround the key event is going to the terminal window instead of the stage. In that same environment the StageFocusTest in this PR always succeeds. I've never seen it fail. I've run these tests from both a Cygwin terminal and a MSYS2 MINGW terminal using bash and sh and StageFocusTest always passes. I also notice that even when my test case fails the window is still on top, it just doesn't have focus. So the color testing in this PR seems unnecessary (?) Are you seeing cases where the stage is below other windows when it shows up? |
I started to see this problem as well while working on JDK-8359899 (the Stage.isFocused discrepancy). I have a suspicion that switching to VisualTestBase caused some sort of difference - VisualTestBase creates new Stages separately and doesn't take into account the primary Stage that you would get via overriding Application.start(). I noticed your test does not use VisualTestBase so it might be it. I have not looked into a solution yet outside of the obvious - reverting to not using VisualTestBase. I will get that figured out very soon with Andy's review suggestions.
I did have situations where the Stage would appear under currently active window, sometimes even under the terminal window that runs the test. |
This is to check if Stage showing works also for Stages other than the primary Stage provided by Application.start()
|
I did a couple of extra changes in addition to the regular fixes:
I did notice this morning that on a fresh system start, the test can pass even with
For me the test then fails when checking the primary Stage delivered by |
There was a problem hiding this comment.
Code looks fine. I left a comment about providing a help message when the color sampling test fails.
What does it mean to integrate this? We would be adding a test designed to fail with the default testing environment. After failure a developer would have to re-run with the no daemon flag set. Shouldn't that become the default? If not, can we ensure this test runs early so it truly serves as a canary test? Should folks developing new tests assume that they don't have to add the usual workarounds like sending a mouse click to the window?
FWIW, adding the no daemon flag ensures that this test succeeds consistently with Cygwin. With the Mingw shell turning off the daemon increases the chance that it will succeed but doesn't guarantee it.
I agree that this way of running the tests should be the default but there is no way (as far as I know) to do this from the project level. The only path would be to add a According to Gradle manual other ways to disable the daemon are user-specific (
I am not sure if that is possible, the only thing coming to my mind is executing that test specifically via
Some tests did that, some tests did not. This whole ordeal started because I have a patch queued up after this gets introduced which fixes the problem we talked about earlier with |
Ack! I wrote that test! And I should know better. I'll make sure it gets focus and stops using Ctrl+C as a test case. I'm sure I ran MenuDoubleShortcutTest on Windows before submitting it. I just tried it again and it worked fine. Once. The second time it failed. I wish we could find a formula which guarantees these tests always work OR that they always fail. |
This also started being a problem at some later point in time; on my machine tests used to do a full run with no failures with the Gradle daemon being enabled. I can't unfortunately pinpoint when the problems started happening, I only found out after the fact that AFAIK I also submitted a (for now draft) PR for the |
|
Yes, I always disable the gradle daemon in my |
Originally this issue was supposed to resolve problems with some system tests (
MenuDoubleShortcutTest,TextAreaBehaviorTestand others) failing on my Windows machine. In the process of figuring this out I found out the problem is Windows::SetForegroundWindow()API refusing to give focus to JFX Stage upon callingStage.show().The problem happened only when running system tests via Gradle, and with more investigation it turned out the culprit is actually running tests via a Gradle Daemon, which is the default behavior. According to SetForegroundWindow API remarks there is a list of conditions a process must meet to be granted a privilege of receiving focus, which is supposed to prevent focus stealing. While we do meet the required conditions, we don't meet "one of" additional conditions listed in the reference:
The calling process is the foreground process.andThe calling process was started by the foreground process.conditionsThere is currently no foreground window, and thus no foreground process.condition fails - the foreground window is the Terminal itself.The calling process received the last input event.condition cannot be met and would require either Robot workarounds or manual interaction before each test case.Either the foreground process or the calling process is being debugged.is also not met.As such, Windows refuses to grant JFX Stage focus, which fails some system tests relying on it.
While we cannot remedy these conditions in-code (outside of hacky solutions I found with
AttachThreadInputAPI which I am not a fan of) the only solution seems to be running the tests on Windows via eithergradle --no-daemonor by settingorg.gradle.daemon=falseproperty somewhere ingradle.properties.In the process of debugging this problem I wrote a canary test to detect whether a Stage receives focus right after calling
show(). I ran this test on all (accessible to me) platforms (Windows, Linux, macOS) - on both Linux and macOS the test passes regardless of whether the Gradle deamon is used or not. On my Windows machine (Win 11 24H2) it fails when testing through Gradle Daemon (among tests mentioned in the first paragraph) and succeeds when--no-daemonflag is used.I figured the test could be added to the repository and serve as a helper if someone else encounters similar problems.
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1804/head:pull/1804$ git checkout pull/1804Update a local copy of the PR:
$ git checkout pull/1804$ git pull https://git.openjdk.org/jfx.git pull/1804/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1804View PR using the GUI difftool:
$ git pr show -t 1804Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1804.diff
Using Webrev
Link to Webrev Comment