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

tomtongue · 2024-02-07T06:45:04Z

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

Current progress

…tyle

nastra · 2024-02-07T09:37:33Z

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

  }

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


CatalogTestBase already has temp, so this shouldn't be required

Thanks for the quick review. Will remove this in the next commit.

nastra · 2024-02-13T07:01:22Z

...sions/src/test/java/org/apache/iceberg/spark/extensions/SparkRowLevelOperationsTestBase.java

-    this.planningMode = planningMode;
-  }
+  @Parameter(index = 3)
+  protected String fileFormat;


Suggested change

protected String fileFormat;

protected FileFormat fileFormat;

Agree. Let me do this.

nastra · 2024-02-13T07:06:36Z

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

+  @TestTemplate
  public void testDeleteFileThenMetadataDelete() throws Exception {
-    Assume.assumeFalse("Avro does not support metadata delete", fileFormat.equals("avro"));
+    assumeThat(fileFormat.equalsIgnoreCase("avro"))


once fileFormat is of type FileFormat, this check can be changed to assumeThat(fileFormat).as(..).isNotEqualTo(FileFormat.AVRO)

nastra · 2024-02-13T07:07:35Z

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

    List<DataFile> dataFilesAfter = TestHelpers.dataFiles(table, branch);
-    Assert.assertTrue(
-        "Data file should have been removed", dataFilesBefore.size() > dataFilesAfter.size());
+    assertThat(dataFilesBefore.size() > dataFilesAfter.size())


you could use .hasSizeGreaterThan() here

nastra · 2024-02-13T07:08:24Z

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

+  @TestTemplate
  public void testDeleteFromEmptyTable() {
-    Assume.assumeFalse("Custom branch does not exist for empty table", "test".equals(branch));
+    assumeThat("test".equals(branch)).as("Custom branch does not exist for empty table").isFalse();


Suggested change

assumeThat("test".equals(branch)).as("Custom branch does not exist for empty table").isFalse();

assumeThat(branch).as("Custom branch does not exist for empty table").isNotEqualTo("test");

nastra · 2024-02-13T07:08:36Z

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

+  @TestTemplate
  public void testDeleteFromNonExistingCustomBranch() {
-    Assume.assumeTrue("Test only applicable to custom branch", "test".equals(branch));
+    assumeThat("test".equals(branch)).as("Test only applicable to custom branch").isTrue();


same as above

nastra · 2024-02-13T07:09:15Z

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

+  @TestTemplate
  public void testDeleteWithMultipleRowGroupsParquet() throws NoSuchTableException {
-    Assume.assumeTrue(fileFormat.equalsIgnoreCase("parquet"));
+    assumeThat(fileFormat.equalsIgnoreCase("parquet")).isTrue();


Suggested change

assumeThat(fileFormat.equalsIgnoreCase("parquet")).isTrue();

assumeThat(fileFormat).isEqualTo(FileFormat.PARQUET);

nastra · 2024-02-13T07:09:30Z

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

+  @TestTemplate
  public void testDeleteOnNonIcebergTableNotSupported() {
-    Assume.assumeTrue(catalogName.equalsIgnoreCase("spark_catalog"));
+    assumeThat(catalogName.equalsIgnoreCase("spark_catalog")).isTrue();


same as above

nastra · 2024-02-13T07:09:43Z

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

  public synchronized void testDeleteWithSerializableIsolation() throws InterruptedException {
    // cannot run tests with concurrency for Hadoop tables without atomic renames
-    Assume.assumeFalse(catalogName.equalsIgnoreCase("testhadoop"));
+    assumeThat(catalogName.equalsIgnoreCase("testhadoop")).isFalse();


same as above

please also update all the other places in this PR

nastra · 2024-02-13T07:10:36Z

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

+  @TestTemplate
  public void testDeleteToWapBranch() throws NoSuchTableException {
-    Assume.assumeTrue("WAP branch only works for table identifier without branch", branch == null);
+    assumeThat(branch == null)


Suggested change

assumeThat(branch == null)

assumeThat(branch).as(..).isNull();

nastra · 2024-02-13T07:11:03Z

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

+  @TestTemplate
  public void testDeleteToWapBranchWithTableBranchIdentifier() throws NoSuchTableException {
-    Assume.assumeTrue("Test must have branch name part in table identifier", branch != null);
+    assumeThat(branch != null).as("Test must have branch name part in table identifier").isTrue();


please use .isNotNull()

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

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

+  @TestTemplate
  public void testUpdateEmptyTable() {
-    Assume.assumeFalse("Custom branch does not exist for empty table", "test".equals(branch));
+    assumeThat("test".equals(branch)).as("Custom branch does not exist for empty table").isFalse();


same as previously mentioned. We want to avoid using isFalse / isTrue as much as possible, because it won't print enough context when the assertion/assumption ever fails

Sure, thank you!

…umptions

tomtongue · 2024-02-13T13:05:57Z

Working on the fix of errors.
When I tested them on local, SnapshotIsolation tests in TestMerge/TestUpdate were sometimes successful, but sometimes failure.

nastra · 2024-02-15T09:29:31Z

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

              for (int numOperations = 0; numOperations < 20; numOperations++) {
                while (shouldAppend.get() && barrier.get() < numOperations * 2) {
-                  sleep(10);
+                  sleep(200);


is this being flaky and requires a bump in wait time? We might want to update this to use Awaitility in a follow-up PR

thanks! yes, the test seems to be flaky. After the updates, the test sometimes failed and sometimes succeeded. I was investigating the issue, but let me update the part with Awaitlity.

nastra · 2024-02-15T09:31:26Z

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


                  appendFiles.commit();
-                  sleep(10);
+                  sleep(1000);


we might want to update this to Awaitility in a follow-up PR

nastra · 2024-02-15T09:32:36Z

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


                  appendFiles.commit();
-                  sleep(10);
+                  sleep(1000);


same as previously mentioned about using Awaitility here

nastra

@tomtongue the changes LGTM, I just left a few questions. Did you plan to do anything else because the title still contains WIP?

tomtongue · 2024-02-15T12:58:28Z

Thanks for the review @nastra . No, there's one thing that just I update snapshotIsolation tests by adding Awaitility. I'm working on implementing the Awaitility to this PR. (If it should be included in another PR, please let me know).

nastra · 2024-02-19T07:38:43Z

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


                  appendFiles.commit();
-                  sleep(10);
+                  Awaitility.await().pollInterval(Duration.ofMillis(10)).until(() -> true);


I don't think this makes a lot of sense to replace this sleep with Awaitility. I wonder whether this sleep here is even necessary

Let me revert this to sleep. Currently the snapshotIsolation tests for TestDelete, TestMerge and TestUpdate are still flaky. From my current investigation, the test failure seems to be related to the appendFiles with other write operations. And the sleep seems to work for waiting for the appendFiles.commit().

Got the reason of the test failure. The reason seems to be the conflicts of the appended file names. When I tested using a randomized name for each appendFile, all the tests succeeded (the previous version sets the randomized names). And in this case, as you said, sleep is no longer needed. Let me fix the file name configuration.

…ility

nastra · 2024-02-19T13:30:37Z

...sions/src/test/java/org/apache/iceberg/spark/extensions/SparkRowLevelOperationsTestBase.java

  protected DataFile writeDataFile(Table table, List<GenericRecord> records) {
    try {
-      OutputFile file = Files.localOutput(temp.newFile());
+


nit: unnecessary newline

thanks. Will remove it.

nastra · 2024-02-19T14:21:38Z

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

-                }
+                int currentNumOperations = numOperations;
+                Awaitility.await()
+                    .pollInterval(Duration.ofMillis(10))


I think all of these should have .atMost(5, TimeUnit.SECONDS) because we don't want to wait indefinitely for the condition to happen

Thanks for the suggestion. ~~I will add atMost to all of the tests~~. Updated.

tomtongue · 2024-02-20T01:29:52Z

@nastra Reflected on your comments. If there's any part that I should change, please let me know.

tomtongue · 2024-02-20T08:25:24Z

Thank you!

nastra · 2024-02-20T08:28:29Z

thanks for getting this done @tomtongue. Looking forward to other PRs from you for the remaining files in spark-extensions

tomtongue · 2024-02-20T08:29:42Z

Of course, I'm working on migrating procedures and other tests. Will create 2 or 3 PRs later👍

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

Migrate Write sub-classes in spark-extensions to JUnit5 and AssertJ s…

e069cee

…tyle

tomtongue force-pushed the mig-junit5-write-extensions branch from 8b6abaf to e069cee Compare February 7, 2024 06:47

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

nastra reviewed Feb 7, 2024

View reviewed changes

tomtongue added 2 commits February 12, 2024 00:33

Update TestDelete related sub-classes with JUnit 5 and AssertJ

4f8c57d

Update TestUpdate related sub-classes with JUnit 5 and AssertJ

d7e2e82

nastra reviewed Feb 13, 2024

View reviewed changes

Update TestMerge related sub-classes and remove isTrue or isFalse ass…

d3460dc

…umptions

tomtongue force-pushed the mig-junit5-write-extensions branch from cd62c06 to d3460dc Compare February 13, 2024 12:01

tomtongue added 3 commits February 14, 2024 22:38

Change the sleep duration to avoid data confliction

a468369

Fix list validation

f05d850

Fix confliction error

184473f

nastra reviewed Feb 15, 2024

View reviewed changes

nastra approved these changes Feb 15, 2024

View reviewed changes

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

Update Thread.sleep to Awaitility

abb8f62

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

nastra reviewed Feb 19, 2024

View reviewed changes

Make the temp file name randomized and update other sleeps with Await…

d81094c

…ility

nastra reviewed Feb 19, 2024

View reviewed changes

Nit: remove one unnecessary line

74c6d92

nastra reviewed Feb 19, 2024

View reviewed changes

Add timeout to avoid long time wait

099fd31

nastra approved these changes Feb 20, 2024

View reviewed changes

nastra merged commit c4d827e into apache:main Feb 20, 2024

tomtongue deleted the mig-junit5-write-extensions branch February 20, 2024 11:48

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

Spark: Migrate tests to JUnit5 (apache#9670)

a6cdde7

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

Spark: Migrate tests to JUnit5 (apache#9670)

da37bb6

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

Spark: Migrate tests to JUnit5 (apache#9670)

45fb717

	protected String fileFormat;
	protected FileFormat fileFormat;

	assumeThat("test".equals(branch)).as("Custom branch does not exist for empty table").isFalse();
	assumeThat(branch).as("Custom branch does not exist for empty table").isNotEqualTo("test");

	assumeThat(fileFormat.equalsIgnoreCase("parquet")).isTrue();
	assumeThat(fileFormat).isEqualTo(FileFormat.PARQUET);

	assumeThat(branch == null)
	assumeThat(branch).as(..).isNull();

Conversation

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

Uh oh!

Current progress

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!

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 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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 left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomtongue Feb 19, 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 19, 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 19, 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 commented Feb 20, 2024

Uh oh!

tomtongue commented Feb 20, 2024

Uh oh!

nastra commented Feb 20, 2024

Uh oh!

tomtongue commented Feb 7, 2024 •

edited

Loading

tomtongue commented Feb 13, 2024 •

edited

Loading

tomtongue commented Feb 15, 2024 •

edited

Loading

tomtongue Feb 19, 2024 •

edited

Loading

tomtongue Feb 19, 2024 •

edited

Loading

tomtongue Feb 19, 2024 •

edited

Loading