You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "nastra (via GitHub)" <gi...@apache.org> on 2023/05/31 07:56:29 UTC
[GitHub] [iceberg] nastra commented on a diff in pull request #7655: Avro: Switch tests to Junit5
nastra commented on code in PR #7655:
URL: https://github.com/apache/iceberg/pull/7655#discussion_r1211231496
##########
core/src/test/java/org/apache/iceberg/avro/TestAvroDataWriter.java:
##########
@@ -90,23 +91,24 @@ public void testDataWriter() throws IOException {
DataFile dataFile = dataWriter.toDataFile();
- Assert.assertEquals("Format should be Avro", FileFormat.AVRO, dataFile.format());
- Assert.assertEquals("Should be data file", FileContent.DATA, dataFile.content());
- Assert.assertEquals("Record count should match", records.size(), dataFile.recordCount());
- Assert.assertEquals("Partition should be empty", 0, dataFile.partition().size());
- Assert.assertEquals(
- "Sort order should match", sortOrder.orderId(), (int) dataFile.sortOrderId());
- Assert.assertNull("Key metadata should be null", dataFile.keyMetadata());
+ assertThat(dataFile.format()).as("Format should be Avro").isEqualTo(FileFormat.AVRO);
+ assertThat(dataFile.content()).as("Should be data file").isEqualTo(FileContent.DATA);
+ assertThat(dataFile.recordCount()).as("Record count should match").isEqualTo(records.size());
+ assertThat(dataFile.partition().size()).as("Partition should be empty").isEqualTo(0);
+ assertThat((int) dataFile.sortOrderId())
Review Comment:
I think we can remove the casting to `int` here
##########
core/src/test/java/org/apache/iceberg/avro/TestAvroEnums.java:
##########
@@ -53,15 +53,15 @@ public void writeAndValidateEnums() throws IOException {
.endRecord();
org.apache.avro.Schema enumSchema = avroSchema.getField("enumCol").schema().getTypes().get(0);
- Record enumRecord1 = new GenericData.Record(avroSchema);
+ Record enumRecord1 = new Record(avroSchema);
Review Comment:
are these changes needed?
##########
core/src/test/java/org/apache/iceberg/avro/TestAvroDeleteWriters.java:
##########
@@ -56,9 +56,9 @@ public class TestAvroDeleteWriters {
private List<Record> records;
- @Rule public TemporaryFolder temp = new TemporaryFolder();
+ @TempDir java.nio.file.Path temp;
Review Comment:
do we need the fully qualified name here? I think we can just add an import here and in the other files
##########
core/src/test/java/org/apache/iceberg/avro/TestAvroFileSplit.java:
##########
@@ -113,16 +112,17 @@ public void testPosField() throws IOException {
List<Record> records = readAvro(file, projection, 0, file.getLength());
for (int i = 0; i < expected.size(); i += 1) {
- Assert.assertEquals(
- "Field _pos should match",
- (long) i,
- records.get(i).getField(MetadataColumns.ROW_POSITION.name()));
- Assert.assertEquals(
- "Field id should match", expected.get(i).getField("id"), records.get(i).getField("id"));
- Assert.assertEquals(
- "Field data should match",
- expected.get(i).getField("data"),
- records.get(i).getField("data"));
+ assertThat(records.get(i).getField(MetadataColumns.ROW_POSITION.name()))
+ .as("Field _pos should match")
+ .isEqualTo((long) i);
Review Comment:
I believe we should be able to remove the casting here
##########
core/src/test/java/org/apache/iceberg/avro/TestAvroFileSplit.java:
##########
@@ -136,42 +136,37 @@ public void testPosFieldWithSplits() throws IOException {
List<Record> secondHalf =
readAvro(file, projection, splitLocation + 1, end - splitLocation - 1);
- Assert.assertNotEquals("Second split should not be empty", 0, secondHalf.size());
+ assertThat(secondHalf.size()).as("Second split should not be empty").isNotEqualTo(0);
List<Record> firstHalf = readAvro(file, projection, 0, splitLocation);
- Assert.assertNotEquals("First split should not be empty", 0, firstHalf.size());
+ assertThat(firstHalf.size()).as("First split should not be empty").isNotEqualTo(0);
- Assert.assertEquals(
- "Total records should match expected",
- expected.size(),
- firstHalf.size() + secondHalf.size());
+ assertThat(firstHalf.size() + secondHalf.size())
+ .as("Total records should match expected")
+ .isEqualTo(expected.size());
for (int i = 0; i < firstHalf.size(); i += 1) {
- Assert.assertEquals(
- "Field _pos should match",
- (long) i,
- firstHalf.get(i).getField(MetadataColumns.ROW_POSITION.name()));
- Assert.assertEquals(
- "Field id should match", expected.get(i).getField("id"), firstHalf.get(i).getField("id"));
- Assert.assertEquals(
- "Field data should match",
- expected.get(i).getField("data"),
- firstHalf.get(i).getField("data"));
+ assertThat(firstHalf.get(i).getField(MetadataColumns.ROW_POSITION.name()))
+ .as("Field _pos should match")
+ .isEqualTo((long) i);
Review Comment:
casting can be removed
##########
core/src/test/java/org/apache/iceberg/avro/TestAvroFileSplit.java:
##########
@@ -136,42 +136,37 @@ public void testPosFieldWithSplits() throws IOException {
List<Record> secondHalf =
readAvro(file, projection, splitLocation + 1, end - splitLocation - 1);
- Assert.assertNotEquals("Second split should not be empty", 0, secondHalf.size());
+ assertThat(secondHalf.size()).as("Second split should not be empty").isNotEqualTo(0);
List<Record> firstHalf = readAvro(file, projection, 0, splitLocation);
- Assert.assertNotEquals("First split should not be empty", 0, firstHalf.size());
+ assertThat(firstHalf.size()).as("First split should not be empty").isNotEqualTo(0);
- Assert.assertEquals(
- "Total records should match expected",
- expected.size(),
- firstHalf.size() + secondHalf.size());
+ assertThat(firstHalf.size() + secondHalf.size())
+ .as("Total records should match expected")
+ .isEqualTo(expected.size());
for (int i = 0; i < firstHalf.size(); i += 1) {
- Assert.assertEquals(
- "Field _pos should match",
- (long) i,
- firstHalf.get(i).getField(MetadataColumns.ROW_POSITION.name()));
- Assert.assertEquals(
- "Field id should match", expected.get(i).getField("id"), firstHalf.get(i).getField("id"));
- Assert.assertEquals(
- "Field data should match",
- expected.get(i).getField("data"),
- firstHalf.get(i).getField("data"));
+ assertThat(firstHalf.get(i).getField(MetadataColumns.ROW_POSITION.name()))
+ .as("Field _pos should match")
+ .isEqualTo((long) i);
+ assertThat(firstHalf.get(i).getField("id"))
+ .as("Field id should match")
+ .isEqualTo(expected.get(i).getField("id"));
+ assertThat(firstHalf.get(i).getField("data"))
+ .as("Field data should match")
+ .isEqualTo(expected.get(i).getField("data"));
}
for (int i = 0; i < secondHalf.size(); i += 1) {
- Assert.assertEquals(
- "Field _pos should match",
- (long) (firstHalf.size() + i),
- secondHalf.get(i).getField(MetadataColumns.ROW_POSITION.name()));
- Assert.assertEquals(
- "Field id should match",
- expected.get(firstHalf.size() + i).getField("id"),
- secondHalf.get(i).getField("id"));
- Assert.assertEquals(
- "Field data should match",
- expected.get(firstHalf.size() + i).getField("data"),
- secondHalf.get(i).getField("data"));
+ assertThat(secondHalf.get(i).getField(MetadataColumns.ROW_POSITION.name()))
+ .as("Field _pos should match")
+ .isEqualTo((long) (firstHalf.size() + i));
Review Comment:
casting
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org