nastra commented on code in PR #7954:
URL: https://github.com/apache/iceberg/pull/7954#discussion_r1250376081
##########
build.gradle:
##########
@@ -957,4 +960,3 @@ apply from: 'jmh.gradle'
apply from: 'baseline.gradle'
apply from: 'deploy.gradle'
apply from: 'tasks.gradle'
-
Review Comment:
nit: please revert this change
##########
orc/src/test/java/org/apache/iceberg/orc/TestOrcDataWriter.java:
##########
@@ -141,7 +137,7 @@ public void testUsingFileIO() throws IOException {
// local files that would be the LocalFileSystem. To prevent this we use
the Proxy classes to
// use a scheme `dummy` that is not handled. Note that Hadoop 2.7.3 throws
IOException
// while latest Hadoop versions throw UnsupportedFileSystemException
(extends IOException)
- ProxyOutputFile outFile = new
ProxyOutputFile(Files.localOutput(temp.newFile()));
+ ProxyOutputFile outFile = new
ProxyOutputFile(Files.localOutput(String.valueOf(temp.isFile())));
Review Comment:
this seems wrong. This expects a file and not a "true"/"false" string
##########
orc/src/test/java/org/apache/iceberg/orc/TestOrcDeleteWriters.java:
##########
@@ -163,13 +162,14 @@ public void testPositionDeleteWriter() throws IOException
{
deletedRecords = Lists.newArrayList(reader);
}
- Assert.assertEquals(
- "Deleted records should match expected", expectedDeleteRecords,
deletedRecords);
+ Assertions.assertThat(deletedRecords)
+ .as("Deleted records should match expected")
+ .isEqualTo(expectedDeleteRecords);
}
@Test
public void testPositionDeleteWriterWithEmptyRow() throws IOException {
- File deleteFile = temp.newFile();
+ File deleteFile = temp;
Review Comment:
same here
##########
orc/src/test/java/org/apache/iceberg/orc/TestTableProperties.java:
##########
@@ -50,7 +49,8 @@ public class TestTableProperties {
Types.NestedField.optional(1, "id", Types.IntegerType.get()),
Types.NestedField.optional(2, "data", Types.StringType.get()));
- @ClassRule public static final TemporaryFolder TEMPORARY_FOLDER = new
TemporaryFolder();
+ @TempDir File folder;
+ @TempDir File testFile;
Review Comment:
both can be private
##########
orc/src/test/java/org/apache/iceberg/orc/TestOrcDataWriter.java:
##########
@@ -62,9 +61,9 @@ public class TestOrcDataWriter {
private List<Record> records;
- @Rule public TemporaryFolder temp = new TemporaryFolder();
+ @TempDir File temp;
Review Comment:
private
##########
orc/src/test/java/org/apache/iceberg/orc/TestOrcDeleteWriters.java:
##########
@@ -106,12 +104,14 @@ public void testEqualityDeleteWriter() throws IOException
{
deletedRecords = Lists.newArrayList(reader);
}
- Assert.assertEquals("Deleted records should match expected", records,
deletedRecords);
+ Assertions.assertThat(deletedRecords)
+ .as("Deleted records should match expected")
+ .isEqualTo(records);
}
@Test
public void testPositionDeleteWriter() throws IOException {
- File deleteFile = temp.newFile();
+ File deleteFile = temp;
Review Comment:
`deleteFile` can be removed
##########
orc/src/test/java/org/apache/iceberg/orc/TestOrcDeleteWriters.java:
##########
@@ -73,7 +72,7 @@ public void createDeleteRecords() {
@Test
public void testEqualityDeleteWriter() throws IOException {
- File deleteFile = temp.newFile();
+ File deleteFile = temp;
Review Comment:
we can probably remove `deleteFile` alltogether
##########
orc/src/test/java/org/apache/iceberg/orc/TestBloomFilter.java:
##########
@@ -54,12 +52,11 @@ public class TestBloomFilter {
required(101, "name", Types.StringType.get()),
required(102, "price", Types.DoubleType.get()));
- @Rule public TemporaryFolder temp = new TemporaryFolder();
+ @TempDir File testFile;
Review Comment:
nit: can be made private
##########
orc/src/test/java/org/apache/iceberg/orc/TestOrcDataReader.java:
##########
@@ -43,13 +43,13 @@
import org.apache.iceberg.types.Types;
import org.apache.orc.OrcConf;
import org.assertj.core.api.WithAssertions;
-import org.junit.BeforeClass;
-import org.junit.ClassRule;
-import org.junit.Test;
-import org.junit.rules.TemporaryFolder;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
public class TestOrcDataReader implements WithAssertions {
- @ClassRule public static TemporaryFolder temp = new TemporaryFolder();
+
+ @TempDir static File temp;
Review Comment:
nit: can probably be made private
##########
orc/src/test/java/org/apache/iceberg/orc/TestOrcDeleteWriters.java:
##########
@@ -55,9 +54,9 @@ public class TestOrcDeleteWriters {
private List<Record> records;
- @Rule public TemporaryFolder temp = new TemporaryFolder();
+ @TempDir File temp;
Review Comment:
private
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
