-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
HIVE-28421: Iceberg: mvn test can not run UTs in iceberg-cacatlog #5376
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.
LGTM +1, pending tests
It's weird that UTs in iceberg-catalog are failed with
|
HiveIcebergStorageHandler is in |
I have checked previous CI, and found that the CI never ran the unit tests for iceberg-catalog module. So any new added UT in iceberg-catalog won't be ran, and the CI will always dispaly green though the ut in iceberg-catalog may be bad. This PR can solve the issue and the CI can run the UTs in iceberg-catalog, but some UTs need the dependency Here is my thought:
|
should we migrate all the catalog tests or just a few, also we might disable that |
|
@deniskuzZ Thanks for your additional change!!! |
What changes were proposed in this pull request?
Added a missing junit5 dependency. Refer to https://stackoverflow.com/questions/46360736/tests-not-running-through-maven
Why are the changes needed?
Maybe this issue was caused by the mixed use of junit4&junit5.
For example if i want to do a mvn test locally:
mvn clean test -Dtest=TestHiveCommits -pl iceberg/iceberg-catalog -Piceberg
will print Tests run: 0, Failures: 0, Errors: 0, Skipped: 0, and acutually the test won't be ran.
Does this PR introduce any user-facing change?
No
Is the change a dependency upgrade?
No
How was this patch tested?
Local mvn test in iceberg-catalog moudle