luoyuxia commented on code in PR #23358:
URL: https://github.com/apache/flink/pull/23358#discussion_r1319321344
##########
flink-table/flink-table-planner/src/test/scala/org/apache/flink/table/planner/expressions/DecimalTypeTest.scala:
##########
@@ -385,11 +385,11 @@ class DecimalTypeTest extends ExpressionTestBase {
testAllApis((-'f51).round(1), "round(-f51,1)", "-100.0")
- testAllApis(('f51).round(-1), "round(f51,-1)", "100")
Review Comment:
Why change this
##########
flink-table/flink-table-planner/src/test/scala/org/apache/flink/table/planner/expressions/ArrayTypeTest.scala:
##########
@@ -21,7 +21,8 @@ import org.apache.flink.table.api._
import org.apache.flink.table.planner.expressions.utils.ArrayTypeTestBase
import org.apache.flink.table.planner.utils.DateTimeTestUtil.{localDate,
localDateTime, localTime => gLocalTime}
-import org.junit.Test
+import org.assertj.core.api.Assertions.{assertThatExceptionOfType,
assertThatThrownBy}
Review Comment:
`Unused import statement` warning in my ide
##########
flink-table/flink-table-planner/src/test/scala/org/apache/flink/table/planner/expressions/DecimalTypeTest.scala:
##########
@@ -385,11 +385,11 @@ class DecimalTypeTest extends ExpressionTestBase {
testAllApis((-'f51).round(1), "round(-f51,1)", "-100.0")
- testAllApis(('f51).round(-1), "round(f51,-1)", "100")
+ testAllApis('f51.round(-1), "round(f51,-1)", "100")
testAllApis((-'f51).round(-1), "round(-f51,-1)", "-100")
- testAllApis(('f52).round(-1), "round(f52,-1)", "NULL")
Review Comment:
dito
##########
flink-table/flink-table-planner/src/test/scala/org/apache/flink/table/planner/expressions/utils/ExpressionTestBase.scala:
##########
@@ -138,33 +131,28 @@ abstract class ExpressionTestBase(isStreaming: Boolean =
true) {
invalidTableApiExprs.clear()
}
- @After
+ @AfterEach
def evaluateExprs(): Unit = {
// evaluate valid expressions
evaluateGivenExprs(validExprs)
// evaluate invalid expressions
invalidSqlExprs.foreach {
- case (sqlExpr, keywords, clazz) => {
- try {
+ case (sqlExpr, keywords, clazz) =>
+ val callable: ThrowingCallable = () => {
val invalidExprs = mutable.ArrayBuffer[(String, RexNode, String)]()
addSqlTestExpr(sqlExpr, keywords, invalidExprs, clazz)
evaluateGivenExprs(invalidExprs)
- fail(s"Expected a $clazz, but no exception is thrown.")
- } catch {
- case e if e.getClass == clazz =>
- if (keywords != null) {
- assertTrue(
- s"The actual exception message \n${e.getMessage}\n" +
- s"doesn't contain expected keyword \n$keywords\n",
- e.getMessage.contains(keywords))
- }
- case e: Throwable =>
- e.printStackTrace()
Review Comment:
Can we still keep the fail message?
##########
flink-table/flink-table-planner/src/test/scala/org/apache/flink/table/planner/expressions/utils/ExpressionTestBase.scala:
##########
@@ -355,10 +341,7 @@ abstract class ExpressionTestBase(isStreaming: Boolean =
true) {
.foreach {
case ((originalExpr, optimizedExpr, expected), actual) =>
val original = if (originalExpr == null) "" else s"for:
[$originalExpr]"
- assertEquals(
- s"Wrong result $original optimized to: [$optimizedExpr]",
Review Comment:
Can we still keep the fail message?
##########
flink-table/flink-table-planner/src/test/scala/org/apache/flink/table/planner/expressions/utils/ExpressionTestBase.scala:
##########
@@ -329,9 +317,7 @@ abstract class ExpressionTestBase(isStreaming: Boolean =
true) {
val optimized = hep.findBestExp()
// throw exception if plan contains more than a calc
- if (!optimized.getInput(0).getInputs.isEmpty) {
- fail("Expression is converted into more than a Calc operation. Use a
different test method.")
Review Comment:
Can we still keep the fail message?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
