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