8169285: Re-enable javafx.swt tests #1783

Maran23 · 2025-04-16T18:48:09Z

This PR enables javafx.swt tests for:

Windows

Disabled on:

Linux, since some distros seems to have problems
MacOS: The following warning is printed right before the error: ***WARNING: Display must be created on main thread due to Cocoa restrictions. Use vmarg -XstartOnFirstThread

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-8169285: Re-enable javafx.swt tests (Bug - P4)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1783

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-04-16T18:48:49Z

👋 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-16T18:49:47Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

mlbridge · 2025-04-16T18:53:19Z

Webrevs

Maran23 · 2025-04-16T19:00:53Z

Checking the GHA build, IS_FULL_TEST actually might be needed. Linux fails with org.eclipse.swt.SWTError: No more handles [gtk_init_check() failed] when creating the Display.

andy-goryachev-oracle · 2025-04-16T20:32:23Z

Linux fails with org.eclipse.swt.SWTError: No more handles [gtk_init_check() failed] when creating the Display.

does it fail with IS_FULL_TEST=true?

kevinrushforth

Note: At one point IS_FULL_TEST was used as well for the enablement of the tests. I don't see any reason for it.

All headful tests are qualified by IS_FULL_TEST, so this should remain here, too. We do the same thing when enabling robot tests, in that you still have to specify -PFULL_TEST=true.

kevinrushforth · 2025-04-16T20:26:17Z

build.gradle

-        //enabled = IS_FULL_TEST && IS_SWT_TEST
-        enabled = false // FIXME: JIGSAW -- support this with modules
-        logger.info("JIGSAW Testing disabled for swt")
+        enabled = IS_SWT_TEST


These tests are not headless tests so they still need to be qualified by IS_FULL_TEST

I tested that in another Branch and therefore GHA build and can confirm that as well! Linux is green with IS_FULL_TEST.

kevinrushforth · 2025-04-16T21:22:28Z

I was curious as to why the tests were being run on GHA without your setting the SWT_TEST flag, and then spotted this in build.gradle, line 502:

// Specifies whether to run system tests that depend on SWT (only used when FULL_TEST is also enabled)
defineProperty("SWT_TEST", "true")

So even though we have a flag, it is on by default (which makes me wonder why we bothered with a flag). If the tests are stable, which will need to be tested (I can do a CI build tomorrow), we can leave it enabled by default, in which case the only flag you would need is -PFULL_TEST=true.

kevinrushforth · 2025-04-16T21:28:05Z

we can leave it enabled by default, in which case the only flag you would need is -PFULL_TEST=true.

I realize my statement might be ambiguous. I am not suggesting further changes to build.gradle: you can leave it as:

    enabled = IS_FULL_TEST && IS_SWT_TEST

What I meant is that developers will not need to explicitly set SWT_TEST=true since it already is by default.

andy-goryachev-oracle · 2025-04-16T21:30:11Z

yes, we still need SWT_TEST so we can disable it on mac.

kevinrushforth · 2025-04-16T21:51:42Z

yes, we still need SWT_TEST so we can disable it on mac.

If that flag is the mechanism we need to use to disable it on macOS, then we will need the following additional change:

 // Specifies whether to run system tests that depend on SWT (only used when FULL_TEST is also enabled)
-defineProperty("SWT_TEST", "true")
+defineProperty("SWT_TEST", IS_MAC ? "false" ? "true")
 ext.IS_SWT_TEST = Boolean.parseBoolean(SWT_TEST);

kevinrushforth · 2025-04-16T22:00:01Z

yes, we still need SWT_TEST so we can disable it on mac.

If that flag is the mechanism we need to use to disable it on macOS, then we will need the following additional change:
 // Specifies whether to run system tests that depend on SWT (only used when FULL_TEST is also enabled)
-defineProperty("SWT_TEST", "true")
+defineProperty("SWT_TEST", IS_MAC ? "false" ? "true")
 ext.IS_SWT_TEST = Boolean.parseBoolean(SWT_TEST);

Never mind. There is already this:

        if (IS_MAC) {
            enabled = false
            logger.info("SWT tests are disabled on MAC, because Gradle test runner does not handle -XstartOnFirstThread properly (https://issues.gradle.org/browse/GRADLE-3290).")
        }

kevinrushforth · 2025-04-24T16:55:58Z

I confirm that this runs fine on my Windows 11 system.

@Maran23 what platforms did you test this on? Windows?

I fired off a CI headful test run, and I see the following failure on Ubuntu 22.04:

> Task :swt:test FAILED

