slfan1989 commented on code in PR #1227:
URL: https://github.com/apache/ratis/pull/1227#discussion_r1988285326
##########
pom.xml:
##########
@@ -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>
Review 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
