Flink v1.16: Switch Flink v1.16 tests to Junit5 (Part 1) #9565
Conversation
1. TestAggregatedStatistics.java 2. TestAggregatedStatisticsTracker.java 3. TestDataStatisticsCoordinator.java 4. TestDataStatisticsCoordinatorProvider.java 5. TestDataStatisticsOperator.java
1. TestAvroGenericRecordToRowDataMapper.java 2. TestRowDataPartitionKey.java
1. TestFlinkPackage.java 2. TestRowDataPartitionKey.java
|
@ilyasahsan123 we typically apply such changes on the latest version first (1.18) and then backport them in a separate PR. Could you apply these changes to Flink 1.18 first please? |
|
Sure, I will do it. Thanks |
|
hi @nastra , I've addressed your suggestion. Could you please take another look? Thanks |
| @@ -22,6 +22,10 @@ String scalaVersion = System.getProperty("scalaVersion") != null ? System.getPro | |||
|
|
|||
| project(":iceberg-flink:iceberg-flink-${flinkMajorVersion}") { | |||
|
|
|||
| test { | |||
There was a problem hiding this comment.
this is already set in L127, so it's not needed here
|
|
||
| int expectedMilliseconds = (int) ((long) expected / 1000_000); | ||
| int actualMilliseconds = (int) ((long) actual / 1000_000); | ||
| Assert.assertEquals(message, expectedMilliseconds, actualMilliseconds); | ||
| assertThat(expectedMilliseconds).as(message).isEqualTo(actualMilliseconds); |
There was a problem hiding this comment.
the actual/expected is the wrong way around. It should be assertThat(actualMilliseconds).isEqualTo(expectedMilliseconds)
| @@ -75,8 +75,8 @@ protected void generateAndValidate(Schema schema, RecordWrapperTest.AssertMethod | |||
| StructLikeWrapper actualWrapper = StructLikeWrapper.forType(schema.asStruct()); | |||
| StructLikeWrapper expectedWrapper = StructLikeWrapper.forType(schema.asStruct()); | |||
| for (int i = 0; i < numRecords; i++) { | |||
| Assert.assertTrue("Should have more records", actual.hasNext()); | |||
| Assert.assertTrue("Should have more RowData", expected.hasNext()); | |||
| assertThat(actual.hasNext()).as("Should have more records").isTrue(); | |||
There was a problem hiding this comment.
| assertThat(actual.hasNext()).as("Should have more records").isTrue(); | |
| assertThat(actual).as("Should have more records").hasNext(); |
same for the other places that use a similar pattern. We want to avoid using isTrue/isFalse as much as possible, because we want to get rich context from AssertJ when the assertion ever fails
| @@ -22,7 +22,7 @@ | |||
| import org.apache.iceberg.flink.DataGenerator; | |||
| import org.apache.iceberg.flink.DataGenerators; | |||
| import org.apache.iceberg.flink.TestHelpers; | |||
| import org.junit.Test; | |||
| import org.junit.jupiter.api.Test; | |||
|
|
|||
| public class TestStructRowData { | |||
There was a problem hiding this comment.
why not also include the other files from the data package in this PR?
| @@ -18,10 +18,11 @@ | |||
| */ | |||
| package org.apache.iceberg.flink.sink; | |||
|
|
|||
| import static org.junit.jupiter.api.Assertions.assertEquals; | |||
There was a problem hiding this comment.
wrong import. We should be using AssertJ assertions instead of the JUnit ones
| @@ -18,10 +18,11 @@ | |||
| */ | |||
| package org.apache.iceberg.flink.sink; | |||
There was a problem hiding this comment.
I would have expected to include other test files from the sink package here is well, but just picking 2 files seems quite random to me. I'd suggest to focus on one or more packages per PR (depending on how big the changes are) instead of just randomly picking files across the codebase
|
|
||
| String expectedStr = row.isNullAt(1) ? null : row.getString(1).toString(); | ||
| Assert.assertEquals(expectedStr, partitionKey.get(0, String.class)); | ||
| assertThat(expectedStr).isEqualTo(partitionKey.get(0, String.class)); |
There was a problem hiding this comment.
the ordering of actual/expected needs to be fixed here
| // recordOffset points to the position after this one | ||
| Assert.assertEquals(startingRecordOffset + i + 1, recAndPos.recordOffset()); | ||
| assertThat(elements[i]).isEqualTo(recAndPos.record()); | ||
| assertThat(fileOffset).isEqualTo(recAndPos.fileOffset()); |
There was a problem hiding this comment.
I think actual/expected are in the wrong order here as well
Issue: #9087