Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| { | ||
| "id": "c376a9ee-568d-4638-8f4f-2e9a54f2869c", | ||
| "type": "feature", | ||
| "description": "Failed smoke tests now print exception stack trace" |
There was a problem hiding this comment.
nit/opinion: This does not need a changelog
| object SmokeTestUriValue : SectionId { | ||
| val EndpointProvider: SectionKey<Symbol> = SectionKey("EndpointProvider") | ||
| val EndpointParameters: SectionKey<Symbol> = SectionKey("EndpointParameters") | ||
| } |
There was a problem hiding this comment.
organization/style: Namespace these sections under another object SmokeTestSectionId
There was a problem hiding this comment.
nit: My comment here is still valid
There was a problem hiding this comment.
This isn't part of the diff anymore. But do you mean the SectionKeys?
| write("#L = #L", vendorParam.key.value.toCamelCase(), vendorParam.value.format()) | ||
| when (vendorParam.key.value) { | ||
| "region" -> { | ||
| writeInline("#L = ", vendorParam.key.value.toCamelCase()) |
There was a problem hiding this comment.
style: repeated vendorParam.key.value.toCamelCase() can be stored in a local variable
| declareSection(SmokeTestRegionDefault) | ||
| write("#L", vendorParam.value.format()) | ||
| } | ||
| "sigv4aRegionSet" -> { |
There was a problem hiding this comment.
correctness: smithy-kotlin should not know anything about AWS-specific config options. region was fine because we've treated it like a generic client config option, but is there any way to lift the handling of these AWS-specific vendor params out of smithy-kotlin?
I'm not sure there is, since this implementation heavily relies on sections, but something to consider. Can a single "vendor params" section be declared and the name of the vendor param (i.e. sigv4aRegionSet) be passed in context to aws-sdk-kotlin?
…smoke-tests-trait
0marperez
added
the
no-changelog
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
0marperez
changed the title
| testCase.operationParameters.forEach { param -> | ||
| val paramName = param.key.value.toCamelCase() | ||
| writer.writeInline("#L = ", paramName) | ||
| renderOperationParameter(paramName, param.value, paramsToShapes, testCase) |
There was a problem hiding this comment.
simplify: You shouldn't need to pass the entire paramsToShapes map to this function, just passing paramsToShapes[paramName] should work
| testCase: SmokeTestCase, | ||
| shapeOverride: Shape? = null, | ||
| ) { | ||
| val shape = shapeOverride ?: shapes[paramName] ?: throw Exception("Unable to find shape for operation parameter '$paramName' in smoke test '${testCase.functionName}'.") |
There was a problem hiding this comment.
There should be a better exception type available like IllegalArgumentException or IllegalStateException. Same applies for other Exception below
| // Everything else | ||
| else -> writer.write("#L", node.format()) |
There was a problem hiding this comment.
Do we support "anything else" or should we be throwing an exception if we reach this else?
There was a problem hiding this comment.
Yes, we support "simple" types i.e. Node. If we encounter an unknown Node type we throw an exception.
| // Section IDs | ||
| object AdditionalEnvironmentVariables : SectionId | ||
| object DefaultClientConfig : SectionId | ||
| object HttpEngineOverride : SectionId | ||
| object ServiceFilter : SectionId | ||
| object SkipTags : SectionId | ||
| object ClientConfig : SectionId { | ||
| val Name: SectionKey<String> = SectionKey("aws.smithy.kotlin#SmokeTestClientConfigName") | ||
| val Value: SectionKey<String> = SectionKey("aws.smithy.kotlin#SmokeTestClientConfigValue") | ||
| val EndpointProvider: SectionKey<Symbol> = SectionKey("aws.smithy.kotlin#SmokeTestEndpointProvider") | ||
| val EndpointParams: SectionKey<Symbol> = SectionKey("aws.smithy.kotlin#SmokeTestClientEndpointParams") | ||
| } |
There was a problem hiding this comment.
These should be top-level values, not class members
| // Constants | ||
| private val model = ctx.model | ||
| private val settings = ctx.settings | ||
| private val sdkId = settings.sdkId | ||
| private val symbolProvider = ctx.symbolProvider | ||
| private val service = symbolProvider.toSymbol(model.expectShape(settings.service)) | ||
| private val operations = model.topDownOperations(settings.service).filter { it.hasTrait<SmokeTestsTrait>() } | ||
| } | ||
|
|
||
| /** | ||
| * Derives a function name for a [SmokeTestCase] | ||
| * Env var for smoke test runners. | ||
| * Should be a comma-delimited list of strings that correspond to tags on the test cases. | ||
| * If a test case is tagged with one of the tags indicated by SMOKE_TEST_SKIP_TAGS, it MUST be skipped by the smoke test runner. | ||
| */ | ||
| const val SKIP_TAGS = "SMOKE_TEST_SKIP_TAGS" | ||
|
|
||
| /** | ||
| * Env var for smoke test runners. | ||
| * Should be a comma-separated list of service identifiers to test. | ||
| */ | ||
| private val SmokeTestCase.functionName: String | ||
| get() = this.id.toCamelCase() | ||
| const val SERVICE_FILTER = "SMOKE_TEST_SERVICE_IDS" |
There was a problem hiding this comment.
nit: These should all go at the top of the class / file
This comment has been minimized.
This comment has been minimized.
| object SmokeTestUriValue : SectionId { | ||
| val EndpointProvider: SectionKey<Symbol> = SectionKey("EndpointProvider") | ||
| val EndpointParameters: SectionKey<Symbol> = SectionKey("EndpointParameters") | ||
| } |
There was a problem hiding this comment.
nit: My comment here is still valid
Affected ArtifactsChanged in size
|
2 participants
Issue #
Description of changes
Companion PR: aws/aws-sdk-kotlin#1437
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.