You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "chinmay-bhat (via GitHub)" <gi...@apache.org> on 2023/12/19 12:32:16 UTC
[PR] Spark Migration to JUnit5 AssertJ - non-parameterized, spark/data directory [iceberg]
chinmay-bhat opened a new pull request, #9341:
URL: https://github.com/apache/iceberg/pull/9341
Since migrating Spark 3.5v to Junit5 is a large task, I'm splitting my work into multiple easy-to-review PR's, split by directory.
When it's time to merge into main, after review, I'll combine all the different branches for Spark migration into 1 PR and merge.
Only non-parameter files have been migrated.
TODO:
- [ ] self-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
Re: [PR] Spark Migration to JUnit5 AssertJ - non-parameterized, spark/data directory [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9341:
URL: https://github.com/apache/iceberg/pull/9341#discussion_r1432753231
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/data/parquet/vectorized/TestParquetVectorizedReads.java:
##########
@@ -80,19 +80,21 @@ private void writeAndValidate(
Function<GenericData.Record, GenericData.Record> transform)
throws IOException {
// Write test data
- Assume.assumeTrue(
- "Parquet Avro cannot write non-string map keys",
- null
- == TypeUtil.find(
- schema,
- type -> type.isMapType() && type.asMapType().keyType() != Types.StringType.get()));
+ assumeThat(
Review Comment:
assumeThat(TypeUtil.find...).isNull()
--
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
Re: [PR] Spark Migration to JUnit5 AssertJ - spark/data directory [iceberg]
Posted by "chinmay-bhat (via GitHub)" <gi...@apache.org>.
chinmay-bhat commented on code in PR #9341:
URL: https://github.com/apache/iceberg/pull/9341#discussion_r1434159383
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/data/TestSparkOrcReadMetadataColumns.java:
##########
@@ -95,24 +99,20 @@ public class TestSparkOrcReadMetadataColumns {
}
}
- @Parameterized.Parameters(name = "vectorized = {0}")
- public static Object[] parameters() {
- return new Object[] {false, true};
+ @Parameters(name = "vectorized = {0}")
+ public static Collection<Boolean> parameters() {
+ return Arrays.asList(false, true);
}
- @Rule public TemporaryFolder temp = new TemporaryFolder();
+ @TempDir private java.nio.file.Path temp;
- private boolean vectorized;
+ @Parameter private boolean vectorized;
private File testFile;
- public TestSparkOrcReadMetadataColumns(boolean vectorized) {
- this.vectorized = vectorized;
- }
-
- @Before
+ @BeforeEach
public void writeFile() throws IOException {
- testFile = temp.newFile();
- Assert.assertTrue("Delete should succeed", testFile.delete());
+ testFile = File.createTempFile(UUID.randomUUID().toString(), null, temp.toFile());
Review Comment:
~~I tried a variation of that, but the tests weren't passing.~~ Let me try this and I'll get back to you :)
never mind, tests pass
--
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
Re: [PR] Spark Migration to JUnit5 AssertJ - spark/data directory [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9341:
URL: https://github.com/apache/iceberg/pull/9341#discussion_r1434323504
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/source/TestDataFrameWrites.java:
##########
@@ -63,29 +70,21 @@
import org.apache.spark.sql.SaveMode;
import org.apache.spark.sql.SparkSession;
import org.apache.spark.sql.catalyst.InternalRow;
-import org.assertj.core.api.Assertions;
-import org.junit.AfterClass;
-import org.junit.Assert;
-import org.junit.Assume;
-import org.junit.BeforeClass;
-import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
-
-@RunWith(Parameterized.class)
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.TestTemplate;
+import org.junit.jupiter.api.extension.ExtendWith;
+
+@ExtendWith(ParameterizedTestExtension.class)
public class TestDataFrameWrites extends AvroDataTest {
private static final Configuration CONF = new Configuration();
- private final String format;
-
- @Parameterized.Parameters(name = "format = {0}")
- public static Object[] parameters() {
- return new Object[] {"parquet", "avro", "orc"};
+ @Parameters(name = "format = {0}")
+ public static Collection<String> parameters() {
+ return Arrays.asList("parquet", "avro", "orc");
}
- public TestDataFrameWrites(String format) {
- this.format = format;
- }
+ @Parameter private String format = "parquet";
Review Comment:
hm I see, I didn't realize that. The problem with this is that all of these tests won't be running with the different parameters anymore unfortunately
--
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
Re: [PR] Spark Migration to JUnit5 AssertJ - non-parameterized, spark/data directory [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9341:
URL: https://github.com/apache/iceberg/pull/9341#discussion_r1432750539
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/data/TestSparkParquetReadMetadataColumns.java:
##########
@@ -19,6 +19,8 @@
package org.apache.iceberg.spark.data;
import static org.apache.iceberg.types.Types.NestedField.required;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.jupiter.api.Assumptions.assumeTrue;
Review Comment:
please use Assumptions from AssertJ
--
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
Re: [PR] Spark Migration to JUnit5 AssertJ - spark/data directory [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9341:
URL: https://github.com/apache/iceberg/pull/9341#discussion_r1434218017
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/source/TestDataFrameWrites.java:
##########
@@ -343,7 +343,7 @@ public void testNullableWithSparkSqlOption() throws IOException {
.as("Spark 3 rejects writing nulls to a required column")
.startsWith("2");
- File location = new File(new File(temp.toFile(), "parquet"), "test");
+ File location = new File(temp.resolve("parquet").toFile(), "test");
Review Comment:
you should be able to chain the `resolve()` calls, similar to https://github.com/apache/iceberg/blob/5e0e5e39fa8110b3e9c01e1b7994c575a8dde8ba/flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/TestFlinkAnonymousTable.java#L61
--
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
Re: [PR] Spark Migration to JUnit5 AssertJ - non-parameterized, spark/data directory [iceberg]
Posted by "chinmay-bhat (via GitHub)" <gi...@apache.org>.
chinmay-bhat commented on PR #9341:
URL: https://github.com/apache/iceberg/pull/9341#issuecomment-1865561017
running CI check locally and resolving errors. I'll push once I've cleared the errors / need more help!
--
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
Re: [PR] Spark Migration to JUnit5 AssertJ - non-parameterized, spark/data directory [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9341:
URL: https://github.com/apache/iceberg/pull/9341#discussion_r1432757273
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/data/GenericsHelpers.java:
##########
@@ -264,86 +259,83 @@ private static void assertEqualsUnsafe(Type type, Object expected, Object actual
case LONG:
case FLOAT:
case DOUBLE:
- Assert.assertEquals("Primitive value should be equal to expected", expected, actual);
+ assertThat(actual).as("Primitive value should be equal to expected").isEqualTo(expected);
break;
case DATE:
- Assertions.assertThat(expected)
- .as("Should expect a LocalDate")
- .isInstanceOf(LocalDate.class);
+ assertThat(expected).as("Should expect a LocalDate").isInstanceOf(LocalDate.class);
int expectedDays = (int) ChronoUnit.DAYS.between(EPOCH_DAY, (LocalDate) expected);
- Assert.assertEquals("Primitive value should be equal to expected", expectedDays, actual);
+ assertThat(actual)
+ .as("Primitive value should be equal to expected")
+ .isEqualTo(expectedDays);
break;
case TIMESTAMP:
Types.TimestampType timestampType = (Types.TimestampType) type;
if (timestampType.shouldAdjustToUTC()) {
- Assertions.assertThat(expected)
+ assertThat(expected)
.as("Should expect an OffsetDateTime")
.isInstanceOf(OffsetDateTime.class);
long expectedMicros = ChronoUnit.MICROS.between(EPOCH, (OffsetDateTime) expected);
- Assert.assertEquals(
- "Primitive value should be equal to expected", expectedMicros, actual);
+ assertThat(actual)
+ .as("Primitive value should be equal to expected")
+ .isEqualTo(expectedMicros);
} else {
- Assertions.assertThat(expected)
+ assertThat(expected)
.as("Should expect an LocalDateTime")
.isInstanceOf(LocalDateTime.class);
long expectedMicros =
ChronoUnit.MICROS.between(EPOCH, ((LocalDateTime) expected).atZone(ZoneId.of("UTC")));
- Assert.assertEquals(
- "Primitive value should be equal to expected", expectedMicros, actual);
+ assertThat(actual)
+ .as("Primitive value should be equal to expected")
+ .isEqualTo(expectedMicros);
}
break;
case STRING:
- Assertions.assertThat(actual).as("Should be a UTF8String").isInstanceOf(UTF8String.class);
- Assert.assertEquals("Strings should be equal", expected, actual.toString());
+ assertThat(actual).as("Should be a UTF8String").isInstanceOf(UTF8String.class);
+ assertThat(actual.toString())
+ .as("Strings should be equal")
+ .isEqualTo(String.valueOf(expected));
break;
case UUID:
- Assertions.assertThat(expected).as("Should expect a UUID").isInstanceOf(UUID.class);
- Assertions.assertThat(actual).as("Should be a UTF8String").isInstanceOf(UTF8String.class);
- Assert.assertEquals(
- "UUID string representation should match", expected.toString(), actual.toString());
+ assertThat(expected).as("Should expect a UUID").isInstanceOf(UUID.class);
+ assertThat(actual).as("Should be a UTF8String").isInstanceOf(UTF8String.class);
+ assertThat(actual.toString())
+ .as("UUID string representation should match")
+ .isEqualTo(String.valueOf(expected));
break;
case FIXED:
- Assertions.assertThat(expected).as("Should expect a byte[]").isInstanceOf(byte[].class);
- Assertions.assertThat(actual).as("Should be a byte[]").isInstanceOf(byte[].class);
- Assert.assertArrayEquals("Bytes should match", (byte[]) expected, (byte[]) actual);
+ assertThat(expected).as("Should expect a byte[]").isInstanceOf(byte[].class);
+ assertThat(actual).as("Should be a byte[]").isInstanceOf(byte[].class);
+ assertThat(actual).as("Bytes should match").isEqualTo(expected);
break;
case BINARY:
- Assertions.assertThat(expected)
- .as("Should expect a ByteBuffer")
- .isInstanceOf(ByteBuffer.class);
- Assertions.assertThat(actual).as("Should be a byte[]").isInstanceOf(byte[].class);
- Assert.assertArrayEquals(
- "Bytes should match", ((ByteBuffer) expected).array(), (byte[]) actual);
+ assertThat(expected).as("Should expect a ByteBuffer").isInstanceOf(ByteBuffer.class);
+ assertThat(actual).as("Should be a byte[]").isInstanceOf(byte[].class);
+ assertThat((byte[]) actual)
Review Comment:
cast is probably not needed
--
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
Re: [PR] Spark Migration to JUnit5 AssertJ - non-parameterized, spark/data directory [iceberg]
Posted by "chinmay-bhat (via GitHub)" <gi...@apache.org>.
chinmay-bhat commented on code in PR #9341:
URL: https://github.com/apache/iceberg/pull/9341#discussion_r1431354042
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/data/TestSparkParquetWriter.java:
##########
@@ -20,10 +20,13 @@
import static org.apache.iceberg.types.Types.NestedField.optional;
import static org.apache.iceberg.types.Types.NestedField.required;
+import static org.assertj.core.api.Assertions.assertThat;
import java.io.File;
import java.io.IOException;
+import java.nio.file.Path;
import java.util.Iterator;
+import java.util.UUID;
Review Comment:
not needed
--
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
Re: [PR] Spark Migration to JUnit5 AssertJ - spark/data directory [iceberg]
Posted by "chinmay-bhat (via GitHub)" <gi...@apache.org>.
chinmay-bhat commented on code in PR #9341:
URL: https://github.com/apache/iceberg/pull/9341#discussion_r1434164015
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/data/TestSparkOrcReadMetadataColumns.java:
##########
@@ -95,24 +99,20 @@ public class TestSparkOrcReadMetadataColumns {
}
}
- @Parameterized.Parameters(name = "vectorized = {0}")
- public static Object[] parameters() {
- return new Object[] {false, true};
+ @Parameters(name = "vectorized = {0}")
+ public static Collection<Boolean> parameters() {
+ return Arrays.asList(false, true);
}
- @Rule public TemporaryFolder temp = new TemporaryFolder();
+ @TempDir private java.nio.file.Path temp;
- private boolean vectorized;
+ @Parameter private boolean vectorized;
private File testFile;
- public TestSparkOrcReadMetadataColumns(boolean vectorized) {
- this.vectorized = vectorized;
- }
-
- @Before
+ @BeforeEach
public void writeFile() throws IOException {
- testFile = temp.newFile();
- Assert.assertTrue("Delete should succeed", testFile.delete());
+ testFile = File.createTempFile(UUID.randomUUID().toString(), null, temp.toFile());
Review Comment:
Removing the UUID.
The main problem with using `Files` is that we have another import
`import org.apache.iceberg.Files;` that's being used.
So I end up writing
`java.nio.file.Files.createTempFile(temp, "junit", null).toFile();`
which is not as readable as
`File.createTempFile("junit", null, temp.toFile());`
Which one should I go ahead with?
--
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
Re: [PR] Spark Migration to JUnit5 AssertJ - spark/data directory [iceberg]
Posted by "chinmay-bhat (via GitHub)" <gi...@apache.org>.
chinmay-bhat commented on code in PR #9341:
URL: https://github.com/apache/iceberg/pull/9341#discussion_r1434250422
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/source/TestDataFrameWrites.java:
##########
@@ -63,29 +70,21 @@
import org.apache.spark.sql.SaveMode;
import org.apache.spark.sql.SparkSession;
import org.apache.spark.sql.catalyst.InternalRow;
-import org.assertj.core.api.Assertions;
-import org.junit.AfterClass;
-import org.junit.Assert;
-import org.junit.Assume;
-import org.junit.BeforeClass;
-import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
-
-@RunWith(Parameterized.class)
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.TestTemplate;
+import org.junit.jupiter.api.extension.ExtendWith;
+
+@ExtendWith(ParameterizedTestExtension.class)
public class TestDataFrameWrites extends AvroDataTest {
private static final Configuration CONF = new Configuration();
- private final String format;
-
- @Parameterized.Parameters(name = "format = {0}")
- public static Object[] parameters() {
- return new Object[] {"parquet", "avro", "orc"};
+ @Parameters(name = "format = {0}")
+ public static Collection<String> parameters() {
+ return Arrays.asList("parquet", "avro", "orc");
}
- public TestDataFrameWrites(String format) {
- this.format = format;
- }
+ @Parameter private String format = "parquet";
Review Comment:
it is initialised automatically for the @TestTemplates tests in TestDataFrameWrites, but not for the @Tests it inherits from class `AvroDataTest`.
You don't see this problem with other classes that inherit from `AvroDataTest` because their @Parameter variable is a boolean, which is False by default.
@Parameter `format` being a string needs a default non-null value, otherwise it throws errors
--
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
Re: [PR] Spark Migration to JUnit5 AssertJ - spark/data directory [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9341:
URL: https://github.com/apache/iceberg/pull/9341#discussion_r1434242259
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/source/TestDataFrameWrites.java:
##########
@@ -63,29 +70,21 @@
import org.apache.spark.sql.SaveMode;
import org.apache.spark.sql.SparkSession;
import org.apache.spark.sql.catalyst.InternalRow;
-import org.assertj.core.api.Assertions;
-import org.junit.AfterClass;
-import org.junit.Assert;
-import org.junit.Assume;
-import org.junit.BeforeClass;
-import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
-
-@RunWith(Parameterized.class)
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.TestTemplate;
+import org.junit.jupiter.api.extension.ExtendWith;
+
+@ExtendWith(ParameterizedTestExtension.class)
public class TestDataFrameWrites extends AvroDataTest {
private static final Configuration CONF = new Configuration();
- private final String format;
-
- @Parameterized.Parameters(name = "format = {0}")
- public static Object[] parameters() {
- return new Object[] {"parquet", "avro", "orc"};
+ @Parameters(name = "format = {0}")
+ public static Collection<String> parameters() {
+ return Arrays.asList("parquet", "avro", "orc");
}
- public TestDataFrameWrites(String format) {
- this.format = format;
- }
+ @Parameter private String format = "parquet";
Review Comment:
shouldn't this be initialized automatically from the parameters?
--
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
Re: [PR] Spark Migration to JUnit5 AssertJ - spark/data directory [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9341:
URL: https://github.com/apache/iceberg/pull/9341#discussion_r1434323504
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/source/TestDataFrameWrites.java:
##########
@@ -63,29 +70,21 @@
import org.apache.spark.sql.SaveMode;
import org.apache.spark.sql.SparkSession;
import org.apache.spark.sql.catalyst.InternalRow;
-import org.assertj.core.api.Assertions;
-import org.junit.AfterClass;
-import org.junit.Assert;
-import org.junit.Assume;
-import org.junit.BeforeClass;
-import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
-
-@RunWith(Parameterized.class)
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.TestTemplate;
+import org.junit.jupiter.api.extension.ExtendWith;
+
+@ExtendWith(ParameterizedTestExtension.class)
public class TestDataFrameWrites extends AvroDataTest {
private static final Configuration CONF = new Configuration();
- private final String format;
-
- @Parameterized.Parameters(name = "format = {0}")
- public static Object[] parameters() {
- return new Object[] {"parquet", "avro", "orc"};
+ @Parameters(name = "format = {0}")
+ public static Collection<String> parameters() {
+ return Arrays.asList("parquet", "avro", "orc");
}
- public TestDataFrameWrites(String format) {
- this.format = format;
- }
+ @Parameter private String format = "parquet";
Review Comment:
hm I see, I didn't realize that
--
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
Re: [PR] Spark Migration to JUnit5 AssertJ - spark/data directory [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9341:
URL: https://github.com/apache/iceberg/pull/9341#discussion_r1434195782
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/source/TestAvroScan.java:
##########
@@ -68,7 +68,7 @@ public static void stopSpark() {
@Override
protected void writeAndValidate(Schema schema) throws IOException {
- File parent = temp.newFolder("avro");
+ File parent = new File(temp.toFile(), "avro");
Review Comment:
```suggestion
File parent =temp.resolve("avro").toFile();
```
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/source/TestAvroScan.java:
##########
@@ -68,7 +68,7 @@ public static void stopSpark() {
@Override
protected void writeAndValidate(Schema schema) throws IOException {
- File parent = temp.newFolder("avro");
+ File parent = new File(temp.toFile(), "avro");
Review Comment:
```suggestion
File parent = 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
Re: [PR] Spark Migration to JUnit5 AssertJ - non-parameterized, spark/data directory [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9341:
URL: https://github.com/apache/iceberg/pull/9341#discussion_r1432751135
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/data/TestSparkParquetReader.java:
##########
@@ -20,6 +20,8 @@
import static org.apache.iceberg.spark.data.TestHelpers.assertEqualsUnsafe;
import static org.apache.iceberg.types.Types.NestedField.required;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.jupiter.api.Assumptions.assumeTrue;
Review Comment:
please use Assumptions from AssertJ
--
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
Re: [PR] Spark Migration to JUnit5 AssertJ - non-parameterized, spark/data directory [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9341:
URL: https://github.com/apache/iceberg/pull/9341#discussion_r1432758094
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/data/GenericsHelpers.java:
##########
@@ -116,90 +116,85 @@ private static void assertEqualsSafe(Type type, Object expected, Object actual)
case LONG:
case FLOAT:
case DOUBLE:
- Assert.assertEquals("Primitive value should be equal to expected", expected, actual);
+ assertThat(actual).as("Primitive value should be equal to expected").isEqualTo(expected);
break;
case DATE:
- Assertions.assertThat(expected)
- .as("Should expect a LocalDate")
- .isInstanceOf(LocalDate.class);
- Assertions.assertThat(actual).as("Should be a Date").isInstanceOf(Date.class);
- Assert.assertEquals(
- "ISO-8601 date should be equal", expected.toString(), actual.toString());
+ assertThat(expected).as("Should expect a LocalDate").isInstanceOf(LocalDate.class);
+ assertThat(actual).as("Should be a Date").isInstanceOf(Date.class);
+ assertThat(actual.toString())
+ .as("ISO-8601 date should be equal")
+ .isEqualTo(String.valueOf(expected));
break;
case TIMESTAMP:
Types.TimestampType timestampType = (Types.TimestampType) type;
if (timestampType.shouldAdjustToUTC()) {
// Timestamptz
- Assertions.assertThat(actual).as("Should be a Timestamp").isInstanceOf(Timestamp.class);
+ assertThat(actual).as("Should be a Timestamp").isInstanceOf(Timestamp.class);
Timestamp ts = (Timestamp) actual;
// milliseconds from nanos has already been added by getTime
OffsetDateTime actualTs =
EPOCH.plusNanos((ts.getTime() * 1_000_000) + (ts.getNanos() % 1_000_000));
- Assertions.assertThat(expected)
+ assertThat(expected)
.as("Should expect an OffsetDateTime")
.isInstanceOf(OffsetDateTime.class);
- Assert.assertEquals("Timestamp should be equal", expected, actualTs);
+
+ assertThat(actualTs).as("Timestamp should be equal").isEqualTo(expected);
} else {
// Timestamp
- Assertions.assertThat(actual)
- .as("Should be a LocalDateTime")
- .isInstanceOf(LocalDateTime.class);
- LocalDateTime ts = (LocalDateTime) actual;
+ assertThat(actual).as("Should be a LocalDateTime").isInstanceOf(LocalDateTime.class);
- Assertions.assertThat(expected)
+ assertThat(expected)
.as("Should expect an LocalDateTime")
.isInstanceOf(LocalDateTime.class);
- Assert.assertEquals("Timestamp should be equal", expected, ts);
+
+ assertThat(actual).as("Timestamp should be equal").isEqualTo(expected);
}
break;
case STRING:
- Assertions.assertThat(actual).as("Should be a String").isInstanceOf(String.class);
- Assert.assertEquals("Strings should be equal", String.valueOf(expected), actual);
+ assertThat(actual).as("Should be a String").isInstanceOf(String.class);
+ assertThat(actual.toString())
+ .as("Strings should be equal")
+ .isEqualTo(String.valueOf(expected));
break;
case UUID:
- Assertions.assertThat(expected).as("Should expect a UUID").isInstanceOf(UUID.class);
- Assertions.assertThat(actual).as("Should be a String").isInstanceOf(String.class);
- Assert.assertEquals("UUID string representation should match", expected.toString(), actual);
+ assertThat(expected).as("Should expect a UUID").isInstanceOf(UUID.class);
+ assertThat(actual).as("Should be a String").isInstanceOf(String.class);
+ assertThat(actual.toString())
+ .as("UUID string representation should match")
+ .isEqualTo(String.valueOf(expected));
break;
case FIXED:
- Assertions.assertThat(expected).as("Should expect a byte[]").isInstanceOf(byte[].class);
- Assertions.assertThat(actual).as("Should be a byte[]").isInstanceOf(byte[].class);
- Assert.assertArrayEquals("Bytes should match", (byte[]) expected, (byte[]) actual);
+ assertThat(expected).as("Should expect a byte[]").isInstanceOf(byte[].class);
+ assertThat(actual).as("Should be a byte[]").isInstanceOf(byte[].class);
+ assertThat(actual).as("Bytes should match").isEqualTo(expected);
break;
case BINARY:
- Assertions.assertThat(expected)
- .as("Should expect a ByteBuffer")
- .isInstanceOf(ByteBuffer.class);
- Assertions.assertThat(actual).as("Should be a byte[]").isInstanceOf(byte[].class);
- Assert.assertArrayEquals(
- "Bytes should match", ((ByteBuffer) expected).array(), (byte[]) actual);
+ assertThat(expected).as("Should expect a ByteBuffer").isInstanceOf(ByteBuffer.class);
+ assertThat(actual).as("Should be a byte[]").isInstanceOf(byte[].class);
+ assertThat((byte[]) actual)
Review Comment:
cast is probably not needed
--
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
Re: [PR] Spark Migration to JUnit5 AssertJ - non-parameterized, spark/data directory [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9341:
URL: https://github.com/apache/iceberg/pull/9341#discussion_r1432986527
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/data/TestSparkOrcReadMetadataColumns.java:
##########
@@ -100,7 +99,7 @@ public static Object[] parameters() {
return new Object[] {false, true};
}
- @Rule public TemporaryFolder temp = new TemporaryFolder();
+ @TempDir public java.nio.file.Path temp;
Review Comment:
hm I think you either need to convert this test to a JUnit5-parameterized test or revert the changes here
--
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
Re: [PR] Spark Migration to JUnit5 AssertJ - non-parameterized, spark/data directory [iceberg]
Posted by "chinmay-bhat (via GitHub)" <gi...@apache.org>.
chinmay-bhat commented on code in PR #9341:
URL: https://github.com/apache/iceberg/pull/9341#discussion_r1432997327
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/data/TestSparkOrcReadMetadataColumns.java:
##########
@@ -100,7 +99,7 @@ public static Object[] parameters() {
return new Object[] {false, true};
}
- @Rule public TemporaryFolder temp = new TemporaryFolder();
+ @TempDir public java.nio.file.Path temp;
Review Comment:
agreed, I'll revert 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
Re: [PR] Spark Migration to JUnit5 AssertJ - spark/data directory [iceberg]
Posted by "chinmay-bhat (via GitHub)" <gi...@apache.org>.
chinmay-bhat commented on code in PR #9341:
URL: https://github.com/apache/iceberg/pull/9341#discussion_r1434187605
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/data/TestSparkOrcReadMetadataColumns.java:
##########
@@ -95,24 +99,20 @@ public class TestSparkOrcReadMetadataColumns {
}
}
- @Parameterized.Parameters(name = "vectorized = {0}")
- public static Object[] parameters() {
- return new Object[] {false, true};
+ @Parameters(name = "vectorized = {0}")
+ public static Collection<Boolean> parameters() {
+ return Arrays.asList(false, true);
}
- @Rule public TemporaryFolder temp = new TemporaryFolder();
+ @TempDir private java.nio.file.Path temp;
- private boolean vectorized;
+ @Parameter private boolean vectorized;
private File testFile;
- public TestSparkOrcReadMetadataColumns(boolean vectorized) {
- this.vectorized = vectorized;
- }
-
- @Before
+ @BeforeEach
public void writeFile() throws IOException {
- testFile = temp.newFile();
- Assert.assertTrue("Delete should succeed", testFile.delete());
+ testFile = File.createTempFile(UUID.randomUUID().toString(), null, temp.toFile());
Review Comment:
done!
--
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
Re: [PR] Spark Migration to JUnit5 AssertJ - spark/data directory [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9341:
URL: https://github.com/apache/iceberg/pull/9341#discussion_r1434196320
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/source/TestDataFrameWrites.java:
##########
@@ -144,10 +143,10 @@ protected void writeAndValidate(Schema schema) throws IOException {
writeAndValidateWithLocations(table, location, new File(location, "data"));
}
- @Test
+ @TestTemplate
public void testWriteWithCustomDataLocation() throws IOException {
File location = createTableFolder();
- File tablePropertyDataLocation = temp.newFolder("test-table-property-data-dir");
+ File tablePropertyDataLocation = new File(temp.toFile(), "test-table-property-data-dir");
Review Comment:
same as above
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/source/TestDataFrameWrites.java:
##########
@@ -157,9 +156,9 @@ public void testWriteWithCustomDataLocation() throws IOException {
}
private File createTableFolder() throws IOException {
- File parent = temp.newFolder("parquet");
+ File parent = new File(temp.toFile(), "parquet");
Review Comment:
same here
--
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
Re: [PR] Spark Migration to JUnit5 AssertJ - spark/data directory [iceberg]
Posted by "chinmay-bhat (via GitHub)" <gi...@apache.org>.
chinmay-bhat commented on code in PR #9341:
URL: https://github.com/apache/iceberg/pull/9341#discussion_r1434201795
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/source/TestAvroScan.java:
##########
@@ -68,7 +68,7 @@ public static void stopSpark() {
@Override
protected void writeAndValidate(Schema schema) throws IOException {
- File parent = temp.newFolder("avro");
+ File parent = new File(temp.toFile(), "avro");
Review Comment:
temp.resolve() was throwing errors before, now it's not. ;)
Updating it everywhere
--
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
Re: [PR] Spark Migration to JUnit5 AssertJ - non-parameterized, spark/data directory [iceberg]
Posted by "chinmay-bhat (via GitHub)" <gi...@apache.org>.
chinmay-bhat commented on code in PR #9341:
URL: https://github.com/apache/iceberg/pull/9341#discussion_r1431347428
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/data/GenericsHelpers.java:
##########
@@ -116,88 +116,91 @@ private static void assertEqualsSafe(Type type, Object expected, Object actual)
case LONG:
case FLOAT:
case DOUBLE:
- Assert.assertEquals("Primitive value should be equal to expected", expected, actual);
+ assertThat(expected).as("Primitive value should be equal to expected").isEqualTo(actual);
break;
case DATE:
- Assertions.assertThat(expected)
+ assertThat(expected)
.as("Should expect a LocalDate")
.isInstanceOf(LocalDate.class);
- Assertions.assertThat(actual).as("Should be a Date").isInstanceOf(Date.class);
- Assert.assertEquals(
- "ISO-8601 date should be equal", expected.toString(), actual.toString());
+ assertThat(actual).as("Should be a Date").isInstanceOf(Date.class);
+ assertThat(actual.toString())
+ .as("ISO-8601 date should be equal")
+ .isEqualTo(String.valueOf(expected));
break;
case TIMESTAMP:
Types.TimestampType timestampType = (Types.TimestampType) type;
if (timestampType.shouldAdjustToUTC()) {
// Timestamptz
- Assertions.assertThat(actual).as("Should be a Timestamp").isInstanceOf(Timestamp.class);
+ assertThat(actual).as("Should be a Timestamp").isInstanceOf(Timestamp.class);
Timestamp ts = (Timestamp) actual;
// milliseconds from nanos has already been added by getTime
OffsetDateTime actualTs =
EPOCH.plusNanos((ts.getTime() * 1_000_000) + (ts.getNanos() % 1_000_000));
- Assertions.assertThat(expected)
+ assertThat(expected)
.as("Should expect an OffsetDateTime")
.isInstanceOf(OffsetDateTime.class);
- Assert.assertEquals("Timestamp should be equal", expected, actualTs);
+
+ assertThat(actualTs).as("Timestamp should be equal").isEqualTo(expected);
} else {
// Timestamp
- Assertions.assertThat(actual)
+ assertThat(actual)
.as("Should be a LocalDateTime")
.isInstanceOf(LocalDateTime.class);
- LocalDateTime ts = (LocalDateTime) actual;
- Assertions.assertThat(expected)
+ assertThat(expected)
.as("Should expect an LocalDateTime")
.isInstanceOf(LocalDateTime.class);
- Assert.assertEquals("Timestamp should be equal", expected, ts);
+
+ assertThat(actual).as("Timestamp should be equal").isEqualTo(expected);
}
break;
case STRING:
- Assertions.assertThat(actual).as("Should be a String").isInstanceOf(String.class);
- Assert.assertEquals("Strings should be equal", String.valueOf(expected), actual);
+ assertThat(actual).as("Should be a String").isInstanceOf(String.class);
+ assertThat(actual.toString()).as("Strings should be equal").isEqualTo(String.valueOf(expected));
Review Comment:
Maybe this can be further simplified? Any input would be great!
--
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
Re: [PR] Spark Migration to JUnit5 AssertJ - non-parameterized, spark/data directory [iceberg]
Posted by "chinmay-bhat (via GitHub)" <gi...@apache.org>.
chinmay-bhat commented on PR #9341:
URL: https://github.com/apache/iceberg/pull/9341#issuecomment-1864702341
CI failing.
Error:
```
> Task :iceberg-spark:iceberg-spark-3.5_2.12:jar
/home/runner/work/iceberg/iceberg/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/source/TestDataFrameWrites.java:150: error: cannot find symbol
File tablePropertyDataLocation = temp.newFolder("test-table-property-data-dir");
```
Fix:
`TestDataFrameWrites` extends `AvroDataTest`. Since the temp dir has been updated to JUnit5 In `AvroDataTest`, `TestDataFrameWrites` temp is throwing errors.
To fix, I'll update only the `temp` calls in `TestDataFrameWrites` 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
Re: [PR] Spark Migration to JUnit5 AssertJ - spark/data directory [iceberg]
Posted by "chinmay-bhat (via GitHub)" <gi...@apache.org>.
chinmay-bhat commented on code in PR #9341:
URL: https://github.com/apache/iceberg/pull/9341#discussion_r1434159383
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/data/TestSparkOrcReadMetadataColumns.java:
##########
@@ -95,24 +99,20 @@ public class TestSparkOrcReadMetadataColumns {
}
}
- @Parameterized.Parameters(name = "vectorized = {0}")
- public static Object[] parameters() {
- return new Object[] {false, true};
+ @Parameters(name = "vectorized = {0}")
+ public static Collection<Boolean> parameters() {
+ return Arrays.asList(false, true);
}
- @Rule public TemporaryFolder temp = new TemporaryFolder();
+ @TempDir private java.nio.file.Path temp;
- private boolean vectorized;
+ @Parameter private boolean vectorized;
private File testFile;
- public TestSparkOrcReadMetadataColumns(boolean vectorized) {
- this.vectorized = vectorized;
- }
-
- @Before
+ @BeforeEach
public void writeFile() throws IOException {
- testFile = temp.newFile();
- Assert.assertTrue("Delete should succeed", testFile.delete());
+ testFile = File.createTempFile(UUID.randomUUID().toString(), null, temp.toFile());
Review Comment:
I tried a variation of that, but the tests weren't passing. Let me try this and I'll get back to you :)
--
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
Re: [PR] Spark Migration to JUnit5 AssertJ - spark/data directory [iceberg]
Posted by "chinmay-bhat (via GitHub)" <gi...@apache.org>.
chinmay-bhat commented on code in PR #9341:
URL: https://github.com/apache/iceberg/pull/9341#discussion_r1434257298
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/source/TestDataFrameWrites.java:
##########
@@ -186,21 +185,24 @@ private void writeAndValidateWithLocations(Table table, File location, File expe
while (expectedIter.hasNext() && actualIter.hasNext()) {
assertEqualsSafe(tableSchema.asStruct(), expectedIter.next(), actualIter.next());
}
- Assert.assertEquals(
- "Both iterators should be exhausted", expectedIter.hasNext(), actualIter.hasNext());
+ assertThat(actualIter.hasNext())
+ .as("Both iterators should be exhausted")
+ .isEqualTo(expectedIter.hasNext());
table
.currentSnapshot()
.addedDataFiles(table.io())
.forEach(
dataFile ->
- Assert.assertTrue(
- String.format(
- "File should have the parent directory %s, but has: %s.",
- expectedDataDir.getAbsolutePath(), dataFile.path()),
- URI.create(dataFile.path().toString())
- .getPath()
- .startsWith(expectedDataDir.getAbsolutePath())));
+ assertThat(
+ URI.create(dataFile.path().toString())
+ .getPath()
+ .startsWith(expectedDataDir.getAbsolutePath()))
Review Comment:
true, fixed!
--
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
Re: [PR] Spark Migration to JUnit5 AssertJ - spark/data directory [iceberg]
Posted by "chinmay-bhat (via GitHub)" <gi...@apache.org>.
chinmay-bhat commented on code in PR #9341:
URL: https://github.com/apache/iceberg/pull/9341#discussion_r1434250422
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/source/TestDataFrameWrites.java:
##########
@@ -63,29 +70,21 @@
import org.apache.spark.sql.SaveMode;
import org.apache.spark.sql.SparkSession;
import org.apache.spark.sql.catalyst.InternalRow;
-import org.assertj.core.api.Assertions;
-import org.junit.AfterClass;
-import org.junit.Assert;
-import org.junit.Assume;
-import org.junit.BeforeClass;
-import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
-
-@RunWith(Parameterized.class)
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.TestTemplate;
+import org.junit.jupiter.api.extension.ExtendWith;
+
+@ExtendWith(ParameterizedTestExtension.class)
public class TestDataFrameWrites extends AvroDataTest {
private static final Configuration CONF = new Configuration();
- private final String format;
-
- @Parameterized.Parameters(name = "format = {0}")
- public static Object[] parameters() {
- return new Object[] {"parquet", "avro", "orc"};
+ @Parameters(name = "format = {0}")
+ public static Collection<String> parameters() {
+ return Arrays.asList("parquet", "avro", "orc");
}
- public TestDataFrameWrites(String format) {
- this.format = format;
- }
+ @Parameter private String format = "parquet";
Review Comment:
it is initialised automatically for the `@TestTemplates` tests in `TestDataFrameWrites`, but not for the `@Tests` it inherits from class `AvroDataTest`.
You don't see this problem with other classes that inherit from `AvroDataTest` because their `@Parameter` variable is a boolean, which is False by default.
`@Parameter format` being a string needs a default non-null value, otherwise it throws errors
--
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
Re: [PR] Spark Migration to JUnit5 AssertJ - non-parameterized, spark/data directory [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9341:
URL: https://github.com/apache/iceberg/pull/9341#discussion_r1432755921
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/data/TestHelpers.java:
##########
@@ -304,86 +294,77 @@ private static void assertEqualsUnsafe(Type type, Object expected, Object actual
switch (type.typeId()) {
case LONG:
- Assertions.assertThat(actual).as("Should be a long").isInstanceOf(Long.class);
+ assertThat(actual).as("Should be a long").isInstanceOf(Long.class);
if (expected instanceof Integer) {
- Assert.assertEquals("Values didn't match", ((Number) expected).longValue(), actual);
+ assertThat(actual).as("Values didn't match").isEqualTo(((Number) expected).longValue());
} else {
- Assert.assertEquals("Primitive value should be equal to expected", expected, actual);
+ assertThat(actual).as("Primitive value should be equal to expected").isEqualTo(expected);
}
break;
case DOUBLE:
- Assertions.assertThat(actual).as("Should be a double").isInstanceOf(Double.class);
+ assertThat(actual).as("Should be a double").isInstanceOf(Double.class);
if (expected instanceof Float) {
- Assert.assertEquals(
- "Values didn't match",
- Double.doubleToLongBits(((Number) expected).doubleValue()),
- Double.doubleToLongBits((double) actual));
+ assertThat(Double.doubleToLongBits((double) actual))
+ .as("Values didn't match")
+ .isEqualTo(Double.doubleToLongBits(((Number) expected).doubleValue()));
} else {
- Assert.assertEquals("Primitive value should be equal to expected", expected, actual);
+ assertThat(actual).as("Primitive value should be equal to expected").isEqualTo(expected);
}
break;
case INTEGER:
case FLOAT:
case BOOLEAN:
case DATE:
case TIMESTAMP:
- Assert.assertEquals("Primitive value should be equal to expected", expected, actual);
+ assertThat(actual).as("Primitive value should be equal to expected").isEqualTo(expected);
break;
case STRING:
- Assertions.assertThat(actual).as("Should be a UTF8String").isInstanceOf(UTF8String.class);
- Assert.assertEquals("Strings should be equal", expected, actual.toString());
+ assertThat(actual).as("Should be a UTF8String").isInstanceOf(UTF8String.class);
+ assertThat(actual.toString()).as("Strings should be equal").isEqualTo(expected);
break;
case UUID:
- Assertions.assertThat(expected).as("Should expect a UUID").isInstanceOf(UUID.class);
- Assertions.assertThat(actual).as("Should be a UTF8String").isInstanceOf(UTF8String.class);
- Assert.assertEquals(
- "UUID string representation should match", expected.toString(), actual.toString());
+ assertThat(expected).as("Should expect a UUID").isInstanceOf(UUID.class);
+ assertThat(actual).as("Should be a UTF8String").isInstanceOf(UTF8String.class);
+ assertThat(actual.toString())
+ .as("UUID string representation should match")
+ .isEqualTo(String.valueOf(expected));
break;
case FIXED:
- Assertions.assertThat(expected)
- .as("Should expect a Fixed")
- .isInstanceOf(GenericData.Fixed.class);
- Assertions.assertThat(actual).as("Should be a byte[]").isInstanceOf(byte[].class);
- Assert.assertArrayEquals(
- "Bytes should match", ((GenericData.Fixed) expected).bytes(), (byte[]) actual);
+ assertThat(expected).as("Should expect a Fixed").isInstanceOf(GenericData.Fixed.class);
+ assertThat(actual).as("Should be a byte[]").isInstanceOf(byte[].class);
+ assertThat((byte[]) actual)
Review Comment:
I think this cast isn't needed
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/data/TestHelpers.java:
##########
@@ -304,86 +294,77 @@ private static void assertEqualsUnsafe(Type type, Object expected, Object actual
switch (type.typeId()) {
case LONG:
- Assertions.assertThat(actual).as("Should be a long").isInstanceOf(Long.class);
+ assertThat(actual).as("Should be a long").isInstanceOf(Long.class);
if (expected instanceof Integer) {
- Assert.assertEquals("Values didn't match", ((Number) expected).longValue(), actual);
+ assertThat(actual).as("Values didn't match").isEqualTo(((Number) expected).longValue());
} else {
- Assert.assertEquals("Primitive value should be equal to expected", expected, actual);
+ assertThat(actual).as("Primitive value should be equal to expected").isEqualTo(expected);
}
break;
case DOUBLE:
- Assertions.assertThat(actual).as("Should be a double").isInstanceOf(Double.class);
+ assertThat(actual).as("Should be a double").isInstanceOf(Double.class);
if (expected instanceof Float) {
- Assert.assertEquals(
- "Values didn't match",
- Double.doubleToLongBits(((Number) expected).doubleValue()),
- Double.doubleToLongBits((double) actual));
+ assertThat(Double.doubleToLongBits((double) actual))
+ .as("Values didn't match")
+ .isEqualTo(Double.doubleToLongBits(((Number) expected).doubleValue()));
} else {
- Assert.assertEquals("Primitive value should be equal to expected", expected, actual);
+ assertThat(actual).as("Primitive value should be equal to expected").isEqualTo(expected);
}
break;
case INTEGER:
case FLOAT:
case BOOLEAN:
case DATE:
case TIMESTAMP:
- Assert.assertEquals("Primitive value should be equal to expected", expected, actual);
+ assertThat(actual).as("Primitive value should be equal to expected").isEqualTo(expected);
break;
case STRING:
- Assertions.assertThat(actual).as("Should be a UTF8String").isInstanceOf(UTF8String.class);
- Assert.assertEquals("Strings should be equal", expected, actual.toString());
+ assertThat(actual).as("Should be a UTF8String").isInstanceOf(UTF8String.class);
+ assertThat(actual.toString()).as("Strings should be equal").isEqualTo(expected);
break;
case UUID:
- Assertions.assertThat(expected).as("Should expect a UUID").isInstanceOf(UUID.class);
- Assertions.assertThat(actual).as("Should be a UTF8String").isInstanceOf(UTF8String.class);
- Assert.assertEquals(
- "UUID string representation should match", expected.toString(), actual.toString());
+ assertThat(expected).as("Should expect a UUID").isInstanceOf(UUID.class);
+ assertThat(actual).as("Should be a UTF8String").isInstanceOf(UTF8String.class);
+ assertThat(actual.toString())
+ .as("UUID string representation should match")
+ .isEqualTo(String.valueOf(expected));
break;
case FIXED:
- Assertions.assertThat(expected)
- .as("Should expect a Fixed")
- .isInstanceOf(GenericData.Fixed.class);
- Assertions.assertThat(actual).as("Should be a byte[]").isInstanceOf(byte[].class);
- Assert.assertArrayEquals(
- "Bytes should match", ((GenericData.Fixed) expected).bytes(), (byte[]) actual);
+ assertThat(expected).as("Should expect a Fixed").isInstanceOf(GenericData.Fixed.class);
+ assertThat(actual).as("Should be a byte[]").isInstanceOf(byte[].class);
+ assertThat((byte[]) actual)
+ .as("Bytes should match")
+ .isEqualTo(((GenericData.Fixed) expected).bytes());
break;
case BINARY:
- Assertions.assertThat(expected)
- .as("Should expect a ByteBuffer")
- .isInstanceOf(ByteBuffer.class);
- Assertions.assertThat(actual).as("Should be a byte[]").isInstanceOf(byte[].class);
- Assert.assertArrayEquals(
- "Bytes should match", ((ByteBuffer) expected).array(), (byte[]) actual);
+ assertThat(expected).as("Should expect a ByteBuffer").isInstanceOf(ByteBuffer.class);
+ assertThat(actual).as("Should be a byte[]").isInstanceOf(byte[].class);
+ assertThat((byte[]) actual)
Review Comment:
same here
--
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
Re: [PR] Spark Migration to JUnit5 AssertJ - spark/data directory [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9341:
URL: https://github.com/apache/iceberg/pull/9341#discussion_r1434150438
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/data/TestSparkOrcReadMetadataColumns.java:
##########
@@ -95,24 +99,20 @@ public class TestSparkOrcReadMetadataColumns {
}
}
- @Parameterized.Parameters(name = "vectorized = {0}")
- public static Object[] parameters() {
- return new Object[] {false, true};
+ @Parameters(name = "vectorized = {0}")
+ public static Collection<Boolean> parameters() {
+ return Arrays.asList(false, true);
}
- @Rule public TemporaryFolder temp = new TemporaryFolder();
+ @TempDir private java.nio.file.Path temp;
- private boolean vectorized;
+ @Parameter private boolean vectorized;
private File testFile;
- public TestSparkOrcReadMetadataColumns(boolean vectorized) {
- this.vectorized = vectorized;
- }
-
- @Before
+ @BeforeEach
public void writeFile() throws IOException {
- testFile = temp.newFile();
- Assert.assertTrue("Delete should succeed", testFile.delete());
+ testFile = File.createTempFile(UUID.randomUUID().toString(), null, temp.toFile());
Review Comment:
rather than generating a UUID in every single test it would be better to just use `junit` as the prefix. You could also use `Files.createTempFile(temp, "junit", ...)`, which takes a `Path` as a parameter, so you don't have to call `.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
Re: [PR] Spark Migration to JUnit5 AssertJ - non-parameterized, spark/data directory [iceberg]
Posted by "chinmay-bhat (via GitHub)" <gi...@apache.org>.
chinmay-bhat commented on PR #9341:
URL: https://github.com/apache/iceberg/pull/9341#issuecomment-1866220019
Initially, I assumed I can seperate out parameterize and non-parameterize files. But the CI errors included parameterized files that inherit from `spark/data/AvroDataTest`.
So I'm converting these files from JUnit4 to JUnit5
- the files (parameterized and non-parameterized) in `spark/data` and
- the param files in`spark/source` that inherit from AvroDataTest.
All CI tests pass locally with this command.
`./gradlew -DsparkVersions=3.5 -DscalaVersion=2.13 -DhiveVersions= -DflinkVersions= :iceberg-spark:iceberg-spark-3.5_2.13:check -Pquick=true -x javadoc`
--
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
Re: [PR] Spark Migration to JUnit5 AssertJ - non-parameterized, spark/data directory [iceberg]
Posted by "chinmay-bhat (via GitHub)" <gi...@apache.org>.
chinmay-bhat commented on PR #9341:
URL: https://github.com/apache/iceberg/pull/9341#issuecomment-1864563722
Changes:
1. update assertJ imports
2. remove cast - int, byte[]
3. update tempDir access - solves build errors
4. I had erroneously included a parameterized file in this PR. Reverted the files changes. Will add it in a seperate PR.
--
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
Re: [PR] Spark Migration to JUnit5 AssertJ - non-parameterized, spark/data directory [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9341:
URL: https://github.com/apache/iceberg/pull/9341#discussion_r1432986527
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/data/TestSparkOrcReadMetadataColumns.java:
##########
@@ -100,7 +99,7 @@ public static Object[] parameters() {
return new Object[] {false, true};
}
- @Rule public TemporaryFolder temp = new TemporaryFolder();
+ @TempDir public java.nio.file.Path temp;
Review Comment:
hm I think you either need to convert this test to a JUnit5-parameterized test or revert the changes so that it still runs as a JUnit4 test
--
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
Re: [PR] Spark Migration to JUnit5 AssertJ - non-parameterized, spark/data directory [iceberg]
Posted by "chinmay-bhat (via GitHub)" <gi...@apache.org>.
chinmay-bhat commented on code in PR #9341:
URL: https://github.com/apache/iceberg/pull/9341#discussion_r1431357331
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/data/TestSparkRecordOrcReaderWriter.java:
##########
@@ -130,24 +130,24 @@ private static void assertRecordEquals(
Iterator<Record> expectedIter = expected.iterator();
Iterator<Record> actualIter = actual.iterator();
for (int i = 0; i < size; i += 1) {
- Assert.assertTrue("Expected iterator should have more rows", expectedIter.hasNext());
- Assert.assertTrue("Actual iterator should have more rows", actualIter.hasNext());
- Assert.assertEquals("Should have same rows.", expectedIter.next(), actualIter.next());
+ assertThat(expectedIter.hasNext()).as("Expected iterator should have more rows").isTrue();
+ assertThat(actualIter.hasNext()).as("Actual iterator should have more rows").isTrue();
+ assertThat(actualIter.next()).as("Should have same rows.").isEqualTo(expectedIter.next());
}
- Assert.assertFalse("Expected iterator should not have any extra rows.", expectedIter.hasNext());
- Assert.assertFalse("Actual iterator should not have any extra rows.", actualIter.hasNext());
+ assertThat(expectedIter.hasNext()).as("Expected iterator should not have any extra rows.").isFalse();
+ assertThat(actualIter.hasNext()).as("Actual iterator should not have any extra rows.").isFalse();
Review Comment:
I know we can run
`assertThat(expectedIter.hasNext()).as("Expected iterator should have more rows").isTrue();` as
`assertThat(expectedIter).as("Expected iterator should have more rows").hasNext();`
But I don't see a similar iterator functionality when the case isFalse() i.e
`assertThat(expectedIter).as("Expected iterator should have more rows").doesntHaveNext();`
So I've retained the old format. Do let me know if this can be improved!
--
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
Re: [PR] Spark Migration to JUnit5 AssertJ - spark/data directory [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9341:
URL: https://github.com/apache/iceberg/pull/9341#discussion_r1434197493
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/source/TestParquetScan.java:
##########
@@ -143,7 +143,7 @@ public void testEmptyTableProjection() throws IOException {
}
private Table createTable(Schema schema) throws IOException {
- File parent = temp.newFolder("parquet");
+ File parent = new File(temp.toFile(), "parquet");
Review Comment:
same here
--
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
Re: [PR] Spark 3.5: Migrate tests to JUnit5 in data directory [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra merged PR #9341:
URL: https://github.com/apache/iceberg/pull/9341
--
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
Re: [PR] Spark Migration to JUnit5 AssertJ - spark/data directory [iceberg]
Posted by "chinmay-bhat (via GitHub)" <gi...@apache.org>.
chinmay-bhat commented on code in PR #9341:
URL: https://github.com/apache/iceberg/pull/9341#discussion_r1434221658
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/source/TestDataFrameWrites.java:
##########
@@ -343,7 +343,7 @@ public void testNullableWithSparkSqlOption() throws IOException {
.as("Spark 3 rejects writing nulls to a required column")
.startsWith("2");
- File location = new File(new File(temp.toFile(), "parquet"), "test");
+ File location = new File(temp.resolve("parquet").toFile(), "test");
Review Comment:
didn't know that! updating 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
Re: [PR] Spark Migration to JUnit5 AssertJ - spark/data directory [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9341:
URL: https://github.com/apache/iceberg/pull/9341#discussion_r1434243638
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/source/TestDataFrameWrites.java:
##########
@@ -186,21 +185,24 @@ private void writeAndValidateWithLocations(Table table, File location, File expe
while (expectedIter.hasNext() && actualIter.hasNext()) {
assertEqualsSafe(tableSchema.asStruct(), expectedIter.next(), actualIter.next());
}
- Assert.assertEquals(
- "Both iterators should be exhausted", expectedIter.hasNext(), actualIter.hasNext());
+ assertThat(actualIter.hasNext())
+ .as("Both iterators should be exhausted")
+ .isEqualTo(expectedIter.hasNext());
table
.currentSnapshot()
.addedDataFiles(table.io())
.forEach(
dataFile ->
- Assert.assertTrue(
- String.format(
- "File should have the parent directory %s, but has: %s.",
- expectedDataDir.getAbsolutePath(), dataFile.path()),
- URI.create(dataFile.path().toString())
- .getPath()
- .startsWith(expectedDataDir.getAbsolutePath())));
+ assertThat(
+ URI.create(dataFile.path().toString())
+ .getPath()
+ .startsWith(expectedDataDir.getAbsolutePath()))
Review Comment:
this can be written in a more fluent way (rather than using `isTrue()` at the end)
--
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
Re: [PR] Spark Migration to JUnit5 AssertJ - non-parameterized, spark/data directory [iceberg]
Posted by "chinmay-bhat (via GitHub)" <gi...@apache.org>.
chinmay-bhat commented on PR #9341:
URL: https://github.com/apache/iceberg/pull/9341#issuecomment-1864806108
When I run the build check (`./gradlew -DallVersions build -x test -x javadoc -x integrationTest`) locally, build succeeds.
BUILD SUCCESSFUL in 5m 29s
496 actionable tasks: 373 executed, 123 up-to-date
10:12:39 PM: Execution finished 'build -DallVersions -x test -x javadoc -x integrationTest'.
--
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
Re: [PR] Spark Migration to JUnit5 AssertJ - non-parameterized, spark/data directory [iceberg]
Posted by "chinmay-bhat (via GitHub)" <gi...@apache.org>.
chinmay-bhat commented on code in PR #9341:
URL: https://github.com/apache/iceberg/pull/9341#discussion_r1431484685
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/data/TestSparkRecordOrcReaderWriter.java:
##########
@@ -130,24 +130,24 @@ private static void assertRecordEquals(
Iterator<Record> expectedIter = expected.iterator();
Iterator<Record> actualIter = actual.iterator();
for (int i = 0; i < size; i += 1) {
- Assert.assertTrue("Expected iterator should have more rows", expectedIter.hasNext());
- Assert.assertTrue("Actual iterator should have more rows", actualIter.hasNext());
- Assert.assertEquals("Should have same rows.", expectedIter.next(), actualIter.next());
+ assertThat(expectedIter.hasNext()).as("Expected iterator should have more rows").isTrue();
+ assertThat(actualIter.hasNext()).as("Actual iterator should have more rows").isTrue();
+ assertThat(actualIter.next()).as("Should have same rows.").isEqualTo(expectedIter.next());
}
- Assert.assertFalse("Expected iterator should not have any extra rows.", expectedIter.hasNext());
- Assert.assertFalse("Actual iterator should not have any extra rows.", actualIter.hasNext());
+ assertThat(expectedIter.hasNext()).as("Expected iterator should not have any extra rows.").isFalse();
+ assertThat(actualIter.hasNext()).as("Actual iterator should not have any extra rows.").isFalse();
Review Comment:
I found `.isExhausted()` can be used for the isFalse() case. Will update all files.
--
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
Re: [PR] Spark Migration to JUnit5 AssertJ - non-parameterized, spark/data directory [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9341:
URL: https://github.com/apache/iceberg/pull/9341#discussion_r1432985133
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/data/TestSparkOrcReadMetadataColumns.java:
##########
@@ -100,7 +99,7 @@ public static Object[] parameters() {
return new Object[] {false, true};
}
- @Rule public TemporaryFolder temp = new TemporaryFolder();
+ @TempDir public java.nio.file.Path temp;
Review Comment:
```suggestion
@TempDir private java.nio.file.Path temp;
```
--
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
Re: [PR] Spark Migration to JUnit5 AssertJ - spark/data directory [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9341:
URL: https://github.com/apache/iceberg/pull/9341#discussion_r1434205224
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/source/TestAvroScan.java:
##########
@@ -68,7 +68,7 @@ public static void stopSpark() {
@Override
protected void writeAndValidate(Schema schema) throws IOException {
- File parent = temp.newFolder("avro");
+ File parent = new File(temp.toFile(), "avro");
Review Comment:
it was throwing an error before because `temp` was not initialized, because it is only initialized when the test is a JUnit5 test (using `org.junit.jupiter.api.Test`)
--
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
Re: [PR] Spark Migration to JUnit5 AssertJ - non-parameterized, spark/data directory [iceberg]
Posted by "chinmay-bhat (via GitHub)" <gi...@apache.org>.
chinmay-bhat commented on code in PR #9341:
URL: https://github.com/apache/iceberg/pull/9341#discussion_r1434062826
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/data/TestSparkOrcReadMetadataColumns.java:
##########
@@ -100,7 +99,7 @@ public static Object[] parameters() {
return new Object[] {false, true};
}
- @Rule public TemporaryFolder temp = new TemporaryFolder();
+ @TempDir public java.nio.file.Path temp;
Review Comment:
not reverting anymore, see [comment](https://github.com/apache/iceberg/pull/9341#issuecomment-1866220019)
--
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
Re: [PR] Spark Migration to JUnit5 AssertJ - spark/data directory [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9341:
URL: https://github.com/apache/iceberg/pull/9341#discussion_r1434150438
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/data/TestSparkOrcReadMetadataColumns.java:
##########
@@ -95,24 +99,20 @@ public class TestSparkOrcReadMetadataColumns {
}
}
- @Parameterized.Parameters(name = "vectorized = {0}")
- public static Object[] parameters() {
- return new Object[] {false, true};
+ @Parameters(name = "vectorized = {0}")
+ public static Collection<Boolean> parameters() {
+ return Arrays.asList(false, true);
}
- @Rule public TemporaryFolder temp = new TemporaryFolder();
+ @TempDir private java.nio.file.Path temp;
- private boolean vectorized;
+ @Parameter private boolean vectorized;
private File testFile;
- public TestSparkOrcReadMetadataColumns(boolean vectorized) {
- this.vectorized = vectorized;
- }
-
- @Before
+ @BeforeEach
public void writeFile() throws IOException {
- testFile = temp.newFile();
- Assert.assertTrue("Delete should succeed", testFile.delete());
+ testFile = File.createTempFile(UUID.randomUUID().toString(), null, temp.toFile());
Review Comment:
rather than generating a UUID in every single test it would be better to just use `junit` as the prefix. You could also use `Files.createTempFile(temp, "junit", ...).toFile()`, which takes a `Path` as a parameter
--
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
Re: [PR] Spark Migration to JUnit5 AssertJ - spark/data directory [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9341:
URL: https://github.com/apache/iceberg/pull/9341#discussion_r1434196954
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/source/TestDataFrameWrites.java:
##########
@@ -272,20 +274,22 @@ private Dataset<Row> createDataset(Iterable<Record> records, Schema schema) thro
assertEqualsUnsafe(schema.asStruct(), recordIter.next(), row);
rows.add(row);
}
- Assert.assertEquals(
- "Both iterators should be exhausted", recordIter.hasNext(), readIter.hasNext());
+ assertThat(readIter.hasNext())
+ .as("Both iterators should be exhausted")
+ .isEqualTo(recordIter.hasNext());
}
JavaRDD<InternalRow> rdd = sc.parallelize(rows);
return spark.internalCreateDataFrame(JavaRDD.toRDD(rdd), convert(schema), false);
}
- @Test
+ @TestTemplate
public void testNullableWithWriteOption() throws IOException {
- Assume.assumeTrue(
- "Spark 3 rejects writing nulls to a required column", spark.version().startsWith("2"));
+ assumeThat(spark.version())
+ .as("Spark 3 rejects writing nulls to a required column")
+ .startsWith("2");
- File location = new File(temp.newFolder("parquet"), "test");
+ File location = new File(new File(temp.toFile(), "parquet"), "test");
Review Comment:
same here
--
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
Re: [PR] Spark Migration to JUnit5 AssertJ - spark/data directory [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9341:
URL: https://github.com/apache/iceberg/pull/9341#discussion_r1434336535
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/source/TestDataFrameWrites.java:
##########
@@ -63,29 +70,21 @@
import org.apache.spark.sql.SaveMode;
import org.apache.spark.sql.SparkSession;
import org.apache.spark.sql.catalyst.InternalRow;
-import org.assertj.core.api.Assertions;
-import org.junit.AfterClass;
-import org.junit.Assert;
-import org.junit.Assume;
-import org.junit.BeforeClass;
-import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
-
-@RunWith(Parameterized.class)
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.TestTemplate;
+import org.junit.jupiter.api.extension.ExtendWith;
+
+@ExtendWith(ParameterizedTestExtension.class)
public class TestDataFrameWrites extends AvroDataTest {
private static final Configuration CONF = new Configuration();
- private final String format;
-
- @Parameterized.Parameters(name = "format = {0}")
- public static Object[] parameters() {
- return new Object[] {"parquet", "avro", "orc"};
+ @Parameters(name = "format = {0}")
+ public static Collection<String> parameters() {
+ return Arrays.asList("parquet", "avro", "orc");
}
- public TestDataFrameWrites(String format) {
- this.format = format;
- }
+ @Parameter private String format = "parquet";
Review Comment:
@chinmay-bhat I pushed a commit to fix this and some other places that were wrongly using parameterized tests
--
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
Re: [PR] Spark Migration to JUnit5 AssertJ - spark/data directory [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9341:
URL: https://github.com/apache/iceberg/pull/9341#discussion_r1434181849
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/data/TestSparkOrcReadMetadataColumns.java:
##########
@@ -95,24 +99,20 @@ public class TestSparkOrcReadMetadataColumns {
}
}
- @Parameterized.Parameters(name = "vectorized = {0}")
- public static Object[] parameters() {
- return new Object[] {false, true};
+ @Parameters(name = "vectorized = {0}")
+ public static Collection<Boolean> parameters() {
+ return Arrays.asList(false, true);
}
- @Rule public TemporaryFolder temp = new TemporaryFolder();
+ @TempDir private java.nio.file.Path temp;
- private boolean vectorized;
+ @Parameter private boolean vectorized;
private File testFile;
- public TestSparkOrcReadMetadataColumns(boolean vectorized) {
- this.vectorized = vectorized;
- }
-
- @Before
+ @BeforeEach
public void writeFile() throws IOException {
- testFile = temp.newFile();
- Assert.assertTrue("Delete should succeed", testFile.delete());
+ testFile = File.createTempFile(UUID.randomUUID().toString(), null, temp.toFile());
Review Comment:
either is fine as long as it works ;)
--
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