8339515: [TestBug] Convert web tests to JUnit 5 #1567

jaybhaskar · 2024-09-15T12:31:03Z

Successfully converted web tests from JUnit 4 to JUnit 5, ensuring all tests are fully compliant with the JUnit 5 framework.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8339515: [TestBug] Convert web tests to JUnit 5 (Enhancement - P4)

Reviewers

Marius Hanl (@Maran23 - Committer)
Ambarish Rapte (@arapte - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1567/head:pull/1567
$ git checkout pull/1567

Update a local copy of the PR:
$ git checkout pull/1567
$ git pull https://git.openjdk.org/jfx.git pull/1567/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1567

View PR using the GUI difftool:
$ git pr show -t 1567

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1567.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2024-09-15T12:31:30Z

👋 Welcome back jbhaskar! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-09-15T12:31:37Z

@jaybhaskar This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8339515: [TestBug] Convert web tests to JUnit 5

Reviewed-by: mhanl, arapte

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 1 new commit pushed to the master branch:

5993c09: 8339513: [TestBug] Convert fxml tests to JUnit 5

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

mlbridge · 2024-09-15T12:34:42Z

Webrevs

Maran23

There is one major issue with the parameterized test as far as I can see. Some minor style guide things, looks good otherwise!

modules/javafx.web/src/test/java/test/com/sun/webkit/network/CookieTest.java

modules/javafx.web/src/test/java/test/javafx/scene/web/FileReaderTest.java

modules/javafx.web/src/test/java/test/javafx/scene/web/LoadTest.java

modules/javafx.web/src/test/java/test/javafx/scene/web/SubresourceIntegrityTest.java

Maran23 · 2024-09-15T12:57:39Z

Also one note: The copyright year was not updated, but this could also be done by the script afterwards I guess.

openjdk · 2024-09-15T23:56:14Z

@jaybhaskar this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout testsprint
git fetch https://git.openjdk.org/jfx.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

kevinrushforth

You need to revert the copyright year change, since you've mistakenly removed the start year.

modules/javafx.web/src/test/java/test/com/sun/webkit/SharedBufferTest.java

This reverts commit 27275c3.

andy-goryachev-oracle · 2024-09-16T15:13:47Z

looks like there is a merge conflict label added

arapte

Suggesting a few minor changes.

modules/javafx.web/src/test/java/test/javafx/scene/web/HistoryTest.java

modules/javafx.web/src/test/java/test/javafx/scene/web/MiscellaneousTest.java

modules/javafx.web/src/test/java/test/javafx/scene/web/FormControlsTest.java

modules/javafx.web/src/test/java/test/javafx/scene/web/SubresourceIntegrityTest.java

kevinrushforth · 2024-09-17T00:03:33Z

Note to reviewers: The GHA build compiles the web tests, but doesn't run them. Someone will need to do an offline test run with this patch.

arapte

Providing a few comments, majority are related to correcting indentation correction.

Only space changes: Some indentation changes only add or remove spaces. I would suggest to avoid these changes. Or add only if the existing formatting is very bad. But would recommend to avoid.
Some indentations are bad, which needs correction for example having ); on a separate line.

=> I have not pointed out all the indentation corrections. Please do look at other similar changes too.

Another comment is regarding Parameterized test: modules/javafx.web/src/test/java/test/javafx/scene/web/SubresourceIntegrityTest.java
Looks like the test is not converted correctly. The parameters are never assigned to the member variables. Hence never tested. Please take a look at the specific comment on the test. (please check my reply to previous discussion)

modules/javafx.web/src/test/java/test/javafx/scene/web/BindingTest.java

modules/javafx.web/src/test/java/test/javafx/scene/web/CSSTest.java

modules/javafx.web/src/test/java/test/javafx/scene/web/CallbackTest.java

modules/javafx.web/src/test/java/test/javafx/scene/web/CanvasTest.java

modules/javafx.web/src/test/java/test/javafx/scene/web/WebViewTest.java

modules/javafx.web/src/test/java/test/javafx/scene/web/SubresourceIntegrityTest.java

arapte

LGTM

Maran23

looks good now

andy-goryachev-oracle

query for old imports produces 0 hits (good)
left some minor comments, one (assertArrayEquals) might need a code change

andy-goryachev-oracle · 2024-09-18T20:26:49Z

modules/javafx.web/src/test/java/test/com/sun/webkit/network/CookieTest.java

+        assertEquals("/foo", CookieShim.defaultPath(uri("http://hostname/foo/bar")));
+        assertEquals("/foo", CookieShim.defaultPath(uri("http://hostname/foo/bar?")));
+        assertEquals("/foo", CookieShim.defaultPath(uri("http://hostname/foo/bar?query")));
+        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.

andy-goryachev-oracle · 2024-09-18T20:45:56Z

modules/javafx.web/src/test/java/test/javafx/scene/web/FileReaderTest.java

+                int actualLength = (Integer) obj.getMember("length");
+                String message = String.format("File at %s: Expected length %d but got %d",
+                        filePath, expectedLength, actualLength);
+                assertEquals(expectedLength, actualLength, message);


I think all this new code can be replaced by assertArrayEquals

andy-goryachev-oracle · 2024-09-18T20:51:56Z

