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 source directory #9342

Merged

Conversation

chinmay-bhat
Copy link
Contributor

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

This PR contains the part of the files to migrate for Spark 3.5v in the source directory. The remaining files will be migrated in a seperate PR.

Related to PR : #9341

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 13:56
@nastra nastra self-requested a review December 19, 2023 17:46
@nastra
Copy link
Contributor

nastra commented Dec 19, 2023

can you please run ./gradlew spotlessApply to fix any formatting?

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.

@chinmay-bhat it looks like the actual/expected part is the wrong way in the AssertJ assertions

@chinmay-bhat
Copy link
Contributor Author

Thanks for the review. Yes, I assumed the convention was the other way. Will update all the files.

@chinmay-bhat
Copy link
Contributor Author

Changes made:

  1. fix assertThat() to consistently use (actual, expected) convention
  2. update tempDir access to protected only when temp is used in subclasses.

Notes:

  1. partially updated TestSparkReadProjection.java to be fully updated with parameterized annotations in next PR.

@chinmay-bhat
Copy link
Contributor Author

chinmay-bhat commented Dec 20, 2023

Also, file TestInternalRowWrapper extends RecordWrapperTest.

RecordWrapperTest.java is also used by iceberg-flink and uses JUnit4.
Do we need to make a copy of RecordWrapperTest in JUnit5 to migrate TestInternalRowWrapper? The copy and original RecordWrapperTest will be available at the same time. Maybe in another PR?

I haven't added my changes to TestInternalRowWrapper to this PR for that reason.

@nastra
Copy link
Contributor

nastra commented Dec 20, 2023

We should be able to just switch the imports in RecordWrapperTest to JUnit5. That should make the subclasses run with JUnit in Spark + Flink. Converting the Flink subclasses to AssertJ doesn't have to be done within this PR though

@chinmay-bhat
Copy link
Contributor Author

I don't think that would work. See line 106.
We use Assert:assertEquals through the AssertMethod interface. If we switch the imports, we would have to change the interface & its downstream functions.

@nastra
Copy link
Contributor

nastra commented Dec 20, 2023

I don't think that would work. See line 106. We use Assert:assertEquals through the AssertMethod interface. If we switch the imports, we would have to change the interface & its downstream functions.

The imports I meant is the imports for @Test, not for the Assert class

@github-actions github-actions bot added the data label Dec 20, 2023
@chinmay-bhat
Copy link
Contributor Author

chinmay-bhat commented Dec 20, 2023

gotcha, I've made all the changes. Do let me know if anymore changes are to be made.

Once this PR is merged, I'll continue work on the remaining files (parameterize and non-parameterize)

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.

I left two minor comments, once those are addressed this can go in

@chinmay-bhat
Copy link
Contributor Author

changes made!

@chinmay-bhat chinmay-bhat changed the title Spark Migration to JUnit5 AssertJ: non-parameterized, spark/source directory Spark 3:5 Migrate tests to JUnit5 in source directory Dec 22, 2023
@chinmay-bhat
Copy link
Contributor Author

Since this PR depends on #9341, I added a patch of files changed in #9341. Commit 4f88900.

As long as we merge #9341 first, this PR should be good to go :)

@chinmay-bhat
Copy link
Contributor Author

hmm it seems like even after we merged #9341 into main, Github doesn't recognise and ignore the changes in this PR that now exist in main. Do you recommend that I rebase onto main and force push?

@nastra
Copy link
Contributor

nastra commented Dec 22, 2023

yes you need a rebase + force push

@chinmay-bhat chinmay-bhat force-pushed the spark-source-dir-junit5-nonparameterized branch from 1de56fe to a818fbb Compare December 22, 2023 09:48
@chinmay-bhat
Copy link
Contributor Author

done!

Also, I added the parameterized annotations to CatalogTestBase, since SparkCatalogTestBase was parameterized but CatalogTestBase was surprisingly not.

@nastra nastra merged commit 58d3ad3 into apache:main Dec 22, 2023
41 checks passed
lisirrx pushed a commit to lisirrx/iceberg that referenced this pull request Jan 4, 2024
@chinmay-bhat chinmay-bhat deleted the spark-source-dir-junit5-nonparameterized branch January 15, 2024 15:08
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants