-
Notifications
You must be signed in to change notification settings - Fork 26
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: smoke tests trait #1141
feat: smoke tests trait #1141
Changes from 2 commits
0ffb5a5
aef03ac
3f74489
fc97ab9
f152351
f051f67
4e58bbd
39f66de
faa8652
524c8ca
bcafd53
5a91c98
915e845
1055f1b
bd0072c
c93d8a6
4a10ec7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,5 @@ | ||
{ | ||
"id": "756754c3-f6e1-4ff2-ae31-08b3b67b6750", | ||
"type": "feature", | ||
"description": "Add support for smoke tests" | ||
"description": "Add support for [smoke tests](https://smithy.io/2.0/additional-specs/smoke-tests.html)" | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
package software.amazon.smithy.kotlin.codegen.model.traits | ||
|
||
import software.amazon.smithy.model.node.ObjectNode | ||
import software.amazon.smithy.model.shapes.ShapeId | ||
import software.amazon.smithy.model.traits.AnnotationTrait | ||
|
||
/** | ||
* Indicates the annotated service should always return a failed response. | ||
*/ | ||
class FailedResponseTrait(node: ObjectNode) : AnnotationTrait(ID, node) { | ||
companion object { | ||
val ID: ShapeId = ShapeId.from("com.test#failedResponseTrait") | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
package software.amazon.smithy.kotlin.codegen.model.traits | ||
|
||
import software.amazon.smithy.model.node.ObjectNode | ||
import software.amazon.smithy.model.shapes.ShapeId | ||
import software.amazon.smithy.model.traits.AnnotationTrait | ||
|
||
/** | ||
* Indicates the annotated service should always return a success response. | ||
*/ | ||
class SuccessResponseTrait(node: ObjectNode) : AnnotationTrait(ID, node) { | ||
companion object { | ||
val ID: ShapeId = ShapeId.from("com.test#successResponseTrait") | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,30 +10,21 @@ import software.amazon.smithy.model.Model | |
import software.amazon.smithy.smoketests.traits.SmokeTestsTrait | ||
|
||
/** | ||
* Renders smoke test runner for a service if any of the operations has the smoke test trait. | ||
* Renders smoke test runner for a service if any of the operations have the [SmokeTestsTrait]. | ||
*/ | ||
class SmokeTestsIntegration : KotlinIntegration { | ||
override fun enabledForService(model: Model, settings: KotlinSettings): Boolean = | ||
model.operations(settings.service).any { it.hasTrait<SmokeTestsTrait>() } && !smokeTestDenyList.contains(settings.sdkId) | ||
model.operations(settings.service).any { it.hasTrait<SmokeTestsTrait>() } | ||
|
||
override fun writeAdditionalFiles(ctx: CodegenContext, delegator: KotlinDelegator) = | ||
delegator.useFileWriter("SmokeTests.kt", "smoketests", "./jvm-src/main/java/") { writer -> | ||
delegator.useFileWriter( | ||
"SmokeTests.kt", | ||
"${ctx.settings.pkg.name}.smoketests", | ||
"./jvm-src/test/java/", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, I was trying to follow the convention we have currently without affecting the SDK. I believe we currently code-generate services into a 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is generating JVM-only code necessary? Can't we generate common code to run the tests? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Main reason is that there's no common code for I think I can wrap this in our own KMP function, thoughts? |
||
) { writer -> | ||
SmokeTestsRunnerGenerator( | ||
writer, | ||
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, | ||
ctx, | ||
).render() | ||
} | ||
} | ||
|
||
/** | ||
* SDK ID's of services that model smoke tests incorrectly | ||
*/ | ||
val smokeTestDenyList = setOf( | ||
"Application Auto Scaling", | ||
"SWF", | ||
"WAFV2", | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,49 +1,74 @@ | ||
package software.amazon.smithy.kotlin.codegen.rendering.smoketests | ||
|
||
import software.amazon.smithy.codegen.core.Symbol | ||
import software.amazon.smithy.codegen.core.SymbolProvider | ||
import software.amazon.smithy.kotlin.codegen.core.* | ||
import software.amazon.smithy.kotlin.codegen.integration.SectionId | ||
import software.amazon.smithy.kotlin.codegen.model.expectShape | ||
import software.amazon.smithy.kotlin.codegen.model.getTrait | ||
import software.amazon.smithy.kotlin.codegen.rendering.util.render | ||
import software.amazon.smithy.kotlin.codegen.model.hasTrait | ||
import software.amazon.smithy.kotlin.codegen.model.traits.FailedResponseTrait | ||
import software.amazon.smithy.kotlin.codegen.model.traits.SuccessResponseTrait | ||
import software.amazon.smithy.kotlin.codegen.rendering.util.format | ||
import software.amazon.smithy.kotlin.codegen.utils.dq | ||
import software.amazon.smithy.kotlin.codegen.utils.getOrNull | ||
import software.amazon.smithy.kotlin.codegen.utils.operations | ||
import software.amazon.smithy.kotlin.codegen.utils.toCamelCase | ||
import software.amazon.smithy.model.Model | ||
import software.amazon.smithy.model.shapes.OperationShape | ||
import software.amazon.smithy.model.shapes.ServiceShape | ||
import software.amazon.smithy.smoketests.traits.SmokeTestCase | ||
import software.amazon.smithy.smoketests.traits.SmokeTestsTrait | ||
import kotlin.jvm.optionals.getOrNull | ||
|
||
object SmokeTestsRunner : SectionId | ||
|
||
// Env var constants | ||
const val SKIP_TAGS = "AWS_SMOKE_TEST_SKIP_TAGS" | ||
const val SERVICE_FILTER = "AWS_SMOKE_TEST_SERVICE_IDS" | ||
|
||
/** | ||
* Renders smoke tests runner for a service | ||
*/ | ||
class SmokeTestsRunnerGenerator( | ||
private val writer: KotlinWriter, | ||
private val service: Symbol, | ||
private val operations: List<OperationShape>, | ||
private val model: Model, | ||
private val symbolProvider: SymbolProvider, | ||
private val sdkId: String, | ||
ctx: CodegenContext, | ||
) { | ||
// Generator config | ||
private val model = ctx.model | ||
private val sdkId = ctx.settings.sdkId | ||
private val symbolProvider = ctx.symbolProvider | ||
private val service = symbolProvider.toSymbol(model.expectShape(ctx.settings.service)) | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Generally I prefer to just grab everything from the
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we also need to check that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I don't like it either. I can't think of any other reasonable way to replace the
We would have to add some custom logic for that to work, so the coupling would still be there. |
||
|
||
internal fun render() { | ||
writer.write("import kotlin.system.exitProcess") | ||
writer.emptyLine() | ||
writer.write("private var exitCode = 0") | ||
writer.write("private val regionOverride = System.getenv(\"AWS_SMOKE_TEST_REGION\")") | ||
writer.write("private val skipTags = System.getenv(\"AWS_SMOKE_TEST_SKIP_TAGS\")?.let { it.split(\",\").map { it.trim() }.toSet() }") | ||
writer.emptyLine() | ||
writer.withBlock("public suspend fun main() {", "}") { | ||
renderFunctionCalls() | ||
write("exitProcess(exitCode)") | ||
writer.declareSection(SmokeTestsRunner) { | ||
writer.write("import kotlin.system.exitProcess") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: Generally do not codegen imports directly. Favor |
||
writer.emptyLine() | ||
writer.write("private var exitCode = 0") | ||
writer.write("private val regionOverride = System.getenv(#S)", "AWS_SMOKE_TEST_REGION") | ||
writer.write("private val skipTags = System.getenv(#S)?.let { it.split(\",\").map { it.trim() }.toSet() } ?: emptySet()", SKIP_TAGS) | ||
writer.write("private val serviceFilter = System.getenv(#S)?.let { it.split(\",\").map { it.trim() }.toSet() }", SERVICE_FILTER) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: Replace There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Question: Why not default There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's important to know if the user didn't set this because this is an include filter. It the default is an empty set then no smoke tests will run. We could change the default to an empty set and then check if it's that instead of null |
||
writer.emptyLine() | ||
writer.withBlock("public suspend fun main() {", "}") { | ||
renderFunctionCalls() | ||
write("exitProcess(exitCode)") | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Question: What's the reason to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is part of the design document, we're supposed to return a non-zero exit code when any of the tests don't pass. Just returning the code doesn't set the exit code AFAIK. |
||
writer.emptyLine() | ||
renderFunctions() | ||
} | ||
writer.emptyLine() | ||
renderFunctions() | ||
} | ||
|
||
private fun renderFunctionCalls() { | ||
operations.forEach { operation -> | ||
operation.getTrait<SmokeTestsTrait>()?.testCases?.forEach { testCase -> | ||
writer.write("${testCase.id.toCamelCase()}()") | ||
writer.write("${testCase.functionName}()") | ||
} | ||
} | ||
} | ||
|
@@ -58,9 +83,19 @@ class SmokeTestsRunnerGenerator( | |
} | ||
|
||
private fun renderFunction(operation: OperationShape, testCase: SmokeTestCase) { | ||
writer.withBlock("private suspend fun ${testCase.id.toCamelCase()}() {", "}") { | ||
writer.withBlock("private suspend fun ${testCase.functionName}() {", "}") { | ||
write("val tags = setOf<String>(${testCase.tags.joinToString(",") { it.dq()} })") | ||
write("if (skipTags != null && tags.any { it in skipTags }) return") | ||
writer.withBlock("if ((serviceFilter != null && #S !in serviceFilter) || tags.any { it in skipTags }) {", "}", sdkId) { | ||
printTestResult( | ||
sdkId.filter { !it.isWhitespace() }, | ||
testCase.id, | ||
testCase.expectation.isFailure, | ||
writer, | ||
"ok", | ||
"# skip", | ||
) | ||
writer.write("return") | ||
} | ||
emptyLine() | ||
withInlineBlock("try {", "} ") { | ||
renderClient(testCase) | ||
|
@@ -74,35 +109,40 @@ class SmokeTestsRunnerGenerator( | |
|
||
private fun renderClient(testCase: SmokeTestCase) { | ||
writer.withInlineBlock("#L {", "}", service) { | ||
// Client config | ||
if (testCase.vendorParams.isPresent) { | ||
testCase.vendorParams.get().members.forEach { vendorParam -> | ||
if (vendorParam.key.value == "region") { | ||
write("region = regionOverride ?: #L", vendorParam.value.render()) | ||
write("region = regionOverride ?: #L", vendorParam.value.format()) | ||
} else { | ||
write("#L = #L", vendorParam.key.value.toCamelCase(), vendorParam.value.render()) | ||
write("#L = #L", vendorParam.key.value.toCamelCase(), vendorParam.value.format()) | ||
} | ||
} | ||
} else { | ||
write("region = regionOverride") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What happens when the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In that case |
||
} | ||
val expectingSpecificError = testCase.expectation.failure.getOrNull()?.errorId?.getOrNull() != null | ||
write("interceptors.add(#T($expectingSpecificError))", RuntimeTypes.HttpClient.Interceptors.SmokeTestsInterceptor) | ||
} | ||
checkVendorParamsAreCorrect(testCase) | ||
} | ||
if (!expectingSpecificError) { | ||
write("interceptors.add(#T())", RuntimeTypes.HttpClient.Interceptors.SmokeTestsInterceptor) | ||
} | ||
|
||
/** | ||
* Smithy IDL doesn't check that vendor params are found in vendor params shape so we have to check here. | ||
*/ | ||
private fun checkVendorParamsAreCorrect(testCase: SmokeTestCase) { | ||
if (testCase.vendorParamsShape.isPresent && testCase.vendorParams.isPresent) { | ||
val vendorParamsShape = model.getShape(testCase.vendorParamsShape.get()).get() | ||
val validVendorParams = vendorParamsShape.members().map { it.memberName } | ||
val vendorParams = testCase.vendorParams.get().members.map { it.key.value } | ||
|
||
vendorParams.forEach { vendorParam -> | ||
check(validVendorParams.contains(vendorParam)) { | ||
"Smithy smoke test \"${testCase.id}\" contains invalid vendor param \"$vendorParam\", it was not found in vendor params shape \"${testCase.vendorParamsShape}\"" | ||
// Test config | ||
if (hasSuccessResponseTrait) { | ||
write("httpClient = #T()", RuntimeTypes.HttpTest.TestEngine) | ||
} | ||
if (hasFailedResponseTrait) { | ||
withBlock("httpClient = #T(", ")", RuntimeTypes.HttpTest.TestEngine) { | ||
withBlock("roundTripImpl = { _, request ->", "}") { | ||
write( | ||
"val resp = #T(#T.BadRequest, #T.Empty, #T.Empty)", | ||
RuntimeTypes.Http.Response.HttpResponse, | ||
RuntimeTypes.Http.StatusCode, | ||
RuntimeTypes.Http.Headers, | ||
RuntimeTypes.Http.HttpBody, | ||
) | ||
write("val now = #T.now()", RuntimeTypes.Core.Instant) | ||
write("#T(request, resp, now, now)", RuntimeTypes.Http.HttpCall) | ||
} | ||
} | ||
} | ||
} | ||
|
@@ -111,12 +151,11 @@ class SmokeTestsRunnerGenerator( | |
private fun renderOperation(operation: OperationShape, testCase: SmokeTestCase) { | ||
val operationSymbol = symbolProvider.toSymbol(model.getShape(operation.input.get()).get()) | ||
|
||
writer.addImport(operationSymbol) | ||
writer.withBlock(".use { client ->", "}") { | ||
withBlock("client.#L(", ")", operation.defaultName()) { | ||
withBlock("#L {", "}", operationSymbol.name) { | ||
withBlock("#L {", "}", operationSymbol) { | ||
testCase.params.get().members.forEach { member -> | ||
write("#L = #L", member.key.value.toCamelCase(), member.value.render()) | ||
write("#L = #L", member.key.value.toCamelCase(), member.value.format()) | ||
} | ||
} | ||
} | ||
|
@@ -146,17 +185,11 @@ class SmokeTestsRunnerGenerator( | |
/** | ||
* Tries to get the specific exception required in the failure criterion of a test. | ||
* If no specific exception is required we default to the generic smoke tests failure exception. | ||
* | ||
* Some smoke tests model exceptions not found in the model, in that case we default to the generic smoke tests | ||
* failure exception. | ||
*/ | ||
private fun getFailureCriterion(testCase: SmokeTestCase): Symbol = testCase.expectation.failure.getOrNull()?.errorId?.let { | ||
try { | ||
symbolProvider.toSymbol(model.getShape(it.get()).get()) | ||
} catch (e: Exception) { | ||
RuntimeTypes.HttpClient.Interceptors.SmokeTestsFailureException | ||
} | ||
} ?: RuntimeTypes.HttpClient.Interceptors.SmokeTestsFailureException | ||
private fun getFailureCriterion(testCase: SmokeTestCase): Symbol = | ||
testCase.expectation.failure.getOrNull()?.errorId?.getOrNull()?.let { | ||
symbolProvider.toSymbol(model.getShape(it).get()) | ||
} ?: RuntimeTypes.HttpClient.Interceptors.SmokeTestsFailureException | ||
|
||
/** | ||
* Renders print statement for smoke test result in accordance to design doc & test anything protocol (TAP) | ||
|
@@ -166,9 +199,18 @@ class SmokeTestsRunnerGenerator( | |
testCase: String, | ||
errorExpected: Boolean, | ||
writer: KotlinWriter, | ||
statusOverride: String? = null, | ||
directive: String? = "", | ||
) { | ||
val expectation = if (errorExpected) "error expected from service" else "no error expected from service" | ||
val testResult = "\$status $service $testCase - $expectation" | ||
val status = statusOverride ?: "\$status" | ||
val testResult = "$status $service $testCase - $expectation $directive" | ||
writer.write("println(#S)", testResult) | ||
} | ||
} | ||
|
||
/** | ||
* Derives a function name for a [SmokeTestCase] | ||
*/ | ||
private val SmokeTestCase.functionName: String | ||
get() = this.id.toCamelCase() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
naming: namespace under
smithy.kotlin.traits
like the other traitsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think
FailedResponseTrait
andSuccessResponseTrait
can better go together into a single fileSmokeTestTraits.kt