Skip to content

[Tests] Fix invalid test case in ProcessUtilsTest.testCancelApplication#6130

Closed
zuston wants to merge 1 commit intoapache:devfrom
zuston:logFix
Closed

[Tests] Fix invalid test case in ProcessUtilsTest.testCancelApplication#6130
zuston wants to merge 1 commit intoapache:devfrom
zuston:logFix

Conversation

@zuston
Copy link
Member

@zuston zuston commented Sep 8, 2021

Purpose of the pull request

Brief change log

Verify this pull request

This pull request is code cleanup without any test coverage.

(or)

This pull request is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

@codecov-commenter
Copy link

codecov-commenter commented Sep 8, 2021

Codecov Report

Merging #6130 (7dcffcf) into dev (d7af95f) will decrease coverage by 1.19%.
The diff coverage is 50.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##                dev    #6130      +/-   ##
============================================
- Coverage     44.02%   42.83%   -1.20%     
+ Complexity     3701     3611      -90     
============================================
  Files           634      635       +1     
  Lines         26025    25910     -115     
  Branches       2917     2938      +21     
============================================
- Hits          11457    11098     -359     
- Misses        13467    13735     +268     
+ Partials       1101     1077      -24     
Impacted Files Coverage Δ
...che/dolphinscheduler/common/utils/HadoopUtils.java 28.71% <50.00%> (+0.28%) ⬆️
...he/dolphinscheduler/server/utils/ProcessUtils.java 50.29% <50.00%> (+1.78%) ⬆️
...nscheduler/api/exceptions/ApiExceptionHandler.java 66.66% <0.00%> (-33.34%) ⬇️
...pache/dolphinscheduler/api/dto/TaskStateCount.java 42.85% <0.00%> (-23.81%) ⬇️
...api/service/impl/ProcessDefinitionServiceImpl.java 36.84% <0.00%> (-19.56%) ⬇️
...er/api/service/impl/TaskDefinitionServiceImpl.java 33.33% <0.00%> (-19.45%) ⬇️
...dolphinscheduler/dao/entity/ProcessDefinition.java 54.62% <0.00%> (-14.34%) ⬇️
...uler/api/service/impl/DataAnalysisServiceImpl.java 80.76% <0.00%> (-13.97%) ⬇️
...hinscheduler/api/controller/ProjectController.java 65.21% <0.00%> (-10.79%) ⬇️
...r/api/service/impl/ProcessInstanceServiceImpl.java 63.75% <0.00%> (-9.12%) ⬇️
... and 52 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d7af95f...7dcffcf. Read the comment docs.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Sep 8, 2021

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 0 Code Smells

52.9% 52.9% Coverage
0.0% 0.0% Duplication

Comment on lines +110 to +113
public static HadoopUtils getInstanceForTest() {
return new HadoopUtils(false);
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This approach is not recommended. just for the test.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So directly using new HadoopUtils(false) ? Or other approach ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer other ways. Or it is recommended to refactor it, and it is not recommended to use static code blocks in this case.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you help give me some suggestions? Have no ideas on it. Thanks~

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An important factor in unit testing is structuring your code so that it is suitable for testing. code with excessive static initialization is not a structure that's easily tested. Even you can see that the famous Mockito does not support static method testing so far, my suggestion is You refactor the code and give up the use of static methods. this scenario is not applicable. we cannot use mocks to complete the mocks of external components.

@zuston zuston requested a review from CalvinKirs September 10, 2021 07:09
@zuston zuston closed this Dec 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants