On Thu, 4 May 2023 08:21:15 GMT, Lukasz Kostyra <[email protected]> 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.
>
> Lukasz Kostyra has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Update javafx.web classpath to include JMemoryBuddy
The changes generally look good, although I left a comment about sleeping on
the EDT.
modules/javafx.base/src/test/java/test/util/memory/JMemoryBuddy.java line 95:
> 93: }
> 94:
> 95: public static void assertCollectable(WeakReference[] weakReferences) {
Can you add javadoc comments?
tests/system/src/test/java/test/javafx/embed/swing/SwingNodeMemoryLeakTest.java
line 80:
> 78: });
> 79:
> 80: SwingUtilities.invokeAndWait(() -> Util.sleep(500));
Sleeping on either the JavaFX Application thread or the Swing EDT (as in this
case) is not generally desirable. What is the sleep trying to achieve? Is there
a better way, possibly sleeping on the test thread and then doing a
`SwingUtilities.invokeAndWait` (maybe with a no-op runnable? not sure)?.
-------------
PR Review: https://git.openjdk.org/jfx/pull/1121#pullrequestreview-1412937420
PR Review Comment: https://git.openjdk.org/jfx/pull/1121#discussion_r1184918401
PR Review Comment: https://git.openjdk.org/jfx/pull/1121#discussion_r1184923206
