-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
[Improvement][UT] Upgrade jUnit to 5.+ (#10976) #11332
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #11332 +/- ##
=========================================
Coverage 38.67% 38.67%
Complexity 4005 4005
=========================================
Files 1002 1002
Lines 37216 37216
Branches 4251 4251
=========================================
Hits 14394 14394
Misses 21187 21187
Partials 1635 1635 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
...api/src/test/java/org/apache/dolphinscheduler/api/controller/TaskInstanceControllerTest.java
Outdated
Show resolved
Hide resolved
Please check the sonar result |
BTW, there are two ways to fully migrate those tests from jUnit 4 to jUnit 5:
However, I think the second method is a bit risky. |
I prefer the second way. |
I just tried migrating all the UTs and got blocked. Specifically speaking, there are two risks:
The real issue blocking me is the second one. May I ask whether there is a good way, or some kind of workaround for the second point? @ruanwenjun @kezhenxu94 |
I just worry if we still keep the Junit4, we need to constantly remind contributors to use Junit5. |
This is totally OK to me, "migrations" are always huge changes...
I hope we could finally get rid of powermock one day, did you find any alternative to Powermock in JUnit5 platform? Or it's just impossible? |
Agree, same concern to me |
9f0d4d0
to
83ecf00
Compare
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
@ruanwenjun @kezhenxu94 @caishunfeng I have the same worries that However, even though I have removed the dependency of About For the rest of the legacy tests, we could migrate some of them easily with Above all, there are two action items after this PR, if you agree:
Thanks |
@zhongjiajie The workload to migrate all of them is quite large. Currently, because of those |
@ruanwenjun @kezhenxu94 BTW, for the concern that contributors may write UTs with |
Powermock is getting more and more annoying such as https://github.com/apache/dolphinscheduler/runs/7929368593?check_suite_focus=true I'd rather just review and remove all those tests involving Powermock... |
@kezhenxu94 To follow up on this comment, I did some experiments on replacing For some more difficult cases such as:
We could: |
Good to hear we can remove powermock 🎉 |
8c27640
to
a1ad044
Compare
a1ad044
to
ae6dd2e
Compare
SonarCloud Quality Gate failed. |
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, in this PR we upgrade the unit version to 5.9.0
, and we still keep the powermock(Hope we can remove powermock in the later pr).
@ruanwenjun Certainly we will remove |
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.
+1
* [Improvement][UT] Upgrade jUnit to 5.+ (apache#10976) * Refactor AlertServerTest with jUnit-5 as an example
Purpose of the pull request
AlertServerTest
with jUnit 5 as an example.Brief change log
Verify this pull request