You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "tomtongue (via GitHub)" <gi...@apache.org> on 2024/04/04 09:21:26 UTC
[PR] [WIP] Migrate AWS tests [iceberg]
tomtongue opened a new pull request, #10086:
URL: https://github.com/apache/iceberg/pull/10086
Migrate AWS integration tests to JUnit 5 for https://github.com/apache/iceberg/issues/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/`:
* [] `dynamodb/`
* [ ] `glue/`
* [ ] `lakeformation/`
* [ ] `s3/`
* `S3TestUtil` (not a test file)
* [x] `TestS3FileIOIntegration`
* [ ] `TestS3MultipartUpload`
* `AwsIntegTestUtil` (not a test file)
- [ ] `TestAssumeRoleAwsClientFactory`
- [ ] `TestDefaultAwsClientFactory`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Migrate AWS tests to JUnit5 [iceberg]
Posted by "nk1506 (via GitHub)" <gi...@apache.org>.
nk1506 commented on code in PR #10086:
URL: https://github.com/apache/iceberg/pull/10086#discussion_r1555209833
##########
aws/src/integration/java/org/apache/iceberg/aws/TestDefaultAwsClientFactory.java:
##########
@@ -39,7 +40,7 @@ public void testGlueEndpointOverride() {
properties.put(AwsProperties.GLUE_CATALOG_ENDPOINT, "https://unknown:1234");
AwsClientFactory factory = AwsClientFactories.from(properties);
GlueClient glueClient = factory.glue();
- Assertions.assertThatThrownBy(
+ assertThatThrownBy(
Review Comment:
Can't we avoid these cleanups with this PR? IMO, this PR should have only Migration from Junit4 to Junit5 . WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Migrate AWS tests to JUnit5 [iceberg]
Posted by "nk1506 (via GitHub)" <gi...@apache.org>.
nk1506 commented on code in PR #10086:
URL: https://github.com/apache/iceberg/pull/10086#discussion_r1555485467
##########
aws/src/integration/java/org/apache/iceberg/aws/dynamodb/TestDynamoDbCatalog.java:
##########
@@ -99,22 +100,23 @@ public void testCreateNamespace() {
.tableName(catalogTableName)
.key(DynamoDbCatalog.namespacePrimaryKey(namespace))
.build());
- Assert.assertTrue("namespace must exist", response.hasItem());
- Assert.assertEquals(
- "namespace must be stored in DynamoDB",
- namespace.toString(),
- response.item().get("namespace").s());
- Assertions.assertThatThrownBy(() -> catalog.createNamespace(namespace))
+ assertThat(response.hasItem()).as("namespace must exist").isTrue();
+ assertThat(response.item())
+ .as("namespace must be stored in DynamoDB")
+ .hasEntrySatisfying(
+ "namespace",
+ attributeValue -> assertThat(attributeValue.s()).isEqualTo(namespace.toString()));
Review Comment:
Makes sense, I think `hasEntrySatisfying` sounds good as per the above reasons.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Migrate AWS tests to JUnit5 [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra merged PR #10086:
URL: https://github.com/apache/iceberg/pull/10086
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Migrate AWS tests to JUnit5 [iceberg]
Posted by "tomtongue (via GitHub)" <gi...@apache.org>.
tomtongue commented on code in PR #10086:
URL: https://github.com/apache/iceberg/pull/10086#discussion_r1555216313
##########
aws/src/integration/java/org/apache/iceberg/aws/TestDefaultAwsClientFactory.java:
##########
@@ -39,7 +40,7 @@ public void testGlueEndpointOverride() {
properties.put(AwsProperties.GLUE_CATALOG_ENDPOINT, "https://unknown:1234");
AwsClientFactory factory = AwsClientFactories.from(properties);
GlueClient glueClient = factory.glue();
- Assertions.assertThatThrownBy(
+ assertThatThrownBy(
Review Comment:
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](https://iceberg.apache.org/contribute/#assertj) also shows the tests without `Assertions` .
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Migrate AWS tests to JUnit5 [iceberg]
Posted by "tomtongue (via GitHub)" <gi...@apache.org>.
tomtongue commented on code in PR #10086:
URL: https://github.com/apache/iceberg/pull/10086#discussion_r1555216313
##########
aws/src/integration/java/org/apache/iceberg/aws/TestDefaultAwsClientFactory.java:
##########
@@ -39,7 +40,7 @@ public void testGlueEndpointOverride() {
properties.put(AwsProperties.GLUE_CATALOG_ENDPOINT, "https://unknown:1234");
AwsClientFactory factory = AwsClientFactories.from(properties);
GlueClient glueClient = factory.glue();
- Assertions.assertThatThrownBy(
+ assertThatThrownBy(
Review Comment:
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. Is there any reason to keep `Assertions`?
[Iceberg contribute guide](https://iceberg.apache.org/contribute/#assertj) also shows the tests without `Assertions` .
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Migrate AWS tests to JUnit5 [iceberg]
Posted by "tomtongue (via GitHub)" <gi...@apache.org>.
tomtongue commented on code in PR #10086:
URL: https://github.com/apache/iceberg/pull/10086#discussion_r1555324898
##########
aws/src/integration/java/org/apache/iceberg/aws/lakeformation/TestLakeFormationAwsClientFactory.java:
##########
@@ -165,7 +165,8 @@ public void testLakeFormationEnabledGlueCatalog() throws Exception {
glueCatalog.createNamespace(allowedNamespace);
} catch (GlueException e) {
LOG.error("fail to create Glue database", e);
- Assert.fail("create namespace should succeed");
+
+ fail("create namespace should succeed");
Review Comment:
Thanks. Will update this line.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Migrate AWS tests to JUnit5 [iceberg]
Posted by "nk1506 (via GitHub)" <gi...@apache.org>.
nk1506 commented on code in PR #10086:
URL: https://github.com/apache/iceberg/pull/10086#discussion_r1555586399
##########
aws/src/integration/java/org/apache/iceberg/aws/glue/TestGlueCatalogTable.java:
##########
@@ -229,10 +218,10 @@ public void testRenameTable() {
glueCatalog.renameTable(
TableIdentifier.of(namespace, tableName), TableIdentifier.of(namespace, newTableName));
Table renamedTable = glueCatalog.loadTable(TableIdentifier.of(namespace, newTableName));
- Assert.assertEquals(table.location(), renamedTable.location());
- Assert.assertEquals(table.schema().toString(), renamedTable.schema().toString());
- 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());
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Migrate AWS tests to JUnit5 [iceberg]
Posted by "nk1506 (via GitHub)" <gi...@apache.org>.
nk1506 commented on code in PR #10086:
URL: https://github.com/apache/iceberg/pull/10086#discussion_r1555503258
##########
aws/src/integration/java/org/apache/iceberg/aws/glue/TestGlueCatalogTable.java:
##########
@@ -84,43 +84,35 @@ public void testCreateTable() {
// verify table exists in Glue
GetTableResponse response =
glue.getTable(GetTableRequest.builder().databaseName(namespace).name(tableName).build());
- Assert.assertEquals(namespace, response.table().databaseName());
- Assert.assertEquals(tableName, response.table().name());
- Assert.assertEquals(
- BaseMetastoreTableOperations.ICEBERG_TABLE_TYPE_VALUE.toUpperCase(Locale.ENGLISH),
- response.table().parameters().get(BaseMetastoreTableOperations.TABLE_TYPE_PROP));
- Assert.assertTrue(
- response
- .table()
- .parameters()
- .containsKey(BaseMetastoreTableOperations.METADATA_LOCATION_PROP));
- Assert.assertEquals(
- schema.columns().size(), response.table().storageDescriptor().columns().size());
- Assert.assertEquals(partitionSpec.fields().size(), response.table().partitionKeys().size());
- Assert.assertEquals(
- "additionalLocations should match",
- tableLocationProperties.values().stream().sorted().collect(Collectors.toList()),
- response.table().storageDescriptor().additionalLocations().stream()
- .sorted()
- .collect(Collectors.toList()));
+ assertThat(response.table().databaseName()).isEqualTo(namespace);
+ assertThat(response.table().name()).isEqualTo(tableName);
+ assertThat(response.table().parameters())
+ .containsEntry(
+ BaseMetastoreTableOperations.TABLE_TYPE_PROP,
+ BaseMetastoreTableOperations.ICEBERG_TABLE_TYPE_VALUE.toUpperCase(Locale.ENGLISH))
+ .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())
Review Comment:
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());`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Migrate AWS tests to JUnit5 [iceberg]
Posted by "nk1506 (via GitHub)" <gi...@apache.org>.
nk1506 commented on code in PR #10086:
URL: https://github.com/apache/iceberg/pull/10086#discussion_r1555225734
##########
aws/src/integration/java/org/apache/iceberg/aws/dynamodb/TestDynamoDbCatalog.java:
##########
@@ -99,22 +100,23 @@ public void testCreateNamespace() {
.tableName(catalogTableName)
.key(DynamoDbCatalog.namespacePrimaryKey(namespace))
.build());
- Assert.assertTrue("namespace must exist", response.hasItem());
- Assert.assertEquals(
- "namespace must be stored in DynamoDB",
- namespace.toString(),
- response.item().get("namespace").s());
- Assertions.assertThatThrownBy(() -> catalog.createNamespace(namespace))
+ assertThat(response.hasItem()).as("namespace must exist").isTrue();
+ assertThat(response.item())
+ .as("namespace must be stored in DynamoDB")
+ .hasEntrySatisfying(
+ "namespace",
+ attributeValue -> assertThat(attributeValue.s()).isEqualTo(namespace.toString()));
Review Comment:
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 ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Migrate AWS tests to JUnit5 [iceberg]
Posted by "tomtongue (via GitHub)" <gi...@apache.org>.
tomtongue commented on PR #10086:
URL: https://github.com/apache/iceberg/pull/10086#issuecomment-2041851203
@nastra All AWS classes are migrated to JUnit5 + AssertJ. Could you review this PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Migrate AWS tests to JUnit5 [iceberg]
Posted by "tomtongue (via GitHub)" <gi...@apache.org>.
tomtongue commented on code in PR #10086:
URL: https://github.com/apache/iceberg/pull/10086#discussion_r1555328887
##########
aws/src/integration/java/org/apache/iceberg/aws/dynamodb/TestDynamoDbCatalog.java:
##########
@@ -99,22 +100,23 @@ public void testCreateNamespace() {
.tableName(catalogTableName)
.key(DynamoDbCatalog.namespacePrimaryKey(namespace))
.build());
- Assert.assertTrue("namespace must exist", response.hasItem());
- Assert.assertEquals(
- "namespace must be stored in DynamoDB",
- namespace.toString(),
- response.item().get("namespace").s());
- Assertions.assertThatThrownBy(() -> catalog.createNamespace(namespace))
+ assertThat(response.hasItem()).as("namespace must exist").isTrue();
+ assertThat(response.item())
+ .as("namespace must be stored in DynamoDB")
+ .hasEntrySatisfying(
+ "namespace",
+ attributeValue -> assertThat(attributeValue.s()).isEqualTo(namespace.toString()));
Review Comment:
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 by`get("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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Migrate AWS tests to JUnit5 [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10086:
URL: https://github.com/apache/iceberg/pull/10086#discussion_r1555620195
##########
aws/src/integration/java/org/apache/iceberg/aws/glue/TestGlueCatalogTable.java:
##########
@@ -229,10 +218,10 @@ public void testRenameTable() {
glueCatalog.renameTable(
TableIdentifier.of(namespace, tableName), TableIdentifier.of(namespace, newTableName));
Table renamedTable = glueCatalog.loadTable(TableIdentifier.of(namespace, newTableName));
- Assert.assertEquals(table.location(), renamedTable.location());
- Assert.assertEquals(table.schema().toString(), renamedTable.schema().toString());
- 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());
Review Comment:
it's because `Schema` doesn't implement equals()
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Migrate AWS tests to JUnit5 [iceberg]
Posted by "tomtongue (via GitHub)" <gi...@apache.org>.
tomtongue commented on code in PR #10086:
URL: https://github.com/apache/iceberg/pull/10086#discussion_r1555517631
##########
aws/src/integration/java/org/apache/iceberg/aws/glue/TestGlueCatalogTable.java:
##########
@@ -229,10 +218,10 @@ public void testRenameTable() {
glueCatalog.renameTable(
TableIdentifier.of(namespace, tableName), TableIdentifier.of(namespace, newTableName));
Table renamedTable = glueCatalog.loadTable(TableIdentifier.of(namespace, newTableName));
- Assert.assertEquals(table.location(), renamedTable.location());
- Assert.assertEquals(table.schema().toString(), renamedTable.schema().toString());
- 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());
Review Comment:
It's necessary because the testing the comparisons between objects fail.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Migrate AWS tests to JUnit5 [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10086:
URL: https://github.com/apache/iceberg/pull/10086#discussion_r1555315428
##########
aws/src/integration/java/org/apache/iceberg/aws/lakeformation/TestLakeFormationAwsClientFactory.java:
##########
@@ -165,7 +165,8 @@ public void testLakeFormationEnabledGlueCatalog() throws Exception {
glueCatalog.createNamespace(allowedNamespace);
} catch (GlueException e) {
LOG.error("fail to create Glue database", e);
- Assert.fail("create namespace should succeed");
+
+ fail("create namespace should succeed");
Review Comment:
rather than using `fail` with a try-catch, it's better to have something like `assertThatNoException().isThrownBy(() -> glueCatalog.createNamespace(allowedNamespace))`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Migrate AWS tests to JUnit5 [iceberg]
Posted by "nk1506 (via GitHub)" <gi...@apache.org>.
nk1506 commented on code in PR #10086:
URL: https://github.com/apache/iceberg/pull/10086#discussion_r1555513397
##########
aws/src/integration/java/org/apache/iceberg/aws/glue/TestGlueCatalogTable.java:
##########
@@ -229,10 +218,10 @@ public void testRenameTable() {
glueCatalog.renameTable(
TableIdentifier.of(namespace, tableName), TableIdentifier.of(namespace, newTableName));
Table renamedTable = glueCatalog.loadTable(TableIdentifier.of(namespace, newTableName));
- Assert.assertEquals(table.location(), renamedTable.location());
- Assert.assertEquals(table.schema().toString(), renamedTable.schema().toString());
- 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());
Review Comment:
nit: Just wondering why cast to `asString()` here ? Other Object comparison we are doing using `isEqualTo`
##########
aws/src/integration/java/org/apache/iceberg/aws/glue/TestGlueCatalogTable.java:
##########
@@ -258,10 +247,10 @@ public void testRenameTableFailsToCreateNewTable() {
.hasMessageContaining("Table already exists");
// old table can still be read with same metadata
Table oldTable = glueCatalog.loadTable(id);
- Assert.assertEquals(table.location(), oldTable.location());
- Assert.assertEquals(table.schema().toString(), oldTable.schema().toString());
- 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());
Review Comment:
same
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Migrate AWS tests to JUnit5 [iceberg]
Posted by "tomtongue (via GitHub)" <gi...@apache.org>.
tomtongue commented on code in PR #10086:
URL: https://github.com/apache/iceberg/pull/10086#discussion_r1555519947
##########
aws/src/integration/java/org/apache/iceberg/aws/glue/TestGlueCatalogTable.java:
##########
@@ -84,43 +84,35 @@ public void testCreateTable() {
// verify table exists in Glue
GetTableResponse response =
glue.getTable(GetTableRequest.builder().databaseName(namespace).name(tableName).build());
- Assert.assertEquals(namespace, response.table().databaseName());
- Assert.assertEquals(tableName, response.table().name());
- Assert.assertEquals(
- BaseMetastoreTableOperations.ICEBERG_TABLE_TYPE_VALUE.toUpperCase(Locale.ENGLISH),
- response.table().parameters().get(BaseMetastoreTableOperations.TABLE_TYPE_PROP));
- Assert.assertTrue(
- response
- .table()
- .parameters()
- .containsKey(BaseMetastoreTableOperations.METADATA_LOCATION_PROP));
- Assert.assertEquals(
- schema.columns().size(), response.table().storageDescriptor().columns().size());
- Assert.assertEquals(partitionSpec.fields().size(), response.table().partitionKeys().size());
- Assert.assertEquals(
- "additionalLocations should match",
- tableLocationProperties.values().stream().sorted().collect(Collectors.toList()),
- response.table().storageDescriptor().additionalLocations().stream()
- .sorted()
- .collect(Collectors.toList()));
+ assertThat(response.table().databaseName()).isEqualTo(namespace);
+ assertThat(response.table().name()).isEqualTo(tableName);
+ assertThat(response.table().parameters())
+ .containsEntry(
+ BaseMetastoreTableOperations.TABLE_TYPE_PROP,
+ BaseMetastoreTableOperations.ICEBERG_TABLE_TYPE_VALUE.toUpperCase(Locale.ENGLISH))
+ .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())
Review Comment:
thanks for the suggestion. I think the suggestion drops the sorting check. So this uses `isEqualTo` and it's not flaky test.
##########
aws/src/integration/java/org/apache/iceberg/aws/glue/TestGlueCatalogTable.java:
##########
@@ -84,43 +84,35 @@ public void testCreateTable() {
// verify table exists in Glue
GetTableResponse response =
glue.getTable(GetTableRequest.builder().databaseName(namespace).name(tableName).build());
- Assert.assertEquals(namespace, response.table().databaseName());
- Assert.assertEquals(tableName, response.table().name());
- Assert.assertEquals(
- BaseMetastoreTableOperations.ICEBERG_TABLE_TYPE_VALUE.toUpperCase(Locale.ENGLISH),
- response.table().parameters().get(BaseMetastoreTableOperations.TABLE_TYPE_PROP));
- Assert.assertTrue(
- response
- .table()
- .parameters()
- .containsKey(BaseMetastoreTableOperations.METADATA_LOCATION_PROP));
- Assert.assertEquals(
- schema.columns().size(), response.table().storageDescriptor().columns().size());
- Assert.assertEquals(partitionSpec.fields().size(), response.table().partitionKeys().size());
- Assert.assertEquals(
- "additionalLocations should match",
- tableLocationProperties.values().stream().sorted().collect(Collectors.toList()),
- response.table().storageDescriptor().additionalLocations().stream()
- .sorted()
- .collect(Collectors.toList()));
+ assertThat(response.table().databaseName()).isEqualTo(namespace);
+ assertThat(response.table().name()).isEqualTo(tableName);
+ assertThat(response.table().parameters())
+ .containsEntry(
+ BaseMetastoreTableOperations.TABLE_TYPE_PROP,
+ BaseMetastoreTableOperations.ICEBERG_TABLE_TYPE_VALUE.toUpperCase(Locale.ENGLISH))
+ .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())
Review Comment:
thanks for the suggestion. I think the suggestion drops the sorting check. So this uses `isEqualTo` and it's not flaky.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Migrate AWS tests to JUnit5 [iceberg]
Posted by "tomtongue (via GitHub)" <gi...@apache.org>.
tomtongue commented on code in PR #10086:
URL: https://github.com/apache/iceberg/pull/10086#discussion_r1555517972
##########
aws/src/integration/java/org/apache/iceberg/aws/glue/TestGlueCatalogTable.java:
##########
@@ -258,10 +247,10 @@ public void testRenameTableFailsToCreateNewTable() {
.hasMessageContaining("Table already exists");
// old table can still be read with same metadata
Table oldTable = glueCatalog.loadTable(id);
- Assert.assertEquals(table.location(), oldTable.location());
- Assert.assertEquals(table.schema().toString(), oldTable.schema().toString());
- 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());
Review Comment:
As commented above, it's necessary because the testing the comparisons between objects fail.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org