Create ExtensionTestBase for migration to JUnit5 by tomtongue · Pull Request #9613 · apache/iceberg

tomtongue · 2024-02-01T14:04:13Z

Create ExtensionTestBase to migrate current tests in SparkExtension to JUnit 5 in regards to #9086.

For now, only one test TestAddFilesProcedure is updated with JUnit 5. Once the current migration works well, I will migrate other classes in the extension path.

nastra · 2024-02-01T14:41:21Z

...5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/ExtensionsTestBase.java

+  private static final Random RANDOM = ThreadLocalRandom.current();
+
+  @Parameters(name = "catalogName = {0}, implementation = {1}, config = {2}")
+  protected static Object[][] parameters() {


I don't think this is needed here because the default params are coming from CatalogTestBase

remove the parameters part (keep the RANDOM because the parent class doesn't have it and it's required).

nastra · 2024-02-01T14:41:41Z

...5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/ExtensionsTestBase.java

+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.extension.ExtendWith;
+
+@ExtendWith(ParameterizedTestExtension.class)


I also don't think this is required, because CatalogTestBase already has this

nastra · 2024-02-01T14:42:17Z

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


-  private final int formatVersion;
+  @Parameter(index = 3)
+  protected int formatVersion;


Suggested change

protected int formatVersion;

private int formatVersion;

nastra · 2024-02-01T14:42:47Z

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

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


no need to make this protected, because this class doesn't have any subclasses

nastra · 2024-02-01T14:43:55Z

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

    // Spark Session Catalog cannot load metadata tables
    // with "The namespace in session catalog must have exactly one name part"
-    Assume.assumeFalse(catalogName.equals("spark_catalog"));
+    Assumptions.assumeFalse(catalogName.equals("spark_catalog"));


please use the one from AssertJ: assumeThat(catalog).isNotEqualTo("spark_catalog")

oops, sorry, let me fix this.

nastra · 2024-02-01T14:44:56Z

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

    record2.put("id", 2L);
    record2.put("data", "b");
-    File outputFile = temp.newFile("test.avro");
+    File outputFile = File.createTempFile("test", ".avro", temp.toFile());


Suggested change

File outputFile = File.createTempFile("test", ".avro", temp.toFile());

File outputFile = temp.resolve("test.avro").toFile();

nastra · 2024-02-01T14:45:23Z

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

            catalogName, tableName, fileTableDir.getAbsolutePath());

-    Assert.assertEquals(2L, result);
+    Assertions.assertThat(result).isEqualTo(2L);


it's ok to statically import assertThat() in places where it's newly introduced

nastra · 2024-02-01T14:47:36Z

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


    Matcher matcher = uuidPattern.matcher(manifestPath);
-    Assert.assertTrue("verify manifest path has uuid", matcher.find());
+    Assertions.assertThat(matcher.find()).as("verify manifest path has uuid").isTrue();


Suggested change

Assertions.assertThat(matcher.find()).as("verify manifest path has uuid").isTrue();

assertThat(matcher).as("verify manifest path has uuid").matches();

matches is a full string match, so this couldn't pass the test if this test was changed. Or, should I change the regex pattern?

nastra · 2024-02-01T14:50:45Z

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

-    } catch (IOException e) {
-      throw new RuntimeException(e);
-    }
+    fileTableDir = temp.toFile();


could you please check if we actually need fileTableDir? I think the calls that use fileTableDir.getAbsolutePath() can be replaced with temp.toFile().getAbsolutePath().

I believe temp.toFile().getAbsolutePath() is evaluated in the sql and I couldn't find any errors based on my several attempts. However, fileTableDir is called 33 times in this class, and keep the current one is less changes. What do you think? (committed without changes for now)

ok let's keep fileTableDir in this case

nastra · 2024-02-02T08:47:32Z

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

            catalogName, tableName, fileTableDir.getAbsolutePath());

