-
Notifications
You must be signed in to change notification settings - Fork 471
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
8339510: [TestBug] Convert system tests to JUnit 5 #1569
8339510: [TestBug] Convert system tests to JUnit 5 #1569
Conversation
👋 Welcome back angorya! A progress list of the required criteria for merging this PR into |
@andy-goryachev-oracle 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:
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 5 new commits pushed to the
Please see this link for an up-to-date comparison between the source branch of this pull request and the ➡️ To integrate this PR with the above commit message to the |
/reviewers 2 |
@andy-goryachev-oracle |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I spot checked many of the classes, and most of the changes look reasonable.
One question, though: Why did you replace the per-test timeout with an in-method annotation in many places (especially in the early classes I looked at)? There is a direct replacement in JUnit 5 for the per-test timeout, which seems a more straightforward change. I also think an explicit timeout on the test (or class, if all @Test
methods have the same timeout) will work more consistently with my in-progress fix to add a global timeout.
tests/system/src/test/java/test/com/sun/javafx/application/ConcurrentStartupTest.java
Outdated
Show resolved
Hide resolved
tests/system/src/test/java/test/com/sun/javafx/application/InitializeJavaFXLaunch1Test.java
Outdated
Show resolved
Hide resolved
Testing recommendation: do a test run with |
there were more. I don't have this warning enabled in Eclipse because, if enabled, I see 410+ such warnings. but thank you for noticing - I did forget to optimize imports in a few places. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes you made look good. I still see some unexpected test failures in the (unstable) monocle tests suite. I ran the following command:
gradle --continue --info -PTEST_ONLY=true -PFULL_TEST=true -PUSE_ROBOT=true -PUNSTABLE_TEST=true :systemTests:test --tests "test.robot.com.sun.glass.ui.monocle*"
On master, I get:
1773 tests : 56 failures : 495 ignored
With this PR, I get:
1326 tests : 422 failures: 12 ignored
I note that we are actually running more tests than before: 1773-495=1278
vs 1326-12=1314
As for the failures, since this is an unstable set of tests, the ones that timeout aren't a concern; which ones fail may vary slightly from run to run so a small difference is OK. In this case, the number of additional failures is much higher than I would expect: 56 vs 422.
The ones that fail with an exception other than a timeout suggest a problem with the conversion of parameterized tests. See InputDevicePropertyTest
, for one example. All 45 tests pass with master and all 45 fail with a "Toolkit not initialized" with this patch.
}, null); | ||
// Should throw IllegalStateException | ||
tmpScene.snapshot(p -> { | ||
throw new RuntimeException("Should never get here"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, throw new AssertionError
seems the most equivalent change, in that it preserves the behavior of throwing an Error
rather than a RuntimeException
-- typically an Error is better for a case of "can't get here". I'll admit that it doesn't matter in this particular test, so it's just a suggestion. Feel free to ignore it.
Assertions.assertEquals(0, ((sp.snappedTopInset() * scale) + epsilon) % 1, 0.0001, "Top inset not snapped to pixel"); | ||
Assertions.assertEquals(0, ((sp.snappedBottomInset() * scale) + epsilon) % 1, 0.0001, "Bottom inset not snapped to pixel"); | ||
Assertions.assertEquals(0, ((sp.snappedLeftInset() * scale) + epsilon) % 1, 0.0001, "Left inset not snapped to pixel"); | ||
Assertions.assertEquals(0, ((sp.snappedRightInset() * scale) + epsilon) % 1, 0.0001, "Right inset not snapped to pixel"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, don't worry about the line length, that was just an observation.
I do prefer having the class qualifier over static import (though static imports make more sense in the tests).
So do I in most cases. For tests, I prefer static imports of the methods in Assertions
and Assumptions
, which is also the pattern recommended by the JUnit docs.
Speaking of that, I noticed that the number of failed tests varies between invocations on the same machine. For example with RotateTest on macOS running the command Examples:
3 second timeout seems to be sufficient for the purpose, and I don't see any output captured by the TestLogShim. I haven't looked into the actual tests for long since it's out of scope, but maybe I am doing something wrong. |
Side note: TeshLogShim.log appears to be improperly synchronized in getLog(), countLog(), checkLog(), checkLogContaining(), but even when synchronization is fixed the tests still randomly fail... |
The latest change look correct and fix most of the failures where the entire test class was failing. I still see a discrepancy between number of tests run before / after and there are more failures now. On master, I get: 1773 tests : 56 failures : 495 ignored With the latest version of this PR, I get: 1326 tests : 101 failures: 78 ignored So we are now running fewer tests than before: |
so on macOS, running this command
I got 2274 tests completed, 51 failed, 130 skipped I'll run the same command against master next week. edit: gives a difference of 30 tests lost somewhere. will investigate and report. |
Not sure why I'm seeing something different then. I tried running it on a second macOS system today (macOS 13.x Intel) and got similar results to my earlier runs. I ran the following command to just run the monocle tests:
master1773 tests : 56 failures : 495 ignored -- tests run: 1773-495=1278 this PR1326 tests : 106 failures: 78 ignored -- tests run: 1326-78=1248 |
@@ -255,7 +262,7 @@ public void testBadSceneCallback2() { | |||
Callback cb = (Callback<String, Integer>) param -> { | |||
// Should not get here | |||
latch.countDown(); | |||
throw new AssertionFailedError("Should never get here"); | |||
throw new AssertionError("Should never get here"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should probably be fail
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
won't compile with fail()
, needs an exception
@@ -381,7 +384,7 @@ public void testBadNodeCallback2() { | |||
Callback cb = (Callback<String, Integer>) param -> { | |||
// Should not get here | |||
latch.countDown(); | |||
throw new AssertionFailedError("Should never get here"); | |||
throw new AssertionError("Should never get here"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should probably be fail
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
won't compile with fail()
, needs an exception
The latest commit fixes the number of tests issue. Note: we should not look at the number of tests reported by gradle at the end of the test run, instead we should look at the HTML report. master: tests=2728, failures=49, ignored=554 (tests - ignored = 2174) The actual number of failed tests varies from run to run, and also, being "unstable", differs between master and PR. For example:
I also noticed a bug in USKeyboardTest, see https://bugs.openjdk.org/browse/JDK-8340693 |
I ran the latest and I now see the missing 30 tests being run.
master1773 tests : 56 failures : 495 ignored -- tests run: 1773-495=1278 this PRrun 1: 1326 tests : 72 failures: 48 ignored -- tests run: 1326-48=1278 All good. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/integrate |
Going to push as commit 21601f8.
Your commit was automatically rebased without conflicts. |
@andy-goryachev-oracle Pushed as commit 21601f8. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Converting system tests to junit5.
Please see migration notes:
https://github.com/andy-goryachev-oracle/Test/blob/main/doc/Tests/JUnit5Migration.md
Notes:
I see shutdown timeout on linux, this will be addressed by JDK-8340403
This test might fail intermittently (?) on linux only:
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1569/head:pull/1569
$ git checkout pull/1569
Update a local copy of the PR:
$ git checkout pull/1569
$ git pull https://git.openjdk.org/jfx.git pull/1569/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1569
View PR using the GUI difftool:
$ git pr show -t 1569
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1569.diff
Webrev
Link to Webrev Comment