MR: Migrate parameterized tests to JUni5 #9711

lisirrx · 2024-02-11T16:23:29Z

@nastra Could you please review this pr?

nastra · 2024-02-12T07:14:55Z

mr/src/test/java/org/apache/iceberg/mr/TestIcebergInputFormats.java

@@ -90,52 +93,49 @@ public class TestIcebergInputFormats {
  private static final PartitionSpec SPEC =
      PartitionSpec.builderFor(SCHEMA).identity("date").bucket("id", 1).build();

-  @Rule public TemporaryFolder temp = new TemporaryFolder();
+  @TempDir public Path temp;


Suggested change

@TempDir public Path temp;

@TempDir private Path temp;

nastra · 2024-02-12T07:17:43Z

mr/src/test/java/org/apache/iceberg/mr/TestIcebergInputFormats.java

  public void before() throws IOException {
    conf = new Configuration();
    conf.set(CatalogUtil.ICEBERG_CATALOG_TYPE, Catalogs.LOCATION);
    HadoopTables tables = new HadoopTables(conf);

-    File location = temp.newFolder(testInputFormat.name(), fileFormat.name());
-    Assert.assertTrue(location.delete());
+    File location = temp.resolve(Paths.get(testInputFormat.name(), fileFormat.name())).toFile();


in other places we use Files.createTempDirectory(temp, "junit").toFile(), so I think we should do the same here

In other places, the Files.createTempDirectory(temp, "junit").toFile() is used to replace temp.newFolder(); which create a junit folder under the temp's root path.
However, when comes temp.newFolder(path1, path2), the method create a new leveled path like ${tempRoot}/path1/path2. I read the source code of Files.createTempDirectory and it can only create one level path. So I use temp.resolve here

nastra · 2024-02-12T07:18:13Z

mr/src/test/java/org/apache/iceberg/mr/TestIcebergInputFormats.java

@@ -237,8 +237,8 @@ public void testProjection() throws Exception {

    List<Record> outputRecords = testInputFormat.create(builder.conf()).getRecords();

-    Assert.assertEquals(inputRecords.size(), outputRecords.size());
-    Assert.assertEquals(projection.asStruct(), outputRecords.get(0).struct());
+    assertThat(outputRecords.size()).isEqualTo(inputRecords.size());


Suggested change

assertThat(outputRecords.size()).isEqualTo(inputRecords.size());

assertThat(outputRecords).hasSameSizeAs(inputRecords);

nastra · 2024-02-12T07:19:05Z

mr/src/test/java/org/apache/iceberg/mr/TestIcebergInputFormats.java

  public void testCustomCatalog() throws IOException {
-    String warehouseLocation = temp.newFolder("hadoop_catalog").getAbsolutePath();
+    String warehouseLocation = temp.resolve("hadoop_catalog").toAbsolutePath().toString();


this is correct

nastra · 2024-02-12T07:19:19Z

mr/src/test/java/org/apache/iceberg/mr/TestIcebergInputFormats.java

    }

    public interface Factory<T> {
+


nit: unnecessary newline

nastra · 2024-02-12T07:21:02Z

mr/src/test/java/org/apache/iceberg/mr/hive/TestTables.java


 // Helper class for setting up and testing various catalog implementations
 abstract class TestTables {
+


nit: unnecessary new line

nastra · 2024-02-12T07:21:33Z

mr/src/test/java/org/apache/iceberg/mr/hive/TestTables.java

      this(
          conf,
          temp,
          (HiveVersion.min(HiveVersion.HIVE_3) ? "file:" : "")
-              + temp.newFolder("custom", "warehouse").toString(),
+              + temp.resolve(Paths.get("custom", "warehouse")),


we should also use Files.createTempDirectory(temp, ...) here

nastra · 2024-02-12T07:22:20Z

mr/src/test/java/org/apache/iceberg/mr/hive/TestHiveIcebergStorageHandlerTimezone.java


-  @Rule public TemporaryFolder temp = new TemporaryFolder();
+  @TempDir public Path temp;


Suggested change

@TempDir public Path temp;

@TempDir private Path temp;

nastra · 2024-02-12T07:24:09Z

.../test/java/org/apache/iceberg/mr/hive/TestHiveIcebergStorageHandlerWithMultipleCatalogs.java

  public FileFormat fileFormat1;

-  @Parameterized.Parameter(1)
+  @Parameter(index = 1)
  public FileFormat fileFormat2;


these should all be private instead of public. Could you please adjust all of this parameters to be private?

sure, I'll fix them

mr/src/test/java/org/apache/iceberg/mr/hive/TestHiveIcebergStorageHandlerLocalScan.java

nastra · 2024-02-12T07:25:57Z

mr/src/test/java/org/apache/iceberg/mr/hive/TestHiveIcebergStorageHandlerLocalScan.java

-    Assert.assertArrayEquals(new Object[] {1L, "Bob", "Green"}, rows.get(1));
-    Assert.assertArrayEquals(new Object[] {2L, "Trudy", "Pink"}, rows.get(2));
+    assertThat(rows).hasSize(3);
+    assertThat(rows.get(0)).isEqualTo(new Object[] {0L, "Alice", "Brown"});


nit: this could also be written in a single stmt: assertThat(rows).containsExactly(first, second, third)

nastra · 2024-02-12T07:26:43Z

mr/src/test/java/org/apache/iceberg/mr/hive/TestHiveIcebergStorageHandlerNoScan.java

 public class TestHiveIcebergStorageHandlerNoScan {
+


nastra · 2024-02-12T07:26:54Z

mr/src/test/java/org/apache/iceberg/mr/hive/TestHiveIcebergStorageHandlerNoScan.java

@@ -140,35 +138,34 @@ public static Collection<Object[]> parameters() {

  private TestTables testTables;

-  @Parameter(0)
-  public TestTables.TestTableType testTableType;
+  @Parameter public TestTables.TestTableType testTableType;


Suggested change

@Parameter public TestTables.TestTableType testTableType;

@Parameter private TestTables.TestTableType testTableType;

nastra · 2024-02-12T07:27:02Z

mr/src/test/java/org/apache/iceberg/mr/hive/TestHiveIcebergStorageHandlerNoScan.java


-  @Rule public TemporaryFolder temp = new TemporaryFolder();
+  @TempDir public java.nio.file.Path temp;


Suggested change

@TempDir public java.nio.file.Path temp;

@TempDir private java.nio.file.Path temp;

nastra · 2024-02-12T07:29:01Z

mr/src/test/java/org/apache/iceberg/mr/hive/TestHiveIcebergStorageHandlerNoScan.java

-      Assert.assertNotNull(hmsParams.get(hive_metastoreConstants.DDL_TIME));
-      Assert.assertNotNull(hmsParams.get(InputFormatConfig.PARTITION_SPEC));
+      assertThat(hmsParams).hasSize(14);
+      assertThat(hmsParams.get("custom_property")).isEqualTo("initial_val");


Suggested change

assertThat(hmsParams.get("custom_property")).isEqualTo("initial_val");

assertThat(hmsParams).containsEntry("custom_property", "initial_val");

please also adjust the others as well. The advantage of using containsEntry() is that it will print the content of the map if the assertion ever fails

nastra · 2024-02-12T07:30:49Z

mr/src/test/java/org/apache/iceberg/mr/hive/TestHiveIcebergStorageHandlerWithEngine.java

  public boolean isVectorized;

-  @Rule public TemporaryFolder temp = new TemporaryFolder();
+  @TempDir public Path temp;


Suggested change

@TempDir public Path temp;

@TempDir private Path temp;

same for all the other parameters

nastra · 2024-02-13T09:52:52Z

mr/src/test/java/org/apache/iceberg/mr/hive/TestHiveIcebergStorageHandlerNoScan.java

-      Assert.assertEquals(
-          summary.get(SnapshotSummary.TOTAL_FILE_SIZE_PROP),
-          hmsParams.get(StatsSetupConst.TOTAL_SIZE));
+      assertThat(hmsParams).doesNotContainKey("custom_property");


this seems wrong. Shouldn't this do the same comparison as the previous assertions were doing?

assertThat(hmsParams) .containsEntry(StatsSetupConst.NUM_FILES, summary.get(SnapshotSummary.TOTAL_DATA_FILES_PROP)) .containsEntry(StatsSetupConst.ROW_COUNT, summary.get(SnapshotSummary.TOTAL_RECORDS_PROP)) .containsEntry(...)

nastra · 2024-02-13T09:53:48Z

mr/src/test/java/org/apache/iceberg/mr/hive/TestHiveIcebergStorageHandlerNoScan.java

-      Assert.assertFalse(hmsParams.containsKey("custom_property"));
-      Assert.assertFalse(hmsParams.containsKey("new_prop_1"));
-      Assert.assertTrue(hmsParams.containsKey("new_prop_2"));
+      assertThat(hmsParams).doesNotContainKey("custom_property");


these can be chained together:

assertThat(hmsParams) .doesNotContainKey("custom_property") .doesNotContainKey("new_prop_1") .containsKey("new_prop_2");

can you also please chain together the other assertions against hmsParams

oh, thanks, you teach me something new! I'll fix them.

nastra

LGTM, thanks @lisirrx

lisirrx · 2024-02-14T14:51:45Z

LGTM, thanks @lisirrx

Thanks!

fix apache#9269 Switch class level parameterized for mr

f6bc35a

github-actions bot added the MR label Feb 11, 2024

nastra reviewed Feb 12, 2024

View reviewed changes

mr/src/test/java/org/apache/iceberg/mr/hive/TestHiveIcebergStorageHandlerLocalScan.java Outdated Show resolved Hide resolved

nastra reviewed Feb 12, 2024

View reviewed changes

lisirrx added 2 commits February 13, 2024 17:03

fix apache#9269 Switch class level parameterized for mr

0058718

fix apache#9269 Switch class level parameterized for mr

3335776

nastra changed the title ~~fix #9269 Switch class level parameterized for mr~~ MR: Migrate parameterized tests to JUni5 Feb 13, 2024

nastra reviewed Feb 13, 2024

View reviewed changes

lisirrx added 2 commits February 13, 2024 18:38

fix assert for map using chain

8eaf4c9

format code

e30d121

nastra approved these changes Feb 13, 2024

View reviewed changes

nastra merged commit 50fb400 into apache:main Feb 13, 2024
11 checks passed

nastra mentioned this pull request Feb 13, 2024

iceberg-mr: Switch class level parameterized tests to Junit5 #9269

Closed

bitsondatadev pushed a commit to bitsondatadev/iceberg that referenced this pull request Mar 3, 2024

MR: Migrate parameterized tests to JUni5 (apache#9711)

5fd0bdb

devangjhabakh pushed a commit to cdouglas/iceberg that referenced this pull request Apr 22, 2024

MR: Migrate parameterized tests to JUni5 (apache#9711)

6349f6d

zachdisc pushed a commit to zachdisc/iceberg that referenced this pull request Dec 23, 2024

MR: Migrate parameterized tests to JUni5 (apache#9711)

53f704b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MR: Migrate parameterized tests to JUni5 #9711

MR: Migrate parameterized tests to JUni5 #9711

lisirrx commented Feb 11, 2024

nastra Feb 12, 2024

nastra Feb 12, 2024

lisirrx Feb 13, 2024

nastra Feb 12, 2024

nastra Feb 12, 2024

nastra Feb 12, 2024

nastra Feb 12, 2024

nastra Feb 12, 2024

nastra Feb 12, 2024

nastra Feb 12, 2024

lisirrx Feb 12, 2024

nastra Feb 12, 2024

nastra Feb 12, 2024

nastra Feb 12, 2024

nastra Feb 12, 2024

nastra Feb 12, 2024

nastra Feb 12, 2024

nastra Feb 13, 2024

nastra Feb 13, 2024

nastra Feb 13, 2024

lisirrx Feb 13, 2024

nastra left a comment

lisirrx commented Feb 14, 2024

	assertThat(outputRecords.size()).isEqualTo(inputRecords.size());
	assertThat(outputRecords).hasSameSizeAs(inputRecords);


		// Helper class for setting up and testing various catalog implementations
		abstract class TestTables {


		@Rule public TemporaryFolder temp = new TemporaryFolder();
		@TempDir public Path temp;

	@Parameter public TestTables.TestTableType testTableType;
	@Parameter private TestTables.TestTableType testTableType;


		@Rule public TemporaryFolder temp = new TemporaryFolder();
		@TempDir public java.nio.file.Path temp;

	@TempDir public java.nio.file.Path temp;
	@TempDir private java.nio.file.Path temp;

	assertThat(hmsParams.get("custom_property")).isEqualTo("initial_val");
	assertThat(hmsParams).containsEntry("custom_property", "initial_val");

MR: Migrate parameterized tests to JUni5 #9711

MR: Migrate parameterized tests to JUni5 #9711

Conversation

lisirrx commented Feb 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nastra left a comment

Choose a reason for hiding this comment

lisirrx commented Feb 14, 2024