Codecov Report

Merging #11332 (955e8eb) into dev (955e8eb) will not change coverage.
The diff coverage is n/a.

❗ Current head 955e8eb differs from pull request most recent head ae6dd2e. Consider uploading reports for the commit ae6dd2e to get more accurate results

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

SonarCloud Quality Gate failed.   

21 Bugs 9 Vulnerabilities 37 Security Hotspots 2101 Code Smells

38.0% Coverage 2.4% Duplication

Please check the sonar result

BTW, there are two ways to fully migrate those tests from jUnit 4 to jUnit 5:

1. We keep both versions of jUnit dependencies and migrate those UTs gradually with DSIP-10 [DSIP-10][Unit Tests] Improve DolphinScheduler unit tests #10573.
2. Remove the dependency of jUnit 4 and migrate all the UTs with IDE inspections in this PR.

However, I think the second method is a bit risky.

WDYT? @caishunfeng @kezhenxu94 @SbloodyS @ruanwenjun

BTW, there are two ways to fully migrate those tests from jUnit 4 to jUnit 5:

1. We keep both versions of jUnit dependencies and migrate those UTs gradually with DSIP-10 [DSIP-10][Unit Tests] Improve DolphinScheduler unit tests #10573.
2. Remove the dependency of jUnit 4 and migrate all the UTs with IDE inspections in this PR.

However, I think the second method is a bit risky.

WDYT? @caishunfeng @kezhenxu94 @SbloodyS @ruanwenjun

I prefer the second way.

BTW, there are two ways to fully migrate those tests from jUnit 4 to jUnit 5:

1. We keep both versions of jUnit dependencies and migrate those UTs gradually with DSIP-10 [DSIP-10][Unit Tests] Improve DolphinScheduler unit tests #10573.
2. Remove the dependency of jUnit 4 and migrate all the UTs with IDE inspections in this PR.

However, I think the second method is a bit risky.
WDYT? @caishunfeng @kezhenxu94 @SbloodyS @ruanwenjun

I prefer the second way.

I just tried migrating all the UTs and got blocked.

Specifically speaking, there are two risks:

1. We need to switch all the UTs to use jUnit 5 and Spotless will automatically reformat them. I've tried it, there would be 333 files modified.
2. We used powermock quite a lot in the project and powermock does not support jUnit 5 as stated in [Improvement][UT] Upgrade junit to 5.+ #10976 (comment). It is hardly possible to refactor all the code using powermock in one PR.

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

BTW, there are two ways to fully migrate those tests from jUnit 4 to jUnit 5:

1. We keep both versions of jUnit dependencies and migrate those UTs gradually with DSIP-10 [DSIP-10][Unit Tests] Improve DolphinScheduler unit tests #10573.
2. Remove the dependency of jUnit 4 and migrate all the UTs with IDE inspections in this PR.

However, I think the second method is a bit risky.
WDYT? @caishunfeng @kezhenxu94 @SbloodyS @ruanwenjun

I prefer the second way.

I just tried migrating all the UTs and got blocked.

Specifically speaking, there are two risks:

1. We need to switch all the UTs to use jUnit 5 and Spotless will automatically reformat them. I've tried it, there would be 333 files modified.
2. We used powermock quite a lot in the project and powermock does not support jUnit 5 as stated in [Improvement][UT] Upgrade junit to 5.+ #10976 (comment). It is hardly possible to refactor all the code using powermock in one PR.

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.

BTW, there are two ways to fully migrate those tests from jUnit 4 to jUnit 5:

1. We keep both versions of jUnit dependencies and migrate those UTs gradually with DSIP-10 [DSIP-10][Unit Tests] Improve DolphinScheduler unit tests #10573.
2. Remove the dependency of jUnit 4 and migrate all the UTs with IDE inspections in this PR.

However, I think the second method is a bit risky.
WDYT? @caishunfeng @kezhenxu94 @SbloodyS @ruanwenjun

I prefer the second way.

I just tried migrating all the UTs and got blocked.

Specifically speaking, there are two risks:

1. We need to switch all the UTs to use jUnit 5 and Spotless will automatically reformat them. I've tried it, there would be 333 files modified.

This is totally OK to me, "migrations" are always huge changes...

2. We used powermock quite a lot in the project and powermock does not support jUnit 5 as stated in [Improvement][UT] Upgrade junit to 5.+ #10976 (comment). It is hardly possible to refactor all the code using powermock in one PR.

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 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?

I just worry if we still keep the Junit4, we need to constantly remind contributors to use Junit5.

