[FLINK-33023][table-planner][JUnit5 Migration] Module: flink-table-planner (TableTestBase) #23349

Jiabao-Sun · 2023-09-04T08:57:08Z

What is the purpose of the change

[FLINK-33023][table-planner][JUnit5 Migration] Module: flink-table-planner (TableTestBase)

Brief change log

JUnit5 Migration Module: flink-table-planner (TableTestBase)

Verifying this change

This change is already covered by existing tests

Does this pull request potentially affect one of the following parts:

Dependencies (does it add or upgrade a dependency): no
The public API, i.e., is any changed class annotated with @Public(Evolving): no
The serializers: no
The runtime per-record code paths (performance sensitive): no
Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: no
The S3 file system connector: no

Documentation

Does this pull request introduce a new feature? no
If yes, how is the feature documented? not applicable

flinkbot · 2023-09-04T09:00:09Z

CI report:

8972348 Azure: SUCCESS

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot run azure re-run the last Azure build

Jiabao-Sun · 2023-09-04T16:09:54Z

Hi @leonardBang.
Would you mind to help review this PR when you have time?

Jiabao-Sun · 2023-09-18T07:59:28Z

@flinkbot run azure

leonardBang · 2023-11-02T04:10:36Z

@xuyangzhong Would you like to help review this PR?

xuyangzhong

Thank for your huge contribution about replacing junit4 with junit5 in table-planner module. The whole pr looks good to me. I mainly have two confusion.

Is it a good time to remove the dependency in module table-planner to avoid other contributor continuing using junit4?
I found that you spent some time to remove public for each test functions. But it is a weak constraint for contributor to following this implicit rule. How to make this rule more visible for contributor? (maybe using custom check style or something else)

xuyangzhong · 2023-11-07T10:03:10Z

.../flink-table-planner/src/test/scala/org/apache/flink/table/planner/utils/TableTestBase.scala

-      "Logical plans do not match",
-      LogicalPlanFormatUtils.formatTempTableId(expectedString),
-      LogicalPlanFormatUtils.formatTempTableId(actualString))
+    assertThat(LogicalPlanFormatUtils.formatTempTableId(actualString))


The error message seems to be lost. What about using org.junit.jupiter.api.Assertions#assertEquals?

xuyangzhong · 2023-11-07T10:09:42Z

.../flink-table-planner/src/test/scala/org/apache/flink/table/planner/utils/TableTestBase.scala

@@ -621,8 +638,9 @@ abstract class TableTestUtilBase(test: TableTestBase, isStreamingMode: Boolean)
    val optimizedRel = getPlanner.optimize(relNode)
    val optimizedPlan = getOptimizedRelPlan(Array(optimizedRel), Array.empty, withRowType = false)
    val result = notExpected.forall(!optimizedPlan.contains(_))
-    val message = s"\nactual plan:\n$optimizedPlan\nnot expected:\n${notExpected.mkString(", ")}"
-    assertTrue(message, result)
+    val message: String =


nit, revert this unnecessary changes.

xuyangzhong · 2023-11-07T10:19:32Z

.../flink-table-planner/src/test/scala/org/apache/flink/table/planner/utils/TableTestBase.scala

