On Sat, 5 Feb 2022 05:41:25 GMT, Jay Bhaskar <d...@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
