Migrate Read and other sub-classes in spark-extensions to JUnit5 and AssertJ style #9790
Conversation
tomtongue
changed the title
tomtongue
force-pushed
the
mig-junit5-other-extensions
branch
from
34ba439 to
0c61738
Compare
|
@nastra This is the last PR for the migration of spark-extensions except for SmokeTest in spark-runtime. The current PR doesn't have the SmokeTest migration, but if needed, please let me know. |
| @Before | ||
| public void before() { | ||
| @BeforeEach | ||
| public void setupResource() { |
There was a problem hiding this comment.
rather than renaming, it's better to just add super.before() as the first call in this method
| @Before | ||
| public void before() { | ||
| @BeforeEach | ||
| public void useCatalog() { |
There was a problem hiding this comment.
can we just call super.before() here as the first call instead of renaming?
| assertThat(actualPartitionsWithProjection) | ||
| .as("Metadata table should return two partitions record") | ||
| .hasSize(2); | ||
| assertThat(actualPartitionsWithProjection) |
There was a problem hiding this comment.
| assertThat(actualPartitionsWithProjection) |
this can be just comined with the statement above
| Assert.assertEquals(currentSnapshotId, testTag.get(0).getAs("snapshot_id")); | ||
| Assert.assertEquals(Long.valueOf(50), testTag.get(0).getAs("max_reference_age_in_ms")); | ||
| assertThat(testTag).hasSize(1); | ||
| assertThat(testTag.get(0).schema().fieldNames()) |
There was a problem hiding this comment.
you could combine this with the previous line:
assertThat(testTag).hasSize(1).first().schema().fieldNames()).containsExactly(...)
please also update all the other checks in this file around branch/tag verification
There was a problem hiding this comment.
It seems that calling schema() after the first() as above is not allowed here (I believe those tests can be combined with satisfies). So in the new commit, the tests of hasSize and record check are combined.
|
@tomtongue can you also please migrate and include |
|
Thanks for the review. Sure, will include the SmokeTest with updates based on your reviews. |
tomtongue
force-pushed
the
mig-junit5-other-extensions
branch
from
bcb12a6 to
e00bb2d
Compare
| public void testGettingStarted() throws IOException { | ||
| // Creating a table | ||
| sql("CREATE TABLE %s (id bigint, data string) USING iceberg", tableName); | ||
|
|
||
| // Writing | ||
| sql("INSERT INTO %s VALUES (1, 'a'), (2, 'b'), (3, 'c')", tableName); | ||
| Assert.assertEquals( | ||
| "Should have inserted 3 rows", 3L, scalarSql("SELECT COUNT(*) FROM %s", tableName)); | ||
| Assertions.assertThat(scalarSql("SELECT COUNT(*) FROM %s", tableName)) |
There was a problem hiding this comment.
why not use the static import here?
There was a problem hiding this comment.
The static import is not recommended from the checkstyle. When it's statically imported, the test fails. Here's one of the examples:
> Task :iceberg-spark:iceberg-spark-extensions-3.4_2.12:compileTestJava
Error: eckstyle] [ERROR] /home/runner/work/iceberg/iceberg/spark/v3.5/spark-runtime/src/integration/java/org/apache/iceberg/spark/SmokeTest.java:21:46: Using a static member import should be avoided - org.assertj.core.api.Assertions.assertThat. [AvoidStaticImport]
|
@tomtongue can you rebase this against latest main as there were some changes that just went in for |
tomtongue
force-pushed
the
mig-junit5-other-extensions
branch
from
e00bb2d to
b465306
Compare
|
Okay, rebased this branch onto the latest main. @nastra |
|
Thanks so much for your review! @nastra |
Migrate the following "Read" and other sub-classes in spark-extensions to JUnit 5 and AssertJ style along with #9613, and #9086.
Current Progress
testSetInvalidIdentifierFieldsstill hasAssert.)