RFR: 8315317: Add test for JDK-8262518

Kevin Rushforth kcr at openjdk.org
Fri Sep 1 14:41:49 UTC 2023


On Fri, 1 Sep 2023 14:33:52 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

>> Added automated test for 8262518:SwingNode.setContent does not close previous content, resulting in memory leak
>
> tests/system/src/test/java/test/javafx/embed/swing/SwingNodeContentMemoryLeakTest.java line 94:
> 
>> 92:                 //Lets throw in a little sleep so we can read the output
>> 93:                 try {
>> 94:                     Thread.sleep(100);
> 
> I would suggest to use random delay here (random.nextInt(100)).
> But make sure to set a random seed at the beginning and print it so it can be reproduced.  Although in this test, the outcome depends on many things that the test has no control over.
> What do you think?

No, let's not. While there may be some rare cases where randomness adds something useful to the test, automated tests should be predictable.

> tests/system/src/test/java/test/javafx/embed/swing/SwingNodeContentMemoryLeakTest.java line 116:
> 
>> 114:                 System.out.println("iteration " + count + " Panels in memory: "
>> 115:                                                + panelCount + " fail " + fail);
>> 116:                 assertFalse(fail > 2);
> 
> I actually seen the  number to shoot up to 38 (albeit with Thread.sleep(1)).  I wonder if there is a better way to detect failure?  May be the fact that the 'panelCount` is ever increasing is the sign of failure, whereas if at least one time it's dropping we have succeeded.
> Or perhaps look at the count when the test complete and fail if panelCount < (attempts / 2) or something

I recommend using `JMemoryBuddy`, which is what we use for all of our newer memory leak tests.

-------------

PR Review Comment: https://git.openjdk.org/jfx/pull/1228#discussion_r1313120071
PR Review Comment: https://git.openjdk.org/jfx/pull/1228#discussion_r1313120559


More information about the openjfx-dev mailing list