On Sun, 25 Oct 2020 13:24:44 GMT, Florian Kirmaier <fkirma...@openjdk.org> wrote:
>> It's based on the discussion of my previous PR: >> https://github.com/openjdk/jfx/pull/71 >> >> I Added test utility class copied from JMemoryBuddy and used it to simplify >> 4 of the existing unit tests. >> >> It's a direct copy of my project >> [JMemoryBuddy](https://github.com/Sandec/JMemoryBuddy) without any changes. >> I'm also using it in most of the projects I'm involved with and in my >> experience, the tests with this Library are very stable. I can't remember >> wrong test results. Sometimes the memory behaviour of some libraries itself >> is not stable but the tests with JMemoryBuddy are basically always correct. > > Florian Kirmaier has updated the pull request incrementally with one > additional commit since the last revision: > > JDK-8244297 > Added some more whitespaces based on review Thanks for making the suggested changes. I left one more comment inline and a couple very minor points (which I only mention as something you might want to fix when you take care of the one issue I raised). The rest looks good. I see that you modified the InitialNodesMemoryLeakTest to use JMemeoryBuddy. In order to verify that the assertions are working, I injected an error by reverting the fix for the [JDK-8216377](URL). The modified test still correctly catches the bug: $ gradle --info -PFULL_TEST=true cleanTest :systemTests:test --tests InitialNodesMemoryLeakTest ... test.javafx.scene.InitialNodesMemoryLeakTest > testRootNodeMemoryLeak STANDARD_OUT No Heapdump was created. You might want to change the configuration to get a HeapDump. Gradle Test Executor 1 finished executing tests. > Task :systemTests:test FAILED test.javafx.scene.InitialNodesMemoryLeakTest > testRootNodeMemoryLeak FAILED java.lang.AssertionError: Content of WeakReference was not collected. content: Group@149ab2ce at test.util.memory.JMemoryBuddy.assertCollectable(JMemoryBuddy.java:91) at test.javafx.scene.InitialNodesMemoryLeakTest.testRootNodeMemoryLeak(InitialNodesMemoryLeakTest.java:89) 1 test completed, 1 failed Good. modules/javafx.base/src/test/java/test/util/memory/JMemoryBuddy.java line 134: > 132: > 133: /** > 134: * Checks whether the content of the WeakReference can not be > collected. Minor: cannot is one word. modules/javafx.base/src/test/java/test/util/memory/JMemoryBuddy.java line 150: > 148: public static boolean checkNotCollectable(WeakReference > weakReference) { > 149: createGarbage(); > 150: System.gc(); I recommend calling `System.runFinalization();` here. Also, do you think there is value in checking in a loop? Or is that overkill? One drawback of checking in a loop for the negative case is that you end up going through the loop all the way to the end in the expected case where a reference is not collectable, so it might be best to leave it as a single check as you have done. modules/javafx.base/src/test/java/test/util/memory/JMemoryBuddy.java line 216: > 214: } > 215: > 216: Minor: there are extra blank lines here. ------------- PR: https://git.openjdk.java.net/jfx/pull/204
