Create SparkCatalogTestBase class for the migration to JUnit5 #9129

tomtongue · 2023-11-22T08:47:53Z

Base class creation for the migration to JUnit5 in regards to #9076 .

tomtongue · 2023-11-22T08:48:46Z

Working on updating other tests for JUnit5 and AssertJ. After updating the tests the class is merged into the original class (if I should separate other test updates to other PRs or there are another way, please let me know).

spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/SparkCatalogTestBaseForJU5.java

nastra · 2023-11-22T11:29:53Z

@tomtongue I think we first need to have a JUnit5 equivalent of SparkTestBase and SparkTestBaseWithCatalog before we can migrate SparkCatalogTestBase. This could be done as part of this PR as the required changes should be fairly small.

tomtongue · 2023-11-22T12:40:03Z

Thank you! I'm working on changes. Let me do this.

tomtongue · 2023-11-22T18:30:02Z

Add the following classes that basically cover JUnit5 and AssertJ styles. Each class has a lot of inheritants so that new classes are created for now. And, can I update existing Spark tests to use the new base classes? @nastra

nastra · 2023-11-23T07:53:34Z

spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestHelperBase.java

+import org.assertj.core.api.Assertions;
+import org.junit.Assert;
+
+public class TestHelperBase {


I don't think we need this class, we can just use SparkTestHelperBase because there's nothing JUnit4-specific in there

Yes, that's correct, thank you! I remove TestHelperBase and add changes to SparkTestHelperBase with JUnit5 updates.

nastra · 2023-11-23T07:54:31Z

spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBase.java

+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
+
+public abstract class TestBase extends TestHelperBase {


this should extend SparkTestHelperBase

nastra · 2023-11-23T07:55:56Z

spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java

+    }
+  }
+
+  @TempDir public File temp;


for JUnit5 this can be protected

nastra · 2023-11-23T07:56:21Z

spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/CatalogTestBase.java

+            SparkCatalogConfig.SPARK.properties()));
+  }
+
+  @TempDir public File temp;


can be protected

actually I don't think we need this here, because we already have it in the super class

nastra

this looks almost ready, just a few small things. Also could you please convert TestSparkFileRewriter to use this new base class?

nastra · 2023-11-23T08:03:51Z

Add the following classes that basically cover JUnit5 and AssertJ styles. Each class has a lot of inheritants so that new classes are created for now. And, can I update existing Spark tests to use the new base classes

Let's first get the new base classes in and convert a single test to the new base class. Then we can handle other classes in a separate PR

…riter parent to new TestBase

tomtongue · 2023-11-23T16:10:37Z

Thanks for the review, @nastra! I add the following changes based on the reviews:

Change SparkTestHelperBase with the AssertJ style (removed TestHelperBase)
Switch TestSparkFileRewriter's super class to TestBase (from SparkTestBase)
Change TestSparkFileRewriter with the AssertJ style
Remove temp from CatalogTestBase and change temp scope in its super class to protected

nastra · 2023-11-24T07:12:14Z

spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/SparkTestHelperBase.java

