feat: add ability to run protocol tests as unit tests of a service #1590

dayaffe · 2024-06-25T18:01:32Z

Issue #

Description of changes

SignerMiddleware will check and update signing properties with X-Amz-Content-Sha256 if it is pre-computed by SHA256 middleware
SHA256 Middleware now occurs before signing rather than before transmit. It also no longer adds the X-Amz-Content-Sha256 header and instead computes the value and passes it to CRT to add during signing. (Glacier fix)
Create s3-tests.smithy with protocol tests that require using the actual service client. These tests will get generated and copied into the relevant service client test folder.

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.

Sources/Core/AWSClientRuntime/Middlewares/Sha256TreeHashMiddleware.swift

dayaffe · 2024-07-02T15:23:50Z

codegen/.swiftpm/xcode/xcshareddata/xcschemes/aws-sdk-swift-protocol-tests-Package.xcscheme

+      shouldUseLaunchSchemeArgsEnv = "YES">
+      <TestPlans>
+         <TestPlanReference
+            reference = "container:XCTestPlans/ProtocolTestPlan.xctestplan"


We need to upload the scheme so that we ensure it includes our reference to using ProtocolTestPlan

codegen/XCTestPlans/ProtocolTestPlan.xctestplan

dayaffe · 2024-07-02T15:25:14Z

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

+        "S3PreservesEmbeddedDotSegmentInUriLabel", // moved to s3-tests.smithy
+        "S3PreservesLeadingDotSegmentInUriLabel", // moved to s3-tests.smithy


Tests that have been moved to s3-tests.smithy would still remain ignored here for when they are pulled from smithy. They have been copied and renamed.

If tests that have been copied over to s3-tests.smithy are modified or added to, how will we know (other than manually checking those test models)?

they'll continue to be marked as disabled here with a comment noting they've been moved. Another place to check is in AdditionalServiceTests/ folder for the smithy file

jbelkins

Overall good, but it appears there are several things in test setup & config that can be eliminated.

jbelkins · 2024-07-02T21:38:58Z

.github/workflows/codegen-build-test.yml

@@ -6,6 +6,9 @@ on:
  workflow_dispatch:
  pull_request:

+permissions:
+  id-token: write
+


Do we need this? Doesn't look like we use write in this job.

I'll try removing it

.github/workflows/continuous-integration.yml

.github/workflows/codegen-build-test.yml

jbelkins · 2024-07-02T21:46:08Z

Sources/Services/AWSS3/Tests/AWSS3Tests/DeleteObjectTaggingRequestTest.swift

