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

[nastra]

Feature Request / Improvement

The goal is to switch all imports to JUni5 imports and to use AssertJ-style assertions

depends on #9073

Query engine

None

[seyeh]

@nastra Could I be assigned this issue? Thank you!

[tomtongue]

@nastra Is anyone working on this now? May I help the migration to JUnit5?

[nastra]

@tomtongue feel free to work on this. It's also perfectly fine to have multiple people work on this (as long as they don't convert the same test files). Maybe it makes sense to just update this issue here and mention which tests you'll be converting, so that others don't work on the same things.

[tomtongue]

@nastra Thanks, sure it can avoid the duplicate efforts. Will list the test files here.

[tomtongue]

I'm working on the following currently (will update)

Current Progress

Read:

• MetadataTableScanTestBase (extends TableTestBase)
  • TestMetadataTableScans
  • TestMetadataTableScansWithPartitionEvolution
• TestEntriesMetadataTable
• TestBatchScans
• TestIncrementalDataTableScan
• TestFindFiles

[nk1506]

@tomtongue , I was working with TestOverwrite and to move this to jUnit5 I had to change TableTestBase, which ended up changing all the dependent class. With this PR I will plan to change only
@ExtendWith(ParameterizedTestExtension.class) and related issues. It seems it's not allowing me to keep mix of jUnit4 and jUnit5. For the above classes I wont make any changes related to assertions.

[tomtongue]

@nk1506 thanks for sharing the situation. In your commit, there are some classes that I already migrated to JUnit 5, the classes are listed above. And the TableTestBase shouldn't be migrated to JUnit 5, instead of the TableTestBase, I believe we should use TestBase which was created in #9217 for this migration.

And for Spark 3.3 and Spark 3.4, should we migrate to JUnit 5? @nastra Could you have a look at @nk1506's commit? (If you already had conversation about it, please let me know)

[nastra]

Yes we should be using TestBase as the new base class for JUnit5 tests that previously extended TableTestBase. That allows us to do the migration in small chunks rather than having to change all subclasses of TableTestBase in a single PR.

And for Spark 3.3 and Spark 3.4, should we migrate to JUnit 5?

We do this for Flink, but for Spark the situation is slightly different and the amount of tests is quite a lot. It would be nice to migrate at least Spark 3.4 so that backporting features and tests from Spark 3.5 to Spark 3.4 is easier. It took us already quite long to migrate Spark 3.5 tests. @aokolnychyi @RussellSpitzer any thoughts on also migrating Spark 3.4 tests to JUnit5?

[nk1506]

@tomtongue , I have reverted changed for the above files. As recommended by @nastra I will work with TestBase for the related files.

[tomtongue]

@nk1506 Sure thanks so much. It would be great if you list the classes you're working on in this issue, for avoiding the conflicts.

[tomtongue]

Additionally created a PR for the migration of the files related to "snapshot". Here's the progress:

Snapshots:

• TestSnapshot
• TestSnapshotJson
  • LocalTableOperations
  • TestTableMetadata (partially changed, the change is related to LocalTableOperations)
• TestSnapshotLoading
• TestSnapshotSelection
• TestSnapshotSummary

The following classes are for Iceberg procedures:

• TestSnapshotManager
• TestSnapshotRefParser

[tomtongue]

Created a PR for the migration of the files related to "metadata". Here's the progress:

• TableMetadataParserCodecTest
• TableMetadataParserTest
• TestMetadataUpdateParser
• TestMetadataTableFilters
• TestTableMetadata
• TestTableMetadataSerialization

[seyeh]

@tomtongue After you make changes, do you run ./gradlew build? If so, how long does this process take on your computer.

Also since you've been working this ticket, do you want to just take ownership of this ticket or would you like help with the rest of the tests?

[tomtongue]

@seyeh

After you make changes, do you run ./gradlew build? If so, how long does this process take on your computer.

basically run ./gradlew test for specific classes and packages. I don't remember what it takes exactly, but It takes less than 1 hour.

If it's no problem with you, I can take the ownership because I'm migrating most of classes.

[seyeh]

@tomtongue Makes sense, I figured you'd probably build that test/classes specific package instead of all of them since ./gradlew build took forever on my computer.

Sure you can take ownership.

I've unassigned myself

[tomtongue]

@nastra If possible, could you assign this issue with me? I'll have the owenership for this core migration.

[tomtongue]

Created a PR for the migration of the files related to "manifest". Currently completed and working on the following classes:

• TestManifestCaching.java
• TestManifestCleanup.java
• TestManifestEncryption.java
• TestManifestListVersions.java
• TestManifestReader.java (already uses JUnit5)
• TestManifestReaderStats.java
• TestManifestWriter.java
• TestManifestWriterVersions.java

Additional classes:

• TestFormatVersions.java
• TestLocationProvider.java

[nastra]

I think the only task left for this is to convert subclasses of TableTestBase to JUnit5 and then remove TableTestBase

[tomtongue]

Yes, the subclasses of TableTestBase are related to several packages like Flink, Spark etc. I will create PRs.