add: tests to ensure all interceptors are run if an error occurs in one #2664

Velfi · 2023-05-02T17:51:01Z

Motivation and Context

Description

This PR adds tests for all interceptor hooks to ensure the correct error handling behavior and defines a set of stub components for orchestrator testing.

Testing

This PR includes tests.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

add: stub components for orchestrator testing

rust-runtime/aws-smithy-runtime/src/client/orchestrator.rs

github-actions · 2023-05-02T18:28:40Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
No codegen difference in the Client Test
No codegen difference in the Server Test
No codegen difference in the Server Test Python
No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

jjant · 2023-05-03T15:09:22Z

You can write Closes #2662 in the PR description so that merging this PR will automatically close the issue, see: https://docs.github.com/en/get-started/writing-on-github/working-with-advanced-formatting/using-keywords-in-issues-and-pull-requests#linking-a-pull-request-to-an-issue.

Velfi · 2023-05-03T15:18:10Z

You can write Closes #2662 in the PR description so that merging this PR will automatically close the issue, see: https://docs.github.com/en/get-started/writing-on-github/working-with-advanced-formatting/using-keywords-in-issues-and-pull-requests#linking-a-pull-request-to-an-issue.

I actually don't want to close that issue since I still have more work to do. This PR adds tests that check error handling flow for individual hooks. I'm going to create another PR that tests the behavior across multiple hooks, since most (IIRC) times we're still supposed to run the modify_before_completion hook before we exit invoke.

This PR was already getting a bit long and that's why I'm splitting the work up. I also wanted to make sure that I'm in-sync with John and the work he's doing.

github-actions · 2023-05-03T16:19:34Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
No codegen difference in the Client Test
No codegen difference in the Server Test
No codegen difference in the Server Test Python
No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

Velfi · 2023-05-03T21:13:14Z

I'm waiting on the changes introduced by #2670

Once those make it to main, I'll update this PR

…-handling-tests

remove: trace probe

github-actions · 2023-05-04T15:13:37Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
No codegen difference in the Client Test
No codegen difference in the Server Test
No codegen difference in the Server Test Python
No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

jdisanti

Nice!

rust-runtime/aws-smithy-runtime/src/client/orchestrator.rs

jdisanti · 2023-05-04T15:26:47Z

rust-runtime/aws-smithy-runtime/src/client/test_util/auth.rs

+}
+
+#[derive(Debug, Default)]
+pub struct AnonymousAuthScheme {


There is a bunch of anonymous auth scheme stuff in crate::client::identity already. I think it belongs outside of test utils since it is a legitimate use case.

I created a runtime plugin for anonymous auth that uses the pre-existing stuff. Let me know what you think.

ysaito1001

Love the use of the interceptor_error_handling_test macro to simplify each test call site!

…eptor test add: runtime plugin for anonymous auth fix: clippy lints for explicit lifetimes remove: test-utils endpoints and auth modules

Velfi · 2023-05-08T15:45:14Z

rust-runtime/aws-smithy-runtime-api/src/client/auth.rs

-    fn resolve_auth_options<'a>(
-        &'a self,
+    fn resolve_auth_options(
+        &self,
        params: &AuthOptionResolverParams,
-    ) -> Result<Cow<'a, [AuthSchemeId]>, BoxError>;
+    ) -> Result<Cow<'_, [AuthSchemeId]>, BoxError>;
 }

 impl AuthOptionResolver for Box<dyn AuthOptionResolver> {
-    fn resolve_auth_options<'a>(
-        &'a self,
+    fn resolve_auth_options(
+        &self,
        params: &AuthOptionResolverParams,
-    ) -> Result<Cow<'a, [AuthSchemeId]>, BoxError> {


The linter was complaining about an unnecessary explicit lifetime here and in the auth option resolver so I switched it to an anonymous lifetime.

Interesting. I take it to mean that the compiler deterministically derives the anomalous lifeitme in this case from the first &.

Yeah, that's what happens AFAICT

Velfi · 2023-05-08T15:46:08Z

rust-runtime/aws-smithy-runtime-api/Cargo.toml

@@ -12,6 +12,7 @@ repository = "https://github.com/awslabs/smithy-rs"
 [features]
 default = []
 http-auth = ["dep:zeroize"]
+anonymous-auth = []


Previously, the anonymous auth stuff was always available. I added some more stuff related to anonymous auth and feature-gated it since we were already doing so for http-auth.

Velfi · 2023-05-08T15:46:48Z

rust-runtime/aws-smithy-runtime-api/src/client/identity.rs

@@ -65,30 +68,6 @@ impl Identity {
    }
 }

-#[derive(Debug)]


Moved to aws_smithy_runtime_api::client::identity::anonymous

stale

github-actions · 2023-05-08T16:36:00Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
No codegen difference in the Client Test
No codegen difference in the Server Test
No codegen difference in the Server Test Python
No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

ysaito1001 · 2023-05-08T16:21:34Z

rust-runtime/aws-smithy-runtime-api/src/client/auth.rs

