On Mon, 4 May 2020 13:47:58 GMT, Kevin Rushforth <[email protected]> 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
