RATIS-2251. Migrate ratis-test tests to Junit 5 - Part 3. #1227
Conversation
There was a problem hiding this comment.
@slfan1989 , thanks a lot for working on this! The change looks good in general. Let's also clean up TestReConfigProperty; see https://issues.apache.org/jira/secure/attachment/13074931/1227_review.patch
BTW, there are test failures. Please take a look.
| Assertions.assertFalse( | ||
| exceptionCaught, "received unexpected exception"); |
There was a problem hiding this comment.
Let's use fail(..) inside the catch-block instead of assertFalse(..).
There was a problem hiding this comment.
@szetszwo Thank you very much for reviewing this PR and providing suggestions! I will continue to improve this PR.
There was a problem hiding this comment.
I need some time to validate the unit tests for the issue and will provide a response as soon as possible.
|
Not sure why it has IllegalStateException in https://github.com/apache/ratis/actions/runs/13490213066/job/37726235717?pr=1227#step:11:886 |
@szetszwo Thank you for your message. I am currently conducting local tests to identify the cause of the issue. |
|
@slfan1989 , Thanks for the update! The subclass tests related to |
|
@szetszwo Thank you very much for your message! I will attempt to roll back these two classes and continue testing locally. I have upgraded the JUnit dependency and the version of the surefire-plugin. The previous java.lang.IllegalStateException error is no longer occurring, and I will continue testing today. |
|
I have carefully reviewed the CI report, and these errors are likely unrelated to the JUnit 5 upgrade. The same errors existed in JUnit 4 as well and can still be reproduced locally. Let me summarize the current situation regarding the unit test errors.
Junit4 Success.
|
|
@slfan1989 , after RATIS-2124 is merged, we need to resolve the conflicts here. Thanks. |
@szetszwo Thank you for your message! I will continue to follow up on this PR. |
There was a problem hiding this comment.
@slfan1989 , thanks a lot for you hard work! Not sure why the unit tests keep failing here but not the other PRs such as #1228 . Let's try updating surefire to see if it make a difference.
| @@ -174,7 +174,7 @@ | |||
| <maven-pdf-plugin.version>1.6.1</maven-pdf-plugin.version> | |||
| <maven-remote-resources-plugin.version>3.3.0</maven-remote-resources-plugin.version> | |||
| <maven-shade-plugin.version>3.6.0</maven-shade-plugin.version> | |||
| <maven-surefire-plugin.version>3.0.0-M4</maven-surefire-plugin.version> | |||
| <maven-surefire-plugin.version>3.0.0-M9</maven-surefire-plugin.version> | |||
There was a problem hiding this comment.
How about try updating it to the latest 3.5.2?
https://maven.apache.org/surefire/maven-surefire-plugin/usage.html
There was a problem hiding this comment.
I tried updating surefire to 3.5.2 (RATIS-2257), but had some problems:
Failed to execute goal org.apache.maven.plugins:maven-surefire-plugin:3.5.2:test (default-test) on project ratis-docs: groups/excludedGroups require TestNG, JUnit48+ or JUnit 5 (a specific engine required on classpath) on project test classpath
Failed to execute goal org.apache.maven.plugins:maven-surefire-plugin:3.5.2:test (default-test) on project ratis-proto: groups/excludedGroups require TestNG, JUnit48+ or JUnit 5 (a specific engine required on classpath) on project test classpath
These modules have skipTests=true, so I don't understand why surefire wants to run tests. Also tried with maven.test.skip=true, which should even skip test compilation, but the problem still happened.
It would be great if you can make it work.
adoroszlai@d53a586
https://github.com/adoroszlai/ratis/actions/runs/13670191390
There was a problem hiding this comment.
@adoroszlai , thanks for checking! It succeeded when running it locally but it failed in GitHub https://github.com/szetszwo/ratis/actions/runs/13725256050
It may be a problem in our githut action configuration.
There was a problem hiding this comment.
The error says it's related to using excludedGroups:
groups/excludedGroups require TestNG, JUnit48+ or JUnit 5 (a specific engine required on classpath) on project test classpath
which is set in CI:
Line 1122 in d740e51
This should not happen with skipTests, which we set for ratis-docs here:
Lines 31 to 32 in d740e51
There was a problem hiding this comment.
Added echo to print out the command
mvnw -V -B -Dsurefire.rerunFailingTestsCount=5 -Dsurefire.timeout=1200 --fail-at-end -Djacoco.skip test -Pflaky-tests
Now, I can reproduce it locally using the above command.
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 13.154 s
[INFO] Finished at: 2025-03-10T17:51:38-07:00
[INFO] ------------------------------------------------------------------------
[INFO] 42 goals, 42 executed
[INFO] Build cache (/Users/szetszwo/.m2/.develocity/build-cache/v1) removing files not accessed on or after Mon Mar 03 16:51:38 PST 2025.
[INFO] Build cache (/Users/szetszwo/.m2/.develocity/build-cache/v1) cleanup deleted 0 files/directories.
[INFO] Build cache (/Users/szetszwo/.m2/.develocity/build-cache/v1) cleaned up in 0.003 secs.
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-surefire-plugin:3.5.2:test (default-test) on project ratis-docs: groups/excludedGroups require TestNG, JUnit48+ or JUnit 5 (a specific engine required on classpath) on project test classpath -> [Help 1]
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-surefire-plugin:3.5.2:test (default-test) on project ratis-proto: groups/excludedGroups require TestNG, JUnit48+ or JUnit 5 (a specific engine required on classpath) on project test classpath -> [Help 1]
There was a problem hiding this comment.
@szetszwo @adoroszlai Thank you for the discussion! it has been very helpful to me. However, I would like to share my personal thoughts. Based on my own debugging, I confirm that there are issues with the current unit tests. Whether or not JUnit 5 is used, errors still occur.
I haven’t debugged all the unit tests, but I did focus on TestInstallSnapshotNotificationWithGrpc.testAddNewFollowersNoSnapshot. This unit test seems to have been affected by code changes in subsequent commits, which led to the error. When I rolled back to a much earlier version, I found that this test passed without issues.
Additionally, thank you for providing https://github.com/apache/ratis/actions/runs/13530776533/job/37812219666?pr=1228#step:11:923.
However, in the test report, we noticed that TestInstallSnapshotNotificationWithGrpc didn’t actually run. It seems that after TestRaftWithGrpc, the unit tests were interrupted.
I plan to submit a PR that triggers unit tests. We can use this PR to confirm whether the unit tests are working properly.
The upgrade of the Surefire plugin is indeed challenging. In the Hadoop project, we attempted to upgrade twice but encountered some issues both times(Many unit tests were skipped.). Therefore, I think it’s best to handle this separately, as doing everything at once could make things more complicated.
My suggestion is that if a PR triggering unit tests shows errors in the same class, we can temporarily disable the problematic tests and continue with the JUnit 5 upgrade. After that, we can address these failing tests one by one. Is this approach acceptable?
There was a problem hiding this comment.
Please do not disable tests.
- Trigger repeated run to confirm flakiness and establish failure rate.
- Create Jira issue to record the problem.
- Mark test as
@Flaky, referencing the Jira issue.
@Flaky tests are repeated automatically in CI.
Note, it does not help for tests failing due to leaks, as state of leak detector is not reset between retries.
There was a problem hiding this comment.
I agree with your suggestion, and I will continue to follow up on solving the unit testing issue.
What changes were proposed in this pull request?
Trying to upgrade ratis-server's Junit4 unit tests to Junit5.
What is the link to the Apache JIRA
JIRA: RATIS-2251. Migrate ratis-test tests to Junit 5 - Part 3.
How was this patch tested?
Junit Test & mvn clean test.