RFR: 8255940: localStorage is null after window.close() [v2]

[Kevin Rushforth]

On Sat, 5 Feb 2022 05:41:25 GMT, Jay Bhaskar <duke at openjdk.java.net> wrote:

>> modules/javafx.web/src/test/java/test/javafx/scene/web/LocalStorageTest.java line 2:
>> 
>>> 1: /*
>>> 2:  * Copyright (c) 2011, 2020, Oracle and/or its affiliates. All rights reserved.
>> 
>> Copyright should be a single year (2022)
>
> done

Did you forget to push the change? It doesn't show up in the PR.

>> modules/javafx.web/src/test/java/test/javafx/scene/web/LocalStorageTest.java line 48:
>> 
>>> 46:         final WebEngine webEngine = getEngine();
>>> 47:         webEngine.setJavaScriptEnabled(true);
>>> 48:         webEngine.setUserDataDirectory(new File("/tmp/java-store"));
>> 
>> You should not hard-code /tmp/. You can either use a local subdirectory in the build dir (which some other tests do), or else you will need to use something like `Files.createTempDirectory("webstorage")`. If you use the latter, then you will need to worry about how to clean up after the test, so the former seems better.
>
> i think /tmp/java-store is ok, as it would not require cleanup

No, it's not OK for two reasons:

1. It prevents concurrent execution of tests from two different directories
2. The Java temp directory might be something other than "/tmp" on some systems.

Best is to use a local subdir under the build directory.

>> modules/javafx.web/src/test/java/test/javafx/scene/web/LocalStorageTest.java line 70:
>> 
>>> 68:             WebView view = getView();
>>> 69:             view.getEngine().executeScript("test_local_storage_set();");
>>> 70:             String s = (String) view.getEngine().executeScript("document.getElementById('key').innerText;");
>> 
>> This will work, but it might be cleaner to add a JavaScript `getLocalStorage` method so you don't have to get it from a DOM element.
>
> The method webEngine.executeScript("localStorage;")  returns JSObject and in to get data members we need to call js.getMember(...) , getMember is not available in our code base, so i used tring s = (String) view.getEngine().executeScript("document.getElementById('key').innerText;");

OK. You could also have gotten it by calling a JavaScript method that you provided, but if this works, then that's fine.

>> modules/javafx.web/src/test/java/test/javafx/scene/web/LocalStorageTest.java line 80:
>> 
>>> 78:         submit(() -> {
>>> 79:             WebView view = getView();
>>> 80:             view.getEngine().executeScript("delete_items();");
>> 
>> You need to set some items first before deleting them if you want it to be an effective test of this case.
>
> The test runs after testLocalStorageSet , so there would be items in localstorage

No, this is a common misconception when writing JUnit tests. The test execution order is _not_ guaranteed and will change. Each test method needs to run as if it were the first (or only) test.

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

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