-    Assert.assertEquals(2L, result);
+    Assertions.assertThat(result).isEqualTo(2L);


please use the statically imported method

nastra · 2024-02-02T08:47:41Z

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

    // Spark Session Catalog cannot load metadata tables
    // with "The namespace in session catalog must have exactly one name part"
-    Assume.assumeFalse(catalogName.equals("spark_catalog"));
+    Assumptions.assumeThat(catalogName).isNotEqualTo("spark_catalog");


please use the statically imported method

nastra · 2024-02-02T08:48:13Z

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

    // verify manifest file name has uuid pattern
    String manifestPath = (String) sql("select path from %s.manifests", tableName).get(0)[0];
-
+    System.out.println(manifestPath);


should be removed

Forgot to remove...will do it.

nastra · 2024-02-02T08:53:09Z

please also add the below diff to iceberg-spark-extensions so that JUnit5 tests are properly executed

diff --git a/spark/v3.5/build.gradle b/spark/v3.5/build.gradle
index eeef75be6..5b07a3c38 100644
--- a/spark/v3.5/build.gradle
+++ b/spark/v3.5/build.gradle
@@ -172,6 +172,10 @@ project(":iceberg-spark:iceberg-spark-extensions-${sparkMajorVersion}_${scalaVer
     antlr libs.antlr.antlr4
   }
 
+  test {
+    useJUnitPlatform()
+  }
+

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

…unnecessary print-debug

nastra

LGTM, thanks @tomtongue. It would be great if you could also move subclasses of SparkExtensionsTestBase to ExtensionsTestBase in a few follow-up PRs

tomtongue · 2024-02-02T09:53:17Z

@nastra thanks a lot for the review. I will create a new PR to move other classes in the extension after this.

Create ExtensionTestBase for migration to JUnit5

dcd7d39

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

tomtongue added 2 commits February 1, 2024 23:04

Run SpotlessApply

fa65766

Re-run SpotlessApply

908ed6f

nastra reviewed Feb 1, 2024

View reviewed changes

Remove unnecessary annot and update with assertJ

4e81fd7

nastra reviewed Feb 2, 2024

View reviewed changes

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

Statically import asserts, add parameterizing tests annot and remove …

e16c82c

…unnecessary print-debug

github-actions bot added the build label Feb 2, 2024

nastra approved these changes Feb 2, 2024

View reviewed changes

nastra merged commit d1e24f4 into apache:main Feb 2, 2024

tomtongue deleted the mig-junit4-to-5-spark-extensions branch February 2, 2024 10:31

This was referenced Feb 2, 2024

Migrate DDL sub-classes in spark-extensions to JUnit5 and AssertJ style #9624

Merged

Migrate Write sub-classes in spark-extensions to JUnit5 and AssertJ style #9670

Merged

This was referenced Feb 20, 2024

Migrate Procedure sub-classes in spark-extensions to JUnit5 and AssertJ style #9760

Merged

Migrate Read and other sub-classes in spark-extensions to JUnit5 and AssertJ style #9790

Merged

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

Spark: Create ExtensionTestBase for migration to JUnit5 (apache#9613)

12d0eda

	File outputFile = File.createTempFile("test", ".avro", temp.toFile());
	File outputFile = temp.resolve("test.avro").toFile();

	Assertions.assertThat(matcher.find()).as("verify manifest path has uuid").isTrue();
	assertThat(matcher).as("verify manifest path has uuid").matches();

Conversation

tomtongue commented Feb 1, 2024

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!

nastra Feb 1, 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 2, 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!

tomtongue Feb 2, 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!

nastra commented Feb 2, 2024

Uh oh!

Uh oh!

nastra left a comment

Choose a reason for hiding this comment

Uh oh!

tomtongue commented Feb 2, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

nastra Feb 1, 2024 •

edited

Loading

tomtongue Feb 2, 2024 •

edited

Loading

tomtongue Feb 2, 2024 •

edited

Loading