You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "tomtongue (via GitHub)" <gi...@apache.org> on 2024/04/26 17:25:57 UTC

[PR] Migrate FlinkTestBase related tests [iceberg]

tomtongue opened a new pull request, #10232:
URL: https://github.com/apache/iceberg/pull/10232

   Migrate the following classes in `iceberg-flink` and `iceberg-data` to JUnit 5 for https://github.com/apache/iceberg/issues/9087
   
   `FlinkTestBase` inherited classes (including v1.19, v1.18 and v1.17)
   - [x] `ChangeLogTableTestBase`
     - [x] `TestBoundedTableFactory`
     - [x] `TestChangeLogTable`
   - [x] `TestFlinkAnonymousTable` 
   - [x] `TestCatalogLoader`
   - [x] `TestFlinkHiveCatalog`
   - [x] `TestIcebergConnector`


-- 
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] Migrate FlinkTestBase related tests [iceberg]

Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10232:
URL: https://github.com/apache/iceberg/pull/10232#discussion_r1582655786


##########
flink/v1.19/flink/src/test/java/org/apache/iceberg/flink/TestIcebergConnector.java:
##########
@@ -32,33 +37,37 @@
 import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.hive.metastore.HiveMetaStoreClient;
 import org.apache.iceberg.CatalogProperties;
+import org.apache.iceberg.Parameter;
+import org.apache.iceberg.ParameterizedTestExtension;
+import org.apache.iceberg.Parameters;
 import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
 import org.apache.iceberg.relocated.com.google.common.collect.Lists;
 import org.apache.iceberg.relocated.com.google.common.collect.Maps;
-import org.apache.iceberg.relocated.com.google.common.collect.Sets;
 import org.apache.thrift.TException;
-import org.assertj.core.api.Assertions;
-import org.junit.After;
-import org.junit.Assert;
-import org.junit.ClassRule;
-import org.junit.Test;
-import org.junit.rules.TemporaryFolder;
-import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.TestTemplate;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.junit.jupiter.api.io.TempDir;
 
-@RunWith(Parameterized.class)
-public class TestIcebergConnector extends FlinkTestBase {
+@ExtendWith(ParameterizedTestExtension.class)
+public class TestIcebergConnector extends TestBase {
 
   private static final String TABLE_NAME = "test_table";
 
-  @ClassRule public static final TemporaryFolder WAREHOUSE = new TemporaryFolder();
+  @TempDir private static Path warehouse;

Review Comment:
   shouldn't be static



-- 
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] Migrate FlinkTestBase related tests [iceberg]

Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on PR #10232:
URL: https://github.com/apache/iceberg/pull/10232#issuecomment-2084737589

   @tomtongue let's just add them as part of this PR as I'm assuming the changes across Flink versions should be identical


-- 
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] Migrate FlinkTestBase related tests [iceberg]

Posted by "tomtongue (via GitHub)" <gi...@apache.org>.
tomtongue commented on PR #10232:
URL: https://github.com/apache/iceberg/pull/10232#issuecomment-2083084976

   Thanks for the review. Sure, will add the changes for `TestFlinkSourceConfig` and removing the testbase.


-- 
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] Migrate FlinkTestBase related tests [iceberg]

Posted by "tomtongue (via GitHub)" <gi...@apache.org>.
tomtongue commented on PR #10232:
URL: https://github.com/apache/iceberg/pull/10232#issuecomment-2081755863

   @nastra when you have a chance, could you review this PR?


-- 
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] Migrate FlinkTestBase related tests [iceberg]

Posted by "tomtongue (via GitHub)" <gi...@apache.org>.
tomtongue commented on PR #10232:
URL: https://github.com/apache/iceberg/pull/10232#issuecomment-2084939590

   @nastra Thank you! Yes, there are still some test classes to be migrated in `sink` and `source`, and two files (`HadoopCatalogResource` and `SimpleDataUtil`) in `flink` package. Will create PRs for them.


-- 
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] Migrate FlinkTestBase related tests [iceberg]

Posted by "tomtongue (via GitHub)" <gi...@apache.org>.
tomtongue commented on PR #10232:
URL: https://github.com/apache/iceberg/pull/10232#issuecomment-2084694272

   @nastra Along with the reflection of your comment, add `TestFlinkSourceConfig` and its parent test class `TestFlinkTableSource` only for Flink v1.19. I will create a backport PR for other versions Flink v1.17 and v1.18 for the backport of `TestFlinkSourceConfig` and `TestFlinkTableSource` because the total changes of these classes (v1.17 and v1.18) are a bit big, around 700 lines. If it's no problem for me to add the other versions to this PR, please let me know.


-- 
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] Migrate FlinkTestBase related tests [iceberg]

Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10232:
URL: https://github.com/apache/iceberg/pull/10232#discussion_r1582655185


##########
flink/v1.19/flink/src/test/java/org/apache/iceberg/flink/TestChangeLogTable.java:
##########
@@ -37,23 +43,22 @@
 import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
 import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
 import org.apache.iceberg.relocated.com.google.common.collect.Lists;
-import org.apache.iceberg.relocated.com.google.common.collect.Sets;
 import org.apache.iceberg.util.StructLikeSet;
-import org.junit.After;
-import org.junit.Assert;
-import org.junit.Before;
-import org.junit.BeforeClass;
-import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.TestTemplate;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.junit.jupiter.api.io.TempDir;
 
 /**
  * In this test case, we mainly cover the impact of primary key selection, multiple operations
  * within a single transaction, and multiple operations between different txn on the correctness of
  * the data.
  */
-@RunWith(Parameterized.class)
+@ExtendWith(ParameterizedTestExtension.class)
 public class TestChangeLogTable extends ChangeLogTableTestBase {
+  @TempDir private static Path temp;

Review Comment:
   I don't think this should be static



-- 
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] Migrate FlinkTestBase related tests [iceberg]

Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra merged PR #10232:
URL: https://github.com/apache/iceberg/pull/10232


-- 
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