RFR: 8339515: [TestBug] Convert web tests to JUnit 5 [v10]

[Andy Goryachev]

On Tue, 17 Sep 2024 13:48:50 GMT, Jay Bhaskar <jbhaskar at 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