[Improvement][Test] Fully remove the usage of powermock from the whole project by EricGao888 · Pull Request #12244 · apache/dolphinscheduler

EricGao888 · 2022-10-02T06:43:23Z

Purpose of the pull request

This PR closes: [Improvement][Test] Remove powermock in common, alert, worker, etc. modules #12208

Brief change log

Fully remove the usage of Powermock from the whole project.

Verify this pull request

Verified by UT.

EricGao888 · 2022-10-02T06:57:32Z

...hinscheduler-common/src/test/java/org/apache/dolphinscheduler/common/utils/NetUtilsTest.java

 import org.junit.After;
 import org.junit.Test;
-import org.powermock.reflect.Whitebox;
+import org.springframework.test.util.ReflectionTestUtils;


@kezhenxu94 @ruanwenjun @caishunfeng @SbloodyS Currently springframework.test.util.ReflectionTestUtils is the only thing I find that could replace powermock.reflect.Whitebox. However, it could not modify final field. That's why I remove final keyword of KUBERNETES_MODE in Constant.java. I'm not sure whether there is a better way.

You don't need to set that field. You can use https://github.com/stefanbirkner/system-rules to mock the system env used here

dolphinscheduler/dolphinscheduler-common/src/main/java/org/apache/dolphinscheduler/common/Constants.java

Line 800 in 66958b9

public static final boolean KUBERNETES_MODE = !StringUtils.isEmpty(System.getenv("KUBERNETES_SERVICE_HOST"))

.

You don't need to set that field. You can use https://github.com/stefanbirkner/system-rules to mock the system env used here

dolphinscheduler/dolphinscheduler-common/src/main/java/org/apache/dolphinscheduler/common/Constants.java

Line 800 in 66958b9

public static final boolean KUBERNETES_MODE = !StringUtils.isEmpty(System.getenv("KUBERNETES_SERVICE_HOST"))

.

Thanks for the help. I will take a look into it : )

kezhenxu94 · 2022-10-03T10:47:18Z

tools/dependencies/known-dependencies.txt

 jamon-runtime-2.3.1.jar
 janino-3.0.16.jar
-javassist-3.27.0-GA.jar
+javassist-3.26.0-GA.jar


Can we keep the newer version of this.

A bit wired that I didn't touch anything related to this dependency, but somehow CI detected we have both javassist-3.27.0-GA.jar
and javassist-3.26.0-GA.jar and failed. I checked the dependency tree and found org.reflections depends on javassist-3.26.0-GA.jar but com.cronutils depends on javassist-3.27.0-GA.jar.
At first, I made them both depend on javassist-3.27.0-GA.jar but RpcTest.java failed with message like :

java.lang.RuntimeException: Timeout exception. Request id: 1. Request class name: IUserService. Request method: hi at org.apache.dolphinscheduler.rpc.future.RpcFuture.get(RpcFuture.java:67) at org.apache.dolphinscheduler.rpc.remote.NettyClient.sendMsg(NettyClient.java:220) at org.apache.dolphinscheduler.rpc.client.ConsumerInterceptor.intercept(ConsumerInterceptor.java:72) at org.apache.dolphinscheduler.rpc.IUserService$ByteBuddy$YDGonXwq.hi(Unknown Source) at org.apache.dolphinscheduler.rpc.RpcTest.sendTest(RpcTest.java:49) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.lang.reflect.Method.invoke(Method.java:498) at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59) at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12) at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:56) at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17) at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26) at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27) at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306) at org.junit.runners.BlockJUnit4ClassRunner$1.evaluate(BlockJUnit4ClassRunner.java:100) at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:366) at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:103) at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:63) at org.junit.runners.ParentRunner$4.run(ParentRunner.java:331) at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:79) at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:329) at org.junit.runners.ParentRunner.access$100(ParentRunner.java:66) at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:293) at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306) at org.junit.runners.ParentRunner.run(ParentRunner.java:413) at org.junit.runner.JUnitCore.run(JUnitCore.java:137) at com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:69) at com.intellij.rt.junit.IdeaTestRunner$Repeater$1.execute(IdeaTestRunner.java:38) at com.intellij.rt.execution.junit.TestsRepeater.repeat(TestsRepeater.java:11) at com.intellij.rt.junit.IdeaTestRunner$Repeater.startRunnerWithArgs(IdeaTestRunner.java:35) at com.intellij.rt.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:235) at com.intellij.rt.junit.JUnitStarter.main(JUnitStarter.java:54)

