On Wed, 19 Apr 2023 13:25:07 GMT, Jay Bhaskar <jbhas...@openjdk.org> wrote:
> Issue: "LocalStorageTest and UserDataDirectoryTest don't always cleanup data
> dirs"
> Solution:
> 1. All data directories must only be created under the "build" dir. In
> addition to being a best practice, at least they will be cleaned up by
> "gradle clean"
> 2. There should be an @AfterClass method that deletes the data dirs
> 3. There should be an @BeforeClass method that also deletes the data dirs
> prior to running the tests in case it wasn't cleaned up previously
I haven't tested it yet, but I did leave a couple comments.
modules/javafx.web/src/test/java/test/javafx/scene/web/LocalStorageTest.java
line 45:
> 43:
> 44: private static final File LOCAL_STORAGE_DIR_PATH = new File("build/");
> 45: private static final File LOCAL_STORAGE_DIR = new
> File("build/localstorage");
I don't see the need for two variables here. Why not use the updated value of
`LOCAL_STORAGE_DIR` everywhere?
modules/javafx.web/src/test/java/test/javafx/scene/web/LocalStorageTest.java
line 51:
> 49: for (File f : file.listFiles()) {
> 50: deleteRecursively(f);
> 51: f.delete();
This will miss deleting the original directory passed into this method. What
was wrong with the old way of doing it?
modules/javafx.web/src/test/java/test/javafx/scene/web/LocalStorageTest.java
line 84:
> 82: final WebEngine webEngine = getEngine();
> 83: webEngine.setJavaScriptEnabled(true);
> 84: webEngine.setUserDataDirectory(LOCAL_STORAGE_DIR_PATH);
I think this change should be reverted (and similarly in other places)
-------------
PR Review: https://git.openjdk.org/jfx/pull/1100#pullrequestreview-1392117603
PR Review Comment: https://git.openjdk.org/jfx/pull/1100#discussion_r1171374713
PR Review Comment: https://git.openjdk.org/jfx/pull/1100#discussion_r1171376803
PR Review Comment: https://git.openjdk.org/jfx/pull/1100#discussion_r1171377563
