dolphinscheduler icon indicating copy to clipboard operation
dolphinscheduler copied to clipboard

Published 20 hours ago • verified publisher icon apache

[Improvement][UT] Upgrade jUnit to 5.+ (#10976)

Open EricGao888 opened this issue 3 years ago • 11 comments

Purpose of the pull request

  • Update jUnit to 5.+ following the official jUnit migration instructions: https://github.com/junit-team/junit5-samples/blob/main/junit5-migration-maven/pom.xml
  • Refactor AlertServerTest with jUnit 5 as an example.
  • This PR closes: #10976

Brief change log

  • Already described above.

Verify this pull request

  • Verified by passing all UTs.

EricGao888 avatar Aug 06 '22 10:08 EricGao888

Codecov Report

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

:exclamation: 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

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

codecov-commenter avatar Aug 06 '22 10:08 codecov-commenter

SonarCloud Quality Gate failed.    Quality Gate failed

Bug C 21 Bugs Vulnerability E 9 Vulnerabilities Security Hotspot E 37 Security Hotspots Code Smell A 2101 Code Smells

38.0% 38.0% Coverage 2.4% 2.4% Duplication

Please check the sonar result

caishunfeng avatar Aug 10 '22 03:08 caishunfeng

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

EricGao888 avatar Aug 10 '22 09:08 EricGao888

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.

ruanwenjun avatar Aug 10 '22 11:08 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.

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 https://github.com/apache/dolphinscheduler/issues/10976#issuecomment-1207427898. 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

EricGao888 avatar Aug 10 '22 12:08 EricGao888

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.

ruanwenjun avatar Aug 10 '22 13:08 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.

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

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

kezhenxu94 avatar Aug 10 '22 13:08 kezhenxu94

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

Agree, same concern to me

kezhenxu94 avatar Aug 10 '22 13:08 kezhenxu94

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

sonarqubecloud[bot] avatar Aug 10 '22 17:08 sonarqubecloud[bot]

@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. https://github.com/apache/dolphinscheduler/issues/11405.
  • [ ] Migrate legacy code and replace junit-vintage-engine with junit-jupiter-engine.

Thanks

EricGao888 avatar Aug 10 '22 17:08 EricGao888

FYI here is a list of UT files which uses Powermock:

Found Occurrences in Directory dolphinscheduler with mask '*.java'  (192 usages found)
    Unclassified  (192 usages found)
        dolphinscheduler-alert-server  (1 usage found)
            org.apache.dolphinscheduler.alert  (1 usage found)
                AlertServerTest.java  (1 usage found)
        dolphinscheduler-api  (27 usages found)
            org.apache.dolphinscheduler.api.controller  (5 usages found)
                DataAnalysisControllerTest.java  (1 usage found)
                ProcessInstanceControllerTest.java  (1 usage found)
                ProcessTaskRelationControllerTest.java  (1 usage found)
                ResourcesControllerTest.java  (1 usage found)
                WorkerGroupControllerTest.java  (1 usage found)
            org.apache.dolphinscheduler.api.interceptor  (2 usages found)
                RateLimitInterceptorTest.java  (2 usages found)
            org.apache.dolphinscheduler.api.service  (20 usages found)
                BaseServiceTest.java  (3 usages found)
                DataAnalysisServiceTest.java  (2 usages found)
                DataSourceServiceTest.java  (4 usages found)
                LoggerServiceTest.java  (1 usage found)
                ResourcesServiceTest.java  (4 usages found)
                SchedulerServiceTest.java  (2 usages found)
                TenantServiceTest.java  (1 usage found)
                UdfFuncServiceTest.java  (3 usages found)
        dolphinscheduler-common  (10 usages found)
            org.apache.dolphinscheduler.common.utils  (10 usages found)
                CommonUtilsTest.java  (2 usages found)
                FileUtilsTest.java  (3 usages found)
                HadoopUtilsTest.java  (4 usages found)
                NetUtilsTest.java  (1 usage found)
        dolphinscheduler-datasource-api  (9 usages found)
            org.apache.dolphinscheduler.plugin.datasource.api.client  (4 usages found)
                CommonDataSourceClientTest.java  (4 usages found)
            org.apache.dolphinscheduler.plugin.datasource.api.utils  (5 usages found)
                CommonUtilsTest.java  (5 usages found)
        dolphinscheduler-datasource-clickhouse  (4 usages found)
            org.apache.dolphinscheduler.plugin.datasource.clickhouse  (4 usages found)
                ClickHouseDataSourceChannelTest.java  (4 usages found)
        dolphinscheduler-datasource-db2  (7 usages found)
            org.apache.dolphinscheduler.plugin.datasource.db2  (4 usages found)
                DB2DataSourceChannelTest.java  (4 usages found)
            org.apache.dolphinscheduler.plugin.datasource.db2.param  (3 usages found)
                Db2DataSourceProcessorTest.java  (3 usages found)
        dolphinscheduler-datasource-hive  (7 usages found)
            org.apache.dolphinscheduler.plugin.datasource.hive  (4 usages found)
                HiveDataSourceChannelTest.java  (4 usages found)
            org.apache.dolphinscheduler.plugin.datasource.hive.param  (3 usages found)
                HiveDataSourceProcessorTest.java  (3 usages found)
        dolphinscheduler-datasource-mysql  (14 usages found)
            org.apache.dolphinscheduler.plugin.datasource.mysql  (4 usages found)
                MySQLDataSourceChannelTest.java  (4 usages found)
            org.apache.dolphinscheduler.plugin.datasource.mysql.param  (3 usages found)
                MySQLDataSourceProcessorTest.java  (3 usages found)
            org.apache.dolphinscheduler.plugin.datasource.mysql.provider  (3 usages found)
                JDBCDataSourceProviderTest.java  (3 usages found)
            org.apache.dolphinscheduler.plugin.datasource.mysql.utils  (4 usages found)
                DataSourceUtilsTest.java  (4 usages found)
        dolphinscheduler-datasource-oracle  (7 usages found)
            org.apache.dolphinscheduler.plugin.datasource.oracle  (4 usages found)
                OracleDataSourceChannelTest.java  (4 usages found)
            org.apache.dolphinscheduler.plugin.datasource.oracle.param  (3 usages found)
                OracleDataSourceProcessorTest.java  (3 usages found)
        dolphinscheduler-datasource-postgresql  (7 usages found)
            org.apache.dolphinscheduler.plugin.datasource.postgresql  (4 usages found)
                PostgreSQLDataSourceChannelTest.java  (4 usages found)
            org.apache.dolphinscheduler.plugin.datasource.postgresql.param  (3 usages found)
                PostgreSQLDataSourceProcessorTest.java  (3 usages found)
        dolphinscheduler-datasource-presto  (7 usages found)
            org.apache.dolphinscheduler.plugin.datasource.presto  (4 usages found)
                PrestoDataSourceChannelTest.java  (4 usages found)
            org.apache.dolphinscheduler.plugin.datasource.presto.param  (3 usages found)
                PrestoDataSourceProcessorTest.java  (3 usages found)
        dolphinscheduler-datasource-redshift  (3 usages found)
            org.apache.dolphinscheduler.plugin.datasource.redshift.param  (3 usages found)
                RedshiftDataSourceProcessorTest.java  (3 usages found)
        dolphinscheduler-datasource-spark  (7 usages found)
            org.apache.dolphinscheduler.plugin.datasource.spark  (4 usages found)
                SparkDataSourceChannelTest.java  (4 usages found)
            org.apache.dolphinscheduler.plugin.datasource.spark.param  (3 usages found)
                SparkDataSourceProcessorTest.java  (3 usages found)
        dolphinscheduler-datasource-sqlserver  (7 usages found)
            org.apache.dolphinscheduler.plugin.datasource.sqlserver  (4 usages found)
                SQLServerDataSourceChannelTest.java  (4 usages found)
            org.apache.dolphinscheduler.plugin.datasource.sqlserver.param  (3 usages found)
                SQLServerDataSourceProcessorTest.java  (3 usages found)
        dolphinscheduler-log-server  (3 usages found)
            org.apache.dolphinscheduler.server.log  (3 usages found)
                LoggerRequestProcessorTest.java  (3 usages found)
        dolphinscheduler-master  (18 usages found)
            org.apache.dolphinscheduler.server.master  (3 usages found)
                SubProcessTaskTest.java  (3 usages found)
            org.apache.dolphinscheduler.server.master.processor  (5 usages found)
                CacheProcessorTest.java  (1 usage found)
                TaskAckProcessorTest.java  (3 usages found)
                TaskKillResponseProcessorTest.java  (1 usage found)
            org.apache.dolphinscheduler.server.master.registry  (3 usages found)
                MasterRegistryClientTest.java  (3 usages found)
            org.apache.dolphinscheduler.server.master.runner  (4 usages found)
                MasterTaskExecThreadTest.java  (1 usage found)
                WorkflowExecuteRunnableTest.java  (3 usages found)
            org.apache.dolphinscheduler.server.master.service  (3 usages found)
                FailoverServiceTest.java  (3 usages found)
        dolphinscheduler-server  (4 usages found)
            org.apache.dolphinscheduler.server.utils  (4 usages found)
                ProcessUtilsTest.java  (4 usages found)
        dolphinscheduler-service  (8 usages found)
            org.apache.dolphinscheduler.service.alert  (4 usages found)
                AlertClientServiceTest.java  (3 usages found)
                ProcessAlertManagerTest.java  (1 usage found)
            org.apache.dolphinscheduler.service.log  (3 usages found)
                LogClientServiceTest.java  (3 usages found)
            org.apache.dolphinscheduler.service.process  (1 usage found)
                ProcessServiceTest.java  (1 usage found)
        dolphinscheduler-task-dvc  (5 usages found)
            org.apache.dolphinscheduler.plugin.task.dvc  (5 usages found)
                DvcTaskTest.java  (5 usages found)
        dolphinscheduler-task-emr  (8 usages found)
            org.apache.dolphinscheduler.plugin.task.emr  (8 usages found)
                EmrAddStepsTaskTest.java  (4 usages found)
                EmrJobFlowTaskTest.java  (4 usages found)
        dolphinscheduler-task-jupyter  (5 usages found)
            org.apache.dolphinscheduler.plugin.task.jupyter  (5 usages found)
                JupyterTaskTest.java  (5 usages found)
        dolphinscheduler-task-mlflow  (5 usages found)
            org.apache.dolphinler.plugin.task.mlflow  (5 usages found)
                MlflowTaskTest.java  (5 usages found)
        dolphinscheduler-task-openmldb  (1 usage found)
            org.apache.dolphinscheduler.plugin.task.openmldb  (1 usage found)
                OpenmldbTaskTest.java  (1 usage found)
        dolphinscheduler-task-python  (1 usage found)
            org.apache.dolphinscheduler.plugin.task.python  (1 usage found)
                PythonTaskTest.java  (1 usage found)
        dolphinscheduler-task-sagemaker  (5 usages found)
            org.apache.dolphinscheduler.plugin.task.sagemaker  (5 usages found)
                SagemakerTaskTest.java  (5 usages found)
        dolphinscheduler-task-spark  (4 usages found)
            org.apache.dolphinscheduler.plugin.task.spark  (4 usages found)
                SparkTaskTest.java  (4 usages found)
        dolphinscheduler-task-zeppelin  (4 usages found)
            org.apache.dolphinscheduler.plugin.task.zeppelin  (4 usages found)
                ZeppelinTaskTest.java  (4 usages found)
        dolphinscheduler-worker  (4 usages found)
            org.apache.dolphinscheduler.server.worker.processor  (3 usages found)
                TaskDispatchProcessorTest.java  (3 usages found)
            org.apache.dolphinscheduler.server.worker.runner  (1 usage found)
                TaskExecuteThreadTest.java  (1 usage found)

EricGao888 avatar Aug 11 '22 05:08 EricGao888

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/[email protected]/msg47657.html

WDYT @kezhenxu94 @ruanwenjun @caishunfeng @SbloodyS

EricGao888 avatar Aug 15 '22 03:08 EricGao888

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/[email protected]/msg47657.html

WDYT @kezhenxu94 @ruanwenjun @caishunfeng @SbloodyS

cc @zhongjiajie

EricGao888 avatar Aug 15 '22 03:08 EricGao888

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/[email protected]/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 avatar Aug 15 '22 07:08 zhongjiajie

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/[email protected]/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.

EricGao888 avatar Aug 15 '22 07:08 EricGao888

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.

EricGao888 avatar Aug 15 '22 09:08 EricGao888

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

https://github.com/powermock/powermock/issues/1112

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

kezhenxu94 avatar Aug 20 '22 06:08 kezhenxu94

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: image

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.

EricGao888 avatar Aug 22 '22 06:08 EricGao888

Good to hear we can remove powermock 🎉

kezhenxu94 avatar Aug 22 '22 06:08 kezhenxu94

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

sonarqubecloud[bot] avatar Sep 20 '22 14:09 sonarqubecloud[bot]

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: https://github.com/apache/dolphinscheduler/pull/12062#issuecomment-1251952599

EricGao888 avatar Sep 21 '22 02:09 EricGao888