On Mon, 1 May 2023 14:48:32 GMT, Lukasz Kostyra <lkost...@openjdk.org> wrote:
> Verified these changes on all platforms and saw no abnormalities in test
> execution.
>
> This change is based on a preliminary patch done by @aghaisas , with some
> minor changes and the addition of one Leak-related web test.
> EventListenerLeak test was not touched, as it is a separate manual test app
> instead of a JUnit test.
LG, with minor suggestions. What do you think?
build.gradle line 3468:
> 3466: implementation project(":controls")
> 3467: implementation project(":media")
> 3468: testImplementation project(":base").sourceSets.test.output
does the header year need to be changed?
modules/javafx.web/src/test/java/test/javafx/scene/web/LeakTest.java line 50:
> 48: import org.w3c.dom.NodeList;
> 49: import static org.junit.Assert.*;
> 50: import test.util.memory.JMemoryBuddy;
same comment - year in the header?
tests/system/src/test/java/test/javafx/embed/swing/SwingNodeDnDMemoryLeakTest.java
line 74:
> 72: });
> 73:
> 74: for (WeakReference<SwingNode> ref : weakRefArrSN) {
I wonder if we should add a utility method to JMemoryBuddy to operate on an
array/collection, so we don't have to repeat this code again and again?
tests/system/src/test/java/test/javafx/embed/swing/SwingNodeMemoryLeakTest.java
line 82:
> 80: SwingUtilities.invokeAndWait(() -> Util.sleep(500));
> 81:
> 82: for (WeakReference<SwingNode> ref : weakRefArrSN) {
same comment as before about a new utility method
tests/system/src/test/java/test/javafx/scene/control/TabPaneHeaderLeakTest.java
line 98:
> 96:
> 97: JMemoryBuddy.assertCollectable(tabWeakRef);
> 98: JMemoryBuddy.assertCollectable(textFieldWeakRef);
perhaps the utility method operating on an array should accept varargs, like so:
`assertCollectable(WeakRef a, WeakRef ... b)`
(to differentiate it from a single arg method, and to make sure there are 2+
args)
-------------
PR Review: https://git.openjdk.org/jfx/pull/1121#pullrequestreview-1407777336
PR Review Comment: https://git.openjdk.org/jfx/pull/1121#discussion_r1181683614
PR Review Comment: https://git.openjdk.org/jfx/pull/1121#discussion_r1181683783
PR Review Comment: https://git.openjdk.org/jfx/pull/1121#discussion_r1181685233
PR Review Comment: https://git.openjdk.org/jfx/pull/1121#discussion_r1181686826
PR Review Comment: https://git.openjdk.org/jfx/pull/1121#discussion_r1181687839
