Migrate AWS tests to JUnit5 by tomtongue · Pull Request #10086 · apache/iceberg

tomtongue · 2024-04-04T09:21:24Z

Migrate AWS integration tests to JUnit 5 for #9080

All tests in iceberg-aws/test already use JUnit5 and AssertJ.

The following integration tests need to be migrated.

iceberg-aws/integration/java/org.apache.iceberg.aws/:

TestAssumeRoleAwsClientFactory
TestDefaultAwsClientFactory

nk1506 · 2024-04-08T04:34:10Z

    AwsClientFactory factory = AwsClientFactories.from(properties);
    GlueClient glueClient = factory.glue();
-    Assertions.assertThatThrownBy(
+    assertThatThrownBy(


Can't we avoid these cleanups with this PR? IMO, this PR should have only Migration from Junit4 to Junit5 . WDYT?

I believe the changes for removing Assertion are very small in this package, and by the changes, it can avoid the situation that two methods like Assertions.assertThat and assertThat exist in a same file. So it won't be a big problem. If there's any reason to keep Assertions, please let me know.
Iceberg contribute guide also shows the tests without Assertions .

tomtongue · 2024-04-08T04:42:33Z

@nastra All AWS classes are migrated to JUnit5 + AssertJ. Could you review this PR?

nk1506 · 2024-04-08T05:03:28Z

+        .as("namespace must be stored in DynamoDB")
+        .hasEntrySatisfying(
+            "namespace",
+            attributeValue -> assertThat(attributeValue.s()).isEqualTo(namespace.toString()));


for a single key match what about simple assertion like

assertThat(response.item().get("namespace").s()).as("namespace must be stored in DynamoDB").isEqualTo(namespace.toString());

For multiple keys validation it looks good to use hasEntrySatisfying. WDYT ?

Thanks for the suggestion. I think your suggestion seems to be fine, but my change also checks if response.item has namespace or not. But if the value extraction byget("namespace"), it doesn't check in the test. So if this part is like your suggestion, it's better to write like:

assertThat(response.item()).containsKey("namespace") assertThat(response.item().get("namespace").s())...

That's why I'm using hasEntrySatisfying here. If I'm wrong here, please correct me.

Makes sense, I think hasEntrySatisfying sounds good as per the above reasons.

nastra · 2024-04-08T07:00:53Z

      LOG.error("fail to create Glue database", e);
-      Assert.fail("create namespace should succeed");
+
+      fail("create namespace should succeed");


rather than using fail with a try-catch, it's better to have something like assertThatNoException().isThrownBy(() -> glueCatalog.createNamespace(allowedNamespace))

Thanks. Will update this line.

nk1506 · 2024-04-08T09:20:49Z

+        .containsKey(BaseMetastoreTableOperations.METADATA_LOCATION_PROP);
+    assertThat(response.table().storageDescriptor().columns()).hasSameSizeAs(schema.columns());
+    assertThat(response.table().partitionKeys()).hasSameSizeAs(partitionSpec.fields());
+    assertThat(response.table().storageDescriptor().additionalLocations())


previously the assertion was happening after sorting the values. Do you think it can become flaky someday?
Why not somethng like
assertThat(response.table().storageDescriptor().additionalLocations()).containsAll(tableLocationProperties.values());

thanks for the suggestion. I think the suggestion drops the sorting check. So this uses isEqualTo and it's not flaky.

nk1506 · 2024-04-08T09:28:27Z

-    Assert.assertEquals(table.spec(), renamedTable.spec());
-    Assert.assertEquals(table.currentSnapshot(), renamedTable.currentSnapshot());
+    assertThat(renamedTable.location()).isEqualTo(table.location());
+    assertThat(renamedTable.schema()).asString().isEqualTo(table.schema().toString());


nit: Just wondering why cast to asString() here ? Other Object comparison we are doing using isEqualTo

It's necessary because the testing the comparisons between objects fail.

Yeah Just wondering , Why PartiotionSpec and Snapshot comparison are working.
As per my understanding isEqualTo compares object values. So ideally it should not fail unless I am missing something.

it's because Schema doesn't implement equals()

nk1506 · 2024-04-08T09:28:39Z

-    Assert.assertEquals(table.spec(), oldTable.spec());
-    Assert.assertEquals(table.currentSnapshot(), oldTable.currentSnapshot());
+    assertThat(oldTable.location()).isEqualTo(table.location());
+    assertThat(oldTable.schema()).asString().isEqualTo(table.schema().toString());


As commented above, it's necessary because the testing the comparisons between objects fail.

Migrate S3 tests

7b0f7d4

github-actions Bot added build AWS labels Apr 4, 2024

Add assertJ to the checkstyle

774aba7

github-actions Bot added the INFRA label Apr 4, 2024

tomtongue added 2 commits April 6, 2024 01:12

Add DynamoDB, Glue and AwsClientFactory classes

e36590f

Migrate other files

9f2fe0d

tomtongue changed the title ~~[WIP] Migrate AWS tests~~ Migrate AWS tests Apr 6, 2024

tomtongue changed the title ~~Migrate AWS tests~~ Migrate AWS tests to JUnit5 Apr 6, 2024

nk1506 reviewed Apr 8, 2024

View reviewed changes

nastra reviewed Apr 8, 2024

View reviewed changes

nk1506 reviewed Apr 8, 2024

View reviewed changes

tomtongue added 2 commits April 8, 2024 18:50

Remove Assertions.fail

6a08ebe

Remove catch block

3f88d69

tomtongue force-pushed the mig-junit5-aws branch from 3ae68a2 to 3f88d69 Compare April 8, 2024 09:58

nastra approved these changes Apr 8, 2024

View reviewed changes

nastra merged commit b3261d0 into apache:main Apr 8, 2024

tomtongue deleted the mig-junit5-aws branch April 8, 2024 11:04

nastra mentioned this pull request Apr 8, 2024

iceberg-aws: Switch tests to JUnit5 + AssertJ-style assertions #9080

Closed

aajisaka mentioned this pull request Apr 25, 2024

AWS: Fix TestGlueCatalogTable#testCreateTable #10221

Merged

sasankpagolu pushed a commit to sasankpagolu/iceberg that referenced this pull request Oct 27, 2024

AWS: Migrate tests to JUnit5 (apache#10086)

a3ee2e0

zachdisc pushed a commit to zachdisc/iceberg that referenced this pull request Dec 23, 2024

AWS: Migrate tests to JUnit5 (apache#10086)

5398f2d

Conversation

tomtongue commented Apr 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomtongue Apr 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomtongue commented Apr 8, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomtongue Apr 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tomtongue commented Apr 4, 2024 •

edited

Loading

tomtongue Apr 8, 2024 •

edited

Loading

tomtongue Apr 8, 2024 •

edited

Loading