Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Spark 3.5: Migrate tests to JUnit5 in data directory #9341

Merged

Conversation

chinmay-bhat
Copy link
Contributor

@chinmay-bhat chinmay-bhat commented Dec 19, 2023

Since migrating Spark 3.5v to Junit5 is a large task, I'm splitting my work into multiple easy-to-review PR's, split by directory.

This PR contains the Junit5 migration for Spark 3.5v for the spark/data directory, and any files that inherit from this directory.

Issue: #9086

TODO:

  • self-review

@github-actions github-actions bot added the spark label Dec 19, 2023
@chinmay-bhat chinmay-bhat marked this pull request as ready for review December 19, 2023 12:47
@chinmay-bhat
Copy link
Contributor Author

Changes:

  1. update assertJ imports
  2. remove cast - int, byte[]
  3. update tempDir access - solves build errors
  4. I had erroneously included a parameterized file in this PR. Reverted the files changes. Will add it in a seperate PR.

@chinmay-bhat
Copy link
Contributor Author

CI failing.
Error:

> Task :iceberg-spark:iceberg-spark-3.5_2.12:jar
/home/runner/work/iceberg/iceberg/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/source/TestDataFrameWrites.java:150: error: cannot find symbol
    File tablePropertyDataLocation = temp.newFolder("test-table-property-data-dir");

Fix:
TestDataFrameWrites extends AvroDataTest. Since the temp dir has been updated to JUnit5 In AvroDataTest, TestDataFrameWrites temp is throwing errors.
To fix, I'll update only the temp calls in TestDataFrameWrites to JUnit5.

@chinmay-bhat
Copy link
Contributor Author

When I run the build check (./gradlew -DallVersions build -x test -x javadoc -x integrationTest) locally, build succeeds.

BUILD SUCCESSFUL in 5m 29s
496 actionable tasks: 373 executed, 123 up-to-date
10:12:39 PM: Execution finished 'build -DallVersions -x test -x javadoc -x integrationTest'.

@chinmay-bhat
Copy link
Contributor Author

running CI check locally and resolving errors. I'll push once I've cleared the errors / need more help!

@chinmay-bhat
Copy link
Contributor Author

Initially, I assumed I can seperate out parameterize and non-parameterize files. But the CI errors included parameterized files that inherit from spark/data/AvroDataTest.

So I'm converting these files from JUnit4 to JUnit5

  • the files (parameterized and non-parameterized) in spark/data and
  • the param files inspark/source that inherit from AvroDataTest.

All CI tests pass locally with this command.
./gradlew -DsparkVersions=3.5 -DscalaVersion=2.13 -DhiveVersions= -DflinkVersions= :iceberg-spark:iceberg-spark-3.5_2.13:check -Pquick=true -x javadoc

@chinmay-bhat chinmay-bhat changed the title Spark Migration to JUnit5 AssertJ - non-parameterized, spark/data directory Spark Migration to JUnit5 AssertJ - spark/data directory Dec 21, 2023
Copy link
Contributor

@nastra nastra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM once CI passes

@nastra nastra changed the title Spark Migration to JUnit5 AssertJ - spark/data directory Spark 3.5: Migrate tests to JUnit5 in data directory Dec 21, 2023
chinmay-bhat added a commit to chinmay-bhat/iceberg that referenced this pull request Dec 22, 2023
@nastra nastra merged commit 3d53060 into apache:main Dec 22, 2023
31 checks passed
lisirrx pushed a commit to lisirrx/iceberg that referenced this pull request Jan 4, 2024
@chinmay-bhat chinmay-bhat deleted the spark-data-dir-junit5-nonparameterized branch January 15, 2024 15:09
geruh pushed a commit to geruh/iceberg that referenced this pull request Jan 26, 2024
devangjhabakh pushed a commit to cdouglas/iceberg that referenced this pull request Apr 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants