Migrate Procedure sub-classes in spark-extensions to JUnit5 and AssertJ style by tomtongue · Pull Request #9760 · apache/iceberg

tomtongue · 2024-02-20T11:45:15Z

Migrate the following "Procedure" sub-classes in spark-extensions to JUnit 5 and AssertJ style along with #9613, and #9086.

TestAddFiles was included in #9613, it's skipped in this PR.

Current Progress

…tJ style

tomtongue · 2024-02-21T11:41:05Z

@nastra the migration for procedures is complete. When you have a chance, could you review the changes?

nastra · 2024-02-21T15:07:10Z

...rk-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestCallStatementParser.java

        (CallStatement) parser.parsePlan("CALL c.n.func(1, '2', 3L, true, 1.0D, 9.0e1, 900e-1BD)");
-    Assert.assertEquals(
-        ImmutableList.of("c", "n", "func"), JavaConverters.seqAsJavaList(call.name()));
+    assertThat(seqAsJavaList(call.name())).hasSameElementsAs(ImmutableList.of("c", "n", "func"));


Suggested change

assertThat(seqAsJavaList(call.name())).hasSameElementsAs(ImmutableList.of("c", "n", "func"));

assertThat(seqAsJavaList(call.name())).containsExactly("c", "n", "func");

please also update the other places

nastra · 2024-02-21T15:08:42Z

