xuyangzhong commented on code in PR #23349:
URL: https://github.com/apache/flink/pull/23349#discussion_r1384671836
##########
flink-table/flink-table-planner/src/test/scala/org/apache/flink/table/planner/utils/TableTestBase.scala:
##########
@@ -126,10 +118,35 @@ abstract class TableTestBase {
def verifyTableEquals(expected: Table, actual: Table): Unit = {
val expectedString =
FlinkRelOptUtil.toString(TableTestUtil.toRelNode(expected))
val actualString =
FlinkRelOptUtil.toString(TableTestUtil.toRelNode(actual))
- assertEquals(
- "Logical plans do not match",
- LogicalPlanFormatUtils.formatTempTableId(expectedString),
- LogicalPlanFormatUtils.formatTempTableId(actualString))
+ assertThat(LogicalPlanFormatUtils.formatTempTableId(actualString))
Review Comment:
The error message seems to be lost. What about using
`org.junit.jupiter.api.Assertions#assertEquals`?
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/plan/rules/logical/PushFilterIntoTableSourceScanRuleTest.java:
##########
@@ -30,15 +30,14 @@
import org.apache.calcite.plan.hep.HepMatchOrder;
import org.apache.calcite.rel.rules.CoreRules;
import org.apache.calcite.tools.RuleSets;
-import org.junit.Before;
-import org.junit.Test;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
/** Test for {@link PushFilterIntoTableSourceScanRule}. */
-public class PushFilterIntoTableSourceScanRuleTest
- extends PushFilterIntoTableSourceScanRuleTestBase {
+class PushFilterIntoTableSourceScanRuleTest extends
PushFilterIntoTableSourceScanRuleTestBase {
Review Comment:
ditto, this class only has some unresolved test cases where the 'public'
need to be removed.
##########
flink-table/flink-table-planner/src/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"
Review Comment:
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.
##########
flink-table/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 =
Review Comment:
nit, revert this unnecessary changes.
##########
flink-table/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
Review Comment:
nit, use `assertTrue(file.createNewFile())` since you have intruduced
`org.junit.jupiter.api.Assertions.assertTrue` with 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-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
