-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Spark 3:5 Migrate tests to JUnit5 in source directory #9342
Conversation
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/source/TestAvroScan.java
Outdated
Show resolved
Hide resolved
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/source/TestBaseReader.java
Outdated
Show resolved
Hide resolved
can you please run |
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/source/TestDataSourceOptions.java
Outdated
Show resolved
Hide resolved
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/source/TestDataSourceOptions.java
Outdated
Show resolved
Hide resolved
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/source/TestIcebergSpark.java
Outdated
Show resolved
Hide resolved
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/source/TestPathIdentifier.java
Outdated
Show resolved
Hide resolved
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/source/TestReadProjection.java
Outdated
Show resolved
Hide resolved
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/source/TestSparkAggregates.java
Outdated
Show resolved
Hide resolved
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/source/TestSparkReadProjection.java
Outdated
Show resolved
Hide resolved
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/source/TestSparkStagedScan.java
Outdated
Show resolved
Hide resolved
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/source/TestStreamingOffset.java
Outdated
Show resolved
Hide resolved
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/source/TestStreamingOffset.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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
Thanks for the review. Yes, I assumed the convention was the other way. Will update all the files. |
Changes made:
Notes:
|
Also, file TestInternalRowWrapper extends RecordWrapperTest.
I haven't added my changes to TestInternalRowWrapper to this PR for that reason. |
We should be able to just switch the imports in |
I don't think that would work. See line 106. |
The imports I meant is the imports for |
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) |
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/source/TestDataSourceOptions.java
Outdated
Show resolved
Hide resolved
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/source/TestIcebergSpark.java
Outdated
Show resolved
Hide resolved
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/source/TestIcebergSourceTablesBase.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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
changes made! |
CatalogTestBase is supposed to replace SparkCatalogTestBase, which is parameterized. But, parameterize tests were never added to CatalogTestBase. This commit introduces it.
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? |
yes you need a rebase + force push |
1de56fe
to
a818fbb
Compare
done! Also, I added the parameterized annotations to |
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: