feat: Smoke Tests V2 #1791

sichanyoo · 2024-10-16T18:59:43Z

Companion smithy-swift PR: smithy-lang/smithy-swift#841

Issue #

1779

Description of changes

Adds smoke test code generator that extends smithy-swift's generic smoke test code generator
Adds Package.swift codegen for the new smoke tests module

New/existing dependencies impact assessment, if applicable

Conventional Commits

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

…ass SmokeTestGenerator in smithy-swift.

…swift package manifeset generation command.

…test tag. Also, add rm -rf for previously generated smoke test artifacts to codegen.sh script.

…_SMOKE_TEST_SERVICE_IDS environment variable.

…oke tests with each new release.

sichanyoo

Comments for reviewers.

sichanyoo · 2024-10-16T21:56:38Z

AWSSDKSwiftCLI/Package.swift

@@ -26,6 +26,7 @@ let package = Package(
            resources: [
                .process("Resources/Package.Prefix.txt"),
                .process("Resources/Package.Base.txt"),
+                .process("Resources/SmokeTestsPackage.Base.txt"),


The package manifest for SmokeTests module will also be generated along with the Package.swift for aws-sdk-swift project. The static file SmokeTestsPackage.Base.txt is appended after generated content during manifest generation, similar to how it's done for root project. Both manifests will use same file for prefix (Package.Prefix.txt).

sichanyoo · 2024-10-16T21:57:33Z

AWSSDKSwiftCLI/Sources/AWSCLIUtils/FileManager+Utils.swift

+    /// Returns the list of services that have smoke tests and test runner generated for them.
+    /// Service names are extracted from service name prefix of directory names under `SmokeTests/`.
+    /// E.g., extract `AWSS3` from `SmokeTests/AWSS3SmokeTestRunner/`.
+    ///
+    /// - Returns: The list of services with generated smoke tests.
+    func servicesWithSmokeTests() throws -> [String] {
+        try FileManager.default
+            .contentsOfDirectory(atPath: "SmokeTests")
+            .sorted()
+            .filter { !$0.hasPrefix(".") && $0.hasSuffix("SmokeTestRunner") }
+            .map { $0.replacingOccurrences(of: "SmokeTestRunner", with: "") }
+    }
+


Similar logic to getting enabled services - by looking at generated directories.

sichanyoo · 2024-10-16T21:57:58Z

...CLI/Sources/AWSSDKSwiftCLI/Commands/AWSSDKSwiftCLI/Subcommands/GeneratePackageManifest.swift

    /// - Parameter contents: The contents of the package manifest.
-    func savePackageManifest(_ contents: String) throws {
-        log("Saving package manifest to \(packageFileName)...")
+    func savePackageManifest(_ contents: String, _ packageFilePath: String) throws {
+        log("Saving package manifest to \(packageFilePath)...")
        try contents.write(
-            toFile: packageFileName,
+            toFile: packageFilePath,
            atomically: true,
            encoding: .utf8
        )


A small refactor to make this function reusable for different manifest filepaths.

sichanyoo · 2024-10-16T21:58:38Z

...CLI/Sources/AWSSDKSwiftCLI/Commands/AWSSDKSwiftCLI/Subcommands/GeneratePackageManifest.swift

+        // Generate package manifest for smoke tests and save it as aws-sdk-swift/SmokeTests/Package.swift
+        let smokeTestsContents = try generateSmokeTestsPackageManifestContents()
+        try savePackageManifest(smokeTestsContents, "SmokeTests/\(packageFileName)")


Generate manifest for smoke tests module at SmokeTests/Package.manifest.

sichanyoo · 2024-10-16T22:01:32Z

AWSSDKSwiftCLI/Sources/AWSSDKSwiftCLI/Resources/SmokeTestsPackage.Base.txt

@@ -0,0 +1,39 @@
+// MARK: - Static Content


The static content of package manifest. The only generated content, which comes right before this, is serviceNames array which will contain names of all services that have smoke tests. Then, that array is used in this static content for adding all the executable targets and executable products needed to build & run our test runners.

sichanyoo · 2024-10-16T22:03:14Z

codegen/sdk-codegen/build.gradle.kts

@@ -203,13 +203,23 @@ val AwsService.modelExtrasDir: String



Changes in this file adds staging logic for moving generated smoke tests for each service to corresponding directory under SmokeTests/.

sichanyoo · 2024-10-16T22:04:58Z

.../src/main/kotlin/software/amazon/smithy/aws/swift/codegen/AWSHTTPBindingProtocolGenerator.kt

+    open val protocolTestsToIgnore: Set<String> = setOf()
+    open val protocolTestTagsToIgnore: Set<String> = setOf()


Small rename for specifying type of tests these apply to, just for additional clarity.

sichanyoo · 2024-10-16T22:05:44Z

...ft-codegen/src/main/kotlin/software/amazon/smithy/aws/swift/codegen/AWSSmokeTestGenerator.kt

@@ -0,0 +1,77 @@
+package software.amazon.smithy.aws.swift.codegen


Extends generic smoke test generator in smithy-swift. Overrides methods to provide AWS specific logic.

sichanyoo · 2024-10-16T22:06:44Z

...ft-codegen/src/main/kotlin/software/amazon/smithy/aws/swift/codegen/AWSSmokeTestGenerator.kt

+    // Converts trait definition vendor param key:value pairs to Swift SDK config field:value pairs.
+    private fun getFormattedVendorParams(vendorParams: ObjectNode): Map<String, String> {
+        val formattedMapping = mutableMapOf<String, String>()
+        vendorParams.members.forEach { originalMapping ->
+            when (originalMapping.key.value) {
+                /* BaseAwsVendorParams members */
+                "region" -> {
+                    // Take region value retrieved from environment variable if present; otherwise, take from trait definition.
+                    val regionValue = "regionFromEnv ?? \"${originalMapping.value.expectStringNode().value}\""
+                    formattedMapping.put("region", regionValue)
+                    formattedMapping.put("signingRegion", regionValue)
+                }
+                "sigv4aRegionSet" -> { /* no-op; setting multiple signing regions in config is unsupported atm. */ }
+                "uri" -> { formattedMapping.put("endpoint", "\"${originalMapping.value.expectStringNode().value}\"") }
+                "useFips" -> { formattedMapping.put("useFIPS", originalMapping.value.expectBooleanNode().value.toString()) }
+                "useDualstack" -> { formattedMapping.put("useDualStack", originalMapping.value.expectBooleanNode().value.toString()) }
+                "useAccountIdRouting" -> { /* no-op; setting account ID routing in config is unsupported atm. */ }
+
+                /* S3VendorParams members */
+                "useAccelerate" -> { formattedMapping.put("accelerate", originalMapping.value.expectBooleanNode().value.toString()) }
+                "useMultiRegionAccessPoints" -> {
+                    // Name for corresponding config in Swift SDK is: `disableMultiRegionAccessPoints`; value needs to be flipped.
+                    formattedMapping.put("disableMultiRegionAccessPoints", (!(originalMapping.value.expectBooleanNode().value)).toString())
+                }
+                "useGlobalEndpoint", "forcePathStyle", "useArnRegion" -> {
+                    // No change needed for these
+                    formattedMapping.put(originalMapping.key.value, originalMapping.value.expectBooleanNode().value.toString())
+                }
+            }
+        }
+        return formattedMapping
+    }


This "translates" vendor params officially defined in Smithy vendor param shapes that service teams can use to specify client config values in their smoke tests. There're a couple that we don't have config bindings for yet.

What is the behavior if they use a param we haven't supported yet? Fail codegen or generate without it & presumably fail the test later?

If they use a param we haven't supported yet, the test will be generated without it and test will most likely fail later. If that happens we'll need to disable smoke test temporarily.

sichanyoo · 2024-10-16T22:08:43Z

scripts/run-smoke-tests.sh

@@ -0,0 +1,83 @@
+#!/bin/bash


Convenience script for running multiple test runner executables based on the comma separated string value of the AWS_SMOKE_TEST_SERVICE_IDS environment variable. This will be used in internal build system down the line as well.

sichanyoo · 2024-10-16T23:08:53Z

SmokeTests/placeholder.txt

This is a place holder file just to add SmokeTests/ directory to the repo. In Catapult, with each new release, contents of this directory will be cleared out and filled with most up-to-date package manifest and test runners.

… bug. Add convenience script for running every smoke test.

sichanyoo · 2024-10-16T23:32:43Z

scripts/run-every-smoke-test.sh

@@ -0,0 +1,46 @@
+#!/bin/bash


Convenience script for developers to run every smoke test generated under SmokeTests/ with overall test summary printed at the end.

jbelkins

As discussed:

move smoke test manifest generation to its own CLI command
.gitignore the generated smoke tests, and we will generate them when we need to run them.

jbelkins · 2024-10-28T15:58:00Z

...ft-codegen/src/main/kotlin/software/amazon/smithy/aws/swift/codegen/AWSSmokeTestGenerator.kt

+    // Converts trait definition vendor param key:value pairs to Swift SDK config field:value pairs.
+    private fun getFormattedVendorParams(vendorParams: ObjectNode): Map<String, String> {
+        val formattedMapping = mutableMapOf<String, String>()
+        vendorParams.members.forEach { originalMapping ->
+            when (originalMapping.key.value) {
+                /* BaseAwsVendorParams members */
+                "region" -> {
+                    // Take region value retrieved from environment variable if present; otherwise, take from trait definition.
+                    val regionValue = "regionFromEnv ?? \"${originalMapping.value.expectStringNode().value}\""
+                    formattedMapping.put("region", regionValue)
+                    formattedMapping.put("signingRegion", regionValue)
+                }
+                "sigv4aRegionSet" -> { /* no-op; setting multiple signing regions in config is unsupported atm. */ }
+                "uri" -> { formattedMapping.put("endpoint", "\"${originalMapping.value.expectStringNode().value}\"") }
+                "useFips" -> { formattedMapping.put("useFIPS", originalMapping.value.expectBooleanNode().value.toString()) }
+                "useDualstack" -> { formattedMapping.put("useDualStack", originalMapping.value.expectBooleanNode().value.toString()) }
+                "useAccountIdRouting" -> { /* no-op; setting account ID routing in config is unsupported atm. */ }
+
+                /* S3VendorParams members */
+                "useAccelerate" -> { formattedMapping.put("accelerate", originalMapping.value.expectBooleanNode().value.toString()) }
+                "useMultiRegionAccessPoints" -> {
+                    // Name for corresponding config in Swift SDK is: `disableMultiRegionAccessPoints`; value needs to be flipped.
+                    formattedMapping.put("disableMultiRegionAccessPoints", (!(originalMapping.value.expectBooleanNode().value)).toString())
+                }
+                "useGlobalEndpoint", "forcePathStyle", "useArnRegion" -> {
+                    // No change needed for these
+                    formattedMapping.put(originalMapping.key.value, originalMapping.value.expectBooleanNode().value.toString())
+                }
+            }
+        }
+        return formattedMapping
+    }


What is the behavior if they use a param we haven't supported yet? Fail codegen or generate without it & presumably fail the test later?

jbelkins · 2024-10-28T15:58:55Z

...otlin/software/amazon/smithy/aws/swift/codegen/protocols/restxml/RestXMLProtocolGenerator.kt

@@ -13,7 +13,7 @@ import software.amazon.smithy.swift.codegen.integration.ProtocolGenerator
 class RestXMLProtocolGenerator : AWSHTTPBindingProtocolGenerator(RestXMLCustomizations()) {
    override val defaultContentType: String = "application/xml"
    override val protocol: ShapeId = RestXmlTrait.ID
-    override val testsToIgnore: Set<String> = setOf(
+    override val protocolTestsToIgnore: Set<String> = setOf(


Is there also a list of smokeTestsToIgnore or are you just renaming this for clarity?

The latter, just renaming this to clarify.

…ests package manifest generation to its independent subcommand GenerateSmokeTestsPackageManifest.swift.

sichanyoo · 2024-10-29T00:10:49Z

...s/AWSSDKSwiftCLI/Commands/AWSSDKSwiftCLI/Subcommands/GenerateSmokeTestsPackageManifest.swift

@@ -0,0 +1,72 @@
+//


Separate subcommand for generating package manifest for SmokeTests module.

Sichan Yoo added 10 commits October 8, 2024 11:39

Change var names to be protocol test specific; add basic scaffold.

5d20161

Add AWSSmokeTestGenerator that provides custom behaviors to parent cl…

833e7a4

…ass SmokeTestGenerator in smithy-swift.

Add SmokeTest package manifest generation on top of existing aws-sdk-…

772a902

…swift package manifeset generation command.

Fix compile errors / warnings

90fe001

Fix smoke tests manifest codegen bug.

b0b5438

Modify staging SDK logic for smoke test directories.

ba0b666

Add override variables for ignoring smoke test codegen by test ID or …

83407be

…test tag. Also, add rm -rf for previously generated smoke test artifacts to codegen.sh script.

Add convenience script for running multiple test runners based on AWS…

4ef5af6

…_SMOKE_TEST_SERVICE_IDS environment variable.

Merge branch 'main' into feat/smoke-tests-v2

4576e3c

Place holder for adding directory; will be deleted and filled with sm…

68224c5

…oke tests with each new release.

sichanyoo mentioned this pull request Oct 16, 2024

feat: Smoke Tests V2 smithy-lang/smithy-swift#841

Merged

2 tasks

Sichan Yoo added 2 commits October 16, 2024 12:02

Merge branch 'main' into feat/smoke-tests-v2

3d67e22

temporarily disable reference to main branch for internal build test

cc056bc

sichanyoo requested a review from jbelkins October 16, 2024 21:43

sichanyoo commented Oct 16, 2024

View reviewed changes

Revert temporarily commented logic in PrepareRelease. Fix client name…

19914a1

… bug. Add convenience script for running every smoke test.

sichanyoo commented Oct 16, 2024

View reviewed changes

jbelkins requested changes Oct 28, 2024

View reviewed changes

Sichan Yoo added 2 commits October 28, 2024 11:42

Ignore code-generated SmokeTests; don't commit them to repo.

8452690

Revert changes in GeneratePackageManifest.swift, and separate smoke t…

324ac6f

…ests package manifest generation to its independent subcommand GenerateSmokeTestsPackageManifest.swift.

sichanyoo commented Oct 29, 2024

View reviewed changes

sichanyoo requested a review from jbelkins October 29, 2024 00:11

Merge branch 'main' into feat/smoke-tests-v2

f85d8f3

jbelkins approved these changes Oct 29, 2024

View reviewed changes

sichanyoo merged commit 8e9b698 into main Oct 29, 2024
29 checks passed

sichanyoo deleted the feat/smoke-tests-v2 branch October 29, 2024 16:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Smoke Tests V2 #1791

feat: Smoke Tests V2 #1791

sichanyoo commented Oct 16, 2024 •

edited

Loading

sichanyoo left a comment

sichanyoo Oct 16, 2024

sichanyoo Oct 16, 2024

sichanyoo Oct 16, 2024

sichanyoo Oct 16, 2024

sichanyoo Oct 16, 2024

sichanyoo Oct 16, 2024

sichanyoo Oct 16, 2024

sichanyoo Oct 16, 2024

sichanyoo Oct 16, 2024

jbelkins Oct 28, 2024

sichanyoo Oct 28, 2024

sichanyoo Oct 16, 2024

sichanyoo Oct 16, 2024

sichanyoo Oct 16, 2024

jbelkins left a comment

jbelkins Oct 28, 2024

jbelkins Oct 28, 2024

sichanyoo Oct 28, 2024 •

edited

Loading

sichanyoo Oct 29, 2024

		open val protocolTestsToIgnore: Set<String> = setOf()
		open val protocolTestTagsToIgnore: Set<String> = setOf()

		@@ -0,0 +1,77 @@
		package software.amazon.smithy.aws.swift.codegen

feat: Smoke Tests V2 #1791

feat: Smoke Tests V2 #1791

Conversation

sichanyoo commented Oct 16, 2024 • edited Loading

Issue #

Description of changes

New/existing dependencies impact assessment, if applicable

Conventional Commits

sichanyoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbelkins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sichanyoo Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sichanyoo commented Oct 16, 2024 •

edited

Loading

sichanyoo Oct 28, 2024 •

edited

Loading