-
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 5 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 |
---|---|---|
@@ -0,0 +1,5 @@ | ||
{ | ||
"id": "756754c3-f6e1-4ff2-ae31-08b3b67b6750", | ||
"type": "feature", | ||
"description": "Add support for smoke tests" | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -198,6 +198,8 @@ class KotlinWriter( | |
) | ||
} | ||
|
||
fun emptyLine(): KotlinWriter = this.write("") | ||
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: 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 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: Also 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'll get rid of |
||
|
||
/** | ||
* Clean/escape any content from the doc that would invalidate the Kotlin output. | ||
*/ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -250,4 +250,6 @@ class GradleWriter(parent: GradleWriter? = null) : AbstractCodeWriter<GradleWrit | |
expressionStart = parent?.expressionStart ?: '#' | ||
putFormatter('W', InlineCodeWriterFormatter(::GradleWriter)) | ||
} | ||
|
||
fun emptyLine(): GradleWriter = this.write("") | ||
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: 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 |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
package software.amazon.smithy.kotlin.codegen.rendering.smoketests | ||
|
||
import software.amazon.smithy.kotlin.codegen.KotlinSettings | ||
import software.amazon.smithy.kotlin.codegen.core.CodegenContext | ||
import software.amazon.smithy.kotlin.codegen.core.KotlinDelegator | ||
import software.amazon.smithy.kotlin.codegen.integration.KotlinIntegration | ||
import software.amazon.smithy.kotlin.codegen.model.hasTrait | ||
import software.amazon.smithy.kotlin.codegen.utils.operations | ||
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. | ||
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: "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) | ||
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. simplification for second half of the |
||
|
||
override fun writeAdditionalFiles(ctx: CodegenContext, delegator: KotlinDelegator) = | ||
delegator.useFileWriter("SmokeTests.kt", "smoketests", "./jvm-src/main/java/") { writer -> | ||
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. correctness: package ideally should be |
||
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, | ||
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 see |
||
).render() | ||
} | ||
} | ||
|
||
/** | ||
* 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you include GitHub issues (if they exist) filed against these services for them to fix their smoke 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. Also, this should be defined in aws-sdk-kotlin |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,174 @@ | ||
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.model.getTrait | ||
import software.amazon.smithy.kotlin.codegen.rendering.util.render | ||
import software.amazon.smithy.kotlin.codegen.utils.dq | ||
import software.amazon.smithy.kotlin.codegen.utils.getOrNull | ||
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.smoketests.traits.SmokeTestCase | ||
import software.amazon.smithy.smoketests.traits.SmokeTestsTrait | ||
import kotlin.jvm.optionals.getOrNull | ||
|
||
/** | ||
* 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, | ||
) { | ||
internal fun render() { | ||
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. You should not need to import anything yourself, using 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. I had some issues here doing that. The |
||
writer.emptyLine() | ||
writer.write("private var exitCode = 0") | ||
writer.write("private val regionOverride = System.getenv(\"AWS_SMOKE_TEST_REGION\")") | ||
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. suggestion: Use |
||
writer.write("private val skipTags = System.getenv(\"AWS_SMOKE_TEST_SKIP_TAGS\")?.let { it.split(\",\").map { it.trim() }.toSet() }") | ||
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 missing |
||
writer.emptyLine() | ||
writer.withBlock("public suspend fun main() {", "}") { | ||
renderFunctionCalls() | ||
write("exitProcess(exitCode)") | ||
} | ||
writer.emptyLine() | ||
renderFunctions() | ||
} | ||
|
||
private fun renderFunctionCalls() { | ||
operations.forEach { operation -> | ||
operation.getTrait<SmokeTestsTrait>()?.testCases?.forEach { testCase -> | ||
writer.write("${testCase.id.toCamelCase()}()") | ||
} | ||
} | ||
} | ||
|
||
private fun renderFunctions() { | ||
operations.forEach { operation -> | ||
operation.getTrait<SmokeTestsTrait>()?.testCases?.forEach { testCase -> | ||
renderFunction(operation, testCase) | ||
writer.emptyLine() | ||
} | ||
} | ||
} | ||
|
||
private fun renderFunction(operation: OperationShape, testCase: SmokeTestCase) { | ||
writer.withBlock("private suspend fun ${testCase.id.toCamelCase()}() {", "}") { | ||
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. suggestion: I see |
||
write("val tags = setOf<String>(${testCase.tags.joinToString(",") { it.dq()} })") | ||
write("if (skipTags != null && tags.any { it in skipTags }) return") | ||
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. You could make this always non-null when you create the set. It can be an empty set by default |
||
emptyLine() | ||
withInlineBlock("try {", "} ") { | ||
renderClient(testCase) | ||
renderOperation(operation, testCase) | ||
} | ||
withBlock("catch (e: Exception) {", "}") { | ||
renderCatchBlock(testCase) | ||
} | ||
} | ||
} | ||
|
||
private fun renderClient(testCase: SmokeTestCase) { | ||
writer.withInlineBlock("#L {", "}", service) { | ||
if (testCase.vendorParams.isPresent) { | ||
testCase.vendorParams.get().members.forEach { vendorParam -> | ||
if (vendorParam.key.value == "region") { | ||
write("region = regionOverride ?: #L", vendorParam.value.render()) | ||
} else { | ||
write("#L = #L", vendorParam.key.value.toCamelCase(), vendorParam.value.render()) | ||
} | ||
} | ||
} 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 |
||
} | ||
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: |
||
val expectingSpecificError = testCase.expectation.failure.getOrNull()?.errorId?.getOrNull() != null | ||
write("interceptors.add(#T($expectingSpecificError))", RuntimeTypes.HttpClient.Interceptors.SmokeTestsInterceptor) | ||
} | ||
checkVendorParamsAreCorrect(testCase) | ||
} | ||
|
||
/** | ||
* Smithy IDL doesn't check that vendor params are found in vendor params shape so we have to check here. | ||
*/ | ||
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. Should this be considered an "Invalid model" and added to the deny list? We shouldn't have to be checking this ourselves |
||
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}\"" | ||
} | ||
} | ||
} | ||
} | ||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Avoid manually adding imports if possible |
||
writer.withBlock(".use { client ->", "}") { | ||
withBlock("client.#L(", ")", operation.defaultName()) { | ||
withBlock("#L {", "}", operationSymbol.name) { | ||
testCase.params.get().members.forEach { member -> | ||
write("#L = #L", member.key.value.toCamelCase(), member.value.render()) | ||
} | ||
} | ||
} | ||
} | ||
} | ||
|
||
private fun renderCatchBlock(testCase: SmokeTestCase) { | ||
val successCriterion = RuntimeTypes.HttpClient.Interceptors.SmokeTestsSuccessException | ||
val failureCriterion = getFailureCriterion(testCase) | ||
val expected = if (testCase.expectation.isFailure) { | ||
failureCriterion | ||
} else { | ||
successCriterion | ||
} | ||
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: val expected = if (testCase.expectation.isFailure) {
getFailureCriterion(testCase)
} else {
RuntimeTypes.HttpClient.Interceptors.SmokeTestsSuccessException
} |
||
|
||
writer.write("val success = e is #L", expected) | ||
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: Use |
||
writer.write("val status = if (success) \"ok\" else \"not ok\"") | ||
printTestResult( | ||
sdkId.filter { !it.isWhitespace() }, | ||
testCase.id, | ||
testCase.expectation.isFailure, | ||
writer, | ||
) | ||
writer.write("if (!success) exitCode = 1") | ||
} | ||
|
||
/** | ||
* 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. | ||
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 think these should be considered invalid models and added to the deny list until fixed |
||
*/ | ||
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 | ||
|
||
/** | ||
* Renders print statement for smoke test result in accordance to design doc & test anything protocol (TAP) | ||
*/ | ||
private fun printTestResult( | ||
service: String, | ||
testCase: String, | ||
errorExpected: Boolean, | ||
writer: KotlinWriter, | ||
) { | ||
val expectation = if (errorExpected) "error expected from service" else "no error expected from service" | ||
val testResult = "\$status $service $testCase - $expectation" | ||
writer.write("println(#S)", testResult) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
package software.amazon.smithy.kotlin.codegen.rendering.util | ||
|
||
import software.amazon.smithy.kotlin.codegen.utils.dq | ||
import software.amazon.smithy.model.node.ArrayNode | ||
import software.amazon.smithy.model.node.BooleanNode | ||
import software.amazon.smithy.model.node.Node | ||
import software.amazon.smithy.model.node.NullNode | ||
import software.amazon.smithy.model.node.NumberNode | ||
import software.amazon.smithy.model.node.ObjectNode | ||
import software.amazon.smithy.model.node.StringNode | ||
|
||
/** | ||
* Renders a [Node] into String format for codegen. | ||
*/ | ||
fun Node.render(): String = when (this) { | ||
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. naming: render usually means you are making a |
||
is NullNode -> "null" | ||
is StringNode -> value.dq() | ||
is BooleanNode -> value.toString() | ||
is NumberNode -> value.toString() | ||
is ArrayNode -> elements.joinToString(",", "listOf(", ")") { element -> | ||
element.render() | ||
} | ||
is ObjectNode -> buildString { | ||
append("mapOf(") | ||
stringMap.forEach { (key, value) -> | ||
append("\t${key.dq()} to ${value.render()}") | ||
} | ||
append(")") | ||
} | ||
else -> throw Exception("Unexpected node type: $this") | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
package software.amazon.smithy.kotlin.codegen.utils | ||
|
||
import software.amazon.smithy.model.Model | ||
import software.amazon.smithy.model.knowledge.TopDownIndex | ||
import software.amazon.smithy.model.shapes.OperationShape | ||
import software.amazon.smithy.model.shapes.ShapeId | ||
import software.amazon.smithy.utils.SmithyInternalApi | ||
|
||
/** | ||
* Syntactic sugar for getting a services operations | ||
*/ | ||
@SmithyInternalApi | ||
fun Model.operations(service: ShapeId): Set<OperationShape> { | ||
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. suggestion: Make this an extension property ( 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 are we marking a Kotlin-specific extension method as "Smithy internal"? 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: I think this is confusingly named. |
||
val topDownIndex = TopDownIndex.of(this) | ||
return topDownIndex.getContainedOperations(service) | ||
} |
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.
nit: refer to Smithy smoke tests trait / link to Smithy docs: https://smithy.io/2.0/additional-specs/smoke-tests.html