-    fn resolve_auth_options<'a>(
-        &'a self,
+    fn resolve_auth_options(
+        &self,
        params: &AuthOptionResolverParams,
-    ) -> Result<Cow<'a, [AuthSchemeId]>, BoxError>;
+    ) -> Result<Cow<'_, [AuthSchemeId]>, BoxError>;
 }

 impl AuthOptionResolver for Box<dyn AuthOptionResolver> {
-    fn resolve_auth_options<'a>(
-        &'a self,
+    fn resolve_auth_options(
+        &self,
        params: &AuthOptionResolverParams,
-    ) -> Result<Cow<'a, [AuthSchemeId]>, BoxError> {


Interesting. I take it to mean that the compiler deterministically derives the anomalous lifeitme in this case from the first &.

rust-runtime/aws-smithy-runtime-api/src/client/runtime_plugin/anonymous_auth.rs

github-actions · 2023-05-08T20:57:11Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
No codegen difference in the Client Test
No codegen difference in the Server Test
No codegen difference in the Server Test Python
No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

…ne (#2664) ## Motivation and Context   #2662 ## Description  This PR adds tests for all interceptor hooks to ensure the correct error handling behavior and defines a set of stub components for orchestrator testing. ## Testing    This PR includes tests. ---- _By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice._

add: tests to ensure all interceptors are if an error occurs in one

3ac9c1e

add: stub components for orchestrator testing

Velfi requested a review from a team as a code owner May 2, 2023 17:51

Merge branch 'main' into add/interceptor-error-handling-tests

5725d56

jdisanti reviewed May 2, 2023

View reviewed changes

rust-runtime/aws-smithy-runtime/src/client/orchestrator.rs Outdated Show resolved Hide resolved

Velfi added 2 commits May 3, 2023 09:42

udpate: respond to PR comments

c5b76ee

fix: clippy lints

3d545d5

Velfi requested a review from a team as a code owner May 3, 2023 14:45

Velfi added 2 commits May 3, 2023 10:37

Merge branch 'main' into add/interceptor-error-handling-tests

12bc394

fix: duplicate imports

9c5f1e3

Velfi added 2 commits May 4, 2023 09:20

Merge remote-tracking branch 'origin/main' into add/interceptor-error…

7014da4

…-handling-tests

update: interceptor tests to work with John's phase refactor

c135c68

remove: trace probe

Velfi added the needs-sdk-review label May 4, 2023

jdisanti previously approved these changes May 4, 2023

View reviewed changes

ysaito1001 previously approved these changes May 5, 2023

View reviewed changes

Velfi removed the needs-sdk-review label May 5, 2023

refactor: usage of auth and identity resolvers in orchestrator interc…

fb83922

…eptor test add: runtime plugin for anonymous auth fix: clippy lints for explicit lifetimes remove: test-utils endpoints and auth modules

Velfi added the needs-sdk-review label May 8, 2023

Velfi commented May 8, 2023

View reviewed changes

Velfi added 2 commits May 8, 2023 10:47

Merge branch 'main' into add/interceptor-error-handling-tests

8e3f6aa

fix: cargo doc links

e4690ee

Velfi requested a review from ysaito1001 May 8, 2023 16:06

ysaito1001 reviewed May 8, 2023

View reviewed changes

Velfi added 5 commits May 8, 2023 14:39

update: move anonymous auth runtime plugin to aws-smithy-runtime

21e27dc

fix: doc links

07dc309

move: more anonymous auth stuff into the runtime crate from runtime-api

80d59cb

remove: feature that gates nothing

fe3026e

fix: clippy lint

3c56aa1

ysaito1001 approved these changes May 8, 2023

View reviewed changes

Velfi removed the needs-sdk-review label May 8, 2023

Velfi added this pull request to the merge queue May 8, 2023

Merged via the queue into main with commit 97c70c4 May 8, 2023

Velfi deleted the add/interceptor-error-handling-tests branch May 8, 2023 21:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add: tests to ensure all interceptors are run if an error occurs in one #2664

add: tests to ensure all interceptors are run if an error occurs in one #2664

Velfi commented May 2, 2023

github-actions bot commented May 2, 2023

jjant commented May 3, 2023

Velfi commented May 3, 2023

github-actions bot commented May 3, 2023

Velfi commented May 3, 2023

github-actions bot commented May 4, 2023

jdisanti left a comment

jdisanti May 4, 2023

Velfi May 8, 2023

ysaito1001 left a comment

Velfi May 8, 2023

ysaito1001 May 8, 2023

Velfi May 8, 2023

Velfi May 8, 2023

Velfi May 8, 2023

github-actions bot commented May 8, 2023

ysaito1001 May 8, 2023

github-actions bot commented May 8, 2023

add: tests to ensure all interceptors are run if an error occurs in one #2664

add: tests to ensure all interceptors are run if an error occurs in one #2664

Conversation

Velfi commented May 2, 2023

Motivation and Context

Description

Testing

This PR includes tests.

github-actions bot commented May 2, 2023

jjant commented May 3, 2023

Velfi commented May 3, 2023

github-actions bot commented May 3, 2023

Velfi commented May 3, 2023

github-actions bot commented May 4, 2023

jdisanti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ysaito1001 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

github-actions bot commented May 8, 2023

Choose a reason for hiding this comment

github-actions bot commented May 8, 2023