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 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