Agree, same concern to me

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

No Coverage information
0.0% Duplication

@ruanwenjun @kezhenxu94 @caishunfeng
Finally I managed to remove the dependency of jUnit 4 since it is wired, ugly and confusing to have two versions of jUnit in pom.xml.

I have the same worries that if we still keep the Junit4, we need to constantly remind contributors to use Junit5.

However, even though I have removed the dependency of jUnit 4, contributors still could write jUnit 4 fashion UTs as long as we use junit-vintage-engine. It is quite difficult to migrate all legacy tests in this PR. I will keep working on this but I prefer doing it iteratively.

About Powermock, It seems we could use high version (greater than 3.4.0) Mockito to mock static methods and I've already added the dependency in this PR. I need more time to find out whether the latest Mockito could fully replace Powermock since we use Powermock quite a lot previously. Also, I find someone use bytebuddy to do some trick but I'm not sure whether this trick could apply here: https://git.stklcode.de/stklcode/juraclient/commit/c9444bbc971cd36f80824c3e7ec9baf8ecfbd676.
But I believe we could get rid of Powermock as long as we set our minds to : )
https://wttech.blog/blog/2020/mocking-static-methods-made-possible-in-mockito-3.4.0/

For the rest of the legacy tests, we could migrate some of them easily with Intellij, but still, there are some complicated ones we need to do it manually.

Above all, there are two action items after this PR, if you agree:

• Remove dependency of Powermock. [Improvement][Test] Remove dependency of powermock #11405.
• Migrate legacy code and replace junit-vintage-engine with junit-jupiter-engine.

Thanks

I investigated the migration from jUnit4 to jUnit5 in other open-source projects, such as Apache Flink. Apache Flink community once tried to migrate all stuff from 4 to 5 once for all but finally gave up and decided to do it gradually. Currently, as shown in https://github.com/apache/flink/blob/1232629c80cbb64eb4ca9f6c95d6c5c1a2e8e82d/pom.xml#L147-L148 , they have both versions in the project.
FYI, here is the mailing thread where they discussed about the migration: https://www.mail-archive.com/dev@flink.apache.org/msg47657.html
WDYT @kezhenxu94 @ruanwenjun @caishunfeng @SbloodyS

cc @zhongjiajie

Maybe there are other historical reasons of flink,dolphischeduler do not have many unitest currently, IMO if we can migrate all of them to junit5 now it is the time

@zhongjiajie The workload to migrate all of them is quite large. Currently, because of those powermock, I could not give an exact estimate of the specific workload. If we decide to do so, first of all, we need to remove all the code related to powermock #11405 . Then, we need to get back to this PR to add dependency on jUnit5 and manually migrate the complex part which IDE could not do automatically e.g. change expected = Exception.class to Assertions.assertThrows. After these two done, we could try to use Intellij to migrate the rest and see how many of the rest will break and decide what to do next.

I just worry if we still keep the Junit4, we need to constantly remind contributors to use Junit5.

Agree, same concern to me

@ruanwenjun @kezhenxu94 BTW, for the concern that contributors may write UTs with jUnit 4, we could add regex in Spotless check rules to prevent them from using jUnit 4. This will work in CI too. The same with Powermock.

Powermock is getting more and more annoying such as https://github.com/apache/dolphinscheduler/runs/7929368593?check_suite_focus=true

powermock/powermock#1112

I'd rather just review and remove all those tests involving Powermock...

Powermock is getting more and more annoying such as https://github.com/apache/dolphinscheduler/runs/7929368593?check_suite_focus=true

powermock/powermock#1112

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 Powermock with Mockito and it worked well, see #11588. I think it is possible to remove the dependency of Powermock.

For some more difficult cases such as:

1. Legacy code using some external APIs which are really hard to mock
2. Legacy code instantiating stuff with new keyword

We could:

1. Encapsulate the external APIs
2. Inject dependencies as below:
   

https://stackoverflow.com/questions/15052984/what-is-the-difference-between-mocking-and-spying-when-using-mockito#:~:text=about%20partial%20mocks.-,Mockito.,passed%20to%20spy()%20method.

Good to hear we can remove powermock 🎉

SonarCloud Quality Gate failed.   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

0.0% Coverage
0.0% Duplication

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 powermock in the whole project soon and I will add a spotless step to block it in DS. Removing powermock is of the top priority for DSIP-10 #10573 . BTW, I just found that there had been both jUnit 4 and jUnit 5 style tests in DS project for a long time. see: #12062 (comment)