@@ -55,12 +55,15 @@ private Object[] toJava(Row row) {

  protected void assertEquals(
      String context, List<Object[]> expectedRows, List<Object[]> actualRows) {
-    Assert.assertEquals(
-        context + ": number of results should match", expectedRows.size(), actualRows.size());
+    Assertions.assertThat(actualRows.size())


it's better to have

Assertions.assertThat(actualRows) .as(context + ": number of results should match") .hasSameSizeAs(expectedRows);

because this will show the content when the assertion ever fails

Confirmed that on my end, thanks. Update the tests.

nastra · 2023-11-24T07:12:44Z

spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/SparkTestHelperBase.java

    for (int row = 0; row < expectedRows.size(); row += 1) {
      Object[] expected = expectedRows.get(row);
      Object[] actual = actualRows.get(row);
-      Assert.assertEquals("Number of columns should match", expected.length, actual.length);
+      Assertions.assertThat(actual.length)


for the same reason that I mentioned above, it's better to have Assertions.assertThat(actual).as("Number of columns should match").hasSameSizeAs(expected);

nastra · 2023-11-24T07:12:52Z

spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/SparkTestHelperBase.java

@@ -69,19 +72,25 @@ protected void assertEquals(
  }

  protected void assertEquals(String context, Object[] expectedRow, Object[] actualRow) {
-    Assert.assertEquals("Number of columns should match", expectedRow.length, actualRow.length);
+    Assertions.assertThat(actualRow.length)


same as above

nastra · 2023-11-24T07:13:38Z

spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/SparkTestHelperBase.java

-          Assert.assertArrayEquals(newContext, (byte[]) expectedValue, (byte[]) actualValue);
+          Assertions.assertThat((byte[]) actualValue)
+              .as(newContext)
+              .isEqualTo((byte[]) expectedValue);


this cast shouldn't be required

nastra · 2023-11-24T07:15:09Z

spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/actions/TestSparkFileRewriter.java

@@ -129,9 +128,9 @@ private void checkDataFilesDeleteThreshold(SizeBasedDataRewriter rewriter) {
    rewriter.init(options);

    Iterable<List<FileScanTask>> groups = rewriter.planFileGroups(tasks);
-    Assert.assertEquals("Must have 1 group", 1, Iterables.size(groups));
+    Assertions.assertThat(Iterables.size(groups)).as("Must have 1 group").isEqualTo(1);


Suggested change

Assertions.assertThat(Iterables.size(groups)).as("Must have 1 group").isEqualTo(1);

Assertions.assertThat(groups).hasSize(1);

nastra · 2023-11-24T07:15:22Z

spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/actions/TestSparkFileRewriter.java

@@ -110,9 +109,9 @@ private void checkDataFileSizeFiltering(SizeBasedDataRewriter rewriter) {
    rewriter.init(options);

    Iterable<List<FileScanTask>> groups = rewriter.planFileGroups(tasks);
-    Assert.assertEquals("Must have 1 group", 1, Iterables.size(groups));
+    Assertions.assertThat(Iterables.size(groups)).as("Must have 1 group").isEqualTo(1);


Suggested change

Assertions.assertThat(Iterables.size(groups)).as("Must have 1 group").isEqualTo(1);

Assertions.assertThat(groups).hasSize(1);

nastra · 2023-11-24T07:15:55Z

spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/actions/TestSparkFileRewriter.java

    List<FileScanTask> group = Iterables.getOnlyElement(groups);
-    Assert.assertEquals("Must rewrite 2 files", 2, group.size());
+    Assertions.assertThat(group.size()).as("Must rewrite 2 files").isEqualTo(2);


same as above

nastra · 2023-11-24T07:16:33Z

spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/actions/TestSparkFileRewriter.java

    List<FileScanTask> group = Iterables.getOnlyElement(groups);
-    Assert.assertEquals("Must rewrite big file", 1, group.size());
+    Assertions.assertThat(group.size()).as("Must rewrite big file").isEqualTo(1);


please adjust all of these to use Assertions.assertThat(groups).as(..).hasSize(1);

sure, thanks for the review. I'm working on it.

tomtongue · 2023-11-24T09:49:47Z

Thanks for the review again. Change the tests that are commented here. I reviewed all my changes on my end again, but If there's any part that should be changed, please let me know @nastra

Create SparkCatalogTestBase class for the migration to JUnit5

d6f2a2d

github-actions bot added the spark label Nov 22, 2023

Run SpotlessApply

876738b

nastra reviewed Nov 22, 2023

View reviewed changes

spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/SparkCatalogTestBaseForJU5.java Outdated Show resolved Hide resolved

Add Spark test base classes for JUnit5

5f615f5

nastra reviewed Nov 23, 2023

View reviewed changes

Add JUnit5 changes to SparkTestHelperBase and switch TestSparkFileRew…

f4bcde1

…riter parent to new TestBase

nastra reviewed Nov 24, 2023

View reviewed changes

Change the list size check for debuggability

a2e67c5

nastra approved these changes Nov 24, 2023

View reviewed changes

This was referenced Nov 24, 2023

Create JUnit5-version of SparkTestBaseWithCatalog #9075

Closed

Create JUnit5-version of SparkTestBase #9074

Closed

nastra merged commit b20d30c into apache:main Nov 24, 2023

chinmay-bhat mentioned this pull request Dec 20, 2023

Spark 3:5 Migrate tests to JUnit5 in source directory #9342

Merged

1 task

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

Spark: Create base classes for migration to JUnit5 (apache#9129)

f129b92

tomtongue deleted the switch-junit5-for-sparkcatalogtestbase branch July 30, 2024 14:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create SparkCatalogTestBase class for the migration to JUnit5 #9129

Create SparkCatalogTestBase class for the migration to JUnit5 #9129

tomtongue commented Nov 22, 2023 •

edited by nastra

Loading

tomtongue commented Nov 22, 2023 •

edited

Loading

nastra commented Nov 22, 2023 •

edited

Loading

tomtongue commented Nov 22, 2023

tomtongue commented Nov 22, 2023 •

edited

Loading

nastra Nov 23, 2023

tomtongue Nov 23, 2023 •

edited

Loading

nastra Nov 23, 2023

nastra Nov 23, 2023

nastra Nov 23, 2023

nastra Nov 23, 2023

nastra left a comment

nastra commented Nov 23, 2023

tomtongue commented Nov 23, 2023

nastra Nov 24, 2023

tomtongue Nov 24, 2023

nastra Nov 24, 2023

nastra Nov 24, 2023

nastra Nov 24, 2023

nastra Nov 24, 2023

nastra Nov 24, 2023

nastra Nov 24, 2023

nastra Nov 24, 2023

tomtongue Nov 24, 2023

tomtongue commented Nov 24, 2023 •

edited

Loading

	Assertions.assertThat(Iterables.size(groups)).as("Must have 1 group").isEqualTo(1);
	Assertions.assertThat(groups).hasSize(1);

Create SparkCatalogTestBase class for the migration to JUnit5 #9129

Create SparkCatalogTestBase class for the migration to JUnit5 #9129

Conversation

tomtongue commented Nov 22, 2023 • edited by nastra Loading

tomtongue commented Nov 22, 2023 • edited Loading

nastra commented Nov 22, 2023 • edited Loading

tomtongue commented Nov 22, 2023

tomtongue commented Nov 22, 2023 • edited Loading

Choose a reason for hiding this comment

tomtongue Nov 23, 2023 • edited Loading

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

nastra commented Nov 23, 2023

tomtongue commented Nov 23, 2023

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

tomtongue commented Nov 24, 2023 • edited Loading

tomtongue commented Nov 22, 2023 •

edited by nastra

Loading

tomtongue commented Nov 22, 2023 •

edited

Loading

nastra commented Nov 22, 2023 •

edited

Loading

tomtongue commented Nov 22, 2023 •

edited

Loading

tomtongue Nov 23, 2023 •

edited

Loading

tomtongue commented Nov 24, 2023 •

edited

Loading