+            method: .delete,
+            path: "/my%20key.txt",
+            headers: [
+                "Content-Type": "application/json"


Does this PR need to have its codegen updated?

oh yes it does

jbelkins · 2024-07-02T21:47:28Z

codegen/.swiftpm/xcode/xcshareddata/xcschemes/aws-sdk-swift-protocol-tests-Package.xcscheme

This name matches the scheme that Xcode auto-generates for a package. Are you sure we need this to be committed explicitly?

it doesnt need to be committed as it will get generated, but I thought we keep Sources/Services/ codegen committed and up to date?

jbelkins · 2024-07-02T21:50:36Z

.github/workflows/continuous-integration.yml

@@ -109,8 +112,10 @@ jobs:
        run: |
          cd codegen/
          set -o pipefail && \
-          NSUnbufferedIO=YES xcodebuild \
+          xcodebuild -showTestPlans -scheme aws-sdk-swift-protocol-tests-Package && \


Was this here just for troubleshooting? Can we get rid of it?

.github/workflows/continuous-integration.yml

jbelkins · 2024-07-05T15:48:11Z

.github/workflows/continuous-integration.yml

@@ -7,6 +7,9 @@ on:
 env:
  AWS_SWIFT_SDK_USE_LOCAL_DEPS: 1

+permissions:
+  id-token: write
+


are you sure we can't get rid of these extra permissions in CI? IIRC this is needed to get AWS credentials through OpenID which we don't do in the CI workflow.

On the linux side I'm using aws-actions/configure-aws-credentials@v3 still. Would rather I try to change this one to fake credentials?

Yes, let's not get real credentials where fake ones will do

jbelkins · 2024-07-05T15:50:33Z

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

+        "S3PreservesEmbeddedDotSegmentInUriLabel", // moved to s3-tests.smithy
+        "S3PreservesLeadingDotSegmentInUriLabel", // moved to s3-tests.smithy


If tests that have been copied over to s3-tests.smithy are modified or added to, how will we know (other than manually checking those test models)?

dayaffe added 2 commits June 25, 2024 14:01

add ability to run protocol tests as unit tests of a service

18bc019

Merge branch 'main' into day/enable-s3-tests

b718463

dayaffe mentioned this pull request Jun 25, 2024

feat: change SignedBodyValue enum to allow precomputed sha256 hash awslabs/aws-crt-swift#271

Merged

dayaffe added 27 commits June 25, 2024 15:10

version bump crt to 0.31

b1e5d2d

merge 'main' into day/enable-s3-tests

9dfd788

fix failing test

1bc310d

try adding fake keys for test configuration

dfaf9d9

try again

3a3239b

use v4

61a49cc

try adding env directly in the step

bd7d092

correctly copy protocol tests

cb92276

try different example key/secret

7e24e22

update test

e42ac05

try again

a35df4f

try again

aafe8da

move to right before protocol run

f991512

try directly in the run block

ccbeddb

try again

6ddb43a

try again

09cce79

add back

5248527

remove unnecessary space

766f4d3

id-token: write

f900cf3

switch to key directly

5e3a95e

add protocol test plan

026ada7

also try role arn

a145926

add default region

a692b03

modifyBeforeSigning instead of transmit

8708f5c

change to aws-region

e4b8379

change to role arn

f225dd5

ls to see files in path

c6b9c3a

dayaffe added 5 commits July 1, 2024 11:28

try role arn

4f00274

try again

0526c43

try again

2a4e5fa

try adding env directly to step

ab4025d

try again and again

e36b8a0

dayaffe requested review from milesziemer, jbelkins and sichanyoo July 1, 2024 18:12

dayaffe added 3 commits July 1, 2024 15:56

try creating a ~/.aws/config with a default profile

6f93e8c

create credentials file too

06c7a6f

cleanup workflow files

5006c7a

dayaffe commented Jul 2, 2024

View reviewed changes

Sources/Core/AWSClientRuntime/Middlewares/Sha256TreeHashMiddleware.swift Show resolved Hide resolved

dayaffe commented Jul 2, 2024

View reviewed changes

Sources/Core/AWSClientRuntime/Middlewares/Sha256TreeHashMiddleware.swift Show resolved Hide resolved

dayaffe commented Jul 2, 2024

View reviewed changes

codegen/XCTestPlans/ProtocolTestPlan.xctestplan Show resolved Hide resolved

dayaffe commented Jul 2, 2024

View reviewed changes

jbelkins requested changes Jul 2, 2024

View reviewed changes

dayaffe added 3 commits July 3, 2024 14:00

update codegen tests + use new static endpoint resolver

be7f27b

Merge branch 'main' into day/enable-s3-tests

36f265f

clean up github workflows

8cc787f

dayaffe requested a review from jbelkins July 3, 2024 20:39

jbelkins reviewed Jul 5, 2024

View reviewed changes

dayaffe and others added 4 commits July 5, 2024 12:38

add back NSUnbufferedIO=YES

6d0c925

Merge branch 'main' into day/enable-s3-tests

ca4f03d

try using env creds on linux

4607457

try using profile creds on linux

482b44e

dayaffe requested a review from jbelkins July 5, 2024 20:28

jbelkins approved these changes Jul 5, 2024

View reviewed changes

dayaffe merged commit e550145 into main Jul 5, 2024
29 checks passed

dayaffe deleted the day/enable-s3-tests branch July 5, 2024 21:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add ability to run protocol tests as unit tests of a service #1590

feat: add ability to run protocol tests as unit tests of a service #1590

dayaffe commented Jun 25, 2024 •

edited

Loading

dayaffe Jul 2, 2024

dayaffe Jul 2, 2024

jbelkins Jul 5, 2024

dayaffe Jul 5, 2024

jbelkins left a comment

jbelkins Jul 2, 2024

dayaffe Jul 3, 2024

jbelkins Jul 2, 2024

dayaffe Jul 3, 2024

jbelkins Jul 2, 2024

dayaffe Jul 3, 2024

jbelkins Jul 2, 2024

jbelkins Jul 5, 2024

dayaffe Jul 5, 2024

jbelkins Jul 5, 2024

jbelkins Jul 5, 2024

		"S3PreservesEmbeddedDotSegmentInUriLabel", // moved to s3-tests.smithy
		"S3PreservesLeadingDotSegmentInUriLabel", // moved to s3-tests.smithy

feat: add ability to run protocol tests as unit tests of a service #1590

feat: add ability to run protocol tests as unit tests of a service #1590

Conversation

dayaffe commented Jun 25, 2024 • edited Loading

Issue #

Description of changes

New/existing dependencies impact assessment, if applicable

Conventional Commits

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

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

dayaffe commented Jun 25, 2024 •

edited

Loading