On Tue, 5 Sep 2023 20:18:52 GMT, Andy Goryachev <ango...@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
