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.
| 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.
Affected ArtifactsChanged in size
|
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.