Flink: refactor sink tests to reduce the number of combinations with parameterized tests #10777

stevenzwu · 2024-07-25T00:42:12Z

E.g. it is not really necessary to test different file format for distribution mode (hash, range). I found this when trying to add new tests for the new range distribution feature. We often run 20+ combinations for every test/method in some classes.

stevenzwu · 2024-07-25T00:51:32Z

flink/v1.19/flink/src/test/java/org/apache/iceberg/flink/CatalogTestBase.java

@@ -118,26 +118,4 @@ protected String getFullQualifiedTableName(String tableName) {
  static String getURI(HiveConf conf) {
    return conf.get(HiveConf.ConfVars.METASTOREURIS.varname);
  }
-
-  static String toWithClause(Map<String, String> props) {


this is moved to SqlBase

stevenzwu · 2024-07-25T01:00:57Z

flink/v1.19/flink/src/test/java/org/apache/iceberg/flink/TestBase.java

@@ -41,7 +40,7 @@
 import org.junit.jupiter.api.extension.RegisterExtension;
 import org.junit.jupiter.api.io.TempDir;

-public abstract class TestBase extends TestBaseUtils {


TestBaseUtils seems like a pure util class with static methods only

stevenzwu · 2024-07-25T01:08:45Z

flink/v1.19/flink/src/test/java/org/apache/iceberg/flink/TestFlinkTableSink.java

@@ -169,39 +154,6 @@ public void testOverwriteTable() throws Exception {
        icebergTable, Lists.newArrayList(SimpleDataUtil.createRecord(2, "b")));
  }

-  @TestTemplate


moved these tests to the new TestFlinkTableSinkExtended which only tests on the dimensions of isStreamingJob boolean value. Currently every test run with 20+ combinations of parameters. Don't want to add range distribution test here to further slow down the test run time.

stevenzwu · 2024-07-25T01:11:10Z

flink/v1.19/flink/src/test/java/org/apache/iceberg/flink/sink/TestFlinkIcebergSink.java

-  }
-
-  @TestTemplate
-  public void testJobNoneDistributeMode() throws Exception {


moved all tests related to distribution mode to TestFlinkIcebergSinkDistributionMode. we don't need to test different file formats (Avro, Orc, Parquet) for distribution mode.

pvary · 2024-07-25T05:03:20Z

@rodmeneses: Could you please take a look. I think it will greatly affect your testing for Sink V2

rodmeneses · 2024-07-30T20:07:15Z

flink/v1.19/flink/src/test/java/org/apache/iceberg/flink/TestFlinkTableSinkExtended.java

+import org.junit.jupiter.api.io.TempDir;
+
+/**
+ * This tests the more extended features of Flink sink. Extract them separately since it is


nit: These instead of This

This is actually correct. but I see why it is confusing. let me change it to This class tests ...

rodmeneses · 2024-07-30T20:07:48Z

flink/v1.19/flink/src/test/java/org/apache/iceberg/flink/TestFlinkTableSinkExtended.java

+ */
+@ExtendWith(ParameterizedTestExtension.class)
+public class TestFlinkTableSinkExtended extends SqlBase {
+


nit: extra line

sure. will remove. but one empty line at the beginning is also ok with Iceberg coding style

rodmeneses · 2024-07-30T20:09:26Z

flink/v1.19/flink/src/test/java/org/apache/iceberg/flink/TestFlinkTableSinkExtended.java

+
+  @TempDir protected File warehouseRoot;
+
+  protected final String flinkDatabase = CATALOG + "." + DATABASE;


these ones could be also static. Also make them uppercase when changing to static

rodmeneses · 2024-07-30T20:10:13Z

flink/v1.19/flink/src/test/java/org/apache/iceberg/flink/TestFlinkTableSinkExtended.java

+  protected TableEnvironment getTableEnv() {
+    if (tEnv == null) {
+      synchronized (this) {
+        EnvironmentSettings.Builder settingsBuilder = EnvironmentSettings.newInstance();


typically the implementation of the singleton pattern ask for another if (tEnv == null) after synchronized

I probably copied it somewhere. we don't actually need double checked locking. I will just make the method synchronized to simplify the code.

rodmeneses

some minor comments, otherwise lgtm

…parameterized tests. E.g. it is not really necessary to test different file format for distribution mode (hash, range). I found this necessary when trying to add new tests for the new range distribution feature. We often run 20+ combinations for every test/method in some classes.

…ergSinkExtended without combinational parameters

pvary · 2024-07-31T07:55:15Z

flink/v1.19/flink/src/test/java/org/apache/iceberg/flink/SqlBase.java

+   * @param catalogName The catalog to drop
+   * @param ifExists If we should use the 'IF EXISTS' when dropping the catalog
+   */
+  protected void dropCatalog(String catalogName, boolean ifExists) {


Can we do a throughout cleanup instead of dropping random catalogs?

Like:

Dropping every catalog other than the dafault

Dropping every database other than the default

Dropping every table other than the default?

right now, inherited classes clean up created tables and databases explicitly, e.g. TableSourceTestBase. We can implement what you suggested. but it will be a couple more SQL executions (1) list databases (2) for each database (usually one), list tables.

@AfterEach public void clean() { sql("DROP TABLE IF EXISTS %s.%s", DATABASE_NAME, TABLE_NAME); dropDatabase(DATABASE_NAME, true); dropCatalog(CATALOG_NAME, true); }

I am leaning a little more toward the existing approach. Or if we want to change it, we can do it in a separate PR.

I think it is good to reduce the complexity of writing tests.
If we create a good cleanup method we can use everywhere then we just call that in an AfterEach method and forget about it altogether. Otherwise we need to remember which database/table was created during the tests. Also, if you different test methods creating different tables, then you need to add logic to the cleanup to differentiate those.

I feel more inclined to the generic cleanup method, even if it adds a few more calls to the test runs.

What do you think @rodmeneses?

even if it adds a few more calls to the test runs.

note that this is for each test method. with parameterized tests, some classes have dozens of tests. this can add up quickly.

Do we have numbers?
Higher test cases means higher probability of a mistake to forget to delete a db or a table.

if we want to redo the catalog clean up pattern across the board, I think it is better to do it in a separate PR if we can get consensus on the new approach.

Makes sense

stevenzwu · 2024-08-02T15:39:25Z

thanks @rodmeneses and @pvary for the review

…toring. 1.20 porting is not needed as the 1.20 code was based on the hash that already included PR apache#10777.

#10965)

…parameterized tests (apache#10777)

…toring. (apache#10965)

stevenzwu requested review from nastra and pvary July 25, 2024 00:42

github-actions bot added the flink label Jul 25, 2024

stevenzwu force-pushed the refactor-sink-tests branch 2 times, most recently from 998bee4 to 85168ca Compare July 25, 2024 01:04

stevenzwu commented Jul 25, 2024

View reviewed changes

stevenzwu force-pushed the refactor-sink-tests branch from 85168ca to bf7aaa9 Compare July 25, 2024 01:12

stevenzwu mentioned this pull request Jul 25, 2024

Introduces the new IcebergSink based on the new V2 Flink Sink Abstraction #10179

Merged

stevenzwu mentioned this pull request Jul 26, 2024

Initial committer guidelines and requirements for merging #10780

Merged

rodmeneses reviewed Jul 30, 2024

View reviewed changes

rodmeneses approved these changes Jul 30, 2024

View reviewed changes

stevenzwu added 2 commits July 30, 2024 13:52

Address Rod's review comments

b7f39a2

stevenzwu force-pushed the refactor-sink-tests branch from bf7aaa9 to b7f39a2 Compare July 30, 2024 21:10

Moved 3 more tests out of TestFlinkIcebergSink to a new TestFlinkIceb…

db4ccd8

…ergSinkExtended without combinational parameters

pvary reviewed Jul 31, 2024

View reviewed changes

pvary approved these changes Aug 2, 2024

View reviewed changes

stevenzwu merged commit dc7ad71 into apache:main Aug 2, 2024
24 checks passed

stevenzwu deleted the refactor-sink-tests branch August 19, 2024 22:27

stevenzwu added a commit to stevenzwu/iceberg that referenced this pull request Aug 19, 2024

Flink: backport PR apache#10777 from 1.19 to 1.18 for sink test refac…

cf5985a

…toring. 1.20 porting is not needed as the 1.20 code was based on the hash that already included PR apache#10777.

stevenzwu mentioned this pull request Aug 19, 2024

Flink: backport PR #10777 from 1.19 to 1.18 for sink test refactoring. #10965

Merged

stevenzwu added a commit that referenced this pull request Aug 22, 2024

Flink: backport PR #10777 from 1.19 to 1.18 for sink test refactoring. (

bf459ee

#10965)

zachdisc pushed a commit to zachdisc/iceberg that referenced this pull request Dec 23, 2024

Flink: refactor sink tests to reduce the number of combinations with …

05b1ba5

…parameterized tests (apache#10777)

zachdisc pushed a commit to zachdisc/iceberg that referenced this pull request Dec 23, 2024

Flink: backport PR apache#10777 from 1.19 to 1.18 for sink test refac…

c2c3197

…toring. (apache#10965)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Flink: refactor sink tests to reduce the number of combinations with parameterized tests #10777

Flink: refactor sink tests to reduce the number of combinations with parameterized tests #10777

stevenzwu commented Jul 25, 2024 •

edited

Loading

stevenzwu Jul 25, 2024

stevenzwu Jul 25, 2024

stevenzwu Jul 25, 2024

stevenzwu Jul 25, 2024

pvary commented Jul 25, 2024

rodmeneses Jul 30, 2024

stevenzwu Jul 30, 2024

rodmeneses Jul 30, 2024

stevenzwu Jul 30, 2024

rodmeneses Jul 30, 2024

stevenzwu Jul 30, 2024

rodmeneses Jul 30, 2024

stevenzwu Jul 30, 2024

rodmeneses left a comment

pvary Jul 31, 2024 •

edited

Loading

stevenzwu Jul 31, 2024 •

edited

Loading

pvary Aug 1, 2024

stevenzwu Aug 1, 2024

pvary Aug 1, 2024

stevenzwu Aug 2, 2024 •

edited

Loading

pvary Aug 2, 2024

stevenzwu commented Aug 2, 2024


		@TempDir protected File warehouseRoot;

		protected final String flinkDatabase = CATALOG + "." + DATABASE;

Flink: refactor sink tests to reduce the number of combinations with parameterized tests #10777

Flink: refactor sink tests to reduce the number of combinations with parameterized tests #10777

Conversation

stevenzwu commented Jul 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pvary commented Jul 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodmeneses left a comment

Choose a reason for hiding this comment

pvary Jul 31, 2024 • edited Loading

Choose a reason for hiding this comment

stevenzwu Jul 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevenzwu Aug 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevenzwu commented Aug 2, 2024

stevenzwu commented Jul 25, 2024 •

edited

Loading

pvary Jul 31, 2024 •

edited

Loading

stevenzwu Jul 31, 2024 •

edited

Loading

stevenzwu Aug 2, 2024 •

edited

Loading