8354455: [TestBug] Remove JUnit Vintage Engine with JUnit 4 by Maran23 · Pull Request #1774 · openjdk/jfx

Maran23 · 2025-04-11T20:36:48Z

These are the remaining bits and pieces in order to completely remove the JUnit Vintage Engine, and therefore JUnit 4 from JavaFX.
After that, we should either document, that JUnit5 is used (just as information) or close JDK-8296284 (Since you can not use JUnit4 anymore).

This also removes the org.hamcrest dependency, which is actually only used in one test where it can easily be replaced by a Predicate (instead of Matcher). I'm not convinced that we should keep that dependency for just one test.

Note: I could not get the :swt tests to compile/test, in order to run the tests.
I need some guidance here how to instruct Gradle to compile this module (and if I need anything else? like swt?).

~~Also, this probably needs an extra ticket in the JBS?~~

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-8354455: [TestBug] Remove JUnit Vintage Engine with JUnit 4 (Enhancement - P4)

Reviewers

Andy Goryachev (@andy-goryachev-oracle - Reviewer)
Kevin Rushforth (@kevinrushforth - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1774

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-04-11T20:36:57Z

👋 Welcome back mhanl! 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 · 2025-04-11T20:37:50Z

@Maran23 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:

8354455: [TestBug] Remove JUnit Vintage Engine with JUnit 4

Reviewed-by: angorya, kcr

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 2 new commits pushed to the master branch:

5a897ab: 8354318: freetype.c: compilation error: 'incompatible pointer type' with gcc 14
fcdccd9: 8277000: Tree-/TableRowSkin: replace listener to fixedCellSize by live lookup

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 · 2025-04-11T20:44:39Z

Webrevs

kevinrushforth · 2025-04-11T21:01:13Z

@Maran23 We don't integrate code using the JBS ID of an umbrella tasks. If there is a need for a follow-on,
we would file a new Bug or Enhancement issue in JBS and link it to the umbrella.

For this specific case, this PR may be premature. We have closed tests that still use JUnit 4, and this might break our build. I'll check next week.

/reviewers 2 reviewers

openjdk · 2025-04-11T21:02:47Z

@kevinrushforth
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 2 Reviewers).

kevinrushforth · 2025-04-11T21:07:14Z

For this specific case, this PR may be premature.

To be more precise, it's the changes in build.gradle I'm concerned about. The rest looks like good cleanup (although will need review and testing).

andy-goryachev-oracle · 2025-04-11T21:11:57Z

+import static org.junit.jupiter.api.Assertions.assertNotNull;

-    @Rule
-    public SwtRule ctx = new SwtRule();


this looks like a breaking change - what would be the junit5 equivalent of @Rule?

@ExtendWith, but I think there is no equivalent of a method rule. In any case, another SWT test seems to be just fine without it, so I adapted the pattern to all other tests. We need to check if that works though.

So this was interesting, turns out it wasn't that stable after all even before!
With some adjustments to the Gradle file, I could make the tests compile and run and they are now green.

kevinrushforth · 2025-04-11T21:16:33Z

Note: I could not get the :swt tests to compile/test, in order to run the tests.
I need some guidance here how to instruct Gradle to compile this module (and if I need anything else? like swt?).

The build downloads the needed SWT libraries into build/libs/swt-debug.jar. The swt tests are disabled by default. You could try running the tests with -PSWT_TEST=true although that might not be sufficient, since it's been a while since anyone has run them.

Also, this probably needs an extra ticket in the JBS?

Yes. Please file one and link it in JBS as "Blocks" JDK-8339170
. Then change the title of this PR to the new bug.

Maran23 · 2025-04-11T21:37:08Z

@Maran23 We don't integrate code using the JBS ID of an umbrella tasks. If there is a need for a follow-on, we would file a new Bug or Enhancement issue in JBS and link it to the umbrella.

I thought so, will do!
If needed, we can also split this PR later on, I'm completely open for that.

kevinrushforth · 2025-04-12T14:58:07Z

For this specific case, this PR may be premature. We have closed tests that still use JUnit 4, and this might break our build. I'll check next week.

A quick initial test suggests that this will not be a problem after all. We do not use the gradle-downloaded JUnit 4 jar files in our closed tests. I will need to do a full test run next week to confirm this, but so far this looks OK.