SWTCursorsTest > testImageCursor() FAILED
    org.opentest4j.AssertionFailedError: expected: not <null>
        at app//org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:152)
        at app//org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
        at app//org.junit.jupiter.api.AssertNotNull.failNull(AssertNotNull.java:49)
        at app//org.junit.jupiter.api.AssertNotNull.assertNotNull(AssertNotNull.java:35)
        at app//org.junit.jupiter.api.AssertNotNull.assertNotNull(AssertNotNull.java:30)
        at app//org.junit.jupiter.api.Assertions.assertNotNull(Assertions.java:304)
        at app//test.javafx.embed.swt.SWTCursorsTest.lambda$testImageCursor$0(SWTCursorsTest.java:65)
        at app//org.eclipse.swt.widgets.RunnableLock.run(RunnableLock.java:40)
        at app//org.eclipse.swt.widgets.Synchronizer.runAsyncMessages(Synchronizer.java:132)
        at app//org.eclipse.swt.widgets.Display.runAsyncMessages(Display.java:5039)
        at app//org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:4519)
        at app//test.javafx.embed.swt.SWTCursorsTest.lambda$testImageCursor$1(SWTCursorsTest.java:71)
        at app//test.javafx.embed.swt.SWTTest.lambda$runOnSwtThread$0(SWTTest.java:52)
        at app//org.eclipse.swt.widgets.RunnableLock.run(RunnableLock.java:40)
        at app//org.eclipse.swt.widgets.Synchronizer.runAsyncMessages(Synchronizer.java:132)
        at app//org.eclipse.swt.widgets.Display.runAsyncMessages(Display.java:5039)
        at app//org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:4519)
        at app//test.javafx.embed.swt.SWTTest.runOnSwtThread(SWTTest.java:62)
        at app//test.javafx.embed.swt.SWTCursorsTest.testImageCursor(SWTCursorsTest.java:50)

3 tests completed, 1 failed

I recommend two things:

File a bug to track the testImageCursor test failure on Linux and then skip it using an assumeTrue (with the new bugID added as a comment).
Either remove the SWT_TEST flag, since it is always true and doesn't do anything useful, or leave it in place, change its default to "false" on macOS ("true" otherwise), and remove the check for macOS in the test {} block. We don't need two different ways to skip the tests on macOS.

Maran23 · 2025-04-28T09:35:25Z

@Maran23 what platforms did you test this on? Windows?

Yes, Windows 11.

change its default to "false" on macOS ("true" otherwise)

Yes this seems like a good idea, I think it is more clean to have the definition where the property is (with the conditional for MacOS), instead of the test{} block, as you also suggested.

I want to check the tests on MacOS (See if I can figure out why they fail) and Linux (checkout testImageCursor) if time permits, but I might not be able to make it.

andy-goryachev-oracle · 2025-04-30T15:10:25Z

is this PR description still valid?
there seems to be two GHA failures, are these intermittent or a result of the changes in this PR?

kevinrushforth · 2025-04-30T15:21:17Z

is this PR description still valid?

Good catch. I think the PR Description should be updated.

there seems to be two GHA failures, are these intermittent or a result of the changes in this PR?

They are unrelated intermittent failures. He needs to push another update anyway, to fix or skip one of the tests on Linux as I noted above, so it will rerun then.

Maran23 · 2025-04-30T21:53:38Z

I confirm that this runs fine on my Windows 11 system.

@Maran23 what platforms did you test this on? Windows?

I fired off a CI headful test run, and I see the following failure on Ubuntu 22.04:

I setup an Ubuntu 24.04.2 LTS inside WSL, did export GDK_BACKEND=x11 (otherwise I always got a crash).
The tests are always green for me. Tried several times.

kevinrushforth · 2025-05-01T13:54:49Z

I setup an Ubuntu 24.04.2 LTS inside WSL, did export GDK_BACKEND=x11 (otherwise I always got a crash).

Interesting. The crash is likely due to JDK-8340378. JavaFX glass calls putenv("GDK_BACKEND=x11") at initialization time, but this is likely too late in the case where FXCanvas is embedded in an SWT window. By that time SWT has already initialized the Gtk toolkit, so the env variable needs to be set earlier.

I will check whether this is the case by trying out HelloFXCanvas after first setting that variable. If so, then the fix for JDK-8340378 will be to set that env variable from HelloFXCanvas when running on Linux.

Getting back to this PR For the fix will likely be to modify the test to set that env variable when running on Linux. Although, that presumes the test is stable across multiple Linux platforms.

The tests are always green for me. Tried several times.

Since it works for you on Ubuntu 24.04, the failures I see might be specific to 22.04. We don't have a good way to check specific versions of Ubuntu Linux. I'll retest this when I have time to check the Wayland workaround on 24.04 and let you know.

kevinrushforth · 2025-05-01T14:46:21Z

I will check whether this is the case by trying out HelloFXCanvas after first setting that variable. If so, then the fix for JDK-8340378 will be to set that env variable from HelloFXCanvas when running on Linux.

Running HelloFXCanvas with GDK_BACKEND=x11 does prevent the crash. The SWT part of the app doesn't render well on my system, but that isn't a JavaFX issue.

