On Tue, 17 Sep 2024 13:48:50 GMT, Jay Bhaskar <jbhas...@openjdk.org> wrote:
>> Successfully converted web tests from JUnit 4 to JUnit 5, ensuring all tests
>> are fully compliant with the JUnit 5 framework.
>
> Jay Bhaskar has updated the pull request incrementally with one additional
> commit since the last revision:
>
> remove instance variables in parametrized test
- query for old imports produces 0 hits (good)
- left some minor comments, one (assertArrayEquals) might need a code change
modules/javafx.web/src/test/java/test/com/sun/webkit/network/CookieTest.java
line 581:
> 579: assertEquals("/foo",
> CookieShim.defaultPath(uri("http://hostname/foo/bar?";)));
> 580: assertEquals("/foo",
> CookieShim.defaultPath(uri("http://hostname/foo/bar?query";)));
> 581: assertEquals("/foo",
> CookieShim.defaultPath(uri("http://hostname/foo/bar?query=push";)));
minor: just wanted to say these formatting changes are probably unnecessary,
and there is an added value of being able to set a breakpoint on the CookieShim
method that was lost.
having said that, the changes are ok.
modules/javafx.web/src/test/java/test/javafx/scene/web/FileReaderTest.java line
246:
> 244: String message = String.format("File at %s: Expected
> length %d but got %d",
> 245: filePath, expectedLength, actualLength);
> 246: assertEquals(expectedLength, actualLength, message);
I think all this new code can be replaced by `assertArrayEquals`
modules/javafx.web/src/test/java/test/javafx/scene/web/JavaScriptBridgeTest.java
line 693:
> 691: final JSObject doc = (JSObject) executeScript("document");
> 692: loadContent("<h1></h1>");
> 693: assertThrows(NullPointerException.class, () -> {
I think this is not an equivalent change:
before, the tests expected NPE to come from any statement within the test body,
after, it only expects from line 695
I don't know where the NPE actually comes from (probably from execute, so in
theory the test might still be correct)
(this comment applies to multiple places)
modules/javafx.web/src/test/java/test/javafx/scene/web/MiscellaneousTest.java
line 155:
> 153: // Try accessing the resultObject created in JavaFX
> Application Thread
> 154: // from someother thread. It should throw an exception.
> 155: assertThrows(IllegalStateException.class, () -> {
on second thought, I think this and the previous changes, even though not
technically equivalent, are perfectly fine, in the spirit of the test.
-------------
PR Review: https://git.openjdk.org/jfx/pull/1567#pullrequestreview-2313724822
PR Review Comment: https://git.openjdk.org/jfx/pull/1567#discussion_r1765677624
PR Review Comment: https://git.openjdk.org/jfx/pull/1567#discussion_r1765701013
PR Review Comment: https://git.openjdk.org/jfx/pull/1567#discussion_r1765707526
PR Review Comment: https://git.openjdk.org/jfx/pull/1567#discussion_r1765722359
