RFR: 8244297: memory leak test utility [v6]

Kevin Rushforth kcr at openjdk.java.net
Sat Oct 17 14:07:17 UTC 2020


On Sat, 3 Oct 2020 16:00:48 GMT, Florian Kirmaier <fkirmaier at 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

This is looking quite good now. I really like how it simplifies the tests that you modified to use it. I'll run the
tests next week after you update the PR.

>  I don't plan to confirm all coding guidelines of the JFX-project inside of JMemoryBuddy.

Which ones in particular? I would think that the white space guidelines should be easy to adopt, although since this is
a "downstream" implementation, we can relax this a bit. The only spacing issue I see in the code that really stands out
to me is the missing space after conditional and loop operators (a favorite saying of mine is that "`if` is not a
method"). I note that while different coding style guides vary on many points, all of the ones I have looked at
recommend a space separating these operators from the opening paren. Do you think it would cause you problems to adopt
this in the upstream code?

I'm not really concerned about the naming of non-public variables: as long as the existing names aren't confusing
(which I don't think they are), you can take our suggestions as just that -- suggestions that you can adopt or not.

modules/javafx.base/src/test/java/test/util/memory/JMemoryBuddy.java line 42:

> 40:
> 41: /**
> 42:  * Checkout <a href="https://github.com/Sandec/JMemoryBuddy">https://github.com/Sandec/JMemoryBuddy</a> for more
> documentation.

Can you add 1 or 2 sentences before this pointer describing (at a high level) the purpose of this class?

modules/javafx.base/src/test/java/test/util/memory/JMemoryBuddy.java line 235:

> 233:     }
> 234:
> 235:     public static interface MemoryTestAPI {

Can you add a (short) class javadoc comment? One thing to document is how and whether users of the API should implement
it.

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

PR: https://git.openjdk.java.net/jfx/pull/204


More information about the openjfx-dev mailing list