andy-goryachev-oracle

The test run was successful on my macOS 15.4 running this command:

(git reflog -1; gradle --continue -PFULL_TEST=true -PUSE_ROBOT=true -PSWT_TEST=true cleanTest test --no-daemon -x :web:test --info 2>&1) | tee ~/`date +"test-%Y-%m%d-%H%M%S"`.log

andy-goryachev-oracle · 2025-04-14T17:00:34Z

-import static org.junit.Assume.assumeFalse;
 import static org.junit.jupiter.api.Assertions.fail;
+import static org.junit.jupiter.api.Assumptions.assumeFalse;
+


oops, my mistake! thanks for correcting it.

andy-goryachev-oracle · 2025-04-14T17:54:17Z

+        display = Display.getDefault();
+    }
+
+    protected final void runOnSwtThread(Runnable runnable) throws Throwable {


I like this solution - does not depend on whimsy of junit.

andy-goryachev-oracle · 2025-04-14T20:29:32Z

... however, my test run is not indicative because of this:

SWT tests are disabled on MAC, because Gradle test runner does not handle -XstartOnFirstThread properly (https://issues.gradle.org/browse/GRADLE-3290).

we probably need to enable the SWT tests on mac because the issue seems to have been resolved in 2021:

gradle/gradle#864

andy-goryachev-oracle · 2025-04-14T22:09:29Z

Tried to enable swt tests on macOS (build.gradle lines 3085, 3096), got three tests failed:

[FXCanvasScaledTest](https://github.com/openjdk/jfx/pull/classes/test.javafx.embed.swt.FXCanvasScaledTest.html). [initializationError](https://github.com/openjdk/jfx/pull/classes/test.javafx.embed.swt.FXCanvasScaledTest.html#initializationError)
[FXCanvasTest](https://github.com/openjdk/jfx/pull/classes/test.javafx.embed.swt.FXCanvasTest.html). [initializationError](https://github.com/openjdk/jfx/pull/classes/test.javafx.embed.swt.FXCanvasTest.html#initializationError)
[SWTCursorsTest](https://github.com/openjdk/jfx/pull/classes/test.javafx.embed.swt.SWTCursorsTest.html). [initializationError](https://github.com/openjdk/jfx/pull/classes/test.javafx.embed.swt.SWTCursorsTest.html#initializationError)

all in SWTTest.beforeAll():

org.eclipse.swt.SWTException: Invalid thread access
	at app//org.eclipse.swt.SWT.error(SWT.java:4918)
	at app//org.eclipse.swt.SWT.error(SWT.java:4833)
	at app//org.eclipse.swt.SWT.error(SWT.java:4804)
	at app//org.eclipse.swt.widgets.Display.error(Display.java:1209)
	at app//org.eclipse.swt.widgets.Display.createDisplay(Display.java:960)
	at app//org.eclipse.swt.widgets.Display.create(Display.java:944)
	at app//org.eclipse.swt.graphics.Device.<init>(Device.java:132)
	at app//org.eclipse.swt.widgets.Display.<init>(Display.java:798)
	at app//org.eclipse.swt.widgets.Display.<init>(Display.java:789)
	at app//org.eclipse.swt.widgets.Display.getDefault(Display.java:1543)
	at app//test.javafx.embed.swt.SWTTest.beforeAll(SWTTest.java:43)
	at java.base@23.0.1/java.lang.reflect.Method.invoke(Method.java:580)
	at java.base@23.0.1/java.util.ArrayList.forEach(ArrayList.java:1597)

so probably need to wrap SWTTest:43 in runOnSwtThread().

andy-goryachev-oracle

a problem found in a doctored macOS test, I wonder if we'll see the same issue on windows/linux?

kevinrushforth · 2025-04-14T22:17:58Z

a problem found in a doctored macOS test, I wonder if we'll see the same issue on windows/linux?

The SWT tests have never worked on macOS as far as I know -- even before JDK 9 modularization when they were disabled on all platforms.

Getting these tests to run on macOS seems better for a follow-up.

andy-goryachev-oracle · 2025-04-14T22:24:26Z

Getting these tests to run on macOS seems better for a follow-up.

Quite right.

The exception I see might indicate a problem even there - the exception is coming from Display.getDefault(), and runOnSwtThread uses the display to use its asyncExec() method, so I am not sure how this might work.

I'll do another run on the windows platform tomorrow.

andy-goryachev-oracle · 2025-04-15T16:52:26Z

Re-tested on windows, using the same command:

(git reflog -1; gradle --continue -PFULL_TEST=true -PUSE_ROBOT=true -PSWT_TEST=true cleanTest test --no-daemon -x :web:test --info 2>&1) | tee ~/`date +"test-%Y-%m%d-%H%M%S"`.log

I don't see swt tests being run, here is relevant grep:

> Task :swt:testClasses
Skipping task ':swt:testClasses' as it has no actions.
Resolve mutations for :swt:test (Thread[#88,Execution worker Thread 7,5,main]) started.
:swt:test (Thread[#88,Execution worker Thread 7,5,main]) started.
> Task :swt:test SKIPPED
Skipping task ':swt:test' as task onlyIf 'Task is enabled' is false.

what am I doing wrong?

Maran23 · 2025-04-15T19:19:38Z

SWT tests are disabled on MAC, because Gradle test runner does not handle -XstartOnFirstThread properly (https://issues.gradle.org/browse/GRADLE-3290).

Yeah I saw this as well and looked it up, may worth to do as follow-up, as Kevin already suggested.

...got three tests failed:

Weird. I just rechecked and they run for me. On Windows 11, command: ./gradlew :swt:test -PSWT_TEST=true

what am I doing wrong?

I also could not make it work, so I just modified the build.gradle and set various enabled flag to true, that did it for me.

andy-goryachev-oracle

Re-ran the tests on windows:

enable swt in build.gradle:3085
using command gradlew :swt:test -PSWT_TEST=true
all three swt tests pass.

kevinrushforth · 2025-04-15T22:09:36Z

Re-ran the tests on windows:

enable swt in build.gradle:3085

using command gradlew :swt:test -PSWT_TEST=true
all three swt tests pass.

I did the same, and all three pass for me, too.

Btw, there is already a follow-up bug to re-enable the SWT tests, JDK-8169285, and it seems that we might be ready to do that after this PR is integrated. Depending on how hard it is to get the tests running on macOS, we might want a separate follow-up for that.

kevinrushforth

The changes look good. I note that there is a missing comma in the copyright line of the newly added file. I'll reapprove once you correct this.

All my testing is green, including closed tests.

kevinrushforth · 2025-04-15T22:14:11Z

@@ -0,0 +1,72 @@
+/*
+ * Copyright (c) 2025 Oracle and/or its affiliates. All rights reserved.


You need a comma after 2025

Thanks, changed.

kevinrushforth · 2025-04-15T22:32:38Z

One option for JDK-8169285 would be to enable it -- optionally on the SWT_TEST flag -- as part of this PR. If you choose to do that, add that issue to this PR with/issue add.

Maran23 · 2025-04-16T08:22:10Z

One option for JDK-8169285 would be to enable it -- optionally on the SWT_TEST flag -- as part of this PR. If you choose to do that, add that issue to this PR with/issue add.

I think this is a good idea! I can check it out, but will do after this PR.

kevinrushforth

Looks good. Since the change to add the missing comma is trivial, a single re-review is sufficient.

/reviewers 1

openjdk · 2025-04-16T11:47:24Z

@kevinrushforth
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 1 (with at least 1 Reviewer).

Maran23 · 2025-04-16T17:30:48Z

/integrate

openjdk · 2025-04-16T17:31:57Z

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

5a897ab: 8354318: freetype.c: compilation error: 'incompatible pointer type' with gcc 14
fcdccd9: 8277000: Tree-/TableRowSkin: replace listener to fixedCellSize by live lookup

Your commit was automatically rebased without conflicts.

openjdk · 2025-04-16T17:32:06Z

@Maran23 Pushed as commit 367a170.

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

JDK-8339170: ☂ Convert tests to JUnit 5

60262b5

openjdk Bot changed the title ~~JDK-8339170: ☂ Convert tests to JUnit 5~~ 8339170: ☂ Convert tests to JUnit 5 Apr 11, 2025

openjdk Bot added the rfr Ready for review label Apr 11, 2025

kevinrushforth self-requested a review April 11, 2025 21:02

andy-goryachev-oracle reviewed Apr 11, 2025

View reviewed changes

Maran23 changed the title ~~8339170: ☂ Convert tests to JUnit 5~~ 8354455: [TestBug] Remove JUnit Vintage Engine with JUnit 4 Apr 11, 2025

Fix SWT Tests

dc7925f

andy-goryachev-oracle approved these changes Apr 14, 2025

View reviewed changes

andy-goryachev-oracle suggested changes Apr 14, 2025

View reviewed changes

andy-goryachev-oracle approved these changes Apr 15, 2025

View reviewed changes

kevinrushforth approved these changes Apr 15, 2025

View reviewed changes

openjdk Bot added the ready Ready to be integrated label Apr 15, 2025

Missing comma in copyright

56ac17a

openjdk Bot removed the ready Ready to be integrated label Apr 16, 2025

kevinrushforth approved these changes Apr 16, 2025

View reviewed changes

openjdk Bot added the ready Ready to be integrated label Apr 16, 2025

andy-goryachev-oracle approved these changes Apr 16, 2025

View reviewed changes

openjdk Bot added the integrated Pull request has been integrated label Apr 16, 2025

openjdk Bot closed this Apr 16, 2025

openjdk Bot removed ready Ready to be integrated rfr Ready for review labels Apr 16, 2025

Maran23 deleted the jdk-8339170-full-junit-5 branch October 13, 2025 11:56

Maran23 mentioned this pull request Apr 29, 2026

Fix typo: county -> country. Improve Javadoc @link with WritesDefaultLocale junit-team/junit-framework#5644

Merged

6 tasks

		@@ -0,0 +1,72 @@
		/*
		* Copyright (c) 2025 Oracle and/or its affiliates. All rights reserved.

Conversation

Maran23 commented Apr 11, 2025 • edited by openjdk Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper Bot commented Apr 11, 2025

Uh oh!

openjdk Bot commented Apr 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mlbridge Bot commented Apr 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

kevinrushforth commented Apr 11, 2025

Uh oh!

openjdk Bot commented Apr 11, 2025

Uh oh!

kevinrushforth commented Apr 11, 2025

Uh oh!

andy-goryachev-oracle Apr 11, 2025

Choose a reason for hiding this comment

Uh oh!

Maran23 Apr 11, 2025

Choose a reason for hiding this comment

Uh oh!

Maran23 Apr 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevinrushforth commented Apr 11, 2025

Uh oh!

Maran23 commented Apr 11, 2025

Uh oh!

kevinrushforth commented Apr 12, 2025

Uh oh!

andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

Uh oh!

andy-goryachev-oracle Apr 14, 2025

Choose a reason for hiding this comment

Uh oh!

andy-goryachev-oracle Apr 14, 2025

Choose a reason for hiding this comment

Uh oh!

andy-goryachev-oracle commented Apr 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andy-goryachev-oracle commented Apr 14, 2025

Uh oh!

andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

Uh oh!

kevinrushforth commented Apr 14, 2025

Uh oh!

andy-goryachev-oracle commented Apr 14, 2025

Uh oh!

andy-goryachev-oracle commented Apr 15, 2025

Uh oh!

Maran23 commented Apr 15, 2025

Uh oh!

andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

Uh oh!

kevinrushforth commented Apr 15, 2025

Uh oh!

kevinrushforth left a comment

Choose a reason for hiding this comment

Uh oh!

kevinrushforth Apr 15, 2025

Choose a reason for hiding this comment

Uh oh!

Maran23 Apr 16, 2025

Choose a reason for hiding this comment

Uh oh!

kevinrushforth commented Apr 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Maran23 commented Apr 11, 2025 •

edited by openjdk Bot

Loading

openjdk Bot commented Apr 11, 2025 •

edited

Loading

mlbridge Bot commented Apr 11, 2025 •

edited

Loading

Maran23 Apr 12, 2025 •

edited

Loading

andy-goryachev-oracle commented Apr 14, 2025 •

edited

Loading

kevinrushforth commented Apr 15, 2025 •

edited

Loading