You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "DaVincii (via GitHub)" <gi...@apache.org> on 2023/06/17 16:55:12 UTC
[GitHub] [iceberg] DaVincii opened a new pull request, #7853: Aliyun: Migrate tests to junit5 for aliyun client factory
DaVincii opened a new pull request, #7853:
URL: https://github.com/apache/iceberg/pull/7853
This pull request is related to the issue "Move JUnit4 tests to JUnit5 #7160 ".
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] zinking commented on a diff in pull request #7853: Aliyun: Migrate tests to junit5 for aliyun client factory
Posted by "zinking (via GitHub)" <gi...@apache.org>.
zinking commented on code in PR #7853:
URL: https://github.com/apache/iceberg/pull/7853#discussion_r1236183490
##########
aliyun/src/test/java/org/apache/iceberg/aliyun/TestAliyunClientFactories.java:
##########
@@ -22,43 +22,30 @@
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.junit.jupiter.api.Test;
+import static org.assertj.core.api.Assertions.assertThat;
public class TestAliyunClientFactories {
@Test
public void testLoadDefault() {
- Assert.assertEquals(
- "Default client should be singleton",
- AliyunClientFactories.defaultFactory(),
- AliyunClientFactories.defaultFactory());
+ 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());
+ assertThat(defaultFactory instanceof AliyunClientFactories.DefaultAliyunClientFactory).as("Should load default when factory impl not configured").isTrue();
+ 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());
+ assertThat(defaultFactoryWithConfig instanceof AliyunClientFactories.DefaultAliyunClientFactory).as("Should load default when factory impl not configured").isTrue();
+ 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);
+ assertThat(AliyunClientFactories.from(properties) instanceof CustomFactory).as("Should load custom class").isTrue();
Review Comment:
I was thinking like 'is this PR made by the Text-Davinci openAI ?', fortunately not.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] DaVincii commented on pull request #7853: Aliyun: Migrate tests to junit5 for aliyun client factory
Posted by "DaVincii (via GitHub)" <gi...@apache.org>.
DaVincii commented on PR #7853:
URL: https://github.com/apache/iceberg/pull/7853#issuecomment-1614197335
> @DaVincii are you planning to work on this? Would be great to move the aliyun project to JUnit5
@nastra Sorry, got busy with some other work, I have made the review changes. Are you asking for the migrating the classes which extend Rule class, as well?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] DaVincii commented on a diff in pull request #7853: Aliyun: Migrate tests to junit5 for aliyun client factory
Posted by "DaVincii (via GitHub)" <gi...@apache.org>.
DaVincii commented on code in PR #7853:
URL: https://github.com/apache/iceberg/pull/7853#discussion_r1235844115
##########
aliyun/src/test/java/org/apache/iceberg/aliyun/TestAliyunClientFactories.java:
##########
@@ -22,43 +22,30 @@
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.junit.jupiter.api.Test;
+import static org.assertj.core.api.Assertions.assertThat;
public class TestAliyunClientFactories {
@Test
public void testLoadDefault() {
- Assert.assertEquals(
- "Default client should be singleton",
- AliyunClientFactories.defaultFactory(),
- AliyunClientFactories.defaultFactory());
+ 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());
+ assertThat(defaultFactory instanceof AliyunClientFactories.DefaultAliyunClientFactory).as("Should load default when factory impl not configured").isTrue();
+ 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());
+ assertThat(defaultFactoryWithConfig instanceof AliyunClientFactories.DefaultAliyunClientFactory).as("Should load default when factory impl not configured").isTrue();
+ 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);
+ assertThat(AliyunClientFactories.from(properties) instanceof CustomFactory).as("Should load custom class").isTrue();
Review Comment:
@nastra Thanks for the input, migrating Aliyun is bit complex as it contains test rules which needs to migrated to registerClass and extendWith in junit 5. For now I will park this and try the simpler ones and once I gain some expertise will look into this.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on pull request #7853: Aliyun: Migrate tests to junit5 for aliyun client factory
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on PR #7853:
URL: https://github.com/apache/iceberg/pull/7853#issuecomment-1614212493
I think at the very least we should switch the assertions to AssertJ in the `aliyun` module. Also I think we can convert `TestOSSURI` to JUnit5
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #7853: Aliyun: Migrate tests to junit5 for aliyun client factory
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7853:
URL: https://github.com/apache/iceberg/pull/7853#discussion_r1233615462
##########
aliyun/src/test/java/org/apache/iceberg/aliyun/TestAliyunClientFactories.java:
##########
@@ -22,43 +22,30 @@
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.junit.jupiter.api.Test;
+import static org.assertj.core.api.Assertions.assertThat;
public class TestAliyunClientFactories {
@Test
public void testLoadDefault() {
- Assert.assertEquals(
- "Default client should be singleton",
- AliyunClientFactories.defaultFactory(),
- AliyunClientFactories.defaultFactory());
+ 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());
+ assertThat(defaultFactory instanceof AliyunClientFactories.DefaultAliyunClientFactory).as("Should load default when factory impl not configured").isTrue();
+ 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());
+ assertThat(defaultFactoryWithConfig instanceof AliyunClientFactories.DefaultAliyunClientFactory).as("Should load default when factory impl not configured").isTrue();
Review Comment:
same as above
##########
aliyun/src/test/java/org/apache/iceberg/aliyun/TestAliyunClientFactories.java:
##########
@@ -22,43 +22,30 @@
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.junit.jupiter.api.Test;
+import static org.assertj.core.api.Assertions.assertThat;
public class TestAliyunClientFactories {
@Test
public void testLoadDefault() {
- Assert.assertEquals(
- "Default client should be singleton",
- AliyunClientFactories.defaultFactory(),
- AliyunClientFactories.defaultFactory());
+ 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());
+ assertThat(defaultFactory instanceof AliyunClientFactories.DefaultAliyunClientFactory).as("Should load default when factory impl not configured").isTrue();
+ 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());
+ assertThat(defaultFactoryWithConfig instanceof AliyunClientFactories.DefaultAliyunClientFactory).as("Should load default when factory impl not configured").isTrue();
+ 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);
+ assertThat(AliyunClientFactories.from(properties) instanceof CustomFactory).as("Should load custom class").isTrue();
Review Comment:
same as above
##########
aliyun/src/test/java/org/apache/iceberg/aliyun/TestAliyunClientFactories.java:
##########
@@ -22,43 +22,30 @@
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.junit.jupiter.api.Test;
+import static org.assertj.core.api.Assertions.assertThat;
public class TestAliyunClientFactories {
@Test
public void testLoadDefault() {
- Assert.assertEquals(
- "Default client should be singleton",
- AliyunClientFactories.defaultFactory(),
- AliyunClientFactories.defaultFactory());
+ 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());
+ assertThat(defaultFactory instanceof AliyunClientFactories.DefaultAliyunClientFactory).as("Should load default when factory impl not configured").isTrue();
Review Comment:
`assertThat(defaultFactory).as(..).isInstanceOf(AliyunClientFactories.DefaultAliyunClientFactory.class)`
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on pull request #7853: Aliyun: Migrate tests to junit5 for aliyun client factory
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on PR #7853:
URL: https://github.com/apache/iceberg/pull/7853#issuecomment-1614163032
@DaVincii are you planning to work on this? Would be great to move the aliyun project to JUnit5
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org