You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "skytin1004 (via GitHub)" <gi...@apache.org> on 2023/05/19 06:43:02 UTC
[GitHub] [iceberg] skytin1004 opened a new pull request, #7655: Core:migration to JUnit5 in avro
skytin1004 opened a new pull request, #7655:
URL: https://github.com/apache/iceberg/pull/7655
This pull request is related to the issue "Move JUnit4 tests to JUnit5 https://github.com/apache/iceberg/issues/7160".
I used AssertJ as determined in the issue.
"AvroDataTest","TestAvroDataWriter","TestAvroDeleteWriters","TestAvroEnums","TestAvroFileSplit"
I migrated these five test files from JUnit4 to JUnit5, and they all passed test. and built successfully.
<img width="1235" alt="AvroDataTest_passed" src="https://github.com/apache/iceberg/assets/99078115/8496ea12-e02b-4151-8fd6-f814e58d99e5">
<img width="1243" alt="TestAvroDataWriter_passed" src="https://github.com/apache/iceberg/assets/99078115/ece520ab-4308-42d6-96db-26c8c21158a1">
<img width="1237" alt="TestAvroDeleteWriters_passed" src="https://github.com/apache/iceberg/assets/99078115/3b9b1749-87a7-409d-9412-96f3e2f7c7aa">
<img width="1217" alt="TestAvroEnums_passed" src="https://github.com/apache/iceberg/assets/99078115/78e48e70-fa7d-4daa-a322-220dd46c4d2f">
<img width="1255" alt="TestAvroFileSplit_passed" src="https://github.com/apache/iceberg/assets/99078115/4ed6bb68-303e-4401-bb0b-3e667ae0264b">
--
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
[GitHub] [iceberg] skytin1004 commented on pull request #7655: Avro: Switch tests to Junit5
Posted by "skytin1004 (via GitHub)" <gi...@apache.org>.
skytin1004 commented on PR #7655:
URL: https://github.com/apache/iceberg/pull/7655#issuecomment-1569605460
@nastra If your schedule allows and it's not too much of an inconvenience, would you be willing to lend your time and expertise to review my PR? Thank you in advance for considering this request :)
--
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
[GitHub] [iceberg] nastra merged pull request #7655: Core: Switch tests to Junit5 in avro package
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra merged PR #7655:
URL: https://github.com/apache/iceberg/pull/7655
--
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
[GitHub] [iceberg] skytin1004 commented on pull request #7655: Core: Switch tests to Junit5 in avro package
Posted by "skytin1004 (via GitHub)" <gi...@apache.org>.
skytin1004 commented on PR #7655:
URL: https://github.com/apache/iceberg/pull/7655#issuecomment-1571676337
@nastra Thank you for your review. I modified the code based on your review :)
I think the casting should be left as it is, because the test will fail if the units are different.
When I remove the casting, the test failed because the test is comparing an int value to a long value.
So I think it's correct to cast the i in the long type.
--
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
[GitHub] [iceberg] nastra commented on a diff in pull request #7655: Avro: Switch tests to Junit5
Posted by "nastra (via GitHub)" <gi...@apache.org>.
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
[GitHub] [iceberg] skytin1004 commented on pull request #7655: Core: Switch tests to Junit5 in avro package
Posted by "skytin1004 (via GitHub)" <gi...@apache.org>.
skytin1004 commented on PR #7655:
URL: https://github.com/apache/iceberg/pull/7655#issuecomment-1571228195
@nastra Thank you for your detailed review. I modified the code based on your review.
--
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
[GitHub] [iceberg] nastra commented on a diff in pull request #7655: Core: Switch tests to Junit5 in avro package
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7655:
URL: https://github.com/apache/iceberg/pull/7655#discussion_r1212717781
##########
core/src/test/java/org/apache/iceberg/avro/TestAvroEnums.java:
##########
@@ -71,8 +72,8 @@ public void writeAndValidateEnums() throws IOException {
}
Schema schema = new Schema(AvroSchemaUtil.convert(avroSchema).asStructType().fields());
- List<GenericData.Record> rows;
- try (AvroIterable<GenericData.Record> reader =
+ List<Record> rows;
+ try (AvroIterable<Record> reader =
Review Comment:
nit: I think it would generally be good to not apply changes that aren't related to Junit5 migration
##########
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 is still present
##########
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:
looks like the casting is still present, do we need it?
##########
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 is still present
--
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