Copilot commented on code in PR #12271:
URL: https://github.com/apache/gravitino/pull/12271#discussion_r3704112372
##########
catalogs/catalog-glue/src/test/java/org/apache/gravitino/catalog/glue/TestGlueIcebergTableHelper.java:
##########
@@ -235,4 +239,75 @@ void testLoadTableOverwritesColumnTypesFromIcebergSchema()
{
assertEquals(Types.TimeType.of(6), cols[1].dataType());
assertEquals(Types.DecimalType.of(10, 2), cols[2].dataType());
}
+
+ @Test
+ void testCreateGlueCatalogWithoutStaticCredentials() {
+ Map<String, String> config = new HashMap<>();
+ config.put(GlueConstants.AWS_REGION, "us-east-1");
+ config.put(GlueConstants.WAREHOUSE, "s3://test-bucket/warehouse");
+
+ assertDoesNotThrow(
+ () -> {
+ try (GlueCatalog ignored =
+ (GlueCatalog) GlueIcebergTableHelper.createGlueCatalog(config)) {
+ // No-op.
+ }
+ });
Review Comment:
These tests rely on a downcast inside try-with-resources. Since
`createGlueCatalog` returns `Catalog`, the cast can mask what the test is
actually validating (type + closeability) and makes failures less focused
(e.g., `ClassCastException` rather than a clear assertion). Prefer asserting
the returned type via `assertInstanceOf(GlueCatalog.class, catalog)` and then
closing explicitly (or using try-with-resources on a variable already typed as
`GlueCatalog`) so the intent of the test is clearer.
##########
catalogs/catalog-glue/src/test/java/org/apache/gravitino/catalog/glue/TestGlueIcebergTableHelper.java:
##########
@@ -235,4 +239,75 @@ void testLoadTableOverwritesColumnTypesFromIcebergSchema()
{
assertEquals(Types.TimeType.of(6), cols[1].dataType());
assertEquals(Types.DecimalType.of(10, 2), cols[2].dataType());
}
+
+ @Test
+ void testCreateGlueCatalogWithoutStaticCredentials() {
+ Map<String, String> config = new HashMap<>();
+ config.put(GlueConstants.AWS_REGION, "us-east-1");
+ config.put(GlueConstants.WAREHOUSE, "s3://test-bucket/warehouse");
+
+ assertDoesNotThrow(
+ () -> {
+ try (GlueCatalog ignored =
+ (GlueCatalog) GlueIcebergTableHelper.createGlueCatalog(config)) {
+ // No-op.
+ }
+ });
+ }
+
+ @Test
+ void testCreateGlueCatalogWithBlankStaticCredentials() {
+ Map<String, String> config = new HashMap<>();
+ config.put(GlueConstants.AWS_REGION, "us-east-1");
+ config.put(GlueConstants.WAREHOUSE, "s3://test-bucket/warehouse");
+ config.put(GlueConstants.AWS_ACCESS_KEY_ID, "");
+ config.put(GlueConstants.AWS_SECRET_ACCESS_KEY, " ");
+
+ assertDoesNotThrow(
+ () -> {
+ try (GlueCatalog ignored =
+ (GlueCatalog) GlueIcebergTableHelper.createGlueCatalog(config)) {
+ // No-op.
+ }
+ });
+ }
+
+ @Test
+ void testCreateGlueCatalogWithStaticCredentials() {
+ Map<String, String> config = new HashMap<>();
+ config.put(GlueConstants.AWS_REGION, "us-east-1");
+ config.put(GlueConstants.WAREHOUSE, "s3://test-bucket/warehouse");
+ config.put(GlueConstants.AWS_ACCESS_KEY_ID, "test-access-key");
+ config.put(GlueConstants.AWS_SECRET_ACCESS_KEY, "test-secret-key");
+
+ assertDoesNotThrow(
+ () -> {
+ try (GlueCatalog ignored =
+ (GlueCatalog) GlueIcebergTableHelper.createGlueCatalog(config)) {
+ // No-op.
+ }
+ });
+ }
+
+ @Test
+ void testCreateGlueCatalogWithOnlyAccessKey() {
+ Map<String, String> config = new HashMap<>();
+ config.put(GlueConstants.AWS_REGION, "us-east-1");
+ config.put(GlueConstants.WAREHOUSE, "s3://test-bucket/warehouse");
+ config.put(GlueConstants.AWS_ACCESS_KEY_ID, "test-access-key");
+
+ assertThrows(
+ IllegalArgumentException.class, () ->
GlueIcebergTableHelper.createGlueCatalog(config));
Review Comment:
The new “fail fast” behavior is user-facing (per PR description), but the
tests only assert the exception type. To make the contract explicit and prevent
regressions toward less-informative failures, also assert the exception message
includes both property keys (or a stable substring), matching the
`Preconditions.checkArgument` message.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
