Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Endpoint operation input tests #2204

Merged
merged 10 commits into from
Jan 18, 2023
Merged

Endpoint operation input tests #2204

merged 10 commits into from
Jan 18, 2023

Conversation

rcoh
Copy link
Collaborator

@rcoh rcoh commented Jan 12, 2023

Motivation and Context

Endpoints 2.0 exposes integration-style tests–we need to use those (and this found a bug!)

Description

  • enable operationInput tests. I chose to generate tests using the fluent client and capture request—this seemed like the best end-to-end strategy. Because of this, this currently only runs against the fluent client.
  • a few other things needed to happen to support this:
    • We needed a way to go from builtIn name to applying it to the service config
    • We needed a way to default config parameters for tests—I added with_test_defaults() which we can start using after this PR

Testing

  • tests pass

Checklist

  • I have updated CHANGELOG.next.toml if I made changes to the smithy-rs codegen or runtime crates
  • I have updated CHANGELOG.next.toml if I made changes to the AWS SDK, generated SDK code, or SDK runtime crates

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

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@rcoh rcoh marked this pull request as ready for review January 17, 2023 17:01
@rcoh rcoh requested review from a team as code owners January 17, 2023 17:01
@rcoh rcoh requested review from david-perez and unexge January 17, 2023 17:01
@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

Velfi
Velfi reviewed Jan 17, 2023
View reviewed changes
aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/customize/s3/S3Decorator.kt Outdated
Comment on lines 91 to 93
if (name.startsWith("S3")) {
return null
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What kinds of things does this skip? From my naïve point of view, it looks like we're filtering out stuff related to S3 in an S3 decorator, which I find strange.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was a double bug 🤕 good catch. It doesn't filter out anything because the builtIns start with AWS::S3.

Velfi
Velfi approved these changes Jan 17, 2023
View reviewed changes
Copy link
Contributor

@Velfi Velfi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only had doc and naming-related comments. Good work on this.

rcoh
rcoh commented Jan 18, 2023
View reviewed changes
Copy link
Collaborator Author

@rcoh rcoh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for all the excellent comments!

...oftware/amazon/smithy/rust/codegen/client/smithy/generators/config/ServiceConfigGenerator.kt Outdated
rust("self")
}

// Attribute.CfgTest.render(this)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delete

...sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/endpoints/AwsEndpointDecorator.kt Show resolved Hide resolved
...kotlin/software/amazon/smithy/rust/codegen/client/smithy/customize/RequiredCustomizations.kt Show resolved Hide resolved
aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/EndpointBuiltInsDecorator.kt Show resolved Hide resolved
aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/customize/s3/S3Decorator.kt Outdated
Comment on lines 91 to 93
if (name.startsWith("S3")) {
return null
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was a double bug 🤕 good catch. It doesn't filter out anything because the builtIns start with AWS::S3.

aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/endpoints/StripEndpointTrait.kt Show resolved Hide resolved
.../kotlin/software/amazon/smithy/rust/codegen/client/smithy/endpoint/EndpointTypesGenerator.kt Show resolved Hide resolved
...oftware/amazon/smithy/rust/codegen/client/smithy/generators/config/ServiceConfigGenerator.kt Outdated Show resolved Hide resolved
...oftware/amazon/smithy/rust/codegen/client/smithy/generators/config/ServiceConfigGenerator.kt Show resolved Hide resolved
...e/src/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/generators/Instantiator.kt Show resolved Hide resolved
@rcoh rcoh force-pushed the endpoint-operation-input-tests branch from 7bb3ea7 to 9e17129 Compare January 18, 2023 16:56
Velfi
Velfi reviewed Jan 18, 2023
View reviewed changes
aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/CredentialProviders.kt Outdated Show resolved Hide resolved
aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/EndpointBuiltInsDecorator.kt Show resolved Hide resolved
aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/EndpointBuiltInsDecorator.kt Show resolved Hide resolved
aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/EndpointBuiltInsDecorator.kt Show resolved Hide resolved
aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/EndpointBuiltInsDecorator.kt Show resolved Hide resolved
aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/EndpointBuiltInsDecorator.kt Show resolved Hide resolved
aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/EndpointBuiltInsDecorator.kt
Comment on lines +125 to +127
if (parameter.type == ParameterType.STRING) {
rust(".clone()")
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this would be better represented by implementing an is_not_copy method on parameter. However, since I don't see us adding a profusion of parameter types any time soon, you can probably just ignore this.

aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/EndpointBuiltInsDecorator.kt
else -> null
}

override fun setBuiltInOnServiceConfig(name: String, value: Node, configBuilderRef: String): Writable? {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should consider thinking of a word that means "this function constructs and returns a writable" and using it to name things like this. My reasoning is that this looks like something that sets a field in Kotlin when it actually creates rust code that sets something in Rust. I don't think we have too much of a problem with keeping Kotlin vs. Rust stuff straight in codegen, but I think it's worth considering and defining a boundary.

...egen/src/main/kotlin/software/amazon/smithy/rustsdk/endpoints/OperationInputTestGenerator.kt
*
* Doing this in AWS codegen allows us to actually integration test generated clients.
*/

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

...egen/src/main/kotlin/software/amazon/smithy/rustsdk/endpoints/OperationInputTestGenerator.kt
Comment on lines +70 to +114
/**
* Generate `operationInputTests` for EP2 tests.
*
* These are `tests/` style integration tests that run as a public SDK user against a complete client. `capture_request`
* is used to retrieve the URL.
*
* Example generated test:
* ```rust
* #[tokio::test]
* async fn operation_input_test_get_object_119() {
* /* builtIns: {
* "AWS::Region": "us-west-2",
* "AWS::S3::UseArnRegion": false
* } */
* /* clientParams: {} */
* let (conn, rcvr) = aws_smithy_client::test_connection::capture_request(None);
* let conf = {
* #[allow(unused_mut)]
* let mut builder = aws_sdk_s3::Config::builder()
* .with_test_defaults()
* .http_connector(conn);
* let builder = builder.region(aws_types::region::Region::new("us-west-2"));
* let builder = builder.use_arn_region(false);
* builder.build()
* };
* let client = aws_sdk_s3::Client::from_conf(conf);
* let _result = dbg!(client.get_object()
* .set_bucket(Some(
* "arn:aws:s3-outposts:us-east-1:123456789012:outpost:op-01234567890123456:accesspoint:myaccesspoint".to_owned()
* ))
* .set_key(Some(
* "key".to_owned()
* ))
* .send().await);
* rcvr.expect_no_request();
* let error = _result.expect_err("expected error: Invalid configuration: region from ARN `us-east-1` does not match client region `us-west-2` and UseArnRegion is `false` [outposts arn with region mismatch and UseArnRegion=false]");
* assert!(format!("{:?}", error).contains("Invalid configuration: region from ARN `us-east-1` does not match client region `us-west-2` and UseArnRegion is `false`"), "expected error to contain `Invalid configuration: region from ARN `us-east-1` does not match client region `us-west-2` and UseArnRegion is `false`` but it was {}", format!("{:?}", error));
* }
* ```
*
* Eventually, we need to pull this test into generic smithy. However, this relies on generic smithy clients
* supporting middleware and being instantiable from config (https://github.com/awslabs/smithy-rs/issues/2194)
*
* Doing this in AWS codegen allows us to actually integration test generated clients.
*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@rcoh rcoh merged commit 95dc365 into main Jan 18, 2023
@rcoh rcoh deleted the endpoint-operation-input-tests branch January 18, 2023 19:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Velfi Velfi Velfi approved these changes

@david-perez david-perez Awaiting requested review from david-perez david-perez was automatically assigned from awslabs/smithy-rs-server

@unexge unexge Awaiting requested review from unexge unexge was automatically assigned from awslabs/smithy-rs-server

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rcoh @Velfi