8169285: Re-enable javafx.swt tests

[Maran23]

With this change, you can now run the swt tests as easy as: :swt:test -PSWT_TEST=true.

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, and one flag seems to be enough to me. But open for opinions on this one.

---

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]

👋 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]

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

[mlbridge]

Webrevs

• 02: Full - Incremental (eabc48a2)
• 01: Full - Incremental (a99f75ae)
• 00: Full (16935f94)

[Maran23]

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]

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]

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

[Maran23]

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

[kevinrushforth]

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]

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]

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

[kevinrushforth]

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]

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]

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:

1. 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).
2. 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]

@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.