-
Notifications
You must be signed in to change notification settings - Fork 95
chore: Opt-in service client tests #2059
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
base: main
Are you sure you want to change the base?
Conversation
codegen/sdk-codegen/build.gradle.kts
Outdated
| val fileIsServiceClientTests = details.relativePath.startsWith("Tests") | ||
|
|
||
| fileIsSmokeTests || fileIsInternalClientGeneratedTests | ||
| fileIsSmokeTests || fileIsServiceClientTests |
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.
The logic for copying test files used to omit tests for internal clients, it now omits tests for all clients since tests are run from the directory where they are rendered by Smithy.
| serviceTargets.map(target(_:_:)) + | ||
| serviceTargets.map(unitTestTarget(_:_:)) | ||
| serviceTargets + | ||
| serviceTestTargets |
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.
Some of the logic in the Package initializer above has been extracted to helper methods below.
| ) | ||
| } | ||
|
|
||
| struct ServiceClientData { |
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.
This type has been created to keep multiple pieces of data per service.
| .SmithyTestUtil, | ||
| ], | ||
| path: "Sources/Services/\(service)/Tests/\(testName)" | ||
| path: "codegen/sdk-codegen/build/smithyprojections/sdk-codegen/\(modelName)/swift-codegen/Tests/\(testName)" |
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.
The path for tests now points to the Smithy build folder, where the code generator places them at creation, since they are no longer copied into the project.
| private func unitTestTarget(_ service: String, _ dependencies: [Target.Dependency]) -> Target { | ||
| let testName = "\(service)Tests" | ||
| private var serviceTestTargets: [Target] { | ||
| guard ProcessInfo.processInfo.environment["AWS_SWIFT_SDK_ENABLE_SERVICE_TESTS"] != nil else { return [] } |
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.
Service test targets now default to [] if the new env var isn't set.
| exclude { details -> | ||
| // Exclude Package.swift | ||
| val fileIsPackageManifest = details.file.name.endsWith("Package.swift") | ||
|
|
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.
Since smithy-swift now generates a package manifest in all cases, we simply don't copy it over to the package structure. This allows for the removal of the related SwiftSetting.
| // Exclude test files for internal clients | ||
| val fileIsInternalClientGeneratedTests = it.internalClient && details.relativePath.startsWith("Tests") | ||
| // Exclude service client test files | ||
| val fileIsServiceClientTests = details.relativePath.startsWith("Tests") |
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.
Tests are now not copied for all clients, not just internal.
| import software.amazon.smithy.swift.codegen.integration.SwiftIntegration | ||
| import software.amazon.smithy.swift.codegen.model.expectTrait | ||
| import software.amazon.smithy.swift.codegen.targetName | ||
|
|
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.
An integration that writes the Dependencies.json file that was previously written by smithy-swift.
Since Dependencies.json is only used by AWS SDK, it is moved here.
Also, this class's codegen test has been eliminated because creating a SDK that compiles successfully verifies that this file is correct.
Its structure has changed slightly to pass multiple pieces of data:
{
"modelPath": "code/sdk-codegen/aws-models/s3.json",
"dependencies": [
"AWSClientRuntime",
"AWSIdentity",
"..."
]
}(Two-space indent has been used to minimize the change to the diff)
| let codegenName: String | ||
| let dependencies: [String] | ||
| let isInternal: Bool | ||
| } |
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.
An array of this data structure is passed in, with info about every service client to be included in the manifest.
| .filter { $0 != "SmithyTestUtil" } // links to test files only | ||
| return "[" + dependencies.map { ".\($0)" }.joined(separator: ", ") + "]" | ||
| private func clientDependencies(service: Service) -> String { | ||
| return "[" + service.dependencies.map { ".\($0)" }.joined(separator: ", ") + "]" |
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.
The code for reading the JSON file has been moved from here back up to the subcommand. Instead, the array of Service that is passed into this type contains the dependency info for each service.
| name: "InternalAWSCognitoIdentity", | ||
| dependencies: internalAWSCognitoIdentityDependencies, | ||
| path: "Sources/Core/AWSSDKIdentity/InternalClients/InternalAWSCognitoIdentity/Sources/InternalAWSCognitoIdentity" | ||
| ), |
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.
These hard-coded internal client targets are removed, since internal clients are now listed along with the public ones.
Instead, the internal services are filtered from the main list of services, and the targets for them are appended to runtime targets below.
| ) | ||
| } | ||
|
|
||
| // As of Swift 6.2, @unchecked is not needed, but for Swift 6.0 it is |
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.
In Swift 6.1 or 6.2 (not sure which), Target.Dependency was marked Sendable. No code changes were needed to do so.
We mark ServiceClientData as @unchecked Sendable because in the earlier Swift versions we support, Target.Dependency is not Sendable so compile breaks if we try to mark this type as Sendable and Swift 6 language mode is in use.
It's just to suppress the Swift 6 compile error though, because there is no concurrency in the Package.swift file anyway.
| "SmithyTimestamps" | ||
| ] | ||
| ] | ||
| } |
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.
New Dependencies.json structure can be seen here. Since it's now a JSON object with individual keys & values inside, it's more future-proof.
"SmithyTestUtil" is removed from every Dependencies.json because it will never be linked to a service client (we used to filter this in AWSSDKSwiftCLI but better to do it when Dependencies.json is written, to save the space.)
| } | ||
|
|
||
| data class CodegenTest(val service: String, val module: String, val extraConfig: String? = null) | ||
| data class CodegenTest(val service: String, val module: String) |
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.
Unused extraConfig property is removed
| "copyrightNotice": "//\n// Copyright Amazon.com Inc. or its affiliates.\n// All Rights Reserved.\n//\n// SPDX-License-Identifier: Apache-2.0\n//\n\n// Code generated by smithy-swift-codegen. DO NOT EDIT!\n\n", | ||
| "forProtocolTests": true | ||
| ${it.extraConfig ?: ""} | ||
| "copyrightNotice": "//\n// Copyright Amazon.com Inc. or its affiliates.\n// All Rights Reserved.\n//\n// SPDX-License-Identifier: Apache-2.0\n//\n\n// Code generated by smithy-swift-codegen. DO NOT EDIT!\n\n" |
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.
unused forProtocolTests & extraConfig are removed here.
| let excludeRuntimeUnitTests = false | ||
| let isPreviewBuild = false | ||
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.
Test expectations for the package manifest are changed below.
| } | ||
|
|
||
| func createServiceFolders(_ services: [String]) { | ||
| services.forEach { |
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.
Test setup below is expanded to write Dependencies.json for each of the mocked services, since that file is required on every service to successfully create a manifest.
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.
All the internal clients get mocked similarly as well.
| struct ServiceClientInfo: Decodable { | ||
| let modelPath: String | ||
| let dependencies: [String] | ||
| } |
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.
Above is the decodable Swift type used to read the contents of Dependencies.json
Below is code that reads Dependencies.json and uses its contents to construct a PackageManifestBuilder.Service for each published and internal service.
The combined list of services is then passed into the manifest builder for inclusion in the text of the manifest.
| buildInternalAWSSSODependencies(), | ||
| buildInternalAWSSSOOIDCDependencies(), | ||
| buildInternalAWSCognitoIdentityDependencies(), | ||
| "", |
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.
Here & below, special code for internal services is removed since they are now handled like all other services and filtered as needed into public/internal at runtime.
| val visibility: String, | ||
| val internalClient: Boolean, | ||
| val generatePackageManifest: Boolean, | ||
| val generateDependencyJSON: Boolean, |
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.
These SwiftSettings properties were removed in the companion smithy-swift PR.
|
|
||
| env: | ||
| AWS_SWIFT_SDK_USE_LOCAL_DEPS: 1 | ||
| AWS_SWIFT_SDK_ENABLE_SERVICE_TESTS: 1 |
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.
codegen-build-test is where all service tests run in CI.
Description of changes
This PR moves service client tests (i.e. endpoint tests) from being committed to the project to being an opt-in feature to be utilized only during development & distribution.
buildSdkGradle task. Instead, they are left where Smithy generates them. This is comparable to how we generate & run protocol tests.AWS_SWIFT_SDK_ENABLE_SERVICE_TESTSenables the service client test targets. The package will fail to resolve if service clients have not been locally generated (see step above). This env var is set in CI where unit tests run.Package.swiftfile has been overhauled:ServiceClientInfo) to allow for multiple pieces of data per service.ServiceClientInfodistinguishes public from internal clients so they can be separated when needed.Dependencies.jsonfile has been moved from thePackageManifestBuilderto theGeneratePackageManifestsubcommand. This separates concerns better and facilitates easier testing.Dependencies.jsonfile has been moved in fromsmithy-swiftsince it is only used by AWS SDK.The code-generated result of this change will be -290K LOC in the project due to not committing service client tests.
New/existing dependencies impact assessment, if applicable
No new dependencies were added to this change.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.