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.

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

[Improvement][Test] Block the usage of powermock and move mockito dependencies from sub-modules to root pom #12311

Merged
merged 2 commits into from
Oct 14, 2022

Conversation

EricGao888
Copy link
Member

@EricGao888 EricGao888 commented Oct 11, 2022

Purpose of the pull request

Brief change log

  • Move mockito dependencies from sub-modules to root pom.
  • Add a new step in Spotless to block the usage of powermock .

Verify this pull request

  • Verified by UT cases and manual tests.

@EricGao888 EricGao888 added improvement make more easy to user or prompt friendly backend test labels Oct 11, 2022
@EricGao888 EricGao888 added this to the 3.2.0 milestone Oct 11, 2022
@EricGao888 EricGao888 self-assigned this Oct 11, 2022
@github-actions github-actions bot removed the test label Oct 11, 2022
@codecov-commenter
Copy link

codecov-commenter commented Oct 11, 2022

Codecov Report

Merging #12311 (928e5d5) into dev (875682d) will decrease coverage by 0.01%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##                dev   #12311      +/-   ##
============================================
- Coverage     38.62%   38.60%   -0.02%     
- Complexity     4101     4104       +3     
============================================
  Files          1031     1031              
  Lines         38459    38459              
  Branches       4404     4402       -2     
============================================
- Hits          14854    14848       -6     
- Misses        21872    21881       +9     
+ Partials       1733     1730       -3     
Impacted Files Coverage Δ
...r/plugin/registry/zookeeper/ZookeeperRegistry.java 43.54% <0.00%> (-6.46%) ⬇️
...pache/dolphinscheduler/common/utils/DateUtils.java 74.69% <0.00%> (+0.61%) ⬆️
...r/plugin/task/sqoop/parameter/SqoopParameters.java 56.41% <0.00%> (+1.28%) ⬆️

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

@EricGao888
Copy link
Member Author

Currently we use check in CI but apply in pre-commit hook as execution goal of Spotless, see:

dolphinscheduler/pom.xml

Lines 679 to 686 in 04e1b88

<executions>
<execution>
<goals>
<goal>check</goal>
</goals>
<phase>compile</phase>
</execution>
</executions>

- repo: local
hooks:
# Spotless Hooks
- id: spotless
name: spotless lint
entry: ./mvnw spotless:apply
language: script
pass_filenames: false

I'm thinking about changing the apply in pre-commit hook to check. The reason is if I add a regex step to block import org.powermock.*, contributors may get quite confused when pre-commit hook automatically deletes the import, just the same as the wildcard imports we discussed before: #11458 (comment)

WDYT @kezhenxu94 @zhongjiajie

@kezhenxu94
Copy link
Member

I'm thinking about changing the apply in pre-commit hook to check. The reason is if I add a regex step to block import org.powermock.*, contributors may get quite confused when pre-commit hook automatically deletes the import, just the same as the wildcard imports we discussed before: #11458 (comment)

WDYT @kezhenxu94 @zhongjiajie

Sounds good to me

@zhongjiajie
Copy link
Member

Currently we use check in CI but apply in pre-commit hook as execution goal of Spotless, see:

dolphinscheduler/pom.xml

Lines 679 to 686 in 04e1b88

<executions>
<execution>
<goals>
<goal>check</goal>
</goals>
<phase>compile</phase>
</execution>
</executions>

- repo: local
hooks:
# Spotless Hooks
- id: spotless
name: spotless lint
entry: ./mvnw spotless:apply
language: script
pass_filenames: false

I'm thinking about changing the apply in pre-commit hook to check. The reason is if I add a regex step to block import org.powermock.*, contributors may get quite confused when pre-commit hook automatically deletes the import, just the same as the wildcard imports we discussed before: #11458 (comment)

WDYT @kezhenxu94 @zhongjiajie

+1 for me

@EricGao888 EricGao888 marked this pull request as ready for review October 12, 2022 05:19
@sonarcloud
Copy link

sonarcloud bot commented Oct 12, 2022

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

No Coverage information No Coverage information
0.0% 0.0% Duplication

@EricGao888
Copy link
Member Author

PTAL when available, thx~ @kezhenxu94 @caishunfeng @ruanwenjun @SbloodyS @zhongjiajie

@EricGao888 EricGao888 merged commit 2f37da0 into apache:dev Oct 14, 2022
Chris-Arith pushed a commit to Chris-Arith/dolphinscheduler that referenced this pull request Oct 26, 2022
…endencies from sub-modules to root pom (apache#12311)

* move mockito dependencies from sub-modules to root pom

* Add check in CI to block the usage of powermock
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend improvement make more easy to user or prompt friendly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Improvement][Test] Block the usage of Powermock [Improvement][Test] Remove dependency of powermock
4 participants