On Sat, 3 Oct 2020 16:00:48 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 > Fixed unit-test modules/javafx.base/src/test/java/test/util/memory/JMemoryBuddy.java line 104: > 102: try { > 103: Thread.sleep(sleepTime); > 104: } catch (InterruptedException e) {} Ideally the exception should never occur, but in case it does then I think the test should be marked as failed. modules/javafx.base/src/test/java/test/util/memory/JMemoryBuddy.java line 132: > 130: * Checks whether the content of the WeakReference can not be > collected. > 131: * @param weakReference The WeakReference to check. > 132: * @return Returns true, when the provided WeakReference can be > collected. needs typo correction: can not be collected modules/javafx.base/src/test/java/test/util/memory/JMemoryBuddy.java line 152: > 150: f.accept(new MemoryTestAPI() { > 151: public void assertCollectable(Object ref) { > 152: if(ref == null) throw new NullPointerException(); `Object.requireNonNull()` method would be more suitable choice for null check. modules/javafx.base/src/test/java/test/util/memory/JMemoryBuddy.java line 47: > 45: > 46: private static int steps = 10; > 47: private static int overallTime = 1000; `overallTime` can be named as `totalDuration` or `testDuration` modules/javafx.base/src/test/java/test/util/memory/JMemoryBuddy.java line 48: > 46: private static int steps = 10; > 47: private static int overallTime = 1000; > 48: private static int sleepTime = overallTime / steps; `sleepTime` -> `sleepDuration` modules/javafx.base/src/test/java/test/util/memory/JMemoryBuddy.java line 52: > 50: private static int garbageAmount = 999999; > 51: private static String MX_BEAN_PROXY_TYPE = > "com.sun.management:type=HotSpotDiagnostic"; > 52: private static String outputFolderString = "."; I am not sure of the directory that we use to save any runtime artifacts, but there must be one common directory that we should use here also. modules/javafx.base/src/test/java/test/util/memory/JMemoryBuddy.java line 56: > 54: static { > 55: outputFolderString = > System.getProperty("jmemorybuddy.output","."); > 56: overallTime = > Integer.parseInt(System.getProperty("jmemorybuddy.checktime","1000")); `sleepTime` should also be recomputed here based on `overallTime` modules/javafx.base/src/test/java/test/util/memory/JMemoryBuddy.java line 51: > 49: private static boolean createHeapdump = false; > 50: private static int garbageAmount = 999999; > 51: private static String MX_BEAN_PROXY_TYPE = > "com.sun.management:type=HotSpotDiagnostic"; `MX_BEAN_PROXY_TYPE` is not constant, so it should not be all capital letters but should be named just like other variables. modules/javafx.base/src/test/java/test/util/memory/JMemoryBuddy.java line 88: > 86: * @return Returns true, when the provided WeakReference can be > collected. > 87: */ > 88: public static boolean checkCollectable(WeakReference weakReference) { I would prefer a name like `checkIfCollected` or `checkIfGCed` modules/javafx.base/src/test/java/test/util/memory/JMemoryBuddy.java line 84: > 82: > 83: /** > 84: * Checks whether the content of the WeakReference can be collected. -> Checks whether the content of the WeakReference **is** collected. modules/javafx.base/src/test/java/test/util/memory/JMemoryBuddy.java line 113: > 111: if(weakReference.get() == null && counter < steps / 3) { > 112: int percentageUsed = (int) ((steps - counter) / steps * 100); > 113: System.out.println("Warning test seems to be unstable. time > used: " + percentageUsed + "%"); Seems like candidate for `System.err.` modules/javafx.base/src/test/java/test/util/memory/JMemoryBuddy.java line 79: > 77: AssertCollectable assertCollectable = new > AssertCollectable(weakReference); > 78: createHeapDump(); > 79: throw new AssertionError("Content of WeakReference was not > collected. content: " + weakReference.get()); Can be replaced with `assertNull` modules/javafx.base/src/test/java/test/util/memory/JMemoryBuddy.java line 255: > 253: > 254: static class AssertCollectable { > 255: WeakReference<Object> assertCollectable; There is method, class member variable and class itself with the same name `assertCollectable`. I think they should be named differently, like for instance, 1. Class `AssertCollectable` can be named as `CollectableReferernce` or `ReferenceToBeGC` 2. The member variable `assertCollectable` can be named as just `weakRef` or `reference`. 3. The method `assertCollectable` can be named as `checkIfGCed` or `checkIfCollected` Similar change for `AssertNotCollectable` ------------- PR: https://git.openjdk.java.net/jfx/pull/204
