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

[nastra]

Feature Request / Improvement

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

depends on #9074 #9075 #9076

Query engine

Spark

[chinmay-bhat]

Hi @nastra, can I be assigned to this? Looks like the dependent issues have been resolved/merged.

Also I've read through the discussion on #7160. Is there any other dependency / resource I need to read before beginning?

[nastra]

For parameterized testing we would need to have https://github.com/apache/iceberg/issues/9210 implemented first.

[chinmay-bhat]

Ok, so can I pick this up once the parameterized tests PR is merged?

Also, iceberg-spark has folders for each version (v3.2, 3.3, 3.4, 3.5). Do you recommend creating separate PRs for each?

[nastra]

We should be mostly focusing on migrating Spark 3.5, which is already a big task

[chinmay-bhat]

As it's a big task, I'll get started migrating tests in iceberg-spark v3.5 that are not parameterized, and later open a new PR for the parameterized ones :)

[chinmay-bhat]

can we mark this is as done?

[nastra]

@chinmay-bhat I don't think this is complete yet. There are still plenty of places that use org.junit.Test and @RunWith(Parameterized.class) within the iceberg-spark module

[chinmay-bhat]

I'll raise PRs for the remaining migrations in the coming days.

[tomtongue]

@nastra It seems that the SparkExtensionTestBase class and some of the related extension modules in this path are still using JUnit 4 and non-AssertJ. I'm working on the modules migration, may I create a PR for this?

[nastra]

@tomtongue yes that would be great if you could do that

[tomtongue]

@nastra Thanks so much. I'm working on it now, will submit a CR ~today~ (sorry, tomorrow) .

[tomtongue]

@nastra I believe all of the spark 3.5 classes were migrated to JUnit 5. Is there any remaining actions to close this issue? Do we migrate other versions of Spark as well?

[nastra]

@tomtongue It appears that there are still some references left to org.junit.[Test|Assert|Assume] in Spark 3.5 that we should fix as well.

[tomtongue]

@nastra Thanks for checking. I found the following classes still have the old version. Let me fix them as well

• https://github.com/apache/iceberg/blob/8a16a417492eb6ccb1895b4b6db3536ff2a8b67d/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/source/TestSparkMetadataColumns.java#L62
• https://github.com/apache/iceberg/blob/8a16a417492eb6ccb1895b4b6db3536ff2a8b67d/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/functions/TestSparkFunctions.java#L26

There are also two classes that have org.junit.Assert but they were already migrated to JUnit5 as the different class names like TestBase and TestBaseWithCatalog. I believe they are not necessary to be updated.

• https://github.com/apache/iceberg/blob/8a16a417492eb6ccb1895b4b6db3536ff2a8b67d/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/SparkTestBase.java#L57
• https://github.com/apache/iceberg/blob/8a16a417492eb6ccb1895b4b6db3536ff2a8b67d/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/SparkTestBaseWithCatalog.java#L36

[nastra]

We can also remove SparkTestBase and all of its subclasses, since those have all been migrated to JUnit5 now

[tomtongue]

Sure, will remove them once I confirm the removal is no problem.

[tomtongue]

@nastra I believe all the classes have been migrated to JUnit5. Is there remaining actions for the migration?

[nastra]

Closing this as all of the Spark 3.5 tests have been migrated to JUnit5 + AssertJ. Thanks everyone for helping out and contributing

[tomtongue]

Thanks for your reviews too! Appreciate it.