You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by GitBox <gi...@apache.org> on 2022/05/05 07:28:24 UTC
[GitHub] [flink] zentol commented on a diff in pull request #19538: [FLINK-27323][flink-table-api-java][tests] Migrate tests to JUnit5
zentol commented on code in PR #19538:
URL: https://github.com/apache/flink/pull/19538#discussion_r865611214
##########
flink-table/flink-table-api-java/src/test/java/org/apache/flink/table/typeutils/FieldInfoUtilsTest.java:
##########
@@ -25,55 +25,63 @@
import org.apache.flink.api.java.typeutils.RowTypeInfo;
import org.apache.flink.table.expressions.Expression;
-import org.junit.Test;
-import org.junit.experimental.runners.Enclosed;
-import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
+import org.junit.jupiter.api.Nested;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.TestInstance;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
import java.sql.Timestamp;
import java.util.Arrays;
-import java.util.Collection;
+import java.util.stream.Stream;
import static org.apache.flink.table.api.Expressions.$;
import static org.assertj.core.api.Assertions.assertThat;
/** Test suite for {@link FieldInfoUtils}. */
-@RunWith(Enclosed.class)
-public class FieldInfoUtilsTest {
+class FieldInfoUtilsTest {
/** Test for ByNameMode. */
- @RunWith(Parameterized.class)
- public static final class TestByNameMode {
-
- @Parameterized.Parameters(name = "{0}")
- public static Collection<TypeInformation> parameters() throws Exception {
- return Arrays.asList(
- new RowTypeInfo(
- new TypeInformation[] {Types.INT, Types.LONG, Types.SQL_TIMESTAMP},
- new String[] {"f0", "f1", "f2"}),
- new PojoTypeInfo(
- MyPojo.class,
- Arrays.asList(
- new PojoField(MyPojo.class.getDeclaredField("f0"), Types.INT),
- new PojoField(MyPojo.class.getDeclaredField("f1"), Types.LONG),
- new PojoField(
- MyPojo.class.getDeclaredField("f2"),
- Types.SQL_TIMESTAMP))));
+ @Nested
+ @TestInstance(TestInstance.Lifecycle.PER_CLASS)
+ class TestByNameMode {
Review Comment:
Are these nested tests connected in any way? I'm wondering if we could just split them into 2 test cases.
##########
flink-table/flink-table-api-java/src/test/resources/META-INF/services/org.junit.jupiter.api.extension.Extension:
##########
@@ -0,0 +1,16 @@
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+org.apache.flink.util.TestLoggerExtension
Review Comment:
this needs to be excluded from the test-jar as it otherwise leaks into several other JUnit4 modules.
##########
flink-table/flink-table-api-scala-bridge/src/test/scala/org/apache/flink/table/types/TypeInfoDataTypeConverterScalaTest.scala:
##########
@@ -23,54 +23,55 @@ import org.apache.flink.table.types.TypeInfoDataTypeConverterTest.TestSpec
import org.apache.flink.table.types.utils.DataTypeFactoryMock.dummyRaw
import org.apache.flink.table.types.utils.TypeInfoDataTypeConverter
-import org.hamcrest.CoreMatchers.equalTo
-import org.junit.Assert.assertThat
-import org.junit.Test
-import org.junit.runner.RunWith
-import org.junit.runners.Parameterized
-import org.junit.runners.Parameterized.Parameters
+import org.assertj.core.api.Assertions.assertThat
+import org.junit.jupiter.params.ParameterizedTest
+import org.junit.jupiter.params.provider.{Arguments, MethodSource}
+
+import java.util.stream
/** Scala tests for [[TypeInfoDataTypeConverter]]. */
-@RunWith(classOf[Parameterized])
-class TypeInfoDataTypeConverterScalaTest(testSpec: TypeInfoDataTypeConverterTest.TestSpec) {
+class TypeInfoDataTypeConverterScalaTest {
- @Test
- def testScalaConversion(): Unit = {
+ @ParameterizedTest
+ @MethodSource(Array("testData"))
+ def testScalaConversion(testSpec: TestSpec): Unit = {
val dataType = TypeInfoDataTypeConverter.toDataType(testSpec.typeFactory, testSpec.typeInfo)
- assertThat(dataType, equalTo(testSpec.expectedDataType))
+ assertThat(dataType).isEqualTo(testSpec.expectedDataType)
}
+
}
object TypeInfoDataTypeConverterScalaTest {
-
- @Parameters
- def testData: Array[TestSpec] = Array(
- TestSpec
- .forType(createTypeInformation[ScalaCaseClass])
- .expectDataType(
- DataTypes
- .STRUCTURED(
- classOf[ScalaCaseClass],
- DataTypes.FIELD(
- "primitiveIntField",
- DataTypes.INT().notNull().bridgedTo(java.lang.Integer.TYPE)),
- DataTypes.FIELD("doubleField", DataTypes.DOUBLE().notNull()),
- DataTypes.FIELD("stringField", DataTypes.STRING().nullable())
- )
- .notNull()),
- TestSpec
- .forType(createTypeInformation[(java.lang.Double, java.lang.Float)])
- .expectDataType(
- DataTypes
- .STRUCTURED(
- classOf[(_, _)],
- DataTypes.FIELD("_1", DataTypes.DOUBLE().notNull()),
- DataTypes.FIELD("_2", DataTypes.FLOAT().notNull()))
- .notNull()),
- TestSpec
- .forType(createTypeInformation[Unit])
- .lookupExpects(classOf[Unit])
- .expectDataType(dummyRaw(classOf[Unit]))
+ def testData(): stream.Stream[Arguments] = java.util.stream.Stream.of(
Review Comment:
You don't need to wrap things in `Arguments` if there is only a single parameter.
##########
flink-connectors/flink-connector-hive/src/test/java/org/apache/flink/table/catalog/hive/HiveCatalogHiveMetadataTest.java:
##########
@@ -270,13 +267,18 @@ public void testCreateTableWithConstraints() throws Exception {
new CatalogTableImpl(builder.build(), getBatchTableProperties(), null),
false);
CatalogTable catalogTable = (CatalogTable) hiveCatalog.getTable(path1);
- assertTrue("PK not present", catalogTable.getSchema().getPrimaryKey().isPresent());
+ assertThat(catalogTable.getSchema().getPrimaryKey().isPresent())
+ .as("PK not present")
+ .isTrue();
Review Comment:
```suggestion
assertThat(catalogTable.getSchema().getPrimaryKey())
.as("PK not present")
.isPresent();
```
##########
flink-connectors/flink-connector-hive/src/test/java/org/apache/flink/table/catalog/hive/HiveCatalogHiveMetadataTest.java:
##########
@@ -303,13 +305,13 @@ public void testAlterPartition() throws Exception {
catalog.alterPartition(path1, createPartitionSpec(), another, false);
- assertEquals(
- Collections.singletonList(createPartitionSpec()), catalog.listPartitions(path1));
+ assertThat(catalog.listPartitions(path1))
+ .isEqualTo(Collections.singletonList(createPartitionSpec()));
Review Comment:
containsExactly?
##########
flink-connectors/flink-connector-hive/src/test/java/org/apache/flink/table/catalog/hive/HiveCatalogHiveMetadataTest.java:
##########
@@ -92,23 +89,23 @@ public void testCreateTable_Streaming() throws Exception {}
@Test
// verifies that input/output formats and SerDe are set for Hive tables
- public void testCreateTable_StorageFormatSet() throws Exception {
+ void testCreateTable_StorageFormatSet() throws Exception {
catalog.createDatabase(db1, createDb(), false);
catalog.createTable(path1, createTable(), false);
Table hiveTable = ((HiveCatalog) catalog).getHiveTable(path1);
String inputFormat = hiveTable.getSd().getInputFormat();
String outputFormat = hiveTable.getSd().getOutputFormat();
String serde = hiveTable.getSd().getSerdeInfo().getSerializationLib();
- assertFalse(StringUtils.isNullOrWhitespaceOnly(inputFormat));
- assertFalse(StringUtils.isNullOrWhitespaceOnly(outputFormat));
- assertFalse(StringUtils.isNullOrWhitespaceOnly(serde));
+ assertThat(StringUtils.isNullOrWhitespaceOnly(inputFormat)).isFalse();
+ assertThat(StringUtils.isNullOrWhitespaceOnly(outputFormat)).isFalse();
+ assertThat(StringUtils.isNullOrWhitespaceOnly(serde)).isFalse();
Review Comment:
```suggestion
assertThat(inputFormat).isNotBlank();
assertThat(outputFormat).isNotBlank();
assertThat(serde).isNotBlank();
```
##########
flink-table/flink-table-common/src/test/java/org/apache/flink/table/catalog/CatalogTest.java:
##########
@@ -101,7 +98,7 @@ public void cleanup() throws Exception {
}
}
- @AfterClass
+ @AfterAll
public static void closeup() {
Review Comment:
```suggestion
static void closeup() {
```
##########
flink-table/flink-table-common/src/test/java/org/apache/flink/table/catalog/CatalogTest.java:
##########
@@ -50,6 +48,7 @@
import java.util.Optional;
import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
/** Class for unit tests to run on catalogs. */
public abstract class CatalogTest {
Review Comment:
```suggestion
/** Class for unit tests to run on catalogs. */
@ExtendWith(TestLoggerExtension.class)
public abstract class CatalogTest {
```
##########
flink-table/flink-table-api-java/src/test/java/org/apache/flink/table/typeutils/FieldInfoUtilsTest.java:
##########
@@ -83,20 +91,21 @@ public void testByNameModeReorderAndRename() {
assertThat(schema.getFieldNames()).isEqualTo(new String[] {"aa", "bb", "cc"});
}
+ }
- /** Test Pojo class. */
- public static class MyPojo {
- public int f0;
- public long f1;
- public Timestamp f2;
+ /** Test Pojo class. */
+ public static class MyPojo {
+ public int f0;
+ public long f1;
+ public Timestamp f2;
- public MyPojo() {}
- }
+ public MyPojo() {}
}
/** Test for ByPositionMode. */
- public static final class TestByPositionMode {
- private static final RowTypeInfo rowTypeInfo =
+ @Nested
+ public final class TestByPositionMode {
Review Comment:
```suggestion
final class TestByPositionMode {
```
?
##########
flink-table/flink-table-api-java/src/test/java/org/apache/flink/table/typeutils/FieldInfoUtilsTest.java:
##########
@@ -25,55 +25,63 @@
import org.apache.flink.api.java.typeutils.RowTypeInfo;
import org.apache.flink.table.expressions.Expression;
-import org.junit.Test;
-import org.junit.experimental.runners.Enclosed;
-import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
+import org.junit.jupiter.api.Nested;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.TestInstance;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
import java.sql.Timestamp;
import java.util.Arrays;
-import java.util.Collection;
+import java.util.stream.Stream;
import static org.apache.flink.table.api.Expressions.$;
import static org.assertj.core.api.Assertions.assertThat;
/** Test suite for {@link FieldInfoUtils}. */
-@RunWith(Enclosed.class)
-public class FieldInfoUtilsTest {
+class FieldInfoUtilsTest {
/** Test for ByNameMode. */
- @RunWith(Parameterized.class)
- public static final class TestByNameMode {
-
- @Parameterized.Parameters(name = "{0}")
- public static Collection<TypeInformation> parameters() throws Exception {
- return Arrays.asList(
- new RowTypeInfo(
- new TypeInformation[] {Types.INT, Types.LONG, Types.SQL_TIMESTAMP},
- new String[] {"f0", "f1", "f2"}),
- new PojoTypeInfo(
- MyPojo.class,
- Arrays.asList(
- new PojoField(MyPojo.class.getDeclaredField("f0"), Types.INT),
- new PojoField(MyPojo.class.getDeclaredField("f1"), Types.LONG),
- new PojoField(
- MyPojo.class.getDeclaredField("f2"),
- Types.SQL_TIMESTAMP))));
+ @Nested
+ @TestInstance(TestInstance.Lifecycle.PER_CLASS)
+ class TestByNameMode {
+
+ Stream<Arguments> parameters() throws Exception {
+ return Stream.of(
+ Arguments.of(
Review Comment:
unnecessary wrapping
##########
flink-table/flink-table-api-java/src/test/java/org/apache/flink/table/typeutils/FieldInfoUtilsTest.java:
##########
@@ -25,55 +25,63 @@
import org.apache.flink.api.java.typeutils.RowTypeInfo;
import org.apache.flink.table.expressions.Expression;
-import org.junit.Test;
-import org.junit.experimental.runners.Enclosed;
-import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
+import org.junit.jupiter.api.Nested;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.TestInstance;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
import java.sql.Timestamp;
import java.util.Arrays;
-import java.util.Collection;
+import java.util.stream.Stream;
import static org.apache.flink.table.api.Expressions.$;
import static org.assertj.core.api.Assertions.assertThat;
/** Test suite for {@link FieldInfoUtils}. */
-@RunWith(Enclosed.class)
-public class FieldInfoUtilsTest {
+class FieldInfoUtilsTest {
/** Test for ByNameMode. */
- @RunWith(Parameterized.class)
- public static final class TestByNameMode {
-
- @Parameterized.Parameters(name = "{0}")
- public static Collection<TypeInformation> parameters() throws Exception {
- return Arrays.asList(
- new RowTypeInfo(
- new TypeInformation[] {Types.INT, Types.LONG, Types.SQL_TIMESTAMP},
- new String[] {"f0", "f1", "f2"}),
- new PojoTypeInfo(
- MyPojo.class,
- Arrays.asList(
- new PojoField(MyPojo.class.getDeclaredField("f0"), Types.INT),
- new PojoField(MyPojo.class.getDeclaredField("f1"), Types.LONG),
- new PojoField(
- MyPojo.class.getDeclaredField("f2"),
- Types.SQL_TIMESTAMP))));
+ @Nested
+ @TestInstance(TestInstance.Lifecycle.PER_CLASS)
+ class TestByNameMode {
+
+ Stream<Arguments> parameters() throws Exception {
Review Comment:
I think these can be private.
##########
flink-connectors/flink-connector-hive/src/test/java/org/apache/flink/table/catalog/hive/HiveCatalogGenericMetadataTest.java:
##########
@@ -332,38 +331,38 @@ public void testGenericTableWithoutConnectorProp() throws Exception {
CatalogTable catalogTable = new CatalogTableImpl(tableSchema, Collections.emptyMap(), null);
catalog.createTable(path1, catalogTable, false);
CatalogTable retrievedTable = (CatalogTable) catalog.getTable(path1);
- assertEquals(tableSchema, retrievedTable.getSchema());
- assertEquals(Collections.emptyMap(), retrievedTable.getOptions());
+ assertThat(retrievedTable.getSchema()).isEqualTo(tableSchema);
+ assertThat(retrievedTable.getOptions()).isEqualTo(Collections.emptyMap());
Review Comment:
```suggestion
assertThat(retrievedTable.getOptions()).isEmpty();
```
##########
flink-connectors/flink-connector-hive/src/test/java/org/apache/flink/table/catalog/hive/HiveCatalogHiveMetadataTest.java:
##########
@@ -288,11 +290,11 @@ public void testAlterPartition() throws Exception {
catalog.createTable(path1, createPartitionedTable(), false);
catalog.createPartition(path1, createPartitionSpec(), createPartition(), false);
- assertEquals(
- Collections.singletonList(createPartitionSpec()), catalog.listPartitions(path1));
+ assertThat(catalog.listPartitions(path1))
+ .isEqualTo(Collections.singletonList(createPartitionSpec()));
Review Comment:
containsExactly?
##########
flink-table/flink-table-common/src/test/java/org/apache/flink/table/catalog/CatalogTest.java:
##########
@@ -1130,8 +1151,9 @@ public void testListPartitionPartialSpec() throws Exception {
@Test
public void testGetTableStats_TableNotExistException() throws Exception {
catalog.createDatabase(db1, createDb(), false);
- exception.expect(org.apache.flink.table.catalog.exceptions.TableNotExistException.class);
- catalog.getTableStatistics(path1);
+ assertThatThrownBy(() -> catalog.getTableStatistics(path1))
+ .isInstanceOf(
+ org.apache.flink.table.catalog.exceptions.TableNotExistException.class);
Review Comment:
```suggestion
.isInstanceOf(TableNotExistException.class);
```
This seems to be the same class as in other tests.
##########
flink-table/flink-table-common/src/test/java/org/apache/flink/table/catalog/CatalogTest.java:
##########
@@ -74,9 +73,7 @@ public abstract class CatalogTest {
protected static Catalog catalog;
- @Rule public ExpectedException exception = ExpectedException.none();
-
- @After
+ @AfterEach
public void cleanup() throws Exception {
Review Comment:
```suggestion
void cleanup() throws Exception {
```
--
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@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org