Skip to content
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

Merged
merged 17 commits into from
Sep 26, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changes/756754c3-f6e1-4ff2-ae31-08b3b67b6750.json
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"
Copy link
Contributor

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

}
1 change: 1 addition & 0 deletions codegen/smithy-kotlin-codegen/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ dependencies {
implementation(libs.smithy.aws.traits)
implementation(libs.smithy.protocol.traits)
implementation(libs.smithy.protocol.test.traits)
implementation(libs.smithy.smoke.test.traits)
implementation(libs.jsoup)

// Test dependencies
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -121,9 +121,10 @@ class KotlinDelegator(
*
* @param filename Name of the file to create.
* @param block Lambda that accepts and works with the file.
* @param sourceSetRoot Root directory for source set
*/
fun useFileWriter(filename: String, namespace: String, block: (KotlinWriter) -> Unit) {
val writer: KotlinWriter = checkoutWriter(filename, namespace)
fun useFileWriter(filename: String, namespace: String, sourceSetRoot: String = DEFAULT_SOURCE_SET_ROOT, block: (KotlinWriter) -> Unit) {
val writer: KotlinWriter = checkoutWriter(filename, namespace, sourceSetRoot)
block(writer)
}

Expand Down Expand Up @@ -205,6 +206,6 @@ internal data class GeneratedDependency(
}

fun KotlinDelegator.useFileWriter(symbol: Symbol, block: (KotlinWriter) -> Unit) =
useFileWriter("${symbol.name}.kt", symbol.namespace, block)
useFileWriter("${symbol.name}.kt", symbol.namespace, DEFAULT_SOURCE_SET_ROOT, block)

fun KotlinDelegator.applyFileWriter(symbol: Symbol, block: KotlinWriter.() -> Unit) = useFileWriter(symbol, block)
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,8 @@ class KotlinWriter(
)
}

fun emptyLine(): KotlinWriter = this.write("")
Copy link
Contributor

Choose a reason for hiding this comment

The 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 GradleGenerator too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: Also write("") is two characters shorter than emptyLine()—do we even need this method? We seem to have used the former quite a lot with no issues until now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll get rid of emptyLine()


/**
* Clean/escape any content from the doc that would invalidate the Kotlin output.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,10 @@ object RuntimeTypes {
val FlexibleChecksumsResponseInterceptor = symbol("FlexibleChecksumsResponseInterceptor")
val ResponseLengthValidationInterceptor = symbol("ResponseLengthValidationInterceptor")
val RequestCompressionInterceptor = symbol("RequestCompressionInterceptor")
val SmokeTestsInterceptor = symbol("SmokeTestsInterceptor")
val SmokeTestsFailureException = symbol("SmokeTestsFailureException")
val SmokeTestsSuccessException = symbol("SmokeTestsSuccessException")
val SmokeTestsUnexpectedException = symbol("SmokeTestsUnexpectedException")
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -250,4 +250,6 @@ class GradleWriter(parent: GradleWriter? = null) : AbstractCodeWriter<GradleWrit
expressionStart = parent?.expressionStart ?: '#'
putFormatter('W', InlineCodeWriterFormatter(::GradleWriter))
}

fun emptyLine(): GradleWriter = this.write("")
Copy link
Contributor

Choose a reason for hiding this comment

The 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 AbstractCodeWriter.

}
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.
Copy link
Contributor

Choose a reason for hiding this comment

The 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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

simplification for second half of the &&: settings.sdkId !in smokeTestDenyList


override fun writeAdditionalFiles(ctx: CodegenContext, delegator: KotlinDelegator) =
delegator.useFileWriter("SmokeTests.kt", "smoketests", "./jvm-src/main/java/") { writer ->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see ctx. repeated a lot, have you considered just passing the entire CodegenContext instead of unwrapping it here?

).render()
}
}

/**
* SDK ID's of services that model smoke tests incorrectly
*/
val smokeTestDenyList = setOf(
"Application Auto Scaling",
"SWF",
"WAFV2",
)
Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Contributor

Choose a reason for hiding this comment

The 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")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should not need to import anything yourself, using the #T symbol should add the imports automatically

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had some issues here doing that. The exitProcess function is not available in common, only JVM and Native. See: https://kotlinlang.org/api/latest/jvm/stdlib/kotlin.system/exit-process.html.

writer.emptyLine()
writer.write("private var exitCode = 0")
writer.write("private val regionOverride = System.getenv(\"AWS_SMOKE_TEST_REGION\")")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: Use #S to avoid having to manage the quotes yourself

writer.write("private val skipTags = System.getenv(\"AWS_SMOKE_TEST_SKIP_TAGS\")?.let { it.split(\",\").map { it.trim() }.toSet() }")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is missing AWS_SMOKE_TEST_SERVICE_IDS

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()}() {", "}") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: I see testCase.id.toCamelCase() repeated a lot, you should consider making an extension like SmokeTestCase.fnName to deduplicate it

write("val tags = setOf<String>(${testCase.tags.joinToString(",") { it.dq()} })")
write("if (skipTags != null && tags.any { it in skipTags }) return")
Copy link
Contributor

Choose a reason for hiding this comment

The 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")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens when the AWS_SMOKE_TEST_REGION is not set?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case regionOverride will be set to null

}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: region is an AWS service concept. Why are we codegenning region config in smithy-kotlin?

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.
*/
Copy link
Contributor

Choose a reason for hiding this comment

The 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)
Copy link
Contributor

Choose a reason for hiding this comment

The 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
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: successCriterion and failureCriterion are only used as short names for the if-branch logic. They're unnecessary.

val expected = if (testCase.expectation.isFailure) {
    getFailureCriterion(testCase)
} else {
    RuntimeTypes.HttpClient.Interceptors.SmokeTestsSuccessException
}


writer.write("val success = e is #L", expected)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Use #T for types

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.
Copy link
Contributor

Choose a reason for hiding this comment

The 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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

naming: render usually means you are making a write(...) call, which does not happen here

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> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: Make this an extension property (val) instead of fun

Copy link
Contributor

Choose a reason for hiding this comment

The 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"?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

val topDownIndex = TopDownIndex.of(this)
return topDownIndex.getContainedOperations(service)
}
Original file line number Diff line number Diff line change
Expand Up @@ -12,3 +12,4 @@ software.amazon.smithy.kotlin.codegen.rendering.endpoints.discovery.EndpointDisc
software.amazon.smithy.kotlin.codegen.rendering.endpoints.SdkEndpointBuiltinIntegration
software.amazon.smithy.kotlin.codegen.rendering.compression.RequestCompressionIntegration
software.amazon.smithy.kotlin.codegen.rendering.auth.SigV4AsymmetricAuthSchemeIntegration
software.amazon.smithy.kotlin.codegen.rendering.smoketests.SmokeTestsIntegration
Loading
Loading