morazow commented on code in PR #3742:
URL: https://github.com/apache/flink-cdc/pull/3742#discussion_r1865045505
##########
flink-cdc-connect/flink-cdc-source-connectors/flink-connector-mongodb-cdc/src/test/java/org/apache/flink/cdc/connectors/mongodb/source/MongoDBFullChangelogITCase.java:
##########
@@ -66,160 +66,151 @@
import static org.apache.flink.table.api.DataTypes.STRING;
import static org.apache.flink.table.catalog.Column.physical;
import static org.apache.flink.util.Preconditions.checkState;
-import static org.junit.Assert.assertEquals;
+import static org.assertj.core.api.Assumptions.assumeThat;
/** Integration tests for MongoDB full document before change info. */
-@RunWith(Parameterized.class)
-public class MongoDBFullChangelogITCase extends MongoDBSourceTestBase {
+@Timeout(300)
+class MongoDBFullChangelogITCase extends MongoDBSourceTestBase {
private static final int USE_POST_LOWWATERMARK_HOOK = 1;
private static final int USE_PRE_HIGHWATERMARK_HOOK = 2;
private static final int USE_POST_HIGHWATERMARK_HOOK = 3;
- @Rule public final Timeout timeoutPerTest = Timeout.seconds(300);
-
- private final String mongoVersion;
- private final boolean parallelismSnapshot;
-
- public MongoDBFullChangelogITCase(String mongoVersion, boolean
parallelismSnapshot) {
- super(mongoVersion);
- this.mongoVersion = mongoVersion;
- this.parallelismSnapshot = parallelismSnapshot;
- }
-
- @Parameterized.Parameters(name = "mongoVersion: {0} parallelismSnapshot:
{1}")
- public static Object[] parameters() {
- List<Object[]> parameterTuples = new ArrayList<>();
- for (String mongoVersion : getMongoVersions()) {
- parameterTuples.add(new Object[] {mongoVersion, true});
- parameterTuples.add(new Object[] {mongoVersion, false});
- }
- return parameterTuples.toArray();
- }
-
@Test
- public void testGetMongoDBVersion() {
+ void testGetMongoDBVersion() {
MongoDBSourceConfig config =
new MongoDBSourceConfigFactory()
- .hosts(mongoContainer.getHostAndPort())
+ .hosts(MONGO_CONTAINER.getHostAndPort())
.splitSizeMB(1)
.samplesPerChunk(10)
.pollAwaitTimeMillis(500)
.create(0);
- assertEquals(MongoUtils.getMongoVersion(config), mongoVersion);
+
Assertions.assertThat(MongoUtils.getMongoVersion(config)).isEqualTo(getMongoVersion());
}
- @Test
- public void testReadSingleCollectionWithSingleParallelism() throws
Exception {
+ @ParameterizedTest(name = "parallelismSnapshot: {0}")
+ @ValueSource(booleans = {true, false})
+ void testReadSingleCollectionWithSingleParallelism(boolean
parallelismSnapshot)
+ throws Exception {
testMongoDBParallelSource(
1,
MongoDBTestUtils.FailoverType.NONE,
MongoDBTestUtils.FailoverPhase.NEVER,
- new String[] {"customers"});
+ new String[] {"customers"},
+ parallelismSnapshot);
}
- @Test
- public void testReadSingleCollectionWithMultipleParallelism() throws
Exception {
+ @ParameterizedTest(name = "parallelismSnapshot: {0}")
+ @ValueSource(booleans = {true, false})
+ void testReadSingleCollectionWithMultipleParallelism(boolean
parallelismSnapshot)
+ throws Exception {
testMongoDBParallelSource(
4,
MongoDBTestUtils.FailoverType.NONE,
MongoDBTestUtils.FailoverPhase.NEVER,
- new String[] {"customers"});
+ new String[] {"customers"},
+ parallelismSnapshot);
}
- @Test
- public void testReadMultipleCollectionWithSingleParallelism() throws
Exception {
+ @ParameterizedTest(name = "parallelismSnapshot: {0}")
+ @ValueSource(booleans = {true, false})
+ void testReadMultipleCollectionWithSingleParallelism(boolean
parallelismSnapshot)
+ throws Exception {
testMongoDBParallelSource(
1,
MongoDBTestUtils.FailoverType.NONE,
MongoDBTestUtils.FailoverPhase.NEVER,
- new String[] {"customers", "customers_1"});
+ new String[] {"customers", "customers_1"},
+ parallelismSnapshot);
}
- @Test
- public void testReadMultipleCollectionWithMultipleParallelism() throws
Exception {
+ @ParameterizedTest(name = "parallelismSnapshot: {0}")
+ @ValueSource(booleans = {true, false})
+ void testReadMultipleCollectionWithMultipleParallelism(boolean
parallelismSnapshot)
+ throws Exception {
testMongoDBParallelSource(
4,
MongoDBTestUtils.FailoverType.NONE,
MongoDBTestUtils.FailoverPhase.NEVER,
- new String[] {"customers", "customers_1"});
+ new String[] {"customers", "customers_1"},
+ parallelismSnapshot);
}
// Failover tests
- @Test
- public void testTaskManagerFailoverInSnapshotPhase() throws Exception {
- if (!parallelismSnapshot) {
- return;
- }
+ @ParameterizedTest(name = "parallelismSnapshot: {0}")
+ @ValueSource(booleans = {true, false})
+ void testTaskManagerFailoverInSnapshotPhase(boolean parallelismSnapshot)
throws Exception {
+ assumeThat(parallelismSnapshot).isTrue();
Review Comment:
Maybe remove parameterization from these tests?
##########
tools/maven/checkstyle.xml:
##########
@@ -226,7 +226,36 @@ This file is based on the checkstyle file of Apache Beam.
<property name="illegalPkgs" value="com.google.common"/>
<message key="import.illegal" value="{0}; Use
flink-shaded-guava instead."/>
</module>
-
+ <module name="IllegalImport">
+ <property name="regexp" value="true"/>
+ <!-- Reject any org.junit import that's not also
org.junit.jupiter: -->
+ <property name="illegalClasses"
value="^org\.junit\.(?!jupiter\.).+"/>
+ <message key="import.illegal" value="{0}; Please use
JUnit 5 Jupiter API instead."/>
+ </module>
+ <module name="IllegalImport">
+ <property name="regexp" value="true"/>
+ <!-- Reject any org.junit import that's not also
org.junit.jupiter: -->
+ <property name="illegalClasses"
value="^org\.junit\.jupiter\.api\.Assertions\.*"/>
+ <message key="import.illegal" value="{0}; Prefer using
org.assertj.core.api.Assertions instead."/>
+ </module>
+ <module name="IllegalImport">
+ <property name="regexp" value="true"/>
+ <!-- Reject any org.junit import that's not also
org.junit.jupiter: -->
+ <property name="illegalClasses"
value="^org\.junit\.jupiter\.api\.Assumptions\.*"/>
+ <message key="import.illegal" value="{0}; Prefer using
org.assertj.core.api.Assumptions instead."/>
+ </module>
+ <module name="IllegalImport">
+ <property name="illegalPkgs"
value="org.junit.jupiter.api.Assertions"/>
+ <message key="import.illegal" value="Prefer using
org.assertj.core.api.Assertions instead."/>
+ </module>
+ <module name="IllegalImport">
+ <property name="illegalPkgs"
value="org.junit.jupiter.api.Assumptions"/>
+ <message key="import.illegal" value="Prefer using
org.assertj.core.api.Assumptions instead."/>
+ </module>
+ <module name="IllegalImport">
+ <property name="illegalPkgs" value="org.hamcrest"/>
+ <message key="import.illegal" value="Prefer using
org.assertj.core.api.Assertions instead."/>
+ </module>
Review Comment:
nit: formatting
##########
flink-cdc-migration-tests/flink-cdc-migration-testcases/src/test/java/org/apache/flink/cdc/migration/tests/MigrationTestBase.java:
##########
@@ -111,11 +111,11 @@ protected void testMigrationFromTo(
Class<?> toVersionMockClass = getMockClass(toVersion, caseName);
Object toVersionMockObject = toVersionMockClass.newInstance();
- Assert.assertTrue(
- (boolean)
+ Assertions.assertThat(
toVersionMockClass
.getDeclaredMethod(
"deserializeAndCheckObject",
int.class, byte[].class)
- .invoke(toVersionMockObject,
serializerVersion, serializedObject));
+ .invoke(toVersionMockObject,
serializerVersion, serializedObject))
+ .isEqualTo(true);
Review Comment:
Let's use `.isTrue()`?
##########
flink-cdc-runtime/src/test/java/org/apache/flink/cdc/runtime/parser/JaninoCompilerTest.java:
##########
@@ -17,39 +17,39 @@
package org.apache.flink.cdc.runtime.parser;
+import org.assertj.core.api.Assertions;
import org.codehaus.commons.compiler.CompileException;
import org.codehaus.commons.compiler.Location;
import org.codehaus.janino.ExpressionEvaluator;
import org.codehaus.janino.Java;
import org.codehaus.janino.Parser;
import org.codehaus.janino.Scanner;
import org.codehaus.janino.Unparser;
-import org.junit.Assert;
-import org.junit.Test;
+import org.junit.jupiter.api.Test;
import java.io.IOException;
import java.io.StringReader;
import java.io.StringWriter;
import java.lang.reflect.InvocationTargetException;
import java.util.ArrayList;
-import java.util.Arrays;
+import java.util.Collections;
import java.util.List;
/** Unit tests for the {@link JaninoCompiler}. */
-public class JaninoCompilerTest {
+class JaninoCompilerTest {
@Test
- public void testJaninoParser() throws CompileException, IOException,
InvocationTargetException {
+ void testJaninoParser() throws CompileException, IOException,
InvocationTargetException {
String expression = "1==2";
Parser parser = new Parser(new Scanner(null, new
StringReader(expression)));
ExpressionEvaluator expressionEvaluator = new ExpressionEvaluator();
expressionEvaluator.cook(parser);
Object evaluate = expressionEvaluator.evaluate();
- Assert.assertEquals(false, evaluate);
+ Assertions.assertThat(evaluate).isEqualTo(false);
Review Comment:
isFalse()
##########
flink-cdc-e2e-tests/flink-cdc-source-e2e-tests/src/test/java/org/apache/flink/cdc/connectors/tests/utils/FlinkContainerTestEnvironment.java:
##########
@@ -119,17 +117,16 @@ public abstract class FlinkContainerTestEnvironment
extends TestLogger {
private GenericContainer<?> jobManager;
private GenericContainer<?> taskManager;
- @Parameterized.Parameters(name = "flinkVersion: {0}")
- public static List<String> getFlinkVersion() {
+ public static String getFlinkVersion() {
String flinkVersion = System.getProperty("specifiedFlinkVersion");
- if (flinkVersion != null) {
- return Collections.singletonList(flinkVersion);
- } else {
- return Arrays.asList("1.19.1", "1.20.0");
+ if (Objects.isNull(flinkVersion)) {
+ throw new IllegalArgumentException(
+ "No Flink version specified to run this test. Please use
-DspecifiedFlinkVersion to pass one.");
Review Comment:
Previous test has default value. But it is good idea to throw here, since
the release cycle is very frequent nowadays 👍
##########
flink-cdc-connect/flink-cdc-source-connectors/flink-cdc-base/src/test/java/org/apache/flink/cdc/connectors/base/MySqlChangeEventSourceExampleTest.java:
##########
@@ -262,16 +257,7 @@ private void makeBinlogEvents(JdbcConnection connection,
String tableId) throws
}
public static void assertEqualsInAnyOrder(List<String> expected,
List<String> actual) {
- assertTrue(expected != null && actual != null);
- assertEqualsInOrder(
- expected.stream().sorted().collect(Collectors.toList()),
- actual.stream().sorted().collect(Collectors.toList()));
- }
-
- public static void assertEqualsInOrder(List<String> expected, List<String>
actual) {
- assertTrue(expected != null && actual != null);
- assertEquals(expected.size(), actual.size());
- assertArrayEquals(expected.toArray(new String[0]), actual.toArray(new
String[0]));
+
Assertions.assertThat(actual).containsExactlyInAnyOrderElementsOf(expected);
Review Comment:
👍
##########
flink-cdc-runtime/src/test/java/org/apache/flink/cdc/runtime/parser/JaninoCompilerTest.java:
##########
@@ -63,50 +63,50 @@ public void testJaninoUnParser() {
Unparser unparser = new Unparser(writer);
unparser.unparseAtom(binaryOperation);
unparser.close();
- Assert.assertEquals(expression, writer.toString());
+ Assertions.assertThat(writer).hasToString(expression);
}
@Test
- public void testJaninoNumericCompare() throws InvocationTargetException {
+ void testJaninoNumericCompare() throws InvocationTargetException {
String expression = "col1==3.14";
- List<String> columnNames = Arrays.asList("col1");
- List<Class<?>> paramTypes = Arrays.asList(Double.class);
- List<Object> params = Arrays.asList(3.14);
+ List<String> columnNames = Collections.singletonList("col1");
+ List<Class<?>> paramTypes = Collections.singletonList(Double.class);
+ List<Object> params = Collections.singletonList(3.14);
ExpressionEvaluator expressionEvaluator =
JaninoCompiler.compileExpression(
expression, columnNames, paramTypes, Boolean.class);
Object evaluate = expressionEvaluator.evaluate(params.toArray());
- Assert.assertEquals(true, evaluate);
+ Assertions.assertThat(evaluate).isEqualTo(true);
Review Comment:
isTrue()
and below on two tests also same
##########
flink-cdc-connect/flink-cdc-source-connectors/flink-connector-mongodb-cdc/pom.xml:
##########
@@ -173,6 +180,12 @@ limitations under the License.
<artifactId>commons-lang3</artifactId>
<version>${commons-lang3.version}</version>
</dependency>
+ <dependency>
+ <groupId>ant</groupId>
+ <artifactId>ant</artifactId>
+ <version>1.6.5</version>
+ <scope>test</scope>
+ </dependency>
Review Comment:
Is this dependency needed?
##########
flink-cdc-connect/flink-cdc-source-connectors/flink-connector-db2-cdc/src/test/java/org/apache/flink/cdc/connectors/db2/source/Db2SourceITCase.java:
##########
@@ -45,67 +46,67 @@
import static org.testcontainers.containers.Db2Container.DB2_PORT;
/** IT tests for {@link Db2IncrementalSource}. */
-public class Db2SourceITCase extends Db2TestBase {
-
- @Rule public final Timeout timeoutPerTest = Timeout.seconds(300);
-
- @Rule
- public final MiniClusterWithClientResource miniClusterResource =
- new MiniClusterWithClientResource(
- new MiniClusterResourceConfiguration.Builder()
- .setNumberTaskManagers(1)
- .setNumberSlotsPerTaskManager(DEFAULT_PARALLELISM)
- .setRpcServiceSharing(RpcServiceSharing.DEDICATED)
- .withHaLeadershipControl()
- .build());
+@Timeout(value = 300)
Review Comment:
Should we add explicitly also the time unit? Same for all other Timeout
annotations.
In Junit4 it is explicit, here we may lose that information
##########
flink-cdc-connect/flink-cdc-pipeline-connectors/flink-cdc-pipeline-connector-kafka/src/test/java/org/apache/flink/cdc/connectors/kafka/sink/KafkaDataSinkITCase.java:
##########
@@ -264,7 +264,7 @@ void testDebeziumJsonFormat() throws Exception {
final List<ConsumerRecord<byte[], byte[]>> collectedRecords =
drainAllRecordsFromTopic(topic, false, 0);
final long recordsCount = 5;
- assertThat(recordsCount).isEqualTo(collectedRecords.size());
+ assertThat(collectedRecords.size()).isEqualTo(recordsCount);
Review Comment:
It is small nit, but here you could also use `.hasSize(recordsCount)`
##########
flink-cdc-connect/flink-cdc-source-connectors/flink-connector-mysql-cdc/src/test/java/org/apache/flink/cdc/connectors/mysql/source/MySqlSourceTestBase.java:
##########
@@ -103,24 +101,15 @@ protected static MySqlContainer
createMySqlContainer(MySqlVersion version, Strin
// test utilities
// ------------------------------------------------------------------------
public static void assertEqualsInAnyOrder(List<String> expected,
List<String> actual) {
- assertTrue(expected != null && actual != null);
- assertEqualsInOrder(
- expected.stream().sorted().collect(Collectors.toList()),
- actual.stream().sorted().collect(Collectors.toList()));
+
Assertions.assertThat(actual).containsExactlyInAnyOrderElementsOf(expected);
}
public static void assertEqualsInOrder(List<String> expected, List<String>
actual) {
- assertTrue(expected != null && actual != null);
- assertEquals(expected.size(), actual.size());
- assertArrayEquals(expected.toArray(new String[0]), actual.toArray(new
String[0]));
+ Assertions.assertThat(actual).containsExactlyElementsOf(expected);
}
public static void assertMapEquals(Map<String, ?> expected, Map<String, ?>
actual) {
- assertTrue(expected != null && actual != null);
- assertEquals(expected.size(), actual.size());
- for (String key : expected.keySet()) {
- assertEquals(expected.get(key), actual.get(key));
- }
+ Assertions.assertThat(actual).isEqualTo(expected);
Review Comment:
Should this be `.containsExactly(expected.entrySet())`?
--
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]
