On Fri, 12 Mar 2021 15:11:42 GMT, Florian Kirmaier <fkirma...@openjdk.org>
wrote:
>> tests/system/src/test/java/test/javafx/scene/StyleMemoryLeakTest.java line
>> 106:
>>
>>> 104: });
>>> 105: }
>>> 106: }
>>
>> In order to make this test similar to existing system tests, I made some
>> relevant changes. Modified test is added below.
>> The modified test fails with this fix, but I expected it to pass. Can you
>> please check this.
>>
>> Changes are
>> 1. `Thread.sleep()` is removed.
>> 2. `root` and `toBeRemoved` button are now class members.
>> 3. Scenegraph is constructed and shown in `TestApp.start()` method.
>>
>>
>> public class StyleMemoryLeakTest {
>>
>> static CountDownLatch startupLatch;
>> static Stage stage;
>> static Button toBeRemoved;
>> static Group root;
>>
>> public static class TestApp extends Application {
>>
>> @Override
>> public void start(Stage primaryStage) throws Exception {
>> stage = primaryStage;
>> toBeRemoved = new Button();
>> root = new Group();
>> root.getChildren().add(toBeRemoved);
>> stage.setScene(new Scene(root));
>> stage.setOnShown(l -> {
>> Platform.runLater(() -> startupLatch.countDown());
>> });
>> stage.show();
>> }
>> }
>>
>> @BeforeClass
>> public static void initFX() throws Exception {
>> startupLatch = new CountDownLatch(1);
>> new Thread(() ->
>> Application.launch(StyleMemoryLeakTest.TestApp.class,
>> (String[])null)).start();
>> assertTrue("Timeout waiting for FX runtime to start",
>> startupLatch.await(15, TimeUnit.SECONDS));
>> }
>>
>> @Test
>> public void testRootNodeMemoryLeak() throws Exception {
>> Util.runAndWait(() -> {
>> root.getChildren().clear();
>> stage.hide();
>> });
>> JMemoryBuddy.memoryTest((checker) -> {
>> checker.assertCollectable(stage);
>> checker.setAsReferenced(toBeRemoved);
>> stage = null;
>> });
>> }
>>
>> @AfterClass
>> public static void teardownOnce() {
>> Platform.runLater(() -> {
>> Platform.exit();
>> });
>> }
>> }
>
> I've added your verison of the unit-test. You forgot `root = null;` which was
> why the test failed.
>
> If I would rewrite the test, I would put everything into the TestMethod.
> Because this way it's not necessary to set all the class-variables to null.
> It also wouldn't reuse the Stage of the Application. The scope of the test
> would be much smaller, because the actual test and the initialization of
> JavaFX would be separated.
>
> Should I change it that way?
I am fine with that too. In that case my only suggestion would be, use method
`Stage.setOnShown()` like in the current version to make sure that `Stage` is
shown and not to rely on `Thread.sleep()`.
> It also wouldn't reuse the Stage of the Application.
In that case, test will not require the `TestApp` class. You can use
`Platform.startup()` method to start JavaFX runtime and also **may** need a
call to `Platform.setImplicitExit​(false)` to make sure JavaFX runtime does not
exit when Stage is hidden. This way we can add multiple tests that create and
hide stage in a same test file.
-------------
PR: https://git.openjdk.java.net/jfx/pull/424
