test: allow e2e service tests to be checked in #283

aajtodd · 2021-08-17T20:02:26Z

Issue #

N/A

Description of changes

Define an implicit e2eTest sourceSet. To add e2e style tests to a service merely add the folder and start adding tests. These tests can be run like normal unit tests from Intellij or from command line with the e2eTest task.

Scope

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

aajtodd · 2021-08-17T20:05:05Z

aws-runtime/crt-util/common/src/aws/sdk/kotlin/runtime/crt/ReadChannelBodyStream.kt

                return true
            }
+            // ensure the request context hasn't been cancelled
+            callContext.ensureActive()


this will throw a cancellation exception if the request context was cancelled. It doesn't fix the issue but will keep us from looping endlessly if the outer context is cancelled...

ianbotsf · 2021-08-17T21:35:42Z

aws-runtime/testing/jvm/src/aws/sdk/kotlin/runtime/testing/RandomInputStream.kt

+/**
+ * Test utility InputStream implementation that generates random ASCII data when
+ * read, up to the size specified when constructed.
+ * NOTE: Adapted from


Question: Adapted from where?

Will remove, it's unimportant. This was converted from v2 Java SDK utils though.

ianbotsf · 2021-08-17T21:35:57Z

aws-runtime/testing/jvm/src/aws/sdk/kotlin/runtime/testing/RandomInputStream.kt

+ * read, up to the size specified when constructed.
+ * NOTE: Adapted from
+ */
+public class RandomInputStream @JvmOverloads constructor(


Question: Why is @JvmOverloads necessary?

It probably isn't, auto convert added it and I didn't question it (until now).

ianbotsf · 2021-08-17T21:40:33Z

aws-runtime/testing/jvm/src/aws/sdk/kotlin/runtime/testing/RandomInputStream.kt

+            val bytes = ByteArray(bytesToRead)
+            Random.nextBytes(bytes)
+            System.arraycopy(bytes, 0, b, off, bytesToRead)


Nit: This is less performant than simply writing to the final array:

Random.nextBytes(b, off, bytesToRead)

ianbotsf · 2021-08-17T21:49:10Z

aws-runtime/testing/jvm/src/aws/sdk/kotlin/runtime/testing/RandomInputStream.kt

+            val bytes = ByteArray(1)
+            Random.nextBytes(bytes)
+            bytes[0].toInt()


Nit: This is less performant than simply getting a single byte via int conversion:

Random.nextInt().toByte()

Yup, stupid auto conversion. Be smarter.

ianbotsf · 2021-08-17T21:55:05Z

aws-runtime/testing/jvm/src/aws/sdk/kotlin/runtime/testing/RandomTempFile.kt

+    /**
+     * Creates, and fills, a temp file with a randomly generated name and specified size of random ASCII data.
+     *
+     * @param sizeInBytes The amount of random ASCII data, in bytes, for the new temp
+     * file.
+     * @throws IOException If any problems were encountered creating the new temp file.
+     */
+    public constructor(sizeInBytes: Long) : this(UUID.randomUUID().toString(), sizeInBytes, false)
+
+    /**
+     * Creates, and fills, a temp file with the specified name and specified
+     * size of random ASCII data.
+     *
+     * @param filename
+     * The name for the new temporary file, within the Java temp
+     * directory as declared in the JRE's system properties.
+     * @param sizeInBytes
+     * The amount of random ASCII data, in bytes, for the new temp
+     * file.
+     *
+     * @throws IOException
+     * If any problems were encountered creating the new temp file.
+     */
+    public constructor(filename: String, sizeInBytes: Int) : this(filename, sizeInBytes.toLong(), false)
+
+    /**
+     * Creates, and fills, a temp file with the specified name and specified
+     * size of random binary or character data.
+     *
+     * @param filename
+     * The name for the new temporary file, within the Java temp
+     * directory as declared in the JRE's system properties.
+     * @param sizeInBytes
+     * The amount of random ASCII data, in bytes, for the new temp
+     * file.
+     * @param binaryData Whether to fill the file with binary or character data.
+     *
+     * @throws IOException
+     * If any problems were encountered creating the new temp file.
+     */
+    /**
+     * Creates, and fills, a temp file with the specified name and specified
+     * size of random ASCII data.
+     *
+     * @param filename
+     * The name for the new temporary file, within the Java temp
+     * directory as declared in the JRE's system properties.
+     * @param sizeInBytes
+     * The amount of random ASCII data, in bytes, for the new temp
+     * file.
+     *
+     * @throws IOException
+     * If any problems were encountered creating the new temp file.
+     */
+    @JvmOverloads
+    public constructor(filename: String, sizeInBytes: Long, binaryData: Boolean = false) : super(
+        TEMP_DIR + separator + System.currentTimeMillis().toString() + "-" + filename
+    ) {
+        this.binaryData = binaryData
+        createFile(sizeInBytes)
+    }
+
+    public constructor(root: File?, filename: String?, sizeInBytes: Long) : super(root, filename) {
+        binaryData = false
+        createFile(sizeInBytes)
+    }


Comment: Seems like all these overloads could be replaced by a single constructor with default params.

Yeah I'll take another look at what was converted and collapse the ones that make sense.

ianbotsf · 2021-08-17T21:58:42Z

aws-runtime/testing/jvm/src/aws/sdk/kotlin/runtime/testing/RandomTempFile.kt

+                    val buffer = ByteArray(1024)
+                    var bytesRead: Int
+                    while (inputStream.read(buffer).also { bytesRead = it } > -1) {
+                        bufferedOutputStream.write(buffer, 0, bytesRead)
+                    }


Nit: Could be replaced by copyTo:

inputStream.copyTo(bufferedOutputStream)

Sure will update.

ianbotsf · 2021-08-17T22:00:03Z

aws-runtime/testing/jvm/src/aws/sdk/kotlin/runtime/testing/RandomTempFile.kt

+        public fun randomUncreatedFile(): File =
+            File(TEMP_DIR, UUID.randomUUID().toString()).also { it.deleteOnExit() }


Question: Unused?

Yeah this entire file was copy paste, auto convert to Kotlin from Java (with very minor fixes/updates). We can probably delete though as Files.createTempFile(..) would serve same purpose I think.

ianbotsf · 2021-08-17T22:05:01Z

services/s3/e2eTest/S3IntegrationTest.kt

+        val contents = """
+            Are you suggesting coconuts migrate?
+            Not at all, they could be carried.
+            What -- a swallow carrying a coconut?
+            It could grip it by the husk.
+            It's not a question of where he grips it! It's a simple question of weight ratios! A five ounce bird could not carry a 1 pound coconut.
+            Well, it doesn't matter. Will you go and tell your master that Arthur from the Court of Camelot is here.
+            Listen, in order to maintain air-speed velocity, a swallow needs to beat its wings 43 times times every second, right?
+            ...
+            It could be carried by an African swallow!
+            Oh yeah, an African swallow maybe, but not a European swallow. That's my point.
+            Oh, yeah, I agree with that.
+        """.trimIndent()


Correctness: This conflicts with the copyright header of the file.

Touché...also no fun. Will update

ianbotsf · 2021-08-17T22:13:09Z

services/s3/e2eTest/S3TestUtils.kt

+            do {
+                val bucketExists = try {
+                    client.headBucket { bucket = testBucket }
+                    true
+                } catch (ex: NotFound) {
+                    delay(300)
+                    false
+                }
+            } while (!bucketExists)


Comment: Consider timing out after a while (e.g., 60 seconds) so this doesn't run forever.

Yeah, ideally we would use waiters but of course we aren't there yet. I'll add a timeout for now.

I guess that means "hurry up and finish waiters". Message received. 😉

ianbotsf · 2021-08-17T22:14:44Z

services/s3/e2eTest/S3TestUtils.kt

+    suspend fun getTestBucket(client: S3Client): String = getBucketWithPrefix(client, TEST_BUCKET_PREFIX)
+
+    suspend fun getBucketWithPrefix(client: S3Client, prefix: String): String {
+        var testBucket = client.listBuckets {}
+            .buckets
+            ?.mapNotNull { it.name }
+            ?.firstOrNull { it.startsWith(prefix) }


Comment: This pair of functions gets or creates a bucket but the only place they're used is in the @BeforeAll block of S3BucketOpsIntegrationTest. In that situation, all we really want is create since we'll delete afterwards. You could simplify this into just createTestBucket and skip searching for an existing bucket.

Many of the utils were just adapted from the v2 Java test suite. I kept the behavior because it is a sort of self cleaning mechanism. If a test invocation crashes or otherwise fails to cleanup it will do so on the next run because it will re-use an existing bucket.

kggilmer · 2021-08-18T00:41:43Z

services/s3/e2eTest/S3IntegrationTest.kt

+/**
+ * Tests for bucket operations
+ */
+@TestInstance(TestInstance.Lifecycle.PER_CLASS)


question

for my understanding is this annotation property an optimization or necessary? if the latter why?

It changes the default behavior to create a single instance for all of the test methods instead of creating a new one per test. I did this to only create the bucket once and re-use it for all of the tests.

See https://junit.org/junit5/docs/current/user-guide/#writing-tests-test-instance-lifecycle for more info

So to answer your question directly, probably more of an optimization.

kggilmer · 2021-08-18T00:43:27Z

services/build.gradle.kts


    apply(from = rootProject.file("gradle/publish.gradle"))
+
+    if (project.file("e2eTest").exists()) {


question

what (if any) is the difference between putting tests in the tests sourceset vs this sourceset? Semantically of course the unit tests would go into one but I'm asking if there are any differences between the two?

Yeah there are definitely differences. If we put these into the unit test sourceSet they would run with the rest of the (unit) tests of course which is not what we want. This allows us to separate out these kinds of tests as an independent compilation and setup test tasks dedicated to them.

aajtodd added 3 commits August 17, 2021 11:44

wip

b05d8a7

add s3 e2e test examples and test utils

accf3d0

remove import

53d6fb7

aajtodd requested review from ianbotsf and kggilmer August 17, 2021 20:02

aajtodd commented Aug 17, 2021

View reviewed changes

ianbotsf requested changes Aug 17, 2021

View reviewed changes

cleanup autoconverted files; add timeout; comply with laws and such

84d3933

kggilmer approved these changes Aug 18, 2021

View reviewed changes

ianbotsf approved these changes Aug 18, 2021

View reviewed changes

aajtodd merged commit af5585b into main Aug 18, 2021

aajtodd deleted the e2etest-poc branch August 18, 2021 16:45

aajtodd mentioned this pull request Aug 23, 2021

Setup Integration Tests smithy-lang/smithy-kotlin#225

Closed

		public fun randomUncreatedFile(): File =
		File(TEMP_DIR, UUID.randomUUID().toString()).also { it.deleteOnExit() }


		apply(from = rootProject.file("gradle/publish.gradle"))

		if (project.file("e2eTest").exists()) {

test: allow e2e service tests to be checked in #283

test: allow e2e service tests to be checked in #283

Uh oh!

Conversation

aajtodd commented Aug 17, 2021

Issue #

Description of changes

Scope

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!

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!

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!

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