-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Aliyun: Switch iceberg-aliyun's tests to Junit5 #9122
Conversation
Change Junit4 Assume to Junit5 Assumptions
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.
@lisirrx thanks for working on this. I left a few inital comments and I'll take a closer look tomorrow at the issue around TestUtility
aliyun/src/test/java/org/apache/iceberg/aliyun/oss/mock/TestLocalAliyunOSS.java
Outdated
Show resolved
Hide resolved
aliyun/src/test/java/org/apache/iceberg/aliyun/oss/AliyunOSSExtension.java
Show resolved
Hide resolved
aliyun/src/test/java/org/apache/iceberg/aliyun/TestAliyunClientFactories.java
Outdated
Show resolved
Hide resolved
aliyun/src/test/java/org/apache/iceberg/aliyun/TestAliyunClientFactories.java
Outdated
Show resolved
Hide resolved
aliyun/src/test/java/org/apache/iceberg/aliyun/oss/TestOSSFileIO.java
Outdated
Show resolved
Hide resolved
aliyun/src/test/java/org/apache/iceberg/aliyun/oss/TestOSSFileIO.java
Outdated
Show resolved
Hide resolved
aliyun/src/test/java/org/apache/iceberg/aliyun/oss/TestOSSOutputFile.java
Outdated
Show resolved
Hide resolved
aliyun/src/test/java/org/apache/iceberg/aliyun/oss/TestOSSOutputFile.java
Outdated
Show resolved
Hide resolved
aliyun/src/test/java/org/apache/iceberg/aliyun/oss/TestOSSURI.java
Outdated
Show resolved
Hide resolved
Resolve review comment
053da9a
to
6f77853
Compare
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.
aliyun/src/test/java/org/apache/iceberg/aliyun/TestAliyunClientFactories.java
Outdated
Show resolved
Hide resolved
aliyun/src/test/java/org/apache/iceberg/aliyun/oss/mock/TestLocalAliyunOSS.java
Outdated
Show resolved
Hide resolved
aliyun/src/test/java/org/apache/iceberg/aliyun/oss/AliyunOSSExtension.java
Show resolved
Hide resolved
aliyun/src/test/java/org/apache/iceberg/aliyun/oss/AliyunOSSTestBase.java
Outdated
Show resolved
Hide resolved
Resolve review comment
Thank you for your patient review. I'm sorry for introducing unnecessary blank lines. I will pay attention to this issue when writing code in the future. Btw, should I squash the commits together or you will do that when merging the pr? |
commits will be squashed when the PR is getting merged, so it's up to you if you want to squash them or not. |
hi, @nastra, any addtional work should I do for this PR to move forward? |
Let's give @openinx a few days to review this PR |
Switch iceberg-aliyun's tests to Junit5
Fix: #9081
@nastra Hi, I have changed all of the junit4 code to junit5 with AssertJ, but I have a question here: To fit the Extension class of Junit5, I changed most of the term 'Rule' to 'Extension', but there is a system env variable in
org/apache/iceberg/aliyun/TestUtility.java
calledALIYUN_TEST_OSS_TEST_RULE_CLASS
.I don't know where the CI system runs, and how the variable is set. So I'm not sure if I should change this.