feat: smoke tests trait #1141
feat: smoke tests trait #1141
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| { | ||
| "id": "756754c3-f6e1-4ff2-ae31-08b3b67b6750", | ||
| "type": "feature", | ||
| "description": "Add support for smoke tests" |
There was a problem hiding this comment.
nit: refer to Smithy smoke tests trait / link to Smithy docs: https://smithy.io/2.0/additional-specs/smoke-tests.html
| import software.amazon.smithy.smoketests.traits.SmokeTestsTrait | ||
|
|
||
| /** | ||
| * Renders smoke test runner for a service if any of the operations has the smoke test trait. |
There was a problem hiding this comment.
nit: "if any of the operations have the [SmokeTestsTrait]"
| * Syntactic sugar for getting a services operations | ||
| */ | ||
| @SmithyInternalApi | ||
| fun Model.operations(service: ShapeId): Set<OperationShape> { |
There was a problem hiding this comment.
suggestion: Make this an extension property (val) instead of fun
| */ | ||
| class SmokeTestsIntegration : KotlinIntegration { | ||
| override fun enabledForService(model: Model, settings: KotlinSettings): Boolean = | ||
| model.operations(settings.service).any { it.hasTrait<SmokeTestsTrait>() } && !smokeTestDenyList.contains(settings.sdkId) |
There was a problem hiding this comment.
simplification for second half of the &&: settings.sdkId !in smokeTestDenyList
| ctx.symbolProvider.toSymbol(ctx.model.expectShape(ctx.settings.service)), | ||
| ctx.model.operations(ctx.settings.service).filter { it.hasTrait<SmokeTestsTrait>() }, | ||
| ctx.model, | ||
| ctx.symbolProvider, | ||
| ctx.settings.sdkId, |
There was a problem hiding this comment.
I see ctx. repeated a lot, have you considered just passing the entire CodegenContext instead of unwrapping it here?
| private fun renderOperation(operation: OperationShape, testCase: SmokeTestCase) { | ||
| val operationSymbol = symbolProvider.toSymbol(model.getShape(operation.input.get()).get()) | ||
|
|
||
| writer.addImport(operationSymbol) |
There was a problem hiding this comment.
Avoid manually adding imports if possible
| * Some smoke tests model exceptions not found in the model, in that case we default to the generic smoke tests | ||
| * failure exception. |
There was a problem hiding this comment.
I think these should be considered invalid models and added to the deny list until fixed
| /** | ||
| * Renders a [Node] into String format for codegen. | ||
| */ | ||
| fun Node.render(): String = when (this) { |
There was a problem hiding this comment.
naming: render usually means you are making a write(...) call, which does not happen here
| class SmokeTestsRunnerGeneratorTest { | ||
| private val moneySign = "$" | ||
|
|
||
| private fun codegen(model: Model): String { |
There was a problem hiding this comment.
naming: generateSmokeTests
| fun codegenTest() { | ||
| val model = | ||
| """ | ||
| ${moneySign}version: "2" |
There was a problem hiding this comment.
Other places we do this use ${'$'} which seems more readable
| model.operations(settings.service).any { it.hasTrait<SmokeTestsTrait>() } && !smokeTestDenyList.contains(settings.sdkId) | ||
|
|
||
| override fun writeAdditionalFiles(ctx: CodegenContext, delegator: KotlinDelegator) = | ||
| delegator.useFileWriter("SmokeTests.kt", "smoketests", "./jvm-src/main/java/") { writer -> |
There was a problem hiding this comment.
correctness: package ideally should be aws.sdk.kotlin.services.$SERVICE.smoketests, but we should not reference aws-sdk-kotlin here. Can this be made configurable through the integration somehow?
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| */ | ||
| class FailedResponseTrait(node: ObjectNode) : AnnotationTrait(ID, node) { | ||
| companion object { | ||
| val ID: ShapeId = ShapeId.from("com.test#failedResponseTrait") |
There was a problem hiding this comment.
naming: namespace under smithy.kotlin.traits like the other traits
There was a problem hiding this comment.
I think FailedResponseTrait and SuccessResponseTrait can better go together into a single file SmokeTestTraits.kt
| delegator.useFileWriter( | ||
| "SmokeTests.kt", | ||
| "${ctx.settings.pkg.name}.smoketests", | ||
| "./jvm-src/test/java/", |
There was a problem hiding this comment.
Is jvm-src a standard sourceset location?
There was a problem hiding this comment.
No, I was trying to follow the convention we have currently without affecting the SDK. I believe we currently code-generate services into a src dir and then the SDK moves that code into the <sdk root dir>/services/<relevant service>/generated-src dir in the SDK. Here's where we move the generated code.
I don't know if we have anywhere else that does codegen like this that is intended as JVM only. If we follow source-set convention I think src should be commonMain and jvm-src should be jvmMain but it won't affect functionality.
There was a problem hiding this comment.
Why is generating JVM-only code necessary? Can't we generate common code to run the tests?
There was a problem hiding this comment.
Main reason is that there's no common code for exitProcess
https://kotlinlang.org/api/latest/jvm/stdlib/kotlin.system/exit-process.html
I think I can wrap this in our own KMP function, thoughts?
| private val operations = ctx.model.operations(ctx.settings.service).filter { it.hasTrait<SmokeTestsTrait>() } | ||
|
|
||
| // Test config | ||
| private val hasSuccessResponseTrait = ctx.model.expectShape<ServiceShape>(ctx.settings.service).hasTrait(SuccessResponseTrait.ID) | ||
| private val hasFailedResponseTrait = ctx.model.expectShape<ServiceShape>(ctx.settings.service).hasTrait(FailedResponseTrait.ID) |
There was a problem hiding this comment.
Generally I prefer to just grab everything from the ctx instead of storing in local values, but since you've made private val model = ctx.model, you should not use ctx.model anywhere
ctx.model -> model
| init { | ||
| check(!(hasSuccessResponseTrait && hasFailedResponseTrait)) { | ||
| "A service can't have both the success response trait and the failed response trait." | ||
| } | ||
| } |
There was a problem hiding this comment.
Do we also need to check that hasSuccessResponseTrait || hasFailedResponseTrait (a service has exactly one of the traits)?
There was a problem hiding this comment.
No because the trait is only used for the E2E tests. During code generation for actual real life services neither of the traits is necessary
There was a problem hiding this comment.
I am concerned about the tight coupling between this smoke tests generator and the tests for it. The generator here should not know / have to care about test-specific traits.
If we're just using these traits to configure the httpClient, can it be configured through the smoke test vendor params instead?
There was a problem hiding this comment.
Yeah, I don't like it either. I can't think of any other reasonable way to replace the httpClient without high coupling.
If we're just using these traits to configure the httpClient, can it be configured through the smoke test vendor params instead?
We would have to add some custom logic for that to work, so the coupling would still be there.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| @@ -131,5 +131,6 @@ jobs: | |||
| sed -i "s/smithy-kotlin-runtime-version = .*$/smithy-kotlin-runtime-version = \"$SMITHY_KOTLIN_RUNTIME_VERSION\"/" gradle/libs.versions.toml | |||
| sed -i "s/smithy-kotlin-codegen-version = .*$/smithy-kotlin-codegen-version = \"$SMITHY_KOTLIN_CODEGEN_VERSION\"/" gradle/libs.versions.toml | |||
| ./gradlew --parallel publishToMavenLocal | |||
| ./gradlew build | |||
There was a problem hiding this comment.
What is the reason for adding ./gradlew build here? Don't the test tasks depend on build already?
There was a problem hiding this comment.
I think so, the build task depends on the stageServices task but it doesn't seem to be running when the build task is being called by the test task.
I added a dependency on stageServices to the test tasks and that should allow us to get rid of this ^
This comment has been minimized.
This comment has been minimized.
| @@ -198,6 +198,8 @@ class KotlinWriter( | |||
| ) | |||
| } | |||
|
|
|||
| fun emptyLine(): KotlinWriter = this.write("") | |||
There was a problem hiding this comment.
Nit: This method doesn't create an empty line necessarily—it merely guarantees a new line. For instance, the following code would result in no empty lines:
writer.writeInline("foo")
writer.emptyLine()Applies in GradleGenerator too.
| /** | ||
| * Indicates the annotated service should always return a failed response. | ||
| */ | ||
| class FailedResponseTrait(node: ObjectNode) : AnnotationTrait(ID, node) { | ||
| companion object { | ||
| val ID: ShapeId = ShapeId.from("smithy.kotlin.traits#failedResponseTrait") | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Indicates the annotated service should always return a success response. | ||
| */ | ||
| class SuccessResponseTrait(node: ObjectNode) : AnnotationTrait(ID, node) { | ||
| companion object { | ||
| val ID: ShapeId = ShapeId.from("smithy.kotlin.traits#successResponseTrait") | ||
| } | ||
| } |
There was a problem hiding this comment.
Question: What attaches these traits to shapes? I can see where we check for them but not where we create/apply them...
More broadly, why do we even need these traits? I was under the impression that individual smoke test cases identified whether they should fail/succeed based on an Expectation union.
There was a problem hiding this comment.
The traits are applied here to either of those models for testing purposes
|
|
||
| fun emptyLine(): GradleWriter = this.write("") |
There was a problem hiding this comment.
Nit: If we need this functionality in multiple code writers and its implementation is identical then it seems like we could instead write a single extension method for AbstractCodeWriter.
| delegator.useFileWriter( | ||
| "SmokeTests.kt", | ||
| "${ctx.settings.pkg.name}.smoketests", | ||
| "./jvm-src/test/java/", |
There was a problem hiding this comment.
Why is generating JVM-only code necessary? Can't we generate common code to run the tests?
|
|
||
| internal fun render() { | ||
| writer.declareSection(SmokeTestsRunner) { | ||
| writer.write("import kotlin.system.exitProcess") |
There was a problem hiding this comment.
Nit: Generally do not codegen imports directly. Favor #T placeholders and type references, which automatically add imports as appropriate.
| is ObjectNode -> buildString { | ||
| append("mapOf(") | ||
| stringMap.forEach { (key, value) -> | ||
| append("\t${key.dq()} to ${value.format()}") | ||
| } | ||
| append(")") | ||
| } |
There was a problem hiding this comment.
Correctness: This doesn't seem to add commas between subsequent key-value pairs. I'm not sure why the tabs (\t) are there either but I believe this could be simplified down to a joinToString call similar to the one above:
is ObjectNode -> stringMap.entries().joinToString(", ", "mapOf(", ")") { (key, value) ->
"${key.dq()} to ${value.format()}"
}| /** | ||
| * Syntactic sugar for getting a services operations | ||
| */ | ||
| @SmithyInternalApi | ||
| fun Model.operations(service: ShapeId): Set<OperationShape> { |
There was a problem hiding this comment.
Question: Why are we marking a Kotlin-specific extension method as "Smithy internal"?
| /** | ||
| * Syntactic sugar for getting a services operations | ||
| */ | ||
| @SmithyInternalApi | ||
| fun Model.operations(service: ShapeId): Set<OperationShape> { |
There was a problem hiding this comment.
Nit: I think this is confusingly named. Model already has a member getOperationShapes which returns all operations in the model, not just the ones found by TopDownIndex. If we decide to keep this we should name it something like topDownOperations.
| successCriterion | ||
| } | ||
|
|
||
| writer.write("val success = e is #L", expected) |
| /** | ||
| * Interceptor for smoke test runner clients. | ||
| * | ||
| * A passing test is not predicated on an SDK being able to parse the server response received, it’s based on the | ||
| * response’s HTTP status code. | ||
| */ | ||
| @InternalApi | ||
| public class SmokeTestsInterceptor : HttpInterceptor { | ||
| override fun readBeforeDeserialization(context: ProtocolResponseInterceptorContext<Any, HttpRequest, HttpResponse>) { | ||
| val status = context.protocolResponse.status.value | ||
| when (status) { | ||
| in 400..599 -> throw SmokeTestsFailureException() | ||
| in 200..299 -> throw SmokeTestsSuccessException() | ||
| else -> throw SmokeTestsUnexpectedException() | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Question: How does this interceptor allow parsing response error codes to determine if a particular errorId was found as expected?
There was a problem hiding this comment.
That logic is here in the smoke test runner generator
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| import kotlin.system.exitProcess | ||
|
|
||
| public actual fun exitProcess(status: Int): Nothing = exitProcess(status) | ||
| public actual fun getEnv(name: String): String? = System.getenv(name) |
There was a problem hiding this comment.
Question: This seems like platform functionality to me—getting environment variables is already something we do cross-platform and process control seems like a natural addition. Is there any reason we can't reuse PlatformProvider for env vars and add support for exiting the process with a return code?
There was a problem hiding this comment.
Thanks for calling this out, I almost reinvented the wheel here
Affected ArtifactsChanged in size
|
This reverts commit 872b457.
Issue #
Description of changes
Adds support for smoke tests trait using an integration.
Companion PR: awslabs/aws-sdk-kotlin#1388
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.