...rk-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestCallStatementParser.java

  private <T> T checkCast(Object value, Class<T> expectedClass) {
-    Assert.assertTrue(
-        "Expected instance of " + expectedClass.getName(), expectedClass.isInstance(value));
+    assertThat(expectedClass.isInstance(value))


Suggested change

assertThat(expectedClass.isInstance(value))

assertThat(expectedClass).isInstanceOf(value.getClass())

I tried this previously, but the tests related to this method failed with the following error:

// Example java.lang.AssertionError: Expecting actual: org.apache.spark.sql.catalyst.plans.logical.PositionalArgument to be an instance of: org.apache.spark.sql.catalyst.plans.logical.PositionalArgument but was instance of: java.lang.Class

value.getClass seems not to be resolved the correct class name inside of .isInstanceOf.

This works well when using the current code like expectedClass.isInstance(value)

the problem with the current assertion is that it only will print true/false if the assertion fails. What we rather want to achieve for assertions is to have enough context to know what actual/expected was exactly. That being said, we want to avoid the usage of isTrue() / isFalse() as much as possible.

What was the assertion that you previously used?

Thanks for the reply. Yes, I understand the situation and we should avoid using isTrue()/isFalse().

In the current PR, I change it to assertThat(expectedClass.isInstance(value)).isTrue(),
Before submitting the PR, I tested assertThat(expectedClass).isInstanceOf(value.getClass()), but this failed due to the error above. This is the reason assertThat(expectedClass.isInstance(value)).isTrue() is used here.

sorry my bad, the check should be assertThat(value).isInstanceOf(expectedClass)

Ah yes, you're correct. sorry for that. Let me fix it.

nastra · 2024-02-21T15:09:18Z

...rk-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestCallStatementParser.java

-    Assert.assertTrue(
-        "Expected instance of " + expectedClass.getName(), expectedClass.isInstance(value));
+    assertThat(expectedClass.isInstance(value))
+        .as("Expected instance of " + expectedClass.getName())


Suggested change

.as("Expected instance of " + expectedClass.getName())

nastra · 2024-02-21T15:10:59Z

...ions/src/test/java/org/apache/iceberg/spark/extensions/TestCreateChangelogViewProcedure.java

-            sql(
-                "CALL %s.system.create_changelog_view(table => '%s', identifier_columns => array('id'), net_changes => true)",
-                catalogName, tableName));
+    Assertions.assertThatThrownBy(


Suggested change

Assertions.assertThatThrownBy(

assertThatThrownBy(

nastra · 2024-02-21T15:13:48Z

...-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestMigrateTableProcedure.java


    Map<String, String> props = createdTable.properties();
-    Assert.assertEquals("Should have extra property set", "bar", props.get("foo"));
+    assertThat(props.get("foo")).as("Should have extra property set").isEqualTo("bar");


Suggested change

assertThat(props.get("foo")).as("Should have extra property set").isEqualTo("bar");

assertThat(props).containsEntry("foo", "bar");

nastra · 2024-02-21T15:14:38Z

...-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestMigrateTableProcedure.java


    Table table = validationCatalog.loadTable(tableIdent);
-    Assert.assertEquals("Should override user value", "true", table.properties().get("migrated"));
+    assertThat(table.properties().get("migrated"))


should also be using containsEntry()as this will show the full output of the map if the assertion ever fails

nastra · 2024-02-21T15:17:12Z

...nsions/src/test/java/org/apache/iceberg/spark/extensions/TestRemoveOrphanFilesProcedure.java


-    Assertions.assertThat(statsLocation.exists()).as("stats file should exist").isTrue();
-    Assertions.assertThat(statsLocation.length())
+    assertThat(statsLocation.exists()).as("stats file should exist").isTrue();


Suggested change

assertThat(statsLocation.exists()).as("stats file should exist").isTrue();

assertThat(statsLocation).exists();

nastra · 2024-02-21T15:18:35Z

...nsions/src/test/java/org/apache/iceberg/spark/extensions/TestRemoveOrphanFilesProcedure.java

-    Assertions.assertThat(statsLocation.exists()).as("stats file should exist").isTrue();
-    Assertions.assertThat(statsLocation.length())
+    assertThat(statsLocation.exists()).as("stats file should exist").isTrue();
+    assertThat(statsLocation.length())


Suggested change

assertThat(statsLocation.length())

assertThat(statsLocation).hasSize(statisticsFile.fileSizeInBytes())

nastra · 2024-02-21T15:19:09Z

...nsions/src/test/java/org/apache/iceberg/spark/extensions/TestRemoveOrphanFilesProcedure.java

        .as("Deleted files")
        .containsExactly(statsLocation.toURI().toString());
-    Assertions.assertThat(statsLocation.exists()).as("stats file should be deleted").isFalse();
+    assertThat(statsLocation.exists()).as("stats file should be deleted").isFalse();


Suggested change

assertThat(statsLocation.exists()).as("stats file should be deleted").isFalse();

assertThat(statsLocation).doesNotExist();

nastra · 2024-02-21T15:22:59Z

...c/test/java/org/apache/iceberg/spark/extensions/TestRewritePositionDeleteFilesProcedure.java

-                    + "options => map("
-                    + "'foo', 'bar'))",
-                catalogName, tableIdent));
+    Assertions.assertThatThrownBy(


Suggested change

Assertions.assertThatThrownBy(

assertThatThrownBy(

nastra · 2024-02-21T15:24:20Z

...extensions/src/test/java/org/apache/iceberg/spark/extensions/TestSnapshotTableProcedure.java


    Map<String, String> props = createdTable.properties();
-    Assert.assertEquals("Should have extra property set", "bar", props.get("foo"));
+    assertThat(props.get("foo")).as("Should have extra property set").isEqualTo("bar");


please use containsEntry() here

nastra · 2024-02-21T15:25:04Z

...extensions/src/test/java/org/apache/iceberg/spark/extensions/TestSnapshotTableProcedure.java

+        .as("No Snapshoting with Alternate locations with Hadoop Catalogs")
+        .doesNotContain("hadoop");
+    String location = temp.toFile().toString();
+    String snapshotLocation = temp.toFile().toString();


both now point to the same location, whereas in the previous version of the code they were pointing to different folders, so I think we should fix that

nastra · 2024-02-21T15:25:33Z

...extensions/src/test/java/org/apache/iceberg/spark/extensions/TestSnapshotTableProcedure.java

-    Assert.assertEquals(
-        "Should override user value", "false", props.get(TableProperties.GC_ENABLED));
+    assertThat(props.get("snapshot")).as("Should override user value").isEqualTo("true");
+    assertThat(props.get(TableProperties.GC_ENABLED))


this should also use containsEntry()

tomtongue · 2024-02-21T16:27:28Z

Thanks for the review. fix them tomorrow.

nastra · 2024-02-22T14:59:43Z

...-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestMigrateTableProcedure.java

-    Assume.assumeTrue(catalogName.equals("spark_catalog"));
-    String location = temp.newFolder().toString();
+    assumeThat(catalogName).isEqualToIgnoringCase("spark_catalog");
+    String location = temp.toFile().toString();


in other places we use Files.createTempDirectory(temp, "junit").toFile().toString(). We should probably do the same here and in other places from this PR

Other sub-classes in spark-extensions like TestAddFilesProcedure and TestWriteAborts are newly updated with File.createTempDirectory in the new commit.

nastra · 2024-02-22T15:00:21Z

...nsions/src/test/java/org/apache/iceberg/spark/extensions/TestRemoveOrphanFilesProcedure.java

      sql(
          "CREATE TABLE %s (id bigint NOT NULL, data string) USING iceberg LOCATION '%s'",
-          tableName, temp.newFolder());
+          tableName, temp.toFile());


Files.createTempDirectory(temp, "junit").toFile()

nastra · 2024-02-23T13:37:01Z

...park-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestAddFilesProcedure.java

-  public void setupTempDirs() {
-    fileTableDir = temp.toFile();
+  public void setupTempDirs() throws IOException {
+    fileTableDir = Files.createTempDirectory(temp, "junit").toFile();


temp is already a temporary directory, so we don't need to do this here

Okay sure. will revert this part.

nastra · 2024-02-23T13:47:27Z

...nsions/src/test/java/org/apache/iceberg/spark/extensions/TestRemoveOrphanFilesProcedure.java

 */
 package org.apache.iceberg.spark.extensions;

+import static java.nio.file.Files.createTempDirectory;


we typically only statically import stuff from AssertJ (and a few other things) but avoid statically importing stuff like these methods, so please use Files.createTempDirectory to be in-line with the rest of the codebase

@nastra Sure, got it. I statically import the method here because the java.nio.file.Files is conflicting with the same class name like org.apache.iceberg.Files. Will define a method for this.

If there's something better for this, please let me know.

please use java.nio.file.Files.createTempDirectory in that case

Sure, thank you. Will update all the related lines.

nastra

LGTM, thanks @tomtongue

tomtongue · 2024-02-24T14:16:06Z

@nastra Thanks for your reviews! There are still some spark-extensions sub-classes that are necessary to be migrated (I believe they are 5 classes). I will submit the remaining classes.

Migrate Procedure sub-classes in spark-extensions to JUnit5 and Asser…

2d41bef

…tJ style

tomtongue changed the title ~~Migrate Procedure sub-classes in spark-extensions to JUnit5 and AssertJ style~~ [WIP] Migrate Procedure sub-classes in spark-extensions to JUnit5 and AssertJ style Feb 20, 2024

github-actions bot added the spark label Feb 20, 2024

Update the remaining procedures with JUnit5 and AssertJ

5f20957

tomtongue changed the title ~~[WIP] Migrate Procedure sub-classes in spark-extensions to JUnit5 and AssertJ style~~ Migrate Procedure sub-classes in spark-extensions to JUnit5 and AssertJ style Feb 21, 2024

nastra reviewed Feb 21, 2024

View reviewed changes

Reflect the comments

8f593a8

nastra reviewed Feb 22, 2024

View reviewed changes

Update temp directory creation with Files

a375693

nastra reviewed Feb 23, 2024

View reviewed changes

tomtongue added 2 commits February 23, 2024 23:11

Reflect the comments related to createTempDirectory

e94bd2f

Update createTempDirectory with specifying the full path

ae3d3e6

nastra approved these changes Feb 24, 2024

View reviewed changes

nastra merged commit 56da99b into apache:main Feb 24, 2024

tomtongue deleted the mig-junit5-procedures-extensions branch February 24, 2024 14:16

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

Spark: Migrate procedure tests to JUnit5 (apache#9760)

0ae559d

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

Spark: Migrate procedure tests to JUnit5 (apache#9760)

97da07d

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

Spark: Migrate procedure tests to JUnit5 (apache#9760)

c32699c

	assertThat(seqAsJavaList(call.name())).hasSameElementsAs(ImmutableList.of("c", "n", "func"));
	assertThat(seqAsJavaList(call.name())).containsExactly("c", "n", "func");

	assertThat(expectedClass.isInstance(value))
	assertThat(expectedClass).isInstanceOf(value.getClass())

	assertThat(props.get("foo")).as("Should have extra property set").isEqualTo("bar");
	assertThat(props).containsEntry("foo", "bar");

	assertThat(statsLocation.exists()).as("stats file should exist").isTrue();
	assertThat(statsLocation).exists();

	assertThat(statsLocation.length())
	assertThat(statsLocation).hasSize(statisticsFile.fileSizeInBytes())

	assertThat(statsLocation.exists()).as("stats file should be deleted").isFalse();
	assertThat(statsLocation).doesNotExist();

Conversation

tomtongue commented Feb 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current Progress

Uh oh!

tomtongue commented Feb 21, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nastra Feb 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomtongue Feb 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomtongue Feb 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomtongue commented Feb 21, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomtongue Feb 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomtongue Feb 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomtongue Feb 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nastra left a comment

tomtongue commented Feb 20, 2024 •

edited

Loading

nastra Feb 21, 2024 •

edited

Loading

tomtongue Feb 22, 2024 •

edited

Loading

tomtongue Feb 22, 2024 •

edited

Loading

tomtongue Feb 23, 2024 •

edited

Loading

tomtongue Feb 23, 2024 •

edited

Loading

tomtongue Feb 23, 2024 •

edited

Loading