On Mon, 4 May 2020 13:47:58 GMT, Kevin Rushforth <k...@openjdk.org> wrote:
>> Florian Kirmaier has updated the pull request incrementally with one >> additional commit since the last revision: >> >> JDK-8244297 >> Updated JMemoeryBuddy >> Added Copyright header >> disabled default heapdump > > modules/javafx.base/src/test/java/de/sandec/jmemorybuddy/JMemoryBuddy.java > line 1: > >> 1: package de.sandec.jmemorybuddy; >> 2: > > This file needs a standard Oracle copyright header, which you can copy from > any of the other test files. > > Also, this utility class should be moved into a package matching the pattern > we already use for our tests. Something > like `test.util.memory`? Done! > modules/javafx.base/src/test/java/de/sandec/jmemorybuddy/JMemoryBuddy.java > line 15: > >> 14: >> 15: public class JMemoryBuddy { >> 16: > > Since this is a utility intended to be used by various tests, it would be > very helpful to add some documentation about > how to use it. Where/How should I add the documentation? Ideally, I would reference the documentation of the project itself. > modules/javafx.base/src/test/java/de/sandec/jmemorybuddy/JMemoryBuddy.java > line 145: > >> 144: >> 145: public static void createHeapDump() { >> 146: try { > > I don't think we want automatic creation of heap dumps by our unit test > suite. That would need a larger discussion. I > recommend to either comment this out, or else to qualify it with a system > property that could be passed in via a > `gradle` property (e.g., as is done for `UNSTABLE_TEST`), with a default of > `false`. I've made it configurable in the Library with system-parameters and I've changed the default not create a heap dump. Is that ok? ------------- PR: https://git.openjdk.java.net/jfx/pull/204