On Mon, 20 Sep 2021 12:56:39 GMT, Kevin Rushforth <k...@openjdk.org> wrote:
>> Florian Kirmaier has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> JDK-8273969
>> Some changes based on code review
>
> tests/system/src/test/java/test/javafx/scene/PlatformStartupMemoryLeakTest.java
> line 26:
>
>> 24: */
>> 25:
>> 26: package test.javafx.scene;
>
> Similar tests are in the `test.com.sun.javafx.application` package, so I
> recommend putting this test there.
done
> tests/system/src/test/java/test/javafx/scene/PlatformStartupMemoryLeakTest.java
> line 34:
>
>> 32: public class PlatformStartupMemoryLeakTest {
>> 33:
>> 34: @Test
>
> Since this test calls `Platform::startup`, this class must only have a single
> `@Test` method. I recommend adding a comment to that effect, so that someone
> doesn't try to add a second test method later.
I think that's redundant after moving it to the application package.
In the application package, most of the test classes only have 1 test for this
reason.
> tests/system/src/test/java/test/javafx/scene/PlatformStartupMemoryLeakTest.java
> line 37:
>
>> 35: public void testStartupLeak() {
>> 36: JMemoryBuddy.memoryTest((checker) -> {
>> 37: // Doesn't work as lambda for some reason, due to
>> "BoundMethodHandle$Species_L"
>
> Based on Tom's comment, and also my reading of the spec, this is expected (if
> somewhat counter-intuitive), so please modify this comment to indicate that
> the `Runnable` must not be turned into a lambda.
done
> tests/system/src/test/java/test/javafx/scene/PlatformStartupMemoryLeakTest.java
> line 47:
>
>> 45: checker.assertCollectable(r);
>> 46: });
>> 47: }
>
> I recommend adding an `@After` (or `@AfterClass`) cleanup method that calls
> `Platform::exit`.
done
-------------
PR: https://git.openjdk.java.net/jfx/pull/626