After that, I made them depend on javassist-3.26.0-GA.jar and things looked fine.

Just now I checked the changes in https://github.com/ronmamo/reflections repo and found they skipped javassist-3.27.0-GA.jar, I assume that there could be some incompatibilities between reflections and javassist-3.27.0-GA.jar, see: ronmamo/reflections@0.9.12...0.10

The latest version of org.reflections is 0.10.2, which depends on javassist-3.28.0-GA.jar. I'm going to upgrade it and rerun the CI to see if things work.

Still not working. Seems we have 3 methods to solve this:

Method 1: Downgrade javassist to 3.26.0-GA.jar, this version works well as I have tested.

Method 2: Keep things the way it was, which means org.reflections depends on javassist-3.26.0-GA.jar and com.cronutils depends on javassist-3.27.0-GA.jar and we add both versions to tools/dependencies/known-dependencies.txt

Method 3: Upgrade org.reflections to version 0.10.2 and make both org.reflections and com.cronutils depend on javassist-3.28.0-GA.jar. For this method, we need to solve the NullPointerException for this line of code:

dolphinscheduler/dolphinscheduler-remote/src/main/java/org/apache/dolphinscheduler/rpc/remote/NettyServerHandler.java

Line 84 in 165b9a5

Object object = serviceClass.newInstance();

@kezhenxu94 @ruanwenjun WDYT?

I'd always prefer newer versions of dependencies so method 3 is my preference. 😄

I'd always prefer newer versions of dependencies so method 3 is my preference. 😄

Got it.

codecov-commenter · 2022-10-09T06:15:20Z

Codecov Report

Merging #12244 (0402858) into dev (a865be0) will decrease coverage by 1.13%.
The diff coverage is 53.33%.

@@             Coverage Diff              @@
##                dev   #12244      +/-   ##
============================================
- Coverage     39.68%   38.54%   -1.14%     
+ Complexity     4207     4078     -129     
============================================
  Files          1021     1023       +2     
  Lines         38242    38267      +25     
  Branches       4394     4391       -3     
============================================
- Hits          15175    14751     -424     
- Misses        21315    21793     +478     
+ Partials       1752     1723      -29

Impacted Files	Coverage Δ
.../org/apache/dolphinscheduler/common/Constants.java	`85.71% <ø> (+7.45%)`	⬆️
...dolphinscheduler/common/utils/KubernetesUtils.java	`0.00% <0.00%> (ø)`
...ler/remote/factory/NettyRemotingClientFactory.java	`0.00% <ø> (ø)`
...ler/remote/factory/NettyRemotingServerFactory.java	`0.00% <0.00%> (ø)`
...pache/dolphinscheduler/rpc/config/ServiceBean.java	`0.00% <0.00%> (-80.00%)`	⬇️
...hinscheduler/service/alert/AlertClientService.java	`60.00% <ø> (ø)`
...apache/dolphinscheduler/service/log/LogClient.java	`41.41% <ø> (ø)`
...apache/dolphinscheduler/common/utils/NetUtils.java	`49.64% <83.33%> (+0.35%)`	⬆️
...org/apache/dolphinscheduler/alert/AlertServer.java	`45.94% <100.00%> (-5.34%)`	⬇️
...g/apache/dolphinscheduler/spi/utils/DateUtils.java	`7.61% <100.00%> (+0.88%)`	⬆️
... and 39 more

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

