feat: smoke tests #1388
feat: smoke tests #1388
Conversation
|
A new generated diff is ready to view. |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
opinion: I don't think we need a changelog entry here in aws-sdk-kotlin, it's not customer-facing
| private var hasSmokeTests = AtomicBoolean(false) | ||
|
|
||
| // Only used to access settings - will always be true | ||
| override fun enabledForService(model: Model, settings: KotlinSettings): Boolean { | ||
| hasSmokeTests.set(model.operations(settings.service).any { it.hasTrait<SmokeTestsTrait>() }) | ||
| return super.enabledForService(model, settings) | ||
| } | ||
|
|
There was a problem hiding this comment.
You can access ctx.settings in generateGradleBuild, do you need this atomic boolean / enabledForService?
| withBlock("compilations {", "}") { | ||
| write("val main = getByName(\"main\")") | ||
| withBlock("tasks {", "}") { | ||
| withBlock("register<Jar>(\"smokeTestJar\") {", "}") { |
There was a problem hiding this comment.
Use #S instead of managing the quotes yourself
| tasks.register<Exec>("unstageServices") { | ||
| commandLine("git", "clean", "-fdx", "services") | ||
| } |
There was a problem hiding this comment.
Correctness: This is an unsafe command to use in a cleaning task because it cleans too much. For example, if a developer has added new files in services/ but not yet staged them, this will remove those files irretrievably.
As far as I can see, removing generated code as part of gg clean hasn't been necessary in any of our previous codegen. Why is it necessary now?
There was a problem hiding this comment.
Oh I see, this task only cleans <sdk root dir>/tests/codegen/smoke-tests/services. That's the directory where the code-generated services for the E2E tests go. It doesn't clean the <sdk root dir>/services folder. I don't see a case where we should be writing any code there but I can see how it's unsafe. I added it as a QOL utility I guess. It's not necessary though so I can get rid of it.
| smithyKotlinPlugin { | ||
| serviceShapeId = projection.serviceShapeId | ||
| packageName = "aws.sdk.kotlin.test.smoketests" | ||
| packageVersion = "1.0" |
There was a problem hiding this comment.
Nit: We should version this properly
There was a problem hiding this comment.
With the SDK version? Is there a reason why? These projections are just for testing
There was a problem hiding this comment.
If config this is just for generated service clients from the smoke tests test suite then it doesn't matter. Disregard!
settings.gradle.kts
Outdated
| // generated services by smoke tests - test suite | ||
| file("tests/codegen/smoke-tests/services").listFiles().forEach { | ||
| if (it.isServiceDir) { | ||
| include(":tests:codegen:smoke-tests:services:${it.name}") | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Correctness: The tests/ modules are intended to test our own SDK-developed code. These smoke tests will actually be verifying model correctness, which is a slightly different scope and intent. Moreover, I believe we'll want to package these up and distribute them to test runners at some point, which is more reason for them not to live here.
Have we explored codegenning into the services/ modules directly? We don't have to include them in the main publication (so they're not included in our Maven Central artifacts) but we can create a new publication that would include them separately.
There was a problem hiding this comment.
The actual smoke tests runners are being code-generated directly into the services module. The path is something like: <sdk root dir>/services/<relevant service>/generated-src-jvm/test/java/aws/sdk/kotlin/services/<relevant service>/smoketest/SmokeTests.kt. This block of code is for including the code-generated services that are meant for E2E testing into the project.
There was a problem hiding this comment.
Oh I think I must've gotten confused by the word "services". So /tests/codegen/smoke-tests/services contains generated clients for the services defined in smoke-tests-failure.smithy and smoke-tests-success.smithy? Our other tests/ projects generate those into a build directory so they get cleaned automatically, don't get checked-in, etc. It also means that the set of loaded Gradle subprojects isn't changing dynamically.
For example, our event streams test project codegens the client, model, and serde files into tests/codegen/event-stream/build/smithyprojections/event-stream/restJson1/kotlin-codegen.
Can we do something similar here?
There was a problem hiding this comment.
So /tests/codegen/smoke-tests/services contains generated clients for the services defined in smoke-tests-failure.smithy and smoke-tests-success.smithy?
Yeah, that's right
For example, our event streams test project codegens the client, model, and serde files into tests/codegen/event-stream/build/smithyprojections/event-stream/restJson1/kotlin-codegen.
Can we do something similar here?
I don't think we can because the generated test services need to be inside a gradle project so we can run the gg smokeTest task. I'm not sure that we can run a gradle task from the build directory.
There was a problem hiding this comment.
Our other codegen tests also need to be run after generating them. The event streams tests generate sources via our Smithy build plugin (which go into the build dir) and then add each of the generated client roots to the Kotlin source. Then, statically written tests can invoke the generated code.
It seems to me like a similar setup could work for the smoke tests: use Smithy build plugin (may require new options/tweaks to support smoke test codegen) to generate tests into the build dir and then invoke those tests in statically-written code.
One of my concerns here is repo hygene and consistency. We had to add new .gitignore entries for this which seems wrong. Maybe it's not and this is the best option but I'd like to at least verify this can't be done in a manner more similar to other codegenned tests.
| class SmokeTestE2ETest { | ||
| @Test | ||
| fun successService() { | ||
| val smokeTestRunnerOutput = runSmokeTests("successService") | ||
|
|
||
| assertContains(smokeTestRunnerOutput, "ok SuccessService SuccessTest - no error expected from service") | ||
| assertContains(smokeTestRunnerOutput, "ok SuccessService SuccessTestWithTags - no error expected from service") | ||
| } |
There was a problem hiding this comment.
Question: Are any of these tests SDK specific? Could they live in smithy-kotlin instead?
There was a problem hiding this comment.
They're sort of SDK specific. The tests and infra around them mock the setup we have for copying directories from the smithy projections into the dedicated services folders. So mocking <sdk root dir>/services/<relevant service> but in <sdk root dir>/tests/codegen/smoke-tests/services/<relevant service> for our tests. This includes the gg smokeTest task that I created for the SDK repo and that is not available in smithy kotlin.
They also test the actual runner which doesn't need to be tested in the SDK but it's a lot easier to test here rather than in smithy kotlin. If we move the tests to smithy kotlin we would lose a lot of test coverage around all this SDK specific stuff I mentioned.
There was a problem hiding this comment.
the setup we have for copying directories from the smithy projections into the dedicated services folders
Can you elaborate more? Is this setup defined in our smoke test generators or our manually-maintained build files?
There was a problem hiding this comment.
The smoke tests integration from smithy kotlin writes the smoke test runner file to <relevant service smithy projection dir>/jvm-src/test/java/ and then the stageSdks task that we created moves the whole jvm-src directory into the <sdk root dir>/services/<relevant service>/generated-src-jvm directory.
This is also done for the regular service code generated code. I'm not sure where <relevant service smithy projection dir>/src is created but you can see where it is copied over to <sdk root dir>/services/<relevant service>/generated-src in the stageSdks task.
| @trait(selector: "service") | ||
| structure failedResponseTrait { } | ||
|
|
||
| @failedResponseTrait | ||
| @awsJson1_0 | ||
| @service(sdkId: "Failure") | ||
| @endpointRuleSet( | ||
| version: "1.0", | ||
| parameters: {}, | ||
| rules: [ | ||
| { | ||
| "type": "endpoint", | ||
| "conditions": [], | ||
| "endpoint": { | ||
| "url": "https://static.endpoint" | ||
| } | ||
| } | ||
| ] | ||
| ) | ||
| service FailureService { | ||
| version: "1.0.0", | ||
| operations: [ TestOperation ], | ||
| } |
There was a problem hiding this comment.
Question: Were these model files copied or adapted from somewhere? If so, let's include a comment with a link back to them in case we ever need to refresh them. (If you wrote these from scratch then never mind!)
There was a problem hiding this comment.
These were very loosely based on this example. I don't know if we need to keep up to date with it though, they're only very loosely based on the example. If something changes in the smithy syntax, then the E2E tests would probably break anyway. Do you feel like we should link that example? And do you have a reason in mind for why we should?
There was a problem hiding this comment.
No, we only need a link if it's something we think should be kept up-to-date with some source. Seems like that's not necessary here so never mind.
| private fun generateSmokeTestConfig(writer: GradleWriter, sdkId: String, ctx: CodegenContext) { | ||
| val formattedDenyList = smokeTestDenyList.joinToString(",", "setOf(", ")") { it.dq() } | ||
| writer.withBlock("if (#S !in #L) {", "}", sdkId, formattedDenyList) { | ||
| generateSmokeTestJarTask(writer, ctx) | ||
| emptyLine() | ||
| generateSmokeTestTask(writer, ctx) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
We have the sdkId and the smokeTestDenyList at codegen time, can't we skip generating a smoke test here in codegen instead of doing a runtime check?
| val hasSuccessResponseTrait = ctx.model.expectShape<ServiceShape>(ctx.settings.service).hasTrait(SuccessResponseTrait.ID) | ||
| val hasFailedResponseTrait = ctx.model.expectShape<ServiceShape>(ctx.settings.service).hasTrait(FailedResponseTrait.ID) | ||
|
|
||
| // E2E tests don't have sdkVersion in jar names |
There was a problem hiding this comment.
Can you describe this more? Why is there a difference if we're in a testing environment or not?
There was a problem hiding this comment.
The regular environment would be <sdk root dir>/services/<relevant service>/build/libs that's where the JARs are. I noticed that even though I'm setting the JAR name for the smoke test runner to <relevant service>-smoketestshere, something in the SDK would later add the SDK version to the end of it. So it would turn the JAR name into <relevant service>-smoketests-<SDK version>.
I didn't find what's doing that but it's not happening in the E2E test environment. I tried replicating it with gradle tasks but I had a really hard time trying to do so. Do you know about anything that might be adding the SDK version to the JAR names?
| /** | ||
| * SDK ID's of services that model smoke tests incorrectly | ||
| */ |
There was a problem hiding this comment.
Were you able to open issues with the services / Smithy team to track the deny list? This will likely help other SDKs that implement this in the coming months
There was a problem hiding this comment.
I still need to writes these up but I'll make sure to do so before I'm done with this task
0marperez
added
the
no-changelog
|
A new generated diff is ready to view. |
|
A new generated diff is ready to view. |
|
A new generated diff is ready to view. |
|
A new generated diff is ready to view. |
|
A new generated diff is ready to view. |
|
A new generated diff is ready to view. |
|
A new generated diff is ready to view. |
|
A new generated diff is ready to view. |
This comment has been minimized.
This comment has been minimized.
| write("dependsOn(build)") | ||
| write("mustRunAfter(build)") |
There was a problem hiding this comment.
dependsOn and mustRunAfter seem redundant, why do we need both?
There was a problem hiding this comment.
mustRunAfter only tells gradle that if both tasks are being run then one must run after the other. But it doesn't force both tasks to run. That's what dependsOn is for. Not very intuitive imo.
For each supplied task, this action adds a task 'ordering', and does not specify a 'dependency' between the tasks.
https://docs.gradle.org/8.5/kotlin-dsl/gradle/org.gradle.api/-task/must-run-after.html
There was a problem hiding this comment.
And when running a task with the parallel flag , dependsOn only guarantees that the task that is depended on starts first but not that it ends first. So mustRunAfter fixes that
There was a problem hiding this comment.
And when running a task with the parallel flag , dependsOn only guarantees that the task that is depended on starts first but not that it ends first.
That's kind of surprising. Is that documented somewhere? Or were you seeing errors in the build without this ordering constraint?
There was a problem hiding this comment.
I don't think it's documented anywhere but I was seeing errors in the build without the ordering constraint.
|
A new generated diff is ready to view. |
This comment has been minimized.
This comment has been minimized.
| write("dependsOn(build)") | ||
| write("mustRunAfter(build)") |
There was a problem hiding this comment.
And when running a task with the parallel flag , dependsOn only guarantees that the task that is depended on starts first but not that it ends first.
That's kind of surprising. Is that documented somewhere? Or were you seeing errors in the build without this ordering constraint?
|
A new generated diff is ready to view. |
|
A new generated diff is ready to view. |
This comment has been minimized.
This comment has been minimized.
| /** | ||
| * SDK ID's of services that model smoke tests incorrectly | ||
| */ | ||
| val smokeTestDenyList = setOf( | ||
| "Application Auto Scaling", | ||
| "SWF", | ||
| "WAFV2", | ||
| ) |
There was a problem hiding this comment.
Question: Do we have issues which track fixing these in the models? If so, let's link them here.
| /** | ||
| * 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.
Nit: Now that you've clarified how these are used by our codegen tests only, I think they should be renamed and documented appropriately to reduce future confusion—some wording that clearly describes these relate to locally-run tests only (vs real smoke tests run against a service endpoint).
It'd be ideal if things like our HTTP client overrides could be driven solely off of expectations already defined in the smoke test trait in addition to the code generator knowing that it was being invoked for creating E2E tests (e.g., by a smithyKotlinPlugin config setting). But if that's not possible or not easy then let's at least clarify these synthetic traits a little bit.
| /** | ||
| * Adds [FailedResponseTrait] support to smoke tests | ||
| */ | ||
| class SmokeTestFailHttpEngineIntegration : KotlinIntegration { |
There was a problem hiding this comment.
Nit: Same comment about naming relative to purpose. I wonder if this integration could be combined with SmokeTestSuccessHttpEngineIntegration and named something like SmokeTestLocalOnlyIntegration.
| class SmokeTestFailHttpEngineIntegration : KotlinIntegration { | ||
| override fun enabledForService(model: Model, settings: KotlinSettings): Boolean = | ||
| model.topDownOperations(settings.service).any { it.hasTrait<SmokeTestsTrait>() } && | ||
| settings.sdkId !in smokeTestDenyList && |
There was a problem hiding this comment.
Question: Is checking against the denylist required here? Will the plugin even activate for a service in the denylist?
There was a problem hiding this comment.
It's not required, I ended up removing this check
| override val sectionWriters: List<SectionWriterBinding> | ||
| get() = listOf( | ||
| SectionWriterBinding(SmokeTestAdditionalEnvVars, envVars), | ||
| SectionWriterBinding(SmokeTestDefaultConfig, region), | ||
| SectionWriterBinding(SmokeTestRegionDefault, regionDefault), | ||
| ) | ||
|
|
||
| private val envVars = SectionWriter { writer, _ -> | ||
| writer.write( | ||
| "private val regionOverride = #T(#S)", | ||
| RuntimeTypes.Core.SmokeTests.getEnv, | ||
| "AWS_SMOKE_TEST_REGION", | ||
| ) | ||
| } | ||
|
|
||
| private val region = SectionWriter { writer, _ -> | ||
| writer.write("region = regionOverride") | ||
| } | ||
|
|
||
| private val regionDefault = SectionWriter { writer, _ -> | ||
| writer.write("regionOverride ?:") | ||
| } |
There was a problem hiding this comment.
Fun fact: you can also include writer code inline with the SectionWriterBinding:
override val sectionWriters: List<SectionWriterBinding>
get() = listOf(
SectionWriterBinding(SmokeTestAdditionalEnvVars, envVars),
SectionWriterBinding(SmokeTestDefaultConfig) { w, _ -> w.write("region = regionOverride") },
SectionWriterBinding(SmokeTestRegionDefault) { w, _ -> w.write("regionOverride ?:") },
)That doesn't always make the code cleaner and I'm not asking you to change it if you're already happy with it, just an FYI.
|
|
A new generated diff is ready to view. |
Affected ArtifactsChanged in size
|
This reverts commit 5715cbc.
Issue #
Description of changes
Adds gradle support for smoke tests
Companion PR: smithy-lang/smithy-kotlin#1141
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.