Copilot commented on code in PR #6050:
URL: https://github.com/apache/hive/pull/6050#discussion_r2324218320
##########
iceberg/iceberg-handler/src/test/java/org/apache/iceberg/mr/TestInputFormatReaderDeletes.java:
##########
@@ -36,12 +36,14 @@
import org.apache.iceberg.data.InternalRecordWrapper;
import org.apache.iceberg.hadoop.HadoopTables;
import org.apache.iceberg.util.StructLikeSet;
-import org.junit.Assert;
-import org.junit.Before;
-import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.params.ParameterizedClass;
+import org.junit.jupiter.params.provider.MethodSource;
-@RunWith(Parameterized.class)
+import static org.assertj.core.api.Assertions.assertThat;
+
+@ParameterizedClass(name = "fileFormat = {0}, formatVersion = {1},
inputFormat={2}")
+@MethodSource("parameters")
Review Comment:
The @ParameterizedClass annotation is incorrectly used here. For JUnit 5
parameterized tests on test classes, use @ParameterizedTest on individual test
methods instead of @ParameterizedClass on the class level, or use
@TestInstance(TestInstance.Lifecycle.PER_CLASS) with constructor injection.
##########
iceberg/iceberg-handler/src/test/java/org/apache/iceberg/mr/hive/writer/TestHiveIcebergDeleteWriter.java:
##########
@@ -38,15 +39,20 @@
import org.apache.iceberg.relocated.com.google.common.collect.Sets;
import org.apache.iceberg.util.CharSequenceSet;
import org.apache.iceberg.util.StructLikeSet;
-import org.junit.Assert;
-import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedClass;
+import org.junit.jupiter.params.provider.MethodSource;
-@RunWith(Parameterized.class)
+@ParameterizedClass(name = "fileFormat={0}, partitioned={1}, skipRowData={2}")
+@MethodSource("parameters")
Review Comment:
The @ParameterizedClass annotation is incorrectly used here. For JUnit 5
parameterized tests on test classes, use @ParameterizedTest on individual test
methods instead of @ParameterizedClass on the class level, or use
@TestInstance(TestInstance.Lifecycle.PER_CLASS) with constructor injection.
##########
iceberg/iceberg-handler/src/test/java/org/apache/iceberg/mr/TestIcebergInputFormats.java:
##########
@@ -69,23 +70,23 @@
import org.apache.iceberg.types.TypeUtil;
import org.apache.iceberg.types.Types;
import org.apache.iceberg.util.ThreadPools;
-import org.assertj.core.api.Assertions;
-import org.junit.Assert;
-import org.junit.Assume;
-import org.junit.Before;
-import org.junit.Ignore;
-import org.junit.Rule;
-import org.junit.Test;
-import org.junit.rules.TemporaryFolder;
-import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Disabled;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+import org.junit.jupiter.params.ParameterizedClass;
+import org.junit.jupiter.params.provider.MethodSource;
import static org.apache.iceberg.types.Types.NestedField.required;
import static org.assertj.core.api.Assertions.assertThat;
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertTrue;
+import static org.assertj.core.api.AssertionsForClassTypes.assertThatThrownBy;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.junit.jupiter.api.Assumptions.assumeTrue;
-@RunWith(Parameterized.class)
+@ParameterizedClass(name = "testInputFormat = {0}, fileFormat = {1}")
+@MethodSource("parameters")
Review Comment:
The @ParameterizedClass annotation is incorrectly used here. For JUnit 5
parameterized tests on test classes, use @ParameterizedTest on individual test
methods instead of @ParameterizedClass on the class level, or use
@TestInstance(TestInstance.Lifecycle.PER_CLASS) with constructor injection.
##########
iceberg/iceberg-handler/src/test/java/org/apache/iceberg/mr/TestInputFormatReaderDeletes.java:
##########
@@ -82,8 +83,8 @@ public TestInputFormatReaderDeletes(FileFormat fileFormat,
int formatVersion, St
protected Table createTable(String name, Schema schema, PartitionSpec spec)
throws IOException {
Table table;
- File location = temp.newFolder(inputFormat, fileFormat.name());
- Assert.assertTrue(location.delete());
+ File location =
temp.resolve(inputFormat).resolve(fileFormat.name()).toFile();
+ assertThat(location.mkdirs()).isTrue();
Review Comment:
The assertion checks if mkdirs() returns true, but this doesn't verify the
intended behavior. The original code verified the location doesn't exist after
deletion, but now it creates the directory. This should either verify the
directory doesn't exist before creation or remove the assertion since mkdirs()
can return false for existing directories.
```suggestion
location.mkdirs();
assertThat(location.exists()).isTrue();
```
##########
iceberg/iceberg-handler/src/test/java/org/apache/iceberg/mr/hive/vector/TestHiveVectorizedReader.java:
##########
@@ -63,24 +62,24 @@ public class TestHiveVectorizedReader {
required(2, "id", Types.LongType.get()),
required(3, "date", Types.StringType.get()));
- @Rule
- public TemporaryFolder temp = new TemporaryFolder();
+ @TempDir
+ public java.nio.file.Path tempDir;
private TestHelper helper;
private InputFormatConfig.ConfigBuilder builder;
private final FileFormat fileFormat = FileFormat.PARQUET;
- @Before
+ @BeforeEach
public void before() throws IOException, HiveException {
- File location = temp.newFolder(fileFormat.name());
- Assert.assertTrue(location.delete());
+ File location = tempDir.resolve(fileFormat.name()).toFile();
+ assertThat(location.mkdirs()).isTrue();
Review Comment:
The assertion checks if mkdirs() returns true, but this doesn't verify the
intended behavior. The original code verified the location doesn't exist, but
now it creates the directory. This should either verify the directory doesn't
exist before creation or remove the assertion since mkdirs() can return false
for existing directories.
```suggestion
location.mkdirs();
assertThat(location.exists()).isTrue();
```
--
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: gitbox-unsubscr...@hive.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscr...@hive.apache.org
For additional commands, e-mail: gitbox-h...@hive.apache.org
