RFR: 8299423: JavaFX Mac system menubar leaks

[Andy Goryachev]

On Wed, 13 Mar 2024 13:32:32 GMT, Florian Kirmaier <fkirmaier at 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