@@ -835,28 +848,26 @@ abstract class TableTestUtilBase(test: TableTestBase, isStreamingMode: Boolean)
    if (!file.exists() || "true".equalsIgnoreCase(System.getenv(PLAN_TEST_FORCE_OVERWRITE))) {
      Files.deleteIfExists(path)
      file.getParentFile.mkdirs()
-      assertTrue(file.createNewFile())
+      assertThat(file.createNewFile()).isTrue


nit, use assertTrue(file.createNewFile()) since you have intruduced org.junit.jupiter.api.Assertions.assertTrue with junit5.

xuyangzhong · 2023-11-07T11:15:44Z

...c/test/java/org/apache/flink/table/planner/plan/nodes/exec/stream/CorrelateJsonPlanTest.java

@@ -103,7 +103,7 @@ public void testCrossJoinOverrideParameters() {
    }

    @Test
-    public void testLeftOuterJoinWithLiteralTrue() {
+    void testLeftOuterJoinWithLiteralTrue() {
        String sinkTableDdl =
                "CREATE TABLE MySink (\n"
                        + "  a varchar,\n"


nit, also remove the public in function testJoinWithFilter() below.
BTW, maybe we need to consider how to make the weak constraint about removing public in test functions more conspicuous.

xuyangzhong · 2023-11-07T11:28:39Z

...org/apache/flink/table/planner/plan/rules/logical/PushFilterIntoTableSourceScanRuleTest.java


 /** Test for {@link PushFilterIntoTableSourceScanRule}. */
-public class PushFilterIntoTableSourceScanRuleTest
-        extends PushFilterIntoTableSourceScanRuleTestBase {
+class PushFilterIntoTableSourceScanRuleTest extends PushFilterIntoTableSourceScanRuleTestBase {


ditto, this class only has some unresolved test cases where the 'public' need to be removed.

Jiabao-Sun · 2023-11-09T04:26:57Z

Thank for your huge contribution about replacing junit4 with junit5 in table-planner module. The whole pr looks good to me. I mainly have two confusion.

Is it a good time to remove the dependency in module table-planner to avoid other contributor continuing using junit4?

I found that you spent some time to remove public for each test functions. But it is a weak constraint for contributor to following this implicit rule. How to make this rule more visible for contributor? (maybe using custom check style or something else)

Thanks @xuyangzhong for the hard review.

Currently we can't remove junit4 dependencies from table-planner, because there are still some tests using Junit4 and have not been migrated to complete. After all the tests are migrated by Junit5, we can do this.
I think we can add this check to ARCH Tests, but it may still need to wait until Junit5 is fully migrated.

Please help check it again when you have time.
Thanks again.

xuyangzhong · 2023-11-09T07:19:52Z

LGTM. cc @leonardBang for the final check.

leonardBang

Thanks @Jiabao-Sun for the contribution and @xuyangzhong for the review work, LGTM

…anner/planner/utils (TableTestBase)

…anner/api (TableTestBase)

…rs/flink-connector-hive (TableTestBase)

…anner/connector (TableTestBase)

…anner/planner/alias (TableTestBase)

…anner/planner/analyze (TableTestBase)

…anner/planner/catalog,expressions,runtime (TableTestBase)

…anner/planner/plan (TableTestBase)

Jiabao-Sun force-pushed the junit5-table-test-base branch from 594e1f3 to 4542bbc Compare September 11, 2023 03:58

leonardBang self-requested a review September 11, 2023 17:42

Jiabao-Sun force-pushed the junit5-table-test-base branch 2 times, most recently from 19b263b to c4202ee Compare September 14, 2023 12:45

Jiabao-Sun force-pushed the junit5-table-test-base branch from c4202ee to 332905c Compare September 27, 2023 15:09

Jiabao-Sun force-pushed the junit5-table-test-base branch 3 times, most recently from fa07af0 to c59b139 Compare October 9, 2023 12:02

xuyangzhong reviewed Nov 7, 2023

View reviewed changes

Jiabao-Sun force-pushed the junit5-table-test-base branch from c59b139 to 244c104 Compare November 9, 2023 04:21

xuyangzhong approved these changes Nov 9, 2023

View reviewed changes

Jiabao-Sun force-pushed the junit5-table-test-base branch from 244c104 to 7da0d9b Compare November 9, 2023 08:27

leonardBang approved these changes Nov 9, 2023

View reviewed changes

Jiabao-Sun added 8 commits November 10, 2023 11:28

[FLINK-33023][table-planner][JUnit5 Migration] Module: flink-table-pl…

1787563

…anner/planner/utils (TableTestBase)

[FLINK-33023][table-planner][JUnit5 Migration] Module: flink-table-pl…

42fed0a

…anner/api (TableTestBase)

[FLINK-33023][table-planner][JUnit5 Migration] Module: flink-connecto…

42509b1

…rs/flink-connector-hive (TableTestBase)

[FLINK-33023][table-planner][JUnit5 Migration] Module: flink-table-pl…

2f6abe5

…anner/connector (TableTestBase)

[FLINK-33023][table-planner][JUnit5 Migration] Module: flink-table-pl…

94f5b42

…anner/planner/alias (TableTestBase)

[FLINK-33023][table-planner][JUnit5 Migration] Module: flink-table-pl…

eac5549

…anner/planner/analyze (TableTestBase)

[FLINK-33023][table-planner][JUnit5 Migration] Module: flink-table-pl…

92fc7ee

…anner/planner/catalog,expressions,runtime (TableTestBase)

[FLINK-33023][table-planner][JUnit5 Migration] Module: flink-table-pl…

8972348

…anner/planner/plan (TableTestBase)

Jiabao-Sun force-pushed the junit5-table-test-base branch from 7da0d9b to 8972348 Compare November 10, 2023 03:32

leonardBang merged commit cdb759b into apache:master Nov 10, 2023

flinkbot added the component=TableSQL/Planner label Apr 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-33023][table-planner][JUnit5 Migration] Module: flink-table-planner (TableTestBase) #23349

[FLINK-33023][table-planner][JUnit5 Migration] Module: flink-table-planner (TableTestBase) #23349

Jiabao-Sun commented Sep 4, 2023 •

edited

flinkbot commented Sep 4, 2023 •

edited

Jiabao-Sun commented Sep 4, 2023

Jiabao-Sun commented Sep 18, 2023

leonardBang commented Nov 2, 2023

xuyangzhong left a comment

xuyangzhong Nov 7, 2023

xuyangzhong Nov 7, 2023

xuyangzhong Nov 7, 2023

xuyangzhong Nov 7, 2023

xuyangzhong Nov 7, 2023

Jiabao-Sun commented Nov 9, 2023

xuyangzhong commented Nov 9, 2023

leonardBang left a comment

[FLINK-33023][table-planner][JUnit5 Migration] Module: flink-table-planner (TableTestBase) #23349

[FLINK-33023][table-planner][JUnit5 Migration] Module: flink-table-planner (TableTestBase) #23349

Conversation

Jiabao-Sun commented Sep 4, 2023 • edited

What is the purpose of the change

Brief change log

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

flinkbot commented Sep 4, 2023 • edited

CI report:

Jiabao-Sun commented Sep 4, 2023

Jiabao-Sun commented Sep 18, 2023

leonardBang commented Nov 2, 2023

xuyangzhong left a comment

Choose a reason for hiding this comment

xuyangzhong Nov 7, 2023

Choose a reason for hiding this comment

xuyangzhong Nov 7, 2023

Choose a reason for hiding this comment

xuyangzhong Nov 7, 2023

Choose a reason for hiding this comment

xuyangzhong Nov 7, 2023

Choose a reason for hiding this comment

xuyangzhong Nov 7, 2023

Choose a reason for hiding this comment

Jiabao-Sun commented Nov 9, 2023

xuyangzhong commented Nov 9, 2023

leonardBang left a comment

Choose a reason for hiding this comment

Jiabao-Sun commented Sep 4, 2023 •

edited

flinkbot commented Sep 4, 2023 •

edited