nastra commented on code in PR #9122:
URL: https://github.com/apache/iceberg/pull/9122#discussion_r1400943843
##########
build.gradle:
##########
@@ -17,7 +17,9 @@
* under the License.
*/
+
Review Comment:
unnecessary changes here and below
##########
aliyun/src/test/java/org/apache/iceberg/aliyun/oss/mock/TestLocalAliyunOSS.java:
##########
@@ -58,37 +58,38 @@ private static void assertThrows(Runnable runnable, String
expectedErrorCode) {
.isEqualTo(expectedErrorCode);
}
- @Before
+ @BeforeEach
public void before() {
- OSS_TEST_RULE.setUpBucket(bucketName);
+ OSS_TEST_EXTENSION.setUpBucket(bucketName);
}
- @After
+ @AfterEach
public void after() {
- OSS_TEST_RULE.tearDownBucket(bucketName);
+ OSS_TEST_EXTENSION.tearDownBucket(bucketName);
}
@Test
public void testBuckets() {
- Assume.assumeTrue(
- "Aliyun integration test cannot delete existing bucket from test
environment.",
- OSS_TEST_RULE.getClass() == AliyunOSSMockRule.class);
+ Assumptions.assumeTrue(
Review Comment:
it would be better to use Assumptions from AssertJ as well:
`Assumptions.assumeThat(OSS_TEST_RULE.getClass()).isEqualTo(...)`
##########
aliyun/src/test/java/org/apache/iceberg/aliyun/TestAliyunClientFactories.java:
##########
@@ -22,43 +22,48 @@
import java.util.Map;
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
import org.apache.iceberg.relocated.com.google.common.collect.Maps;
-import org.junit.Assert;
-import org.junit.Test;
+import org.assertj.core.api.Assertions;
+import org.junit.jupiter.api.Test;
public class TestAliyunClientFactories {
@Test
public void testLoadDefault() {
- Assert.assertEquals(
- "Default client should be singleton",
- AliyunClientFactories.defaultFactory(),
- AliyunClientFactories.defaultFactory());
+
+ Assertions.assertThat(AliyunClientFactories.defaultFactory())
+ .as("Default client should be singleton")
+ .isEqualTo(AliyunClientFactories.defaultFactory());
AliyunClientFactory defaultFactory =
AliyunClientFactories.from(Maps.newHashMap());
- Assert.assertTrue(
- "Should load default when factory impl not configured",
- defaultFactory instanceof
AliyunClientFactories.DefaultAliyunClientFactory);
- Assert.assertNull(
- "Should have no Aliyun properties set",
defaultFactory.aliyunProperties().accessKeyId());
+
Review Comment:
unnecessary newline
##########
aliyun/src/test/java/org/apache/iceberg/aliyun/oss/TestOSSFileIO.java:
##########
@@ -39,24 +39,25 @@
import org.apache.iceberg.relocated.com.google.common.io.ByteStreams;
import org.apache.iceberg.util.SerializableSupplier;
import org.apache.iceberg.util.SerializationUtil;
-import org.junit.After;
-import org.junit.Assert;
-import org.junit.Before;
-import org.junit.Test;
+import org.assertj.core.api.Assertions;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
public class TestOSSFileIO extends AliyunOSSTestBase {
+
Review Comment:
newline
##########
aliyun/src/test/java/org/apache/iceberg/aliyun/oss/TestOSSURI.java:
##########
@@ -23,28 +23,28 @@
import org.apache.iceberg.exceptions.ValidationException;
import org.apache.iceberg.relocated.com.google.common.collect.Lists;
import org.assertj.core.api.Assertions;
-import org.junit.Assert;
-import org.junit.Test;
+import org.junit.jupiter.api.Test;
public class TestOSSURI {
+
Review Comment:
newline
##########
aliyun/src/test/java/org/apache/iceberg/aliyun/TestAliyunClientFactories.java:
##########
@@ -22,43 +22,48 @@
import java.util.Map;
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
import org.apache.iceberg.relocated.com.google.common.collect.Maps;
-import org.junit.Assert;
-import org.junit.Test;
+import org.assertj.core.api.Assertions;
+import org.junit.jupiter.api.Test;
public class TestAliyunClientFactories {
@Test
public void testLoadDefault() {
- Assert.assertEquals(
- "Default client should be singleton",
- AliyunClientFactories.defaultFactory(),
- AliyunClientFactories.defaultFactory());
+
+ Assertions.assertThat(AliyunClientFactories.defaultFactory())
+ .as("Default client should be singleton")
+ .isEqualTo(AliyunClientFactories.defaultFactory());
AliyunClientFactory defaultFactory =
AliyunClientFactories.from(Maps.newHashMap());
- Assert.assertTrue(
- "Should load default when factory impl not configured",
- defaultFactory instanceof
AliyunClientFactories.DefaultAliyunClientFactory);
- Assert.assertNull(
- "Should have no Aliyun properties set",
defaultFactory.aliyunProperties().accessKeyId());
+
+ Assertions.assertThat(defaultFactory)
+ .as("Should load default when factory impl not configured")
+ .isInstanceOf(AliyunClientFactories.DefaultAliyunClientFactory.class);
+
+ Assertions.assertThat(defaultFactory.aliyunProperties().accessKeyId())
+ .as("Should have no Aliyun properties set")
+ .isNull();
AliyunClientFactory defaultFactoryWithConfig =
AliyunClientFactories.from(ImmutableMap.of(AliyunProperties.CLIENT_ACCESS_KEY_ID,
"key"));
- Assert.assertTrue(
- "Should load default when factory impl not configured",
- defaultFactoryWithConfig instanceof
AliyunClientFactories.DefaultAliyunClientFactory);
- Assert.assertEquals(
- "Should have access key set",
- "key",
- defaultFactoryWithConfig.aliyunProperties().accessKeyId());
+
+ Assertions.assertThat(defaultFactoryWithConfig)
+ .as("Should load default when factory impl not configured")
+ .isInstanceOf(AliyunClientFactories.DefaultAliyunClientFactory.class);
+
+
Assertions.assertThat(defaultFactoryWithConfig.aliyunProperties().accessKeyId())
+ .as("Should have access key set")
+ .isEqualTo("key");
}
@Test
public void testLoadCustom() {
Map<String, String> properties = Maps.newHashMap();
properties.put(AliyunProperties.CLIENT_FACTORY,
CustomFactory.class.getName());
- Assert.assertTrue(
- "Should load custom class",
- AliyunClientFactories.from(properties) instanceof CustomFactory);
+
Review Comment:
newline
##########
aliyun/src/test/java/org/apache/iceberg/aliyun/TestAliyunClientFactories.java:
##########
@@ -22,43 +22,48 @@
import java.util.Map;
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
import org.apache.iceberg.relocated.com.google.common.collect.Maps;
-import org.junit.Assert;
-import org.junit.Test;
+import org.assertj.core.api.Assertions;
+import org.junit.jupiter.api.Test;
public class TestAliyunClientFactories {
@Test
public void testLoadDefault() {
- Assert.assertEquals(
- "Default client should be singleton",
- AliyunClientFactories.defaultFactory(),
- AliyunClientFactories.defaultFactory());
+
+ Assertions.assertThat(AliyunClientFactories.defaultFactory())
+ .as("Default client should be singleton")
+ .isEqualTo(AliyunClientFactories.defaultFactory());
AliyunClientFactory defaultFactory =
AliyunClientFactories.from(Maps.newHashMap());
- Assert.assertTrue(
- "Should load default when factory impl not configured",
- defaultFactory instanceof
AliyunClientFactories.DefaultAliyunClientFactory);
- Assert.assertNull(
- "Should have no Aliyun properties set",
defaultFactory.aliyunProperties().accessKeyId());
+
+ Assertions.assertThat(defaultFactory)
+ .as("Should load default when factory impl not configured")
+ .isInstanceOf(AliyunClientFactories.DefaultAliyunClientFactory.class);
+
+ Assertions.assertThat(defaultFactory.aliyunProperties().accessKeyId())
+ .as("Should have no Aliyun properties set")
+ .isNull();
AliyunClientFactory defaultFactoryWithConfig =
AliyunClientFactories.from(ImmutableMap.of(AliyunProperties.CLIENT_ACCESS_KEY_ID,
"key"));
- Assert.assertTrue(
- "Should load default when factory impl not configured",
- defaultFactoryWithConfig instanceof
AliyunClientFactories.DefaultAliyunClientFactory);
- Assert.assertEquals(
- "Should have access key set",
- "key",
- defaultFactoryWithConfig.aliyunProperties().accessKeyId());
+
Review Comment:
newline
##########
aliyun/src/test/java/org/apache/iceberg/aliyun/oss/TestOSSOutputFile.java:
##########
@@ -54,11 +53,15 @@ public void testWriteFile() throws IOException {
ByteStreams.copy(is, os);
}
- Assert.assertTrue("OSS file should exist",
ossClient.doesObjectExist(uri.bucket(), uri.key()));
- Assert.assertEquals("Object length should match", ossDataLength(uri),
dataSize);
+ Assertions.assertThat(ossClient.doesObjectExist(uri.bucket(), uri.key()))
+ .as("OSS file should exist")
+ .isTrue();
+
+ Assertions.assertThat(ossDataLength(uri)).as("Object length should
match").isEqualTo(dataSize);
byte[] actual = ossDataContent(uri, dataSize);
- Assert.assertArrayEquals("Object content should match", data, actual);
+
Review Comment:
newline
##########
aliyun/src/test/java/org/apache/iceberg/aliyun/oss/AliyunOSSExtension.java:
##########
@@ -39,18 +39,13 @@ default String testBucketName() {
}
@Override
- default Statement apply(Statement base, Description description) {
- return new Statement() {
- @Override
- public void evaluate() throws Throwable {
- start();
- try {
- base.evaluate();
- } finally {
- stop();
- }
- }
- };
+ default void afterAll(ExtensionContext context) throws Exception {
+ stop();
+ }
+
+ @Override
+ default void beforeAll(ExtensionContext context) throws Exception {
Review Comment:
I don't think we need the `default` for these two methods
##########
aliyun/src/test/java/org/apache/iceberg/aliyun/oss/TestOSSFileIO.java:
##########
@@ -73,28 +74,34 @@ public void testOutputFile() throws IOException {
writeOSSData(out, data);
OSSURI uri = new OSSURI(location);
- Assert.assertTrue(
- "OSS file should exist",
ossClient().get().doesObjectExist(uri.bucket(), uri.key()));
- Assert.assertEquals("Should have expected location", location,
out.location());
- Assert.assertEquals("Should have expected length", dataSize,
ossDataLength(uri));
- Assert.assertArrayEquals("Should have expected content", data,
ossDataContent(uri, dataSize));
+
Review Comment:
newline
##########
aliyun/src/test/java/org/apache/iceberg/aliyun/oss/TestOSSOutputFile.java:
##########
@@ -131,16 +132,21 @@ public void testToInputFile() throws IOException {
}
InputFile in = out.toInputFile();
- Assert.assertTrue("Should be an instance of OSSInputFile", in instanceof
OSSInputFile);
- Assert.assertTrue("OSS file should exist", in.exists());
- Assert.assertEquals("Should have expected location", out.location(),
in.location());
- Assert.assertEquals("Should have expected length", dataSize,
in.getLength());
+
Review Comment:
newline
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