EricGao888 · 2022-10-09T09:15:52Z

Pretty wired. I could not pass src/test/java/org/apache/dolphinscheduler/rpc/RpcTest.java in my local IDE but CI somehow passed. Seems CI somehow skipped it. 😢

sonarqubecloud · 2022-10-09T14:33:10Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
3 Code Smells

46.2% Coverage
0.0% Duplication

EricGao888 · 2022-10-09T15:11:45Z

Pretty wired. I could not pass src/test/java/org/apache/dolphinscheduler/rpc/RpcTest.java in my local IDE but CI somehow passed. Seems CI somehow skipped it. 😢

@kezhenxu94 Fixed, thanks for the help : )

kezhenxu94

lgtm

…e project (apache#12244) * Fully remove the usage of powermock from the whole project * Upgrade org.reflections to 0.10.12

EricGao888 self-assigned this Oct 2, 2022

github-actions bot added the backend label Oct 2, 2022

EricGao888 added improvement make more easy to user or prompt friendly test labels Oct 2, 2022

EricGao888 added this to the 3.2.0 milestone Oct 2, 2022

github-actions bot removed the test label Oct 2, 2022

EricGao888 commented Oct 2, 2022

View reviewed changes

EricGao888 marked this pull request as ready for review October 2, 2022 14:24

EricGao888 requested review from SbloodyS, caishunfeng and ruanwenjun as code owners October 2, 2022 14:24

EricGao888 force-pushed the Fix-12208 branch from 3d98045 to 224e54a Compare October 2, 2022 14:43

EricGao888 requested a review from kezhenxu94 October 3, 2022 07:14

kezhenxu94 reviewed Oct 3, 2022

View reviewed changes

EricGao888 mentioned this pull request Oct 9, 2022

[Improvement][Test] Remove powermock from DatasyncTaskTest #12270

Merged

EricGao888 added 9 commits October 9, 2022 12:45

Fully remove the usage of powermock from the whole project

f43a65b

Remove useless code

5a2c4e9

Add license header

ee3ead2

Refactor some code

d25637e

Close mocked static class

fe7cb69

Fix dependency issue

9f6b408

Fix dependeny version

daccd04

Fix dependency

8a1ebb2

Upgrade org.reflections to 0.10.12

3235e81

EricGao888 force-pushed the Fix-12208 branch from 48d4661 to 3235e81 Compare October 9, 2022 04:46

Update reflections sdk version in known-dependencies.txt

5eb9375

Fix prefix

0402858

EricGao888 requested a review from kezhenxu94 October 10, 2022 01:53

kezhenxu94 approved these changes Oct 10, 2022

View reviewed changes

EricGao888 merged commit 9ab79e0 into apache:dev Oct 10, 2022

Conversation

EricGao888 commented Oct 2, 2022

Purpose of the pull request

Brief change log

Verify this pull request

Uh oh!

EricGao888 Oct 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kezhenxu94 Oct 2, 2022

Choose a reason for hiding this comment

Uh oh!

EricGao888 Oct 2, 2022

Choose a reason for hiding this comment

Uh oh!

kezhenxu94 Oct 3, 2022

Choose a reason for hiding this comment

Uh oh!

EricGao888 Oct 3, 2022

Choose a reason for hiding this comment

Uh oh!

EricGao888 Oct 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EricGao888 Oct 9, 2022

Choose a reason for hiding this comment

Uh oh!

kezhenxu94 Oct 9, 2022

Choose a reason for hiding this comment

Uh oh!

EricGao888 Oct 9, 2022

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Oct 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

EricGao888 commented Oct 9, 2022

Uh oh!

sonarqubecloud bot commented Oct 9, 2022

Uh oh!

EricGao888 commented Oct 9, 2022

Uh oh!

kezhenxu94 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

EricGao888 Oct 2, 2022 •

edited

Loading

EricGao888 Oct 9, 2022 •

edited

Loading

codecov-commenter commented Oct 9, 2022 •

edited

Loading