misc: e2e tests for s3 mrap #1193

@@ -75,6 +75,10 @@ subprojects {
                            implementation(libraries.kotlin.test.junit5)
                            implementation(project(":tests:e2e-test-util"))
                            implementation(libraries.slf4j.simple)
+                            implementation("aws.sdk.kotlin:s3control:+")


fix: We can't just add these to every e2e test. For example sts has it's own e2e tests and taking a dependency on it will cause classpath conflicts (or more likely unpredictable class loading behavior). We're going to need to probably give this more thought.

aajtodd · 2024-02-15T14:10:20Z

services/s3/e2eTest/src/S3TestUtils.kt

+    return accountId ?: throw Exception("Unable to get AWS account ID")
+}
+
+internal suspend fun createS3Bucket(


question: Why can't we re-use the existing S3Utils.getBucket()?

aajtodd · 2024-02-15T14:10:49Z

services/s3/e2eTest/src/S3TestUtils.kt

@@ -116,3 +131,243 @@ object S3TestUtils {
        return connection.responseCode
    }
 }
+
+internal suspend fun getAccountId(): String {


fix: Move to function on S3Utils so these are all grouped.

aajtodd · 2024-02-15T14:12:27Z

services/s3/e2eTest/src/S3TestUtils.kt

+        region = "us-west-2"
+    }
+
+    val accountId = sts.getCallerIdentity(


style: this operation has a default parameter, no need to construct an empty request object

val accountId = sts.getCallerIdentity()

aajtodd · 2024-02-15T14:17:18Z

services/s3/e2eTest/src/S3TestUtils.kt

+    val createRequestToken = s3ControlClient.createMultiRegionAccessPoint(
+        CreateMultiRegionAccessPointRequest {
+            accountId = testAccountId
+            details =


style: use the DSL builder

s3control.createMultiRegionAccessPoint { accountId = "accont" details { name = "access-point-name" regions = listOf( Region { bucket = "one" }, Region { bucket = "two" } ) } }

Not going to mark this everywhere but there are several other spots I'd have the same feedback

aajtodd · 2024-02-15T14:37:48Z

services/s3/e2eTest/src/S3TestUtils.kt

+) {
+    val startTime = System.currentTimeMillis()
+
+    while (System.currentTimeMillis() - startTime < timeLimit) {


fix: You don't need this, just use withTimeout (or withTimeoutOrNull depending)

aajtodd · 2024-02-15T14:38:21Z

services/s3/e2eTest/src/S3TestUtils.kt

+            return
+        }
+
+        TimeUnit.SECONDS.sleep(10L) // Avoid constant status checks


fix: It doesn't matter here since these are all runBlocking tests but this is not coroutine friendly, use delay instead

aajtodd · 2024-02-15T14:42:18Z

services/s3/e2eTest/src/MutliRegionAccessPointTest.kt

+                multiRegionAccessPointArn,
+                keyForObject,
+            )
+        } catch (exception: Throwable) {


style: This is unnecessary, any exception will fail the test with the given cause.

aajtodd · 2024-02-15T14:43:37Z

services/s3/e2eTest/src/MutliRegionAccessPointTest.kt

+    private fun cleanUpMrapTest(): Unit = runBlocking {
+        println("Cleaning up MutliRegionAccessPointTest tests")
+
+        val accountId = getAccountId()


suggestion: You could get this once in setupMrapTest and share it across tests.

e.g.

private lateinit var accountId: String @BeforeAll private fun setupMrapTest() : Unit = runBlocking { ... accountId = getAccountId() }

aajtodd · 2024-02-15T14:43:57Z

services/s3/e2eTest/src/MutliRegionAccessPointTest.kt

+    private val keyForObject = "test.txt"
+
+    @BeforeAll
+    private fun setUpMrapTest(): Unit = runBlocking {


nit: setup and cleanup are one word.

Counter-nit: "setup" and "cleanup" are nouns. "Set up" and "Clean up" are verbs, which is the preferred form of method names.

nit: My thought on this is to just call it setup, the test class is enough of a hint of what you are setting up

lauzadis

The tests look good. I would recommend keeping new functions out of S3TestUtils unless you think other S3 tests will use it. For MRAP-specific things, it makes more sense to define them in your specific test file

lauzadis · 2024-02-15T14:27:45Z

services/build.gradle.kts

-import aws.sdk.kotlin.gradle.kmp.*
+import aws.sdk.kotlin.gradle.kmp.kotlin


question: why did this change?

IntelliJ does that, it doesn't like the * imports. I'll leave it how it was

lauzadis · 2024-02-15T14:29:07Z

services/s3/e2eTest/src/MutliRegionAccessPointTest.kt

+
+    @BeforeAll
+    private fun setUpMrapTest(): Unit = runBlocking {
+        println("Setting up MutliRegionAccessPointTest tests")


nit: unnecessary print

lauzadis · 2024-02-15T14:29:15Z

services/s3/e2eTest/src/MutliRegionAccessPointTest.kt

+
+    @AfterAll
+    private fun cleanUpMrapTest(): Unit = runBlocking {
+        println("Cleaning up MutliRegionAccessPointTest tests")


nit: unnecessary print

lauzadis · 2024-02-15T14:31:00Z

services/s3/e2eTest/src/S3TestUtils.kt

+internal suspend fun getAccountId(): String {
+    println("Getting account ID")
+
+    val sts = StsClient {
+        region = "us-west-2"
+    }
+
+    val accountId = sts.getCallerIdentity(
+        GetCallerIdentityRequest { },
+    ).account
+
+    sts.close()
+
+    return accountId ?: throw Exception("Unable to get AWS account ID")
+}


correctness: because this uses StsClient it's not an "S3 test util", I'd recommend moving it

lauzadis · 2024-02-15T14:32:14Z

services/s3/e2eTest/src/S3TestUtils.kt

+internal suspend fun createS3Bucket(
+    s3Client: S3Client,
+    bucketName: String,
+    location: BucketLocationConstraint,
+) {
+    println("Creating S3 bucket: $bucketName")
+
+    s3Client.createBucket(
+        CreateBucketRequest {
+            bucket = bucketName
+            createBucketConfiguration =
+                CreateBucketConfiguration {
+                    locationConstraint = location
+                }
+        },
+    )
+}


question: This seems really similar to getBucketWithPrefix. Can that function be adapted to fit your requirements instead of making a new function that does roughly the same thing?

lauzadis · 2024-02-15T14:37:45Z

services/s3/e2eTest/src/S3TestUtils.kt

+internal suspend fun deleteS3Bucket(
+    s3Client: S3Client,
+    bucketName: String,
+) {
+    println("Deleting S3 bucket: $bucketName")
+
+    s3Client.deleteBucket(
+        DeleteBucketRequest {
+            bucket = bucketName
+        },
+    )
+}


Same comment here, this function is simple enough, I recommend removing it

lauzadis · 2024-02-15T14:39:11Z

services/s3/e2eTest/src/S3TestUtils.kt

+        ListObjectsV2Request {
+            bucket = bucketName
+        },


simplification: pass prefix = keyName to this call and S3 will do the searching for you

lauzadis · 2024-02-15T14:40:32Z

services/s3/e2eTest/src/S3TestUtils.kt

+internal fun closeClients(
+    vararg clients: SdkClient,
+) {
+    clients.forEach { client ->
+        client.close()
+    }
+}


Another function that's simple enough to remove

lauzadis · 2024-02-15T14:42:16Z

services/s3/e2eTest/src/MutliRegionAccessPointTest.kt

+                keyForObject,
+            )
+        } catch (exception: Throwable) {
+            println("Test failed with exception: ${exception.cause}")


nit: unnecessary print

lauzadis · 2024-02-15T14:42:19Z

services/s3/e2eTest/src/MutliRegionAccessPointTest.kt

+                )
+            }.message.shouldContain("SIGV4_ASYMMETRIC support is not yet implemented for the default signer.")
+        } catch (exception: Throwable) {
+            println("Test failed with exception: ${exception.cause}")


nit: unnecessary print

… e2e-tests

github-actions · 2024-02-22T14:14:26Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)

github-actions · 2024-02-22T14:23:59Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)

0marperez · 2024-02-22T14:31:51Z

services/s3/e2eTest/src/S3TestUtils.kt

+        }
+    }
+
+    internal suspend fun multiRegionAccessPointWasCreated(


This could be done using a get operation and then looking at the error we get if the MRAP doesn't exist. The only issue is it returns a rather generic S3ControlException and all we would have to differentiate a non existent MRAP from a failure would be the error message. Which doesn't seem right as it could change. Either way I can take that approach if that's what everyone thinks

0marperez · 2024-02-22T14:35:44Z

services/s3/e2eTest/src/S3TestUtils.kt

@@ -115,4 +145,126 @@ object S3TestUtils {

        return connection.responseCode
    }
+
+    internal suspend fun getAccountId(): String {


I couldn't think of a better place for this function. It seemed out of place in the tests class instead of here with the other utils. Placing it in a STS test utils folder could be another option but it seems a little odd since it's used for S3

Yeah this seems fine, especially now that S3 E2E tests are guaranteed to have a dependency on STS

github-actions · 2024-02-22T14:51:59Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)

aajtodd · 2024-02-22T19:46:37Z

services/s3/e2eTest/src/S3TestUtils.kt

+    private suspend fun waitUntilMultiRegionAccessPointOperationCompletes(
+        s3ControlClient: S3ControlClient,
+        request: String,
+        duration: Int,


nit: withTimeout also accepts a Duration, instead of passing this as an integer you could just be writing 5.minutes in the calling code and accept a Duration. Also probably rename to something like timeoutAfter

aajtodd · 2024-02-22T19:49:21Z

services/s3/e2eTest/src/S3TestUtils.kt

+                    return@withTimeout
+                }
+
+                if (status == "FAILED") throw Exception("$operation operation failed.")


style:

check(status != "FAILED") { "$operation operation failed")

aajtodd · 2024-02-22T19:51:43Z

services/build.gradle.kts

@@ -88,6 +88,26 @@ subprojects {
                        description = "Run e2e service tests"
                        group = "verification"

+                        if (project.name == "s3") {


I suppose this works but this is getting ugly. I'm thinking we'll be revisiting the structure around e2e tests soon.

I agree,I think we could refactor some of this into functions if it starts getting used more.

lauzadis · 2024-02-22T20:12:33Z

services/s3/e2eTest/src/MutliRegionAccessPointTest.kt

+    private val keyForObject = "test.txt"
+
+    @BeforeAll
+    private fun setUpMrapTest(): Unit = runBlocking {


nit: My thought on this is to just call it setup, the test class is enough of a hint of what you are setting up

lauzadis · 2024-02-22T20:17:00Z

services/s3/e2eTest/src/S3TestUtils.kt

+        var testBucket = if (region != null) {
+            buckets?.firstOrNull {
+                client.getBucketLocation {
+                    bucket = it
+                    expectedBucketOwner = accountId
+                }.locationConstraint?.value == region && it.startsWith(prefix)
+            }
+        } else {
+            buckets?.firstOrNull { it.startsWith(prefix) }
+        }


nit: possible simplification

var testBucket = buckets?.firstOrNull { bucketName -> bucketName.startsWith(prefix) && region?.let { client.getBucketLocation { bucket = bucketName expectedBucketOwner = accountId }.locationConstraint?.value == region } ?: true }

lauzadis · 2024-02-22T20:17:28Z

services/s3/e2eTest/src/S3TestUtils.kt

@@ -115,4 +145,126 @@ object S3TestUtils {

        return connection.responseCode
    }
+
+    internal suspend fun getAccountId(): String {
+        println("Getting account ID")


nit: seems like an unnecessary print to me

There's a couple of other print statements in the utils, why does this one seem unnecessary?

Because all the other prints have debuggable information like the bucket name, operation, etc. This one has nothing, so I think it's not super useful. It's totally not a blocking comment, just something I noticed

lauzadis · 2024-02-22T20:19:19Z

services/s3/e2eTest/src/S3TestUtils.kt

+        val search = s3Control.listMultiRegionAccessPoints {
+            accountId = testAccountId
+        }.accessPoints?.find { it.name == multiRegionAccessPointName }
+
+        return search != null


nit: simplification

return s3Control.listMultiRegionAccessPoints { accountId = testAccountId }.accessPoints?.any { it.name == multiRegionAccessPointName }

lauzadis · 2024-02-22T20:20:21Z

services/s3/e2eTest/src/S3TestUtils.kt

@@ -115,4 +145,126 @@ object S3TestUtils {

        return connection.responseCode
    }
+
+    internal suspend fun getAccountId(): String {


Yeah this seems fine, especially now that S3 E2E tests are guaranteed to have a dependency on STS

sonarqubecloud · 2024-02-22T20:41:08Z

Quality Gate passed

Issues
1 New issue

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

github-actions · 2024-02-22T20:44:00Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)

0marperez added 3 commits January 24, 2024 12:56

First test added

a33e0ad

Add unsupported signing algorithm test

906ee38

Added account id & clean up

5e1bca6

0marperez added the no-changelog Indicates that a changelog entry isn't required for a pull request. Use sparingly. label Jan 24, 2024

Refactoring

49d102b

Empty commit

44f5efa

0marperez added 2 commits January 26, 2024 14:18

Improve logging & add prefix to S3 buckets

80a1bb0

Change bucket names

78a5370

Comment and small fix

bb14a9f

0marperez added 3 commits January 29, 2024 14:21

Refactoring

ae16933

Refactoring

fc59631

refactoring

4da19ee

Fixed before all and after all

ec59a8c

Merge branch 'main' of https://github.com/awslabs/aws-sdk-kotlin into…

e852b30

… e2e-tests

Use util functions already there

190f4eb

Merge branch 'main' of https://github.com/awslabs/aws-sdk-kotlin into…

394da3d

… e2e-tests

0marperez marked this pull request as ready for review February 14, 2024 21:53

0marperez requested a review from a team as a code owner February 14, 2024 21:53

aajtodd requested changes Feb 15, 2024

View reviewed changes

lauzadis reviewed Feb 15, 2024

View reviewed changes

0marperez added 5 commits February 19, 2024 16:34

Merge branch 'main' of https://github.com/awslabs/aws-sdk-kotlin into…

387dd4e

… e2e-tests

Merge branch 'main' of https://github.com/awslabs/aws-sdk-kotlin into…

43420fe

… e2e-tests

Checkpoint

47ae5a4

PR feedback

9a8c055

Refactor

af8ec02

Lint

bb979b1

0marperez commented Feb 22, 2024

View reviewed changes

Small refactor

943c4c3

aajtodd approved these changes Feb 22, 2024

View reviewed changes

Minor Feedback

83d9ef9

lauzadis approved these changes Feb 22, 2024

View reviewed changes

Minor Feedback

c2aa248

0marperez merged commit d16f9f7 into main Feb 22, 2024
16 of 17 checks passed

0marperez deleted the e2e-tests branch February 22, 2024 21:17

		import aws.sdk.kotlin.gradle.kmp.*
		import aws.sdk.kotlin.gradle.kmp.kotlin

misc: e2e tests for s3 mrap #1193

misc: e2e tests for s3 mrap #1193

Conversation

0marperez commented Jan 24, 2024

Issue #

Description of changes

github-actions bot commented Jan 24, 2024

github-actions bot commented Jan 24, 2024

github-actions bot commented Jan 25, 2024

github-actions bot commented Jan 26, 2024

github-actions bot commented Jan 26, 2024

github-actions bot commented Feb 9, 2024

github-actions bot commented Feb 9, 2024

github-actions bot commented Feb 14, 2024

github-actions bot commented Feb 14, 2024

github-actions bot commented Feb 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lauzadis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 22, 2024

github-actions bot commented Feb 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0marperez Feb 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Feb 22, 2024

Quality Gate passed

github-actions bot commented Feb 22, 2024

0marperez Feb 22, 2024 •

edited

Loading