-
Notifications
You must be signed in to change notification settings - Fork 4.8k
HIVE-29043: Ban the Jupiter dependency from hive-unit to prevent unit tests from being skipped #5894
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea but based on the test failures it seems that we already have jupiter dependencies in the project so we have to revisit those.
Other than that left some minor suggestions for potential improvements.
pom.xml
Outdated
<!-- Only remove this rule if you confirmed that adding jupiter dependency --> | ||
<!-- to the project won't make unit tests silently skipped --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment would be more helpful if it was part of the failure. How about putting it in <message>...</message>
standalone-metastore/pom.xml
Outdated
</configuration> | ||
</execution> | ||
<!-- Only remove this rule if you confirmed that adding jupiter dependency --> | ||
<!-- to the project won't make unit tests silently skipped --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment regarding the message.
standalone-metastore/pom.xml
Outdated
</excludes> | ||
</bannedDependencies> | ||
</rules> | ||
<fail>true</fail> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the default is true so possible we could skip this.
standalone-metastore/pom.xml
Outdated
<!-- Only remove this rule if you confirmed that adding jupiter dependency --> | ||
<!-- to the project won't make unit tests silently skipped --> | ||
<execution> | ||
<id>ban-jupiter</id> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we want to put the rule in a separate execution? We could merge enforce-banned-dependencies
and ban-jupiter
and it would lead to less boilerplate code.
thanks for the comments, finally, even if I wanted to ban the dependency globally so bad, I had to face that we do use the jupiter api extensively in some modules, so I ended banning it from itests/hive-unit, where it caused the problem all the time there how I ended up with: 8e57d3b |
|
What changes were proposed in this pull request?
Ban jupiter dependency.
Why are the changes needed?
To prevent skipping hive-unit unit tests silently again.
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Tested with adding the problematic dependency, and instead of silently skipping unit test, it failed.