modules/javafx.web/src/test/java/test/javafx/scene/web/JavaScriptBridgeTest.java

    public void testcheckJSPeerTostring() {
        final JSObject doc = (JSObject) executeScript("document");
        loadContent("<h1></h1>");
-        submit(() -> {
-            getEngine().executeScript(doc.toString());
+        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)

andy-goryachev-oracle · 2024-09-18T21:06:14Z

modules/javafx.web/src/test/java/test/javafx/scene/web/MiscellaneousTest.java

@@ -150,7 +152,9 @@ public void start() {
           submit(obj::start);
           // Try accessing the resultObject created in JavaFX Application Thread
           // from someother thread. It should throw an exception.
-           obj.resultObject.toString();
+           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.

jaybhaskar · 2024-09-19T00:54:18Z

/integrate

openjdk · 2024-09-19T00:55:56Z

Going to push as commit e81b676.
Since your change was applied there have been 3 commits pushed to the master branch:

6d1dd29: 8211234: Open-source simple test programs for FX / Swing interop
f8a2005: 8211247: Open-source simple test programs for FX / SWT interop
5993c09: 8339513: [TestBug] Convert fxml tests to JUnit 5

Your commit was automatically rebased without conflicts.

openjdk · 2024-09-19T00:56:03Z

@jaybhaskar Pushed as commit e81b676.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8339515: [TestBug] Convert web tests to JUnit 5

86f7327

openjdk bot added the rfr Ready for review label Sep 15, 2024

Maran23 suggested changes Sep 15, 2024

View reviewed changes

copyright year updated along with minor spcae related review

27275c3

openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Sep 15, 2024

resolve conflict

23853ba

openjdk bot removed the merge-conflict Pull request has merge conflict with target branch label Sep 16, 2024

converting newly added tests to JUNIT5

af58da2

kevinrushforth reviewed Sep 16, 2024

View reviewed changes

modules/javafx.web/src/test/java/test/com/sun/webkit/SharedBufferTest.java Outdated Show resolved Hide resolved

jaybhaskar added 2 commits September 16, 2024 19:12

Revert "copyright year updated along with minor spcae related review"

fb4e76c

This reverts commit 27275c3.

revert copyright date

b1de60d

openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Sep 16, 2024

Merge branch 'master' into testsprint

780ef84

openjdk bot removed the merge-conflict Pull request has merge conflict with target branch label Sep 16, 2024

arapte suggested changes Sep 16, 2024

View reviewed changes

jaybhaskar added 2 commits September 16, 2024 22:05

remove old juini4 imports

7206657

Year change should be reverted as main

c7cb5cd

arapte suggested changes Sep 17, 2024

View reviewed changes

jaybhaskar added 2 commits September 17, 2024 17:40

Cleaning according review comments

990037a

remove instance variables in parametrized test

54575fe

arapte approved these changes Sep 18, 2024

View reviewed changes

openjdk bot added the ready Ready to be integrated label Sep 18, 2024

Maran23 approved these changes Sep 18, 2024

View reviewed changes

andy-goryachev-oracle reviewed Sep 18, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Sep 19, 2024

openjdk bot closed this Sep 19, 2024

openjdk bot removed ready Ready to be integrated rfr Ready for review labels Sep 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8339515: [TestBug] Convert web tests to JUnit 5 #1567

8339515: [TestBug] Convert web tests to JUnit 5 #1567

jaybhaskar commented Sep 15, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Sep 15, 2024

openjdk bot commented Sep 15, 2024 •

edited

Loading

mlbridge bot commented Sep 15, 2024 •

edited

Loading

Maran23 left a comment

Maran23 commented Sep 15, 2024

openjdk bot commented Sep 15, 2024

kevinrushforth left a comment

andy-goryachev-oracle commented Sep 16, 2024

arapte left a comment

kevinrushforth commented Sep 17, 2024

arapte left a comment •

edited

Loading

arapte left a comment

Maran23 left a comment

andy-goryachev-oracle left a comment

andy-goryachev-oracle Sep 18, 2024

andy-goryachev-oracle Sep 18, 2024

andy-goryachev-oracle Sep 18, 2024

andy-goryachev-oracle Sep 18, 2024

jaybhaskar commented Sep 19, 2024

openjdk bot commented Sep 19, 2024

openjdk bot commented Sep 19, 2024

8339515: [TestBug] Convert web tests to JUnit 5 #1567

8339515: [TestBug] Convert web tests to JUnit 5 #1567

Conversation

jaybhaskar commented Sep 15, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Sep 15, 2024

openjdk bot commented Sep 15, 2024 • edited Loading

mlbridge bot commented Sep 15, 2024 • edited Loading

Webrevs

Maran23 left a comment

Choose a reason for hiding this comment

Maran23 commented Sep 15, 2024

openjdk bot commented Sep 15, 2024

kevinrushforth left a comment

Choose a reason for hiding this comment

andy-goryachev-oracle commented Sep 16, 2024

arapte left a comment

Choose a reason for hiding this comment

kevinrushforth commented Sep 17, 2024

arapte left a comment • edited Loading

Choose a reason for hiding this comment

arapte left a comment

Choose a reason for hiding this comment

Maran23 left a comment

Choose a reason for hiding this comment

andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

andy-goryachev-oracle Sep 18, 2024

Choose a reason for hiding this comment

andy-goryachev-oracle Sep 18, 2024

Choose a reason for hiding this comment

andy-goryachev-oracle Sep 18, 2024

Choose a reason for hiding this comment

andy-goryachev-oracle Sep 18, 2024

Choose a reason for hiding this comment

jaybhaskar commented Sep 19, 2024

openjdk bot commented Sep 19, 2024

openjdk bot commented Sep 19, 2024

jaybhaskar commented Sep 15, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Sep 15, 2024 •

edited

Loading

mlbridge bot commented Sep 15, 2024 •

edited

Loading

arapte left a comment •

edited

Loading