On Thu, 13 Jun 2024 07:17:01 GMT, John Hendrikx <jhendr...@openjdk.org> wrote:
>> Johan Vos has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Add more type info
>
> tests/system/src/test/java/test/com/sun/javafx/tk/quantum/SystemMenuBarTest.java
> line 287:
>
>> 285:
>> 286: @Test
>> 287: public void testJDK8309935() {
>
> minor: Not sure what our policies are, but I find test names like this pretty
> useless, perhaps a short indication what this is doing?
It is a good question. We don't have a consistent approach yet. I personally
like the link to the JBS issue as that has (or should have) all info. I added a
short indication, but the real info should be on the issue, imho.
(this is something we should discuss on the mailing list)
> tests/system/src/test/java/test/com/sun/javafx/tk/quantum/SystemMenuBarTest.java
> line 346:
>
>> 344: });
>> 345: Util.runAndWait(() -> {
>> 346: });
>
> I checked the code that `runAndWait` would do when you pass in 0 runnables,
> and it basically does no waiting at all (less than a microsecond for sure).
> If this is really essential (I removed it and the test still passed) then I
> think a `sleep` might be better.
The Util.runAndWait was indeed not a good idea. The problem is not that the
test might not pass, the problem is that the test might not fail if we don't
wait for things that need to run to complete. But this is better done with a
Platform.runLater() which will add a runnable to the queue, so when that is
executed, we know that all previously scheduled runnables completed.
I changed the approach ehre.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1283#discussion_r1670088321
PR Review Comment: https://git.openjdk.org/jfx/pull/1283#discussion_r1670090848
