On Wed, 13 Mar 2024 13:32:32 GMT, Florian Kirmaier <fkirma...@openjdk.org> wrote:
> Based on my previous PR: https://github.com/openjdk/jfx/pull/987 > This one only contains the test. > Because it was fixed in some other PR. > But i would like to see the Test being merged, to ensure it never breaks > again. Changes requested by angorya (Reviewer). tests/system/src/test/java/test/javafx/scene/control/SystemMenuBarTest.java line 2: > 1: /* > 2: * Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved. 2023, 2024, tests/system/src/test/java/test/javafx/scene/control/SystemMenuBarTest.java line 39: > 37: import org.junit.jupiter.api.BeforeAll; > 38: import org.junit.jupiter.api.AfterAll; > 39: import test.com.sun.javafx.pgstub.StubToolkit; please remove stub toolkit import tests/system/src/test/java/test/javafx/scene/control/SystemMenuBarTest.java line 46: > 44: import java.util.concurrent.TimeUnit; > 45: > 46: import static org.junit.Assert.assertTrue; are we mixing junit4 and junit5? tests/system/src/test/java/test/javafx/scene/control/SystemMenuBarTest.java line 68: > 66: * > 67: */ > 68: JMemoryBuddy.memoryTest((checker) -> { Thank you for explaining what the test attempts to do! minor: move javadoc above the `@Test`, and possibly rephrase the description, maybe something like this: "Tests whether the ActionListener associated with the system menu gets collected after said menu is removed." tests/system/src/test/java/test/javafx/scene/control/SystemMenuBarTest.java line 93: > 91: new Thread(() -> { > 92: try { > 93: Thread.sleep(1000); can be replaced by Util.sleep() which throws nothing tests/system/src/test/java/test/javafx/scene/control/SystemMenuBarTest.java line 107: > 105: assertTrue("Timeout waiting for setOnShown", > latch.await(15, TimeUnit.SECONDS)); > 106: } catch (InterruptedException e) { > 107: e.printStackTrace(); why are we catching this exception? tests/system/src/test/java/test/javafx/scene/control/SystemMenuBarTest.java line 111: > 109: }); > 110: } > 111: please remove empty line ------------- PR Review: https://git.openjdk.org/jfx/pull/1401#pullrequestreview-1934750222 PR Review Comment: https://git.openjdk.org/jfx/pull/1401#discussion_r1523994355 PR Review Comment: https://git.openjdk.org/jfx/pull/1401#discussion_r1523992375 PR Review Comment: https://git.openjdk.org/jfx/pull/1401#discussion_r1523631028 PR Review Comment: https://git.openjdk.org/jfx/pull/1401#discussion_r1523989867 PR Review Comment: https://git.openjdk.org/jfx/pull/1401#discussion_r1523993493 PR Review Comment: https://git.openjdk.org/jfx/pull/1401#discussion_r1523994102 PR Review Comment: https://git.openjdk.org/jfx/pull/1401#discussion_r1523990184
