Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Improvement][dao,server] unit test for ConditionsTask #3385

Merged
merged 2 commits into from Aug 18, 2020
Merged

[Improvement][dao,server] unit test for ConditionsTask #3385

merged 2 commits into from Aug 18, 2020

Conversation

hsupu
Copy link
Contributor

@hsupu hsupu commented Aug 2, 2020 •
edited

What is the purpose of the pull request

refactor the unit test for ConditionsTask

Brief change log

  • refactor and enable the unit test for ConditionsTask
  • add FAILURE dependence test for ConditionsTask
  • fix a potential NPE at TaskInstance.getDependence and add a unit test for it
  • enable unit test for TaskInstance

Verify this pull request

This change added tests and can be verified as follows:

  • Updated org.apache.dolphinscheduler.server.master.ConditionsTaskTest
  • Updated org.apache.dolphinscheduler.dao.entity.TaskInstanceTest
  • Tested locally

@hsupu hsupu changed the title refactor(server): unit test for ConditionsTask [Improvement][server] unit test for ConditionsTask Aug 2, 2020
@hsupu hsupu changed the title [Improvement][server] unit test for ConditionsTask [Improvement][common,dao,server] unit test for ConditionsTask Aug 2, 2020
@Jave-Chen
Copy link
Contributor

Sonar Analysis failed, you can see more at
https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=3385&metric=new_coverage&view=list

@hsupu hsupu force-pushed the dev-test-polish-conditions-task branch from cdb8977 to 6538718 Compare August 3, 2020 08:42
@yangyichao-mango
Copy link
Contributor

Hi,
Please add test case for your new code in TaskInstance to resolve this sonar fail.
image

yangyichao-mango
yangyichao-mango approved these changes Aug 4, 2020
View reviewed changes
Copy link
Contributor

@yangyichao-mango yangyichao-mango left a comment

Choose a reason for hiding this comment

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

LGTM.

@hsupu
Copy link
Contributor Author

hsupu commented Aug 5, 2020

Please add test case for your new code in TaskInstance to resolve this sonar fail.

done.

@hsupu
Copy link
Contributor Author

hsupu commented Aug 5, 2020

@yangqinlong There is a big change in pom.xml to sort tests, plz check.

yangyichao-mango
yangyichao-mango reviewed Aug 5, 2020
View reviewed changes
pom.xml Outdated
<include>**/dao/mapper/AccessTokenMapperTest.java</include>
<include>**/dao/mapper/AlertGroupMapperTest.java</include>
<include>**/dao/mapper/CommandMapperTest.java</include>
<include>**/dao/mapper/ConnectionFactoryTest.java</include>
<include>**/dao/mapper/DataSourceMapperTest.java</include>
<include>**/dao/entity/UdfFuncTest.java</include>
<include>**/dao/mapper/DataSourceUserMapperTest.java</include>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please revert the changes and just add your test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done. and you may wannt to know that there're two parts for /dao here and the dao/entity/ProcessDefinitionTest is missing.

yangyichao-mango
yangyichao-mango approved these changes Aug 5, 2020
View reviewed changes
Copy link
Contributor

@yangyichao-mango yangyichao-mango left a comment

Choose a reason for hiding this comment

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

LGTM.

@hsupu hsupu changed the title [Improvement][common,dao,server] unit test for ConditionsTask [Improvement][dao,server] unit test for ConditionsTask Aug 8, 2020
@hsupu hsupu mentioned this pull request Aug 8, 2020
Closed
@hsupu hsupu force-pushed the dev-test-polish-conditions-task branch from 607c180 to d4b61ed Compare August 10, 2020 08:48
@codecov-commenter
Copy link

Codecov Report

Merging #3385 into dev will increase coverage by 0.29%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##                dev    #3385      +/-   ##
============================================
+ Coverage     35.60%   35.90%   +0.29%     
- Complexity     2569     2587      +18     
============================================
  Files           448      448              
  Lines         20919    20919              
  Branches       2558     2559       +1     
============================================
+ Hits           7448     7510      +62     
+ Misses        12785    12721      -64     
- Partials        686      688       +2
Impacted Files Coverage Δ Complexity Δ
...ache/dolphinscheduler/dao/entity/TaskInstance.java 68.06% <100.00%> (+7.56%) 55.00 <4.00> (+7.00)
...he/dolphinscheduler/common/thread/ThreadUtils.java 66.15% <0.00%> (-3.08%) 13.00% <0.00%> (-1.00%)
...server/master/runner/MasterBaseTaskExecThread.java 38.27% <0.00%> (+1.23%) 7.00% <0.00%> (+1.00%)
...server/master/runner/ConditionsTaskExecThread.java 87.09% <0.00%> (+87.09%) 11.00% <0.00%> (+11.00%)

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 5584f0c...d4b61ed. Read the comment docs.

@hsupu hsupu force-pushed the dev-test-polish-conditions-task branch from d4b61ed to d8b63cb Compare August 18, 2020 02:34
@sonarcloud
Copy link

sonarcloud bot commented Aug 18, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

Jave-Chen
Jave-Chen approved these changes Aug 18, 2020
View reviewed changes
Copy link
Contributor

@Jave-Chen Jave-Chen left a comment

Choose a reason for hiding this comment

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

+1

@Jave-Chen Jave-Chen merged commit 59610a5 into apache:dev Aug 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jave-Chen Jave-Chen Jave-Chen approved these changes

@yangyichao-mango yangyichao-mango yangyichao-mango approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@hsupu @Jave-Chen @yangyichao-mango @codecov-commenter