I did a little more looking, and the SWT interop failure in Wayland mode was discovered and commented on back when we added the putenv in JDK-8210411 in this JBS comment. We should have filed a follow-on bug back then, but at least we have JDK-8340378 now.

Back to this PR: On my Ubuntu 24.04 VM it crashes without setting that env var and fails (hangs) when I do. Given the various problems, I recommend skipping all of the tests on Linux. The testing matrix will be a bit time consuming, and I'm not sure how important it really is.

The easiest way to skip on Linux might be in the build.gradle file when setting the SWT_TEST flag.

Maran23 · 2025-05-09T12:16:05Z

So after checking on MacOS, the following warning is printed right before it fails: ***WARNING: Display must be created on main thread due to Cocoa restrictions. Use vmarg -XstartOnFirstThread. After researching, it seems that there really is no other way than setting that flag.

I think we might wanna set it in a follow up and recheck, if it works?

kevinrushforth · 2025-05-09T19:35:47Z

Can you file follow-up bugs (one each) for the failures on macOS and Linux? Then add a comment in build.gradle as to why we are skipping the SWT tests on those platforms.

…swt-tests

8169285: Re-enable javafx.swt tests

16935f9

openjdk bot added the rfr Ready for review label Apr 16, 2025

kevinrushforth reviewed Apr 16, 2025

View reviewed changes

8169285: SWT tests should run with IS_FULL_TEST

a99f75a

kevinrushforth self-requested a review April 22, 2025 17:19

SWT_TEST default is false on MacOS

eabc48a

Maran23 added 3 commits May 11, 2025 00:37

SWT_TEST default is false on Linux

f091122

Merge branch 'master' of https://github.com/openjdk/jfx into 8169285-…

a0f0cb8

…swt-tests

link JDK tickets

158ac82

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8169285: Re-enable javafx.swt tests #1783

8169285: Re-enable javafx.swt tests #1783

Maran23 commented Apr 16, 2025 •

edited by openjdk bot

Loading

bridgekeeper bot commented Apr 16, 2025

openjdk bot commented Apr 16, 2025

mlbridge bot commented Apr 16, 2025 •

edited

Loading

Maran23 commented Apr 16, 2025 •

edited

Loading

andy-goryachev-oracle commented Apr 16, 2025

kevinrushforth left a comment

kevinrushforth Apr 16, 2025

Maran23 Apr 16, 2025

kevinrushforth commented Apr 16, 2025

kevinrushforth commented Apr 16, 2025

andy-goryachev-oracle commented Apr 16, 2025

kevinrushforth commented Apr 16, 2025 •

edited

Loading

kevinrushforth commented Apr 16, 2025

kevinrushforth commented Apr 24, 2025

Maran23 commented Apr 28, 2025 •

edited

Loading

andy-goryachev-oracle commented Apr 30, 2025

kevinrushforth commented Apr 30, 2025

Maran23 commented Apr 30, 2025

kevinrushforth commented May 1, 2025

kevinrushforth commented May 1, 2025

Maran23 commented May 9, 2025

kevinrushforth commented May 9, 2025

8169285: Re-enable javafx.swt tests #1783

Are you sure you want to change the base?

8169285: Re-enable javafx.swt tests #1783

Conversation

Maran23 commented Apr 16, 2025 • edited by openjdk bot Loading

Progress

Issue

Reviewing

bridgekeeper bot commented Apr 16, 2025

openjdk bot commented Apr 16, 2025

mlbridge bot commented Apr 16, 2025 • edited Loading

Webrevs

Maran23 commented Apr 16, 2025 • edited Loading

andy-goryachev-oracle commented Apr 16, 2025

kevinrushforth left a comment

Choose a reason for hiding this comment

kevinrushforth Apr 16, 2025

Choose a reason for hiding this comment

Maran23 Apr 16, 2025

Choose a reason for hiding this comment

kevinrushforth commented Apr 16, 2025

kevinrushforth commented Apr 16, 2025

andy-goryachev-oracle commented Apr 16, 2025

kevinrushforth commented Apr 16, 2025 • edited Loading

kevinrushforth commented Apr 16, 2025

kevinrushforth commented Apr 24, 2025

Maran23 commented Apr 28, 2025 • edited Loading

andy-goryachev-oracle commented Apr 30, 2025

kevinrushforth commented Apr 30, 2025

Maran23 commented Apr 30, 2025

kevinrushforth commented May 1, 2025

kevinrushforth commented May 1, 2025

Maran23 commented May 9, 2025

kevinrushforth commented May 9, 2025

Maran23 commented Apr 16, 2025 •

edited by openjdk bot

Loading

mlbridge bot commented Apr 16, 2025 •

edited

Loading

Maran23 commented Apr 16, 2025 •

edited

Loading

kevinrushforth commented Apr 16, 2025 •

edited

Loading

Maran23 commented Apr 28, 2025 •

edited

Loading