nastra commented on code in PR #9793:
URL: https://github.com/apache/iceberg/pull/9793#discussion_r1555322359
##########
data/src/test/java/org/apache/iceberg/data/TestDataFileIndexStatsFilters.java:
##########
@@ -137,9 +135,11 @@ public void testPositionDeletePlanningPath() throws
IOException {
tasks = Lists.newArrayList(tasksIterable);
}
- Assert.assertEquals("Should produce one task", 1, tasks.size());
+ Assertions.assertThat(1).isEqualTo(tasks.size()).as("Should produce one
task");
Review Comment:
please statically import `assertThat()`
##########
data/src/test/java/org/apache/iceberg/data/DeleteReadTests.java:
##########
@@ -314,7 +315,7 @@ public void testEqualityDeletesWithRequiredEqColumn()
throws IOException {
StructLikeSet actual = rowSet(tableName, table, "id");
if (expectPruned()) {
- assertThat(actual).as("Table should contain expected
rows").isEqualTo(expected);
+ Assertions.assertThat(actual).as("Table should contain expected
rows").isEqualTo(expected);
Review Comment:
please revert changes to this class
##########
data/src/test/java/org/apache/iceberg/data/TestDataFileIndexStatsFilters.java:
##########
@@ -163,10 +163,11 @@ public void testPositionDeletePlanningPathFilter() throws
IOException {
tasks = Lists.newArrayList(tasksIterable);
}
- Assert.assertEquals("Should produce one task", 1, tasks.size());
+ Assertions.assertThat(tasks.size()).isEqualTo(1).as("Should produce one
task");
Review Comment:
same as above
##########
data/src/test/java/org/apache/iceberg/data/TestLocalScan.java:
##########
@@ -175,67 +173,110 @@ private void overwriteExistingData() throws IOException {
.commit();
}
- private void appendData() throws IOException {
+ private void appendData(Object fileExt) throws IOException {
+ FileFormat fileFormat = FileFormat.fromString(fileExt.toString());
DataFile file12 =
- writeFile(sharedTableLocation, format.addExtension("file-12"),
file1SecondSnapshotRecords);
+ writeFile(
+ sharedTableLocation, fileFormat.addExtension("file-12"),
file1SecondSnapshotRecords);
DataFile file22 =
- writeFile(sharedTableLocation, format.addExtension("file-22"),
file2SecondSnapshotRecords);
+ writeFile(
+ sharedTableLocation, fileFormat.addExtension("file-22"),
file2SecondSnapshotRecords);
DataFile file32 =
- writeFile(sharedTableLocation, format.addExtension("file-32"),
file3SecondSnapshotRecords);
+ writeFile(
+ sharedTableLocation, fileFormat.addExtension("file-32"),
file3SecondSnapshotRecords);
sharedTable.newFastAppend().appendFile(file12).appendFile(file22).appendFile(file32).commit();
DataFile file13 =
- writeFile(sharedTableLocation, format.addExtension("file-13"),
file1ThirdSnapshotRecords);
+ writeFile(
+ sharedTableLocation, fileFormat.addExtension("file-13"),
file1ThirdSnapshotRecords);
DataFile file23 =
- writeFile(sharedTableLocation, format.addExtension("file-23"),
file2ThirdSnapshotRecords);
+ writeFile(
+ sharedTableLocation, fileFormat.addExtension("file-23"),
file2ThirdSnapshotRecords);
DataFile file33 =
- writeFile(sharedTableLocation, format.addExtension("file-33"),
file3ThirdSnapshotRecords);
+ writeFile(
+ sharedTableLocation, fileFormat.addExtension("file-33"),
file3ThirdSnapshotRecords);
sharedTable.newFastAppend().appendFile(file13).appendFile(file23).appendFile(file33).commit();
}
- @Before
+ @BeforeEach
public void createTables() throws IOException {
- File location = temp.newFolder("shared");
- Assert.assertTrue(location.delete());
+ File location = temp;
+ Assertions.assertThat(location.delete()).isTrue();
this.sharedTableLocation = location.toString();
this.sharedTable =
TABLES.create(
SCHEMA,
PartitionSpec.unpartitioned(),
- ImmutableMap.of(TableProperties.DEFAULT_FILE_FORMAT,
format.name()),
+ ImmutableMap.of(TableProperties.DEFAULT_FILE_FORMAT,
temp.getName()),
sharedTableLocation);
Record record = GenericRecord.create(SCHEMA);
- DataFile file1 =
- writeFile(sharedTableLocation, format.addExtension("file-1"),
file1FirstSnapshotRecords);
-
- Record nullData = record.copy();
- nullData.setField("id", 11L);
- nullData.setField("data", null);
-
- DataFile file2 =
- writeFile(sharedTableLocation, format.addExtension("file-2"),
file2FirstSnapshotRecords);
- DataFile file3 =
- writeFile(sharedTableLocation, format.addExtension("file-3"),
file3FirstSnapshotRecords);
-
- // commit the test data
-
sharedTable.newAppend().appendFile(file1).appendFile(file2).appendFile(file3).commit();
+ String[] format = {"parquet", "orc", "avro"};
+ Arrays.stream(format)
+ .forEach(
+ str -> {
+ FileFormat fileFormat = FileFormat.fromString(str);
+ DataFile file1 = null;
+ try {
+ file1 =
+ writeFile(
+ sharedTableLocation,
+ fileFormat.addExtension("file-1"),
+ file1FirstSnapshotRecords);
+ } catch (IOException e) {
+ throw new RuntimeException(e);
+ }
+ Record nullData = record.copy();
+ nullData.setField("id", 11L);
+ nullData.setField("data", null);
+
+ DataFile file2 = null;
+ try {
+ file2 =
+ writeFile(
+ sharedTableLocation,
+ fileFormat.addExtension("file-2"),
+ file2FirstSnapshotRecords);
+ } catch (IOException e) {
+ throw new RuntimeException(e);
+ }
+ DataFile file3 = null;
+ try {
+ file3 =
+ writeFile(
+ sharedTableLocation,
+ fileFormat.addExtension("file-3"),
+ file3FirstSnapshotRecords);
+ } catch (IOException e) {
+ throw new RuntimeException(e);
+ }
+
+ // commit the test data
+ sharedTable
+ .newAppend()
+ .appendFile(file1)
+ .appendFile(file2)
+ .appendFile(file3)
+ .commit();
+ });
}
- @Test
- public void testRandomData() throws IOException {
+ @ParameterizedTest(name = "format = {0}")
+ @MethodSource("data")
Review Comment:
this should be parameterized at the class level. You can check
`TableMetadataParserTest` as an example for how to do that. Test methods would
then get the `@TestTemplate` annotation
##########
data/src/test/java/org/apache/iceberg/data/DataTestHelpers.java:
##########
@@ -84,16 +86,16 @@ private static void assertEquals(Type type, Object
expected, Object actual) {
case UUID:
case BINARY:
case DECIMAL:
- Assert.assertEquals(
- "Primitive value should be equal to expected for type " + type,
expected, actual);
+ org.junit.jupiter.api.Assertions.assertEquals(
+ expected, actual, "Primitive value should be equal to expected for
type " + type);
break;
case FIXED:
Assertions.assertThat(expected)
.as("Expected should be a byte[]")
.isInstanceOf(byte[].class);
Assertions.assertThat(expected).as("Actual should be a
byte[]").isInstanceOf(byte[].class);
- Assert.assertArrayEquals(
- "Array contents should be equal", (byte[]) expected, (byte[])
actual);
+ org.junit.jupiter.api.Assertions.assertArrayEquals(
Review Comment:
@igoradulian this hasn't been addressed yet
##########
data/src/test/java/org/apache/iceberg/data/TestGenericRecord.java:
##########
@@ -35,7 +34,7 @@ public void testGetNullValue() {
GenericRecord record = GenericRecord.create(schema);
record.set(0, null);
- Assert.assertNull(record.get(0, type.typeId().javaClass()));
+ Assertions.assertThat(record.get(0, type.typeId().javaClass())).isNull();
Review Comment:
as previously mentioned, please use the static import of this method
##########
data/src/test/java/org/apache/iceberg/RecordWrapperTest.java:
##########
@@ -103,11 +103,11 @@ public void testNestedSchema() {
}
private void generateAndValidate(Schema schema) {
- generateAndValidate(schema, Assert::assertEquals);
+ generateAndValidate(schema, Assertions::assertThat);
}
public interface AssertMethod {
- void assertEquals(String message, StructLikeWrapper expected,
StructLikeWrapper actual);
+ void assertThat(StructLikeWrapper actual);
Review Comment:
why has this been renamed? This isn't related to JUnit4 -> JUnit5 migration
##########
data/src/test/java/org/apache/iceberg/data/TestLocalScan.java:
##########
@@ -77,19 +76,11 @@ public class TestLocalScan {
private static final Configuration CONF = new Configuration();
private static final Tables TABLES = new HadoopTables(CONF);
- @Rule public final TemporaryFolder temp = new TemporaryFolder();
-
- @Parameterized.Parameters(name = "format = {0}")
- public static Object[] parameters() {
Review Comment:
no need to rename this and change the return type. This is also missing
`@Parameters` annotation
##########
data/src/test/java/org/apache/iceberg/data/TestDataFileIndexStatsFilters.java:
##########
@@ -163,10 +163,11 @@ public void testPositionDeletePlanningPathFilter() throws
IOException {
tasks = Lists.newArrayList(tasksIterable);
}
- Assert.assertEquals("Should produce one task", 1, tasks.size());
+ Assertions.assertThat(tasks.size()).isEqualTo(1).as("Should produce one
task");
FileScanTask task = tasks.get(0);
- Assert.assertEquals(
- "Should not have delete file, filtered by file_path stats", 0,
task.deletes().size());
+ Assertions.assertThat(task.deletes().size())
+ .isEqualTo(0)
+ .as("Should not have delete file, filtered by file_path stats");
Review Comment:
`.as()` needs to come before the final assertion check. Also this should be
`assertThat(task.deletes).as(..).hasSize(..)`
##########
data/src/test/java/org/apache/iceberg/data/TestLocalScan.java:
##########
@@ -175,67 +173,110 @@ private void overwriteExistingData() throws IOException {
.commit();
}
- private void appendData() throws IOException {
+ private void appendData(Object fileExt) throws IOException {
+ FileFormat fileFormat = FileFormat.fromString(fileExt.toString());
DataFile file12 =
- writeFile(sharedTableLocation, format.addExtension("file-12"),
file1SecondSnapshotRecords);
+ writeFile(
+ sharedTableLocation, fileFormat.addExtension("file-12"),
file1SecondSnapshotRecords);
DataFile file22 =
- writeFile(sharedTableLocation, format.addExtension("file-22"),
file2SecondSnapshotRecords);
+ writeFile(
+ sharedTableLocation, fileFormat.addExtension("file-22"),
file2SecondSnapshotRecords);
DataFile file32 =
- writeFile(sharedTableLocation, format.addExtension("file-32"),
file3SecondSnapshotRecords);
+ writeFile(
+ sharedTableLocation, fileFormat.addExtension("file-32"),
file3SecondSnapshotRecords);
sharedTable.newFastAppend().appendFile(file12).appendFile(file22).appendFile(file32).commit();
DataFile file13 =
- writeFile(sharedTableLocation, format.addExtension("file-13"),
file1ThirdSnapshotRecords);
+ writeFile(
+ sharedTableLocation, fileFormat.addExtension("file-13"),
file1ThirdSnapshotRecords);
DataFile file23 =
- writeFile(sharedTableLocation, format.addExtension("file-23"),
file2ThirdSnapshotRecords);
+ writeFile(
+ sharedTableLocation, fileFormat.addExtension("file-23"),
file2ThirdSnapshotRecords);
DataFile file33 =
- writeFile(sharedTableLocation, format.addExtension("file-33"),
file3ThirdSnapshotRecords);
+ writeFile(
+ sharedTableLocation, fileFormat.addExtension("file-33"),
file3ThirdSnapshotRecords);
sharedTable.newFastAppend().appendFile(file13).appendFile(file23).appendFile(file33).commit();
}
- @Before
+ @BeforeEach
public void createTables() throws IOException {
- File location = temp.newFolder("shared");
- Assert.assertTrue(location.delete());
+ File location = temp;
+ Assertions.assertThat(location.delete()).isTrue();
this.sharedTableLocation = location.toString();
this.sharedTable =
TABLES.create(
SCHEMA,
PartitionSpec.unpartitioned(),
- ImmutableMap.of(TableProperties.DEFAULT_FILE_FORMAT,
format.name()),
+ ImmutableMap.of(TableProperties.DEFAULT_FILE_FORMAT,
temp.getName()),
sharedTableLocation);
Record record = GenericRecord.create(SCHEMA);
- DataFile file1 =
- writeFile(sharedTableLocation, format.addExtension("file-1"),
file1FirstSnapshotRecords);
-
- Record nullData = record.copy();
- nullData.setField("id", 11L);
- nullData.setField("data", null);
-
- DataFile file2 =
- writeFile(sharedTableLocation, format.addExtension("file-2"),
file2FirstSnapshotRecords);
- DataFile file3 =
- writeFile(sharedTableLocation, format.addExtension("file-3"),
file3FirstSnapshotRecords);
-
- // commit the test data
-
sharedTable.newAppend().appendFile(file1).appendFile(file2).appendFile(file3).commit();
+ String[] format = {"parquet", "orc", "avro"};
Review Comment:
this isn't correct. As I mentioned further above, the class should be a
parameterized test, thus these changes can all be reverted here
##########
data/src/test/java/org/apache/iceberg/data/TestLocalScan.java:
##########
@@ -60,15 +62,12 @@
import org.apache.iceberg.types.Types;
import org.apache.iceberg.util.DateTimeUtil;
import org.assertj.core.api.Assertions;
-import org.junit.Assert;
-import org.junit.Before;
-import org.junit.Rule;
-import org.junit.Test;
-import org.junit.rules.TemporaryFolder;
-import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
-
-@RunWith(Parameterized.class)
Review Comment:
this should be replaced with
`@ExtendWith(ParameterizedTestExtension.class)`. Please check other tests that
use this on how to parameterize a JUnit5 test at the class level
##########
data/src/test/java/org/apache/iceberg/data/TestDataFileIndexStatsFilters.java:
##########
@@ -137,9 +135,11 @@ public void testPositionDeletePlanningPath() throws
IOException {
tasks = Lists.newArrayList(tasksIterable);
}
- Assert.assertEquals("Should produce one task", 1, tasks.size());
+ Assertions.assertThat(1).isEqualTo(tasks.size()).as("Should produce one
task");
Review Comment:
also the assertion should be `assertThat(tasks).as("Should produce one
task").hasSize(1)`
##########
data/src/test/java/org/apache/iceberg/data/TestLocalScan.java:
##########
@@ -77,19 +76,11 @@ public class TestLocalScan {
private static final Configuration CONF = new Configuration();
private static final Tables TABLES = new HadoopTables(CONF);
- @Rule public final TemporaryFolder temp = new TemporaryFolder();
-
- @Parameterized.Parameters(name = "format = {0}")
- public static Object[] parameters() {
- return new Object[] {"parquet", "orc", "avro"};
- }
-
- private final FileFormat format;
-
- public TestLocalScan(String format) {
- this.format = FileFormat.fromString(format);
+ private static Stream<Object[]> data() {
+ return Arrays.stream(new Object[][] {{"parquet"}, {"orc"}, {"avro"}});
}
+ @TempDir public File temp;
Review Comment:
```suggestion
@TempDir private File temp;
```
##########
data/src/test/java/org/apache/iceberg/data/TestLocalScan.java:
##########
@@ -143,13 +134,17 @@ public TestLocalScan(String format) {
genericRecord.copy(ImmutableMap.of("id", 27L, "data", "overview")),
genericRecord.copy(ImmutableMap.of("id", 28L, "data", "tender")));
- private void overwriteExistingData() throws IOException {
+ private void overwriteExistingData(Object fileExt) throws IOException {
Review Comment:
why is this change needed? The class itself should be parameterized as I
mentioned further above. Then these changes here can be reverted
##########
data/src/test/java/org/apache/iceberg/data/DataTest.java:
##########
@@ -31,7 +32,8 @@
import org.apache.iceberg.types.Types.MapType;
import org.apache.iceberg.types.Types.StructType;
import org.junit.Rule;
-import org.junit.Test;
+import org.junit.jupiter.api.Test;
Review Comment:
@igoradulian you need to revert changes to this class, because there are
subclasses that haven't been converted to JUnit5 yet
##########
data/src/test/java/org/apache/iceberg/data/TestMetricsRowGroupFilterTypes.java:
##########
@@ -71,13 +73,11 @@
import org.apache.parquet.hadoop.metadata.BlockMetaData;
import org.apache.parquet.io.DelegatingSeekableInputStream;
import org.apache.parquet.schema.MessageType;
-import org.junit.Assert;
-import org.junit.Before;
-import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
+import org.assertj.core.api.Assertions;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.MethodSource;
-@RunWith(Parameterized.class)
Review Comment:
same as I mentioned earlier
--
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...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
