AWS: Add AWS integ tests to check task and enable tests based on required environment variables #12671

lliangyu-lin · 2025-03-27T22:53:48Z

Description

Delegate some of the aws integration tests to use minio docker container, that emulates aws services.
- This allows running tests in pipeline without the need to have a dedicated aws account and credentials
Move container tests from aws test to integrationTest folder
Added check task to depend on integrationTest

Testing

./gradlew :iceberg-aws:check Confirmed unit test and aws mocked integration tests are ran successfully. When required AWS environment variables are not set, the tests are correctly skipped.

… enable tests in check task

aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIOProperties.java

nastra · 2025-03-28T12:52:28Z

aws/src/integration/java/org/apache/iceberg/aws/dynamodb/TestMockDynamoDbCatalog.java

@@ -56,8 +58,7 @@
 import software.amazon.awssdk.services.s3.model.HeadObjectRequest;
 import software.amazon.awssdk.services.s3.model.NoSuchKeyException;

-public class TestDynamoDbCatalog {
-
+public class TestMockDynamoDbCatalog extends BaseAwsMockTest {


can we achieve the same without having to rename tests? I don't think it's clear that tests need to have a Mock in the name to properly be executed and it's very likely that this will be missed in the future if new tests are added. Hence I would suggest to keep the test names as they are

Right now I use filtering to only run tests start with TestMock, but I agree it's not very clear. I think we could maybe introduce a custom annotation. Let me try out new approaches and give update on this.

Seems like using Tag and

useJUnitPlatform { includeTags 'verification' }

is the cleanest approach

aws/src/integration/java/org/apache/iceberg/aws/dynamodb/TestMockDynamoDbLockManager.java

aws/src/integration/java/org/apache/iceberg/aws/moto/BaseAwsMockTest.java

lliangyu-lin · 2025-03-31T22:33:51Z

cc: @nastra @xiaoxuandev I made some updates based on the reviews. Feel free to take another look when you have time.

docs/docs/aws.md

aws/src/integration/java/org/apache/iceberg/aws/dynamodb/TestDynamoDbCatalog.java

aws/src/integration/java/org/apache/iceberg/aws/dynamodb/TestDynamoDbLockManager.java

lliangyu-lin · 2025-04-01T20:17:41Z

@xiaoxuandev @nastra There are some concerns on moving the tests to moto since we need to add additional configs to make it work correctly. So I revert those changes in this PR and we can revisit those later with adding separate mock test group that can be enabled in the pipeline while maintaining integration tests that use actual aws services.

But we can still close out on the original issue on moving the docker based tests from test to integrationTest. Let me know what you think.

xiaoxuandev · 2025-04-02T05:56:17Z

@xiaoxuandev @nastra There are some concerns on moving the tests to moto since we need to add additional configs to make it work correctly. So I revert those changes in this PR and we can revisit those later with adding separate mock test group that can be enabled in the pipeline while maintaining integration tests that use actual aws services.

But we can still close out on the original issue on moving the docker based tests from test to integrationTest. Let me know what you think.

Thanks @lliangyu-lin, that sounds good to me. Do we need to change the commit title?

nastra · 2025-04-02T10:08:35Z

Since the PR is now using JUnit tags to distinguish between tests I was wondering whether it would make more sense to actually move the docker-specific tests to integrationTest instead of introducing a verificationIntegrationTest.
We would then annotate the current tests under integrationTest to indicate that those need ENV vars to be set and we would automatically include those annotated tests when those ENV vars are set.
That way we don't need to introduce another test task and we could finally add integrationTest to be executed as part of normal CI.

lliangyu-lin · 2025-04-02T22:36:31Z

We would then annotate the current tests under integrationTest to indicate that those need ENV vars to be set and we would automatically include those annotated tests when those ENV vars are set.

I think in this scenario, custom annotation is better compared to tags since some integration tests require different env vars, and we don't need to introduce another task as well. Otherwise we will need to add many different tags and need to handle the condition in gradle build file.

lliangyu-lin · 2025-04-02T23:29:07Z

Updated based on the comments.
Please take another look when you have time. cc: @nastra @xiaoxuandev

nastra · 2025-04-03T07:32:25Z

build.gradle

@@ -529,7 +529,10 @@ project(':iceberg-aws') {
    inputSpec.set(s3SignerSpec)
    recommend.set(true)
  }
-  check.dependsOn('validateS3SignerSpec')
+  tasks.named('check') {


I would prefer to not change this line and keep it as is. This is so that we're in-line with how we define this in other places. And then right after the integrationTest task (after L526) is defined you would add the check.dependsOn integrationTest

nastra · 2025-04-03T07:35:51Z

aws/src/integration/java/org/apache/iceberg/aws/EnableAwsTest.java

+@Inherited
+@Target({ElementType.TYPE, ElementType.METHOD})
+@Retention(RetentionPolicy.RUNTIME)
+public @interface EnableAwsTest {


I believe we should be able to reuse EnabledIfEnvironmentVariables instead of introducing a separate Java annotation

nastra · 2025-04-03T07:37:49Z

aws/src/integration/java/org/apache/iceberg/aws/TestAssumeRoleAwsClientFactory.java

+      AwsIntegTestUtil.AWS_REGION,
+      AwsIntegTestUtil.AWS_TEST_BUCKET
+    })
+@ExtendWith(EnableAwsTestCondition.class)


could you please try and see whether we can achieve the same thing by using EnabledIfEnvironmentVariables? This would be more straightforward and easier to read/understand when the tests should be executed. Also we don't need to introduce any new classes/annotations

@nastra Thank you for the suggestion! I made a revision to change to EnabledIfEnvironmentVariables and is working the same way, and it's good to reuse as well. But some caveats I found are that EnabledIfEnvironmentVariables cannot be applied on parent class and be inherited to all children classes, which means we need to put these annotations on each test class.

This might be enough, but some other thoughts I have are

Extend the EnabledIfEnvironmentVariables to include @inherited (Need to introduce a class)

Use AssumeNotNull to check env vars in BeforeAll (Remove any annotation, but is not clear to user why the tests are skipped since it's embedded in test setup).

xiaoxuandev · 2025-04-03T16:35:07Z

aws/src/integration/java/org/apache/iceberg/aws/TestAssumeRoleAwsClientFactory.java

@@ -47,6 +49,14 @@
 import software.amazon.awssdk.services.iam.model.PutRolePolicyRequest;
 import software.amazon.awssdk.services.s3.model.S3Exception;

+@EnabledIfEnvironmentVariables({


I think skipping those tests is reasonable, but hardcoding AWS credentials in environment variables poses security risks. While this is an existing issue, it was never enabled for CI. I would prefer a configuration-based approach over using @EnabledIfEnvironmentVariables.

Discussed with @xiaoxuandev offline. Since we are mixing other issues on this PR, I think we are good with moving with environment variables approach and close out on moving the docker based tests first.
I will follow up with separate PRs to address other concerns in the AWS integration tests. I have created this issue to track.
cc: @nastra

Delegate part of AWS integration tests to using mock aws services and…

15c229c

… enable tests in check task

github-actions bot added build AWS labels Mar 27, 2025

manuzhang mentioned this pull request Mar 28, 2025

AWS: Move docker-based tests to integrationTest #12673

Closed

manuzhang reviewed Mar 28, 2025

View reviewed changes

aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIOProperties.java Outdated Show resolved Hide resolved

nastra reviewed Mar 28, 2025

View reviewed changes

aws/src/integration/java/org/apache/iceberg/aws/dynamodb/TestMockDynamoDbLockManager.java Outdated Show resolved Hide resolved

nastra reviewed Mar 28, 2025

View reviewed changes

aws/src/integration/java/org/apache/iceberg/aws/moto/BaseAwsMockTest.java Outdated Show resolved Hide resolved

lliangyu-lin added 3 commits March 28, 2025 23:24

Address test filtering through tag

3e247f6

Merge branch 'master' into iceberg-aws-integ-test

2075e1b

Add documentation, update tests

7529872

github-actions bot added the docs label Mar 31, 2025

xiaoxuandev reviewed Apr 1, 2025

View reviewed changes

docs/docs/aws.md Outdated Show resolved Hide resolved

xiaoxuandev reviewed Apr 1, 2025

View reviewed changes

aws/src/integration/java/org/apache/iceberg/aws/dynamodb/TestDynamoDbCatalog.java Show resolved Hide resolved

xiaoxuandev reviewed Apr 1, 2025

View reviewed changes

aws/src/integration/java/org/apache/iceberg/aws/dynamodb/TestDynamoDbLockManager.java Show resolved Hide resolved

lliangyu-lin added 2 commits April 1, 2025 19:02

Revert moto changes and update tags

2986a68

Update

f20fb83

lliangyu-lin requested review from nastra and xiaoxuandev April 1, 2025 20:17

lliangyu-lin added 3 commits April 2, 2025 14:48

Add custom annotation to enable aws integ tests based on env var

97f5270

Merge branch 'master' into iceberg-aws-integ-test

6b1bb91

Fix test failures

87ff3cd

lliangyu-lin changed the title ~~AWS: Delegate part of AWS integration tests to using mock aws services and enable tests in check task~~ AWS: Add AWS integ tests to check task and enable tests based on required environment variables Apr 2, 2025

nastra reviewed Apr 3, 2025

View reviewed changes

Use EnabledIfEnvironmentVariable

b286f6e

xiaoxuandev reviewed Apr 3, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AWS: Add AWS integ tests to check task and enable tests based on required environment variables #12671

AWS: Add AWS integ tests to check task and enable tests based on required environment variables #12671

lliangyu-lin commented Mar 27, 2025 •

edited

Loading

nastra Mar 28, 2025

lliangyu-lin Mar 28, 2025

lliangyu-lin Mar 28, 2025

lliangyu-lin commented Mar 31, 2025

lliangyu-lin commented Apr 1, 2025

xiaoxuandev commented Apr 2, 2025

nastra commented Apr 2, 2025

lliangyu-lin commented Apr 2, 2025 •

edited

Loading

lliangyu-lin commented Apr 2, 2025

nastra Apr 3, 2025

nastra Apr 3, 2025

nastra Apr 3, 2025

lliangyu-lin Apr 3, 2025 •

edited

Loading

xiaoxuandev Apr 3, 2025

lliangyu-lin Apr 3, 2025

AWS: Add AWS integ tests to check task and enable tests based on required environment variables #12671

Are you sure you want to change the base?

AWS: Add AWS integ tests to check task and enable tests based on required environment variables #12671

Conversation

lliangyu-lin commented Mar 27, 2025 • edited Loading

Description

Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lliangyu-lin commented Mar 31, 2025

lliangyu-lin commented Apr 1, 2025

xiaoxuandev commented Apr 2, 2025

nastra commented Apr 2, 2025

lliangyu-lin commented Apr 2, 2025 • edited Loading

lliangyu-lin commented Apr 2, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lliangyu-lin Apr 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lliangyu-lin commented Mar 27, 2025 •

edited

Loading

lliangyu-lin commented Apr 2, 2025 •

edited

Loading

lliangyu-lin Apr 3, 2025 •

edited

Loading