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] 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
Re: [PR] Aliyun: Migrate tests to junit5 for aliyun client factory [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #7853:
URL: https://github.com/apache/iceberg/pull/7853#issuecomment-2339368891
This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.
--
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
Re: [PR] Aliyun: Migrate tests to junit5 for aliyun client factory [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #7853: Aliyun: Migrate tests to junit5 for aliyun client factory
URL: https://github.com/apache/iceberg/pull/7853
--
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
Re: [PR] Aliyun: Migrate tests to junit5 for aliyun client factory [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #7853:
URL: https://github.com/apache/iceberg/pull/7853#issuecomment-2325403716
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.
--
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