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/08/23 15:59:17 UTC
[GitHub] [iceberg] skytin1004 opened a new pull request, #8380: Core: Switch tests to JUnit5 in avro, data.avro pakages
skytin1004 opened a new pull request, #8380:
URL: https://github.com/apache/iceberg/pull/8380
This pull request is related to the issue "Move JUnit4 tests to JUnit5 https://github.com/apache/iceberg/issues/7160".
I've switched the all files in Core: avro, data.avro pakages to JUnit5.
--
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 #8380: Core: Switch tests to JUnit5 in avro, data.avro pakages
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #8380:
URL: https://github.com/apache/iceberg/pull/8380#discussion_r1322896029
##########
core/src/test/java/org/apache/iceberg/avro/TestAvroNameMapping.java:
##########
@@ -369,15 +378,15 @@ protected Record writeAndRead(
Record record = super.writeAndRead(desc, writeSchema, readSchema, inputRecord);
Record projectedWithNameMapping =
writeAndRead(writeSchema, readSchema, inputRecord, MappingUtil.create(writeSchema));
- Assert.assertEquals(record, projectedWithNameMapping);
+ Assertions.assertThat(projectedWithNameMapping).isEqualTo(record);
return record;
}
private Record writeAndRead(
Schema writeSchema, Schema readSchema, Record record, NameMapping nameMapping)
throws IOException {
- File file = temp.newFile();
+ File file = createAvroTempFile(temp);
Review Comment:
why not just `temp.resolve("avro").toFile()`?
--
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 #8380: Core: Switch tests to JUnit5 in avro, data.avro pakages
Posted by "skytin1004 (via GitHub)" <gi...@apache.org>.
skytin1004 commented on PR #8380:
URL: https://github.com/apache/iceberg/pull/8380#issuecomment-1713065916
@nastra
Thank you for the review, I've modified the code based on your review.
I created a method "createAvroTempFile" in the "AvroWritingTestUtil", referring to the link you attached.
Please review it when you have time. If there are any problems with the method, I will be happy to modify it.
--
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 #8380: Core: Switch tests to JUnit5 in avro, data.avro pakages
Posted by "skytin1004 (via GitHub)" <gi...@apache.org>.
skytin1004 commented on PR #8380:
URL: https://github.com/apache/iceberg/pull/8380#issuecomment-1722389251
Thank you for the detailed review, I've modified the code based on your review. Could you please review this PR when you have a moment?
--
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 #8380: Core: Switch tests to JUnit5 in avro, data.avro pakages
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #8380:
URL: https://github.com/apache/iceberg/pull/8380#discussion_r1310376544
##########
core/src/test/java/org/apache/iceberg/avro/TestAvroNameMapping.java:
##########
@@ -369,15 +377,15 @@ protected Record writeAndRead(
Record record = super.writeAndRead(desc, writeSchema, readSchema, inputRecord);
Record projectedWithNameMapping =
writeAndRead(writeSchema, readSchema, inputRecord, MappingUtil.create(writeSchema));
- Assert.assertEquals(record, projectedWithNameMapping);
+ Assertions.assertThat(projectedWithNameMapping).isEqualTo(record);
return record;
}
private Record writeAndRead(
Schema writeSchema, Schema readSchema, Record record, NameMapping nameMapping)
throws IOException {
- File file = temp.newFile();
+ File file = new File(temp, "test.avro");
Review Comment:
should this be handled similarly to https://github.com/apache/iceberg/pull/8056/files#diff-d4debd1e3696fc5a7684ef1cf178c7d100aedfad26ffbca0560038eb98e90487R98?
--
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 #8380: Core: Switch tests to JUnit5 in avro, data.avro pakages
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #8380:
URL: https://github.com/apache/iceberg/pull/8380#discussion_r1322896439
##########
core/src/test/java/org/apache/iceberg/avro/AvroWritingTestUtils.java:
##########
@@ -0,0 +1,39 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.avro;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Path;
+import java.util.UUID;
+import org.apache.iceberg.FileFormat;
+
+public class AvroWritingTestUtils {
+
+ private AvroWritingTestUtils() {}
+
+ static File createAvroTempFile(Path temp) throws IOException {
Review Comment:
I don't think we need this class. See my other comments
--
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 a diff in pull request #8380: Core: Switch tests to JUnit5 in avro, data.avro pakages
Posted by "skytin1004 (via GitHub)" <gi...@apache.org>.
skytin1004 commented on code in PR #8380:
URL: https://github.com/apache/iceberg/pull/8380#discussion_r1310410446
##########
core/src/test/java/org/apache/iceberg/avro/TestAvroNameMapping.java:
##########
@@ -369,15 +377,15 @@ protected Record writeAndRead(
Record record = super.writeAndRead(desc, writeSchema, readSchema, inputRecord);
Record projectedWithNameMapping =
writeAndRead(writeSchema, readSchema, inputRecord, MappingUtil.create(writeSchema));
- Assert.assertEquals(record, projectedWithNameMapping);
+ Assertions.assertThat(projectedWithNameMapping).isEqualTo(record);
return record;
}
private Record writeAndRead(
Schema writeSchema, Schema readSchema, Record record, NameMapping nameMapping)
throws IOException {
- File file = temp.newFile();
+ File file = new File(temp, "test.avro");
Review Comment:
I will modify the code to use `CreateTempFile(temp)` as you suggested.
--
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 #8380: Core: Switch tests to JUnit5 in avro, data.avro pakages
Posted by "skytin1004 (via GitHub)" <gi...@apache.org>.
skytin1004 commented on PR #8380:
URL: https://github.com/apache/iceberg/pull/8380#issuecomment-1722390094
@nastra Thank you for the detailed review, I've modified the code based on your review. Could you please review this PR when you have a moment?
--
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 #8380: Core: Switch tests to JUnit5 in avro, data.avro pakages
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #8380:
URL: https://github.com/apache/iceberg/pull/8380#discussion_r1310377291
##########
core/src/test/java/org/apache/iceberg/avro/TestAvroReadProjection.java:
##########
@@ -30,16 +30,15 @@
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
import org.apache.iceberg.types.Types;
-import org.junit.Assert;
-import org.junit.Test;
+import org.assertj.core.api.Assertions;
+import org.junit.jupiter.api.Test;
public class TestAvroReadProjection extends TestReadProjection {
@Override
protected GenericData.Record writeAndRead(
String desc, Schema writeSchema, Schema readSchema, GenericData.Record record)
throws IOException {
- File file = temp.newFile(desc + ".avro");
- file.delete();
+ File file = new File(temp, desc + ".avro");
Review Comment:
same question w.r.t new file handling
--
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 a diff in pull request #8380: Core: Switch tests to JUnit5 in avro, data.avro pakages
Posted by "skytin1004 (via GitHub)" <gi...@apache.org>.
skytin1004 commented on code in PR #8380:
URL: https://github.com/apache/iceberg/pull/8380#discussion_r1310410446
##########
core/src/test/java/org/apache/iceberg/avro/TestAvroNameMapping.java:
##########
@@ -369,15 +377,15 @@ protected Record writeAndRead(
Record record = super.writeAndRead(desc, writeSchema, readSchema, inputRecord);
Record projectedWithNameMapping =
writeAndRead(writeSchema, readSchema, inputRecord, MappingUtil.create(writeSchema));
- Assert.assertEquals(record, projectedWithNameMapping);
+ Assertions.assertThat(projectedWithNameMapping).isEqualTo(record);
return record;
}
private Record writeAndRead(
Schema writeSchema, Schema readSchema, Record record, NameMapping nameMapping)
throws IOException {
- File file = temp.newFile();
+ File file = new File(temp, "test.avro");
Review Comment:
I will modify the code to use `CreateTempFile(temp)` as you suggested.
--
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 #8380: Core: Switch tests to JUnit5 in avro, data.avro pakages
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra merged PR #8380:
URL: https://github.com/apache/iceberg/pull/8380
--
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 #8380: Core: Switch tests to JUnit5 in avro, data.avro pakages
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #8380:
URL: https://github.com/apache/iceberg/pull/8380#discussion_r1322898221
##########
core/src/test/java/org/apache/iceberg/avro/TestReadProjection.java:
##########
@@ -329,50 +346,57 @@ public void testMapOfStructsProjection() throws IOException {
Schema idOnly = new Schema(Types.NestedField.required(0, "id", Types.LongType.get()));
Record projected = writeAndRead("id_only", writeSchema, idOnly, record);
- Assert.assertEquals("Should contain the correct id value", 34L, (long) projected.get("id"));
+ Assertions.assertThat((long) projected.get("id"))
+ .as("Should contain the correct id value")
+ .isEqualTo(34L);
assertEmptyAvroField(projected, "locations");
projected = writeAndRead("all_locations", writeSchema, writeSchema.select("locations"), record);
assertEmptyAvroField(projected, "id");
- Assert.assertEquals(
- "Should project locations map",
- record.get("locations"),
- toStringMap((Map) projected.get("locations")));
+ Assertions.assertThat(toStringMap((Map) projected.get("locations")))
+ .as("Should project locations map")
+ .isEqualTo(record.get("locations"));
projected = writeAndRead("lat_only", writeSchema, writeSchema.select("locations.lat"), record);
assertEmptyAvroField(projected, "id");
Map<String, ?> locations = toStringMap((Map) projected.get("locations"));
- Assert.assertNotNull("Should project locations map", locations);
- Assert.assertEquals(
- "Should contain L1 and L2", Sets.newHashSet("L1", "L2"), locations.keySet());
+ Assertions.assertThat(locations).as("Should project locations map").isNotNull();
+ Assertions.assertThat(locations.keySet())
+ .as("Should contain L1 and L2")
+ .isEqualTo(Sets.newHashSet("L1", "L2"));
Review Comment:
.containsExactly("L1", "L2")
--
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 #8380: Core: Switch tests to JUnit5 in avro, data.avro pakages
Posted by "skytin1004 (via GitHub)" <gi...@apache.org>.
skytin1004 commented on PR #8380:
URL: https://github.com/apache/iceberg/pull/8380#issuecomment-1698281203
@nastra Could you please review this PR when you have a moment?
Thank you in advance.
--
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