Aliyun: Add OSS integration test rule #3687
Conversation
| @@ -69,6 +70,9 @@ public void after() { | |||
|
|
|||
| @Test | |||
| public void testBuckets() { | |||
| Assume.assumeTrue("Aliyun integration test cannot delete existing bucket from test environment.", | |||
| OSS_TEST_RULE.getClass() == AliyunOSSMockRule.class); | |||
There was a problem hiding this comment.
Add this sentence because we don't want to run this test cases for integration test, because it will delete the bucket from test environment, while the test environment may have other objects that were used by other tests.
There was a problem hiding this comment.
In AWS we just assumes the bucket exists, and create a random prefix as the root for all tests. By doing that we don't deal with bucket deletion, which is quite dangerous as you say. Would that be better also for Aliyun?
There was a problem hiding this comment.
In AWS we just assumes the bucket exists, and create a random prefix as the root for all tests.
The aliyun integration tests is using the same approach as you said. This suite case was designed to guarantee that the Aliyun Mock OSS Application will have the same semantics as the production aliyun OSS services. If both AliyunOSSMockRule and OSSIntegrationTestRule are successfully applied to this case, then we can ensure that all the unit tests which are built on top of the AliyunOSSMockRule are correctly designed.
As we iceberg FileIO won't depend any bucket creation or deletion interfaces , plus the dangerous bucket deletion behavior. So Here I disabled the buckets tests for integration tests.
There was a problem hiding this comment.
I see, that makes sense to me, thanks for the explanation
| @@ -17,25 +17,36 @@ | |||
| * under the License. | |||
| */ | |||
|
|
|||
| package org.apache.iceberg.aliyun.oss; | |||
| package org.apache.iceberg.aliyun; | |||
There was a problem hiding this comment.
Move this utility class out of the org.apache.iceberg.aliyun.oss because the utility will also be used for other aliyun services , such as the future aliyun dlf catalog etc.
|
Get this merged, Thanks @jackye1995 for the reviewing ! |
This PR addressed all the commented issues from the original one: #3596
I've tested the integration tests successfully in my local: