Data, Spark, Flink: Migrate TestAppenderFactory and subclasses to JUnit5 by nk1506 · Pull Request #9862 · apache/iceberg

nk1506 · 2024-03-04T05:15:34Z

In this patch I planned to replace TestBase with TableTestBase for following base classes.

TestBaseTaskWriter
TestAppenderFactory
TestTaskEqualityDeltaWriter

nastra · 2024-03-04T07:37:53Z

core/src/test/java/org/apache/iceberg/TableTestBase.java

  }

-  @Before
+  public TableTestBase() {


because there are so many subclasses of TableTestBase, we decided to introduce TestBase, which is a JUnit5 version of TableTestBase. That way, we don't have to adjust all subclasses in one go and can therefore split up the required changes into smaller chunks.

As per my understanding, We won't be able to get rid of TableTestBase . TestBase has it's own implementation of ParameterizedTestExtension. Many of the subclass of TableTestBase has different flavour of ParameterizedTestExtension. I will plan to use TestBase wherever it fits. I will also add a javadoc for both class which one should be used when. Meanwhile , Could you please take a highlevel look for the Temp directory or file initialization? Also I will remove all the classes on which @tomtongue was working.

@nk1506 TestBase is what needs to be used and it uses the ParameterizedTestExtension that is compatible with JUnit5. The ParameterizedTestExtension used by TableTestBase and its subclasses only works with JUnit4. If you look for example at #9790, you'll see how parameterized JUnit4 tests need to be migrated to parameterized JUnit5 tests.

That being said, every subclass of TableTestBase that is migrated to JUnit5 needs to now extend TestBase and use org.apache.iceberg.ParameterizedTestExtension

@nastra what will you recommend for class like TestAppenderFactory which has different set of parameters.

@Parameterized.Parameters(name = "FileFormat={0}, Partitioned={1}") public static Object[] parameters() { return new Object[][] { new Object[] {"avro", false}, new Object[] {"avro", true}, new Object[] {"orc", false}, new Object[] {"orc", true}, new Object[] {"parquet", false}, new Object[] {"parquet", true} }; }

If this class will extend TestBase instead of TableTestBase there will be a conflict on parameters definitions.

this can be handled similar to how it's done for TestFlinkMetaDataTable

nastra · 2024-03-04T10:37:49Z

core/src/test/java/org/apache/iceberg/TableTestBase.java

 package org.apache.iceberg;

 import static org.apache.iceberg.types.Types.NestedField.required;
+import static org.assertj.core.api.AssertionsForClassTypes.assertThat;


wrong import

nastra · 2024-03-04T10:38:44Z

core/src/test/java/org/apache/iceberg/TestCreateTransaction.java


 import static org.apache.iceberg.PartitionSpec.unpartitioned;
 import static org.apache.iceberg.types.Types.NestedField.required;
+import static org.assertj.core.api.AssertionsForClassTypes.assertThat;


Suggested change

import static org.assertj.core.api.AssertionsForClassTypes.assertThat;

import static org.assertj.core.api.Assertions.assertThat;

nastra · 2024-03-04T10:38:54Z

core/src/test/java/org/apache/iceberg/TestCreateTransaction.java

 import static org.apache.iceberg.PartitionSpec.unpartitioned;
 import static org.apache.iceberg.types.Types.NestedField.required;
+import static org.assertj.core.api.AssertionsForClassTypes.assertThat;
+import static org.assertj.core.api.AssertionsForClassTypes.assertThatThrownBy;


Suggested change

import static org.assertj.core.api.AssertionsForClassTypes.assertThatThrownBy;

import static org.assertj.core.api.Assertions.assertThatThrownBy;

nastra · 2024-03-04T10:39:30Z

core/src/test/java/org/apache/iceberg/TestCreateTransaction.java

+import org.junit.jupiter.api.extension.ExtendWith;

-@RunWith(Parameterized.class)
+@ExtendWith(ParameterizedTestExtension.class)


this needs to use ParameterizedTestExtension from the iceberg package. Same for Parameter / Parameters

Yes, I am using the same, It might nor be showing with the import . since it's part of same package

nastra · 2024-03-06T08:54:27Z

@nk1506 can you please split this PR up into smaller and more manageble pieces so that it's easier to review the changes? I would suggest to either focus on a subset of tests in iceberg-core or on iceberg-data, but having changes across multiple Gradle modules that aren't related makes it quite difficult to review this PR

nastra · 2024-03-06T08:55:55Z

...k/v1.18/flink/src/test/java/org/apache/iceberg/flink/source/TestStreamingReaderOperator.java


    List<FlinkInputSplit> splits = generateSplits();
-    Assert.assertEquals("Should have 10 splits", 10, splits.size());
+    assertThat(10).as("Should have 10 splits").isEqualTo(splits.size());


the actual/expected is the wrong way here. This should be assertThat(splits).hasSize(10).

nk1506 · 2024-03-07T03:39:49Z

I would suggest to either focus on a subset of tests in iceberg-core or on iceberg-data

@nastra , Most of the changes are coming here because of change in WriterTestBase and impacted subclass . Should I create another PR only with WriterTestBase and it's subclass changes?

nastra · 2024-03-07T08:25:57Z

@nk1506 if changes to WriterTestBase and its subclasses create so many changes, then yes, it's better to isolate this and handle it in a separate PR as otherwise it's difficult to review a PR with diffs to 40+ files

nastra · 2024-03-08T07:36:36Z

data/src/test/java/org/apache/iceberg/io/TestBaseTaskWriter.java

      WriteResult result = writer.complete();
-      Assert.assertEquals(0, result.dataFiles().length);
-      Assert.assertEquals(0, result.deleteFiles().length);
+      assertThat(result.dataFiles().length).isEqualTo(0);


Suggested change

assertThat(result.dataFiles().length).isEqualTo(0);

assertThat(result.dataFiles()).isEmpty();

or assertThat(result.dataFiles()).hasSize(0);

please also update other places in this PR accordingly

nastra · 2024-03-08T07:37:06Z

data/src/test/java/org/apache/iceberg/io/TestBaseTaskWriter.java


    for (Path path : files) {
-      Assert.assertFalse(Files.exists(path));
+      assertThat(Files.exists(path)).isFalse();


Suggested change

assertThat(Files.exists(path)).isFalse();

assertThat(path).doesNotExist();

nastra · 2024-03-08T07:42:02Z

data/src/test/java/org/apache/iceberg/io/TestAppenderFactory.java

-      new Object[] {"parquet", true}
-    };
-  }
+  @Parameter protected FileFormat format;


this should have index = 1 and partitioned should have index = 2. The formatVersion is a parameterized field in TestBase with ìndex = 0, so it needs to be part of parameters()` (similar to how it's done in https://github.com/apache/iceberg/blob/main/data/src/test/java/org/apache/iceberg/io/TestGenericSortedPosDeleteWriter.java#L65-L70)

nastra · 2024-03-08T07:43:01Z

data/src/test/java/org/apache/iceberg/io/TestAppenderFactory.java

-    super(FORMAT_V2);
-    this.format = FileFormat.fromString(fileFormat);
-    this.partitioned = partitioned;
+  @Parameters(name = "FileFormat={0}, partitioned={1}")


Suggested change

@Parameters(name = "FileFormat={0}, partitioned={1}")

@Parameters(name = "FormatVersion={0}, FileFormat={1}, partitioned={2}")

nastra · 2024-03-08T07:43:09Z

data/src/test/java/org/apache/iceberg/io/TestAppenderFactory.java

-    this.format = FileFormat.fromString(fileFormat);
-    this.partitioned = partitioned;
+  @Parameters(name = "FileFormat={0}, partitioned={1}")
+  public static List<Object> parameters() {


Suggested change

public static List<Object> parameters() {

protected static List<Object> parameters() {

nastra · 2024-03-08T07:43:42Z

data/src/test/java/org/apache/iceberg/io/TestAppenderFactory.java

+    this.tableDir = Files.createTempDirectory(temp, "junit").toFile();
+    assertThat(tableDir.delete()).isTrue(); // created by table create
+
+    this.formatVersion = FORMAT_V2;


formatVersion is a parameterized field and needs to be set in parameters() instead of here

nastra · 2024-03-08T07:45:02Z

data/src/test/java/org/apache/iceberg/io/TestBaseTaskWriter.java

+    this.tableDir = Files.createTempDirectory(temp, "junit").toFile();
+    assertThat(tableDir.delete()).isTrue(); // created by table create

+    this.formatVersion = FORMAT_V2;


same as I mentioned in the other class. This should be part of parameters()

nastra · 2024-03-08T07:45:10Z

data/src/test/java/org/apache/iceberg/io/TestBaseTaskWriter.java

-    super(FORMAT_V2);
-    this.format = FileFormat.fromString(fileFormat);
+  @Parameters(name = "FileFormat = {0}")
+  public static List<Object> parameters() {


should be protected

nastra · 2024-03-11T10:05:24Z

data/src/test/java/org/apache/iceberg/io/TestTaskEqualityDeltaWriter.java

-    Assert.assertEquals(
-        ImmutableList.of(posRecord.copy("file_path", dataFile.path(), "pos", 0L)),
-        readRecordsAsList(posDeleteSchema, posDeleteFile.path()));
+    assertThat(FileContent.POSITION_DELETES).isEqualTo(posDeleteFile.content());


actual/expected is the wrong way here

…it5 (apache#9862)

github-actions bot added spark core data flink labels Mar 4, 2024

nk1506 mentioned this pull request Mar 4, 2024

iceberg-core: Switch tests to JUnit5 + AssertJ-style assertions #9085

Closed

nastra reviewed Mar 4, 2024

View reviewed changes

nk1506 force-pushed the junit5 branch 3 times, most recently from 310ccdc to 35a0ddf Compare March 6, 2024 08:33

nk1506 marked this pull request as ready for review March 6, 2024 08:53

nastra reviewed Mar 6, 2024

View reviewed changes

nk1506 force-pushed the junit5 branch from 35a0ddf to 8c02af3 Compare March 6, 2024 19:40

nk1506 changed the title ~~Core, Spark, Flink: Migrate tests that depend on TableTestBase to JUnit5~~ Data, Spark, Flink: Migrate tests that depend on TableTestBase to JUnit5 Mar 6, 2024

nk1506 force-pushed the junit5 branch from 8c02af3 to c6257ec Compare March 7, 2024 03:37

nk1506 force-pushed the junit5 branch from c6257ec to f9cd72b Compare March 8, 2024 05:09

nastra reviewed Mar 8, 2024

View reviewed changes

nastra changed the title ~~Data, Spark, Flink: Migrate tests that depend on TableTestBase to JUnit5~~ Data, Spark, Flink: Migrate TestAppenderFactory and subclasses to JUnit5 Mar 8, 2024

nastra reviewed Mar 8, 2024

View reviewed changes

nk1506 force-pushed the junit5 branch from f9cd72b to ff5e86b Compare March 11, 2024 09:55

nastra reviewed Mar 11, 2024

View reviewed changes

Data: Migrate tests that depend on TableTestBase to JUnit5

1ffb4ff

nk1506 force-pushed the junit5 branch from ff5e86b to 1ffb4ff Compare March 11, 2024 11:03

nastra approved these changes Mar 11, 2024

View reviewed changes

nastra merged commit 8b8907e into apache:main Mar 11, 2024

bk-mz mentioned this pull request Jun 7, 2024

Improve MetricsReporter loading with class loader fallback bk-mz/iceberg#1

Closed

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

Data, Flink, Spark: Migrate TestAppenderFactory and subclasses to JUn…

e8b2e9c

…it5 (apache#9862)

	import static org.assertj.core.api.AssertionsForClassTypes.assertThat;
	import static org.assertj.core.api.Assertions.assertThat;

	assertThat(result.dataFiles().length).isEqualTo(0);
	assertThat(result.dataFiles()).isEmpty();

	assertThat(Files.exists(path)).isFalse();
	assertThat(path).doesNotExist();

	@Parameters(name = "FileFormat={0}, partitioned={1}")
	@Parameters(name = "FormatVersion={0}, FileFormat={1}, partitioned={2}")

	public static List<Object> parameters() {
	protected static List<Object> parameters() {

Conversation

nk1506 commented Mar 4, 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!

nk1506 Mar 4, 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!

nastra commented Mar 6, 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!

nk1506 commented Mar 7, 2024

Uh oh!

nastra commented Mar 7, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

nk1506 commented Mar 4, 2024 •

edited

Loading

nk1506 Mar 4, 2024 •

edited

Loading

nastra commented Mar 6, 2024 •

edited

Loading