RFR: 8308608: [testbug] Use Util::waitForIdle instead of Toolkit::firePulse in system tests [v2]
Karthik P K
kpk at openjdk.org
Wed Sep 6 06:04:49 UTC 2023
On Tue, 5 Sep 2023 20:18:52 GMT, Andy Goryachev <angorya at openjdk.org> wrote:
>> Karthik P K has updated the pull request incrementally with one additional commit since the last revision:
>>
>> Review comments
>
> tests/system/src/test/java/test/robot/javafx/scene/ChoiceBoxScrollUpOnCollectionChangeTest.java line 101:
>
>> 99: });
>> 100:
>> 101: Util.waitForIdle(scene);
>
> is this the right place for waitForIdle? should it be on L98? since there are multiple key presses involved?
>
> and also, do we still need to .sleep() on L103?
Ideally yes it should be on L98. Since we can not call `waitForIdle` from Application thread I added at L101. But if we have to achieve the purpose of using `waitForIdle` in the test then it should be added after each key press. So updated the code to do so.
I had kept the sleep as it is thinking if it might be required in slower systems. Since we are waiting for 10 pulses with `waitForIdle` it might not be needed. Hence removed it.
Since we are using `Util.runAndWait` there is no need for the `scrollLatch` hence removed it.
> tests/system/src/test/java/test/robot/javafx/scene/ContextMenuNPETest.java line 104:
>
>> 102: robot.keyType(KeyCode.ENTER);
>> 103: });
>> 104: Util.waitForIdle(scene);
>
> once we have waitForIdle, do we still need to wait() ?
Same as above comment, I had kept the sleep as it is thinking if it might be required in slower systems. Since we are waiting for 10 pulses with `waitForIdle` it might not be needed. Hence removed it.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1230#discussion_r1316774380
PR Review Comment: https://git.openjdk.org/jfx/pull/1230#discussion_r1316774759
More information about the openjfx-dev
mailing list