Skip to content

switch integration tests to new test-utils #3898

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

Merged
merged 12 commits into from
Nov 8, 2024
Merged

Conversation

aajtodd
Copy link
Contributor

@aajtodd aajtodd commented Oct 30, 2024

Motivation and Context

Follow up PR to #3888

Description

  • Update aws/sdk/integration-tests to use the test-util feature(s) directly from aws-smithy-http-client rather than aws-smithy-runtime
  • Refactor aws-smithy-http to use `http-1.x
  • Fixed an erroneous error message from imds credentials provider in passing while tracking down a test failure
  • Fixed a few tests that were not immune to environment variables (e.g. on dev desktop AWS_EC2_METADATA_ENABLED=false is set by default and was causing some test failures)

I started pulling at some threads and found them to be quite load bearing (e.g. inlineable/serialization_settings.rs) and would require updating all of codegen to use http-1.x. ~This PR is a bit of a half step with some stop gaps put into place (e.g. the new compat feature of aws-smithy-http or inlining a function) to prevent the need to update everything in a single PR. Future PR's will continue where I left off here. ~

One lesson I learned the hard way was that our integration test Cargo.toml have to reflect codegen. I had updated them to all use http = "1" and got it all working only to realize codegen generates http = "0.2.9" and http-1x = { package = "http", version = "1"} and thus the integration tests have to look the same as that.

Future

  • ~Continue to refactor aws-smithy-http to upgrade to http-body-1.x. ~
  • Refactor codegen to update to http-1.x

UPDATE: I reverted the update to http 1.x for aws-smithy-http. This needs to be done and planned with the server and I don't want it to unnecessarily block hyper 1. We will pursue the remaining hyper 1 changes and then make a decision on internal use of http and http-body pre 1.x usage.


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

@aajtodd aajtodd requested review from a team as code owners October 30, 2024 15:56
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

Comment on lines 189 to 191
.build_uri()
.to_string()
.parse()
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks weird. is this a 0.4 to 1.0 conversion? Why do we create a URI, stringify it, and then parse it as a URI?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a 0.4 to 1.0 conversion. This is the exact same code we had for going from 1.0 to 0.4 which you can see removed/relocated on L209

@@ -1,6 +1,6 @@
[package]
name = "aws-smithy-checksums"
version = "0.60.13"
version = "0.70.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Not that it really matters, but I would expect the next version to be v0.61.0

Copy link
Contributor

Choose a reason for hiding this comment

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

I hit this with the flexibile checksums work, 0.61.0 was already published and yanked: https://crates.io/crates/aws-smithy-checksums/versions

But I would probably expect 0.62.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point.

@@ -12,7 +12,7 @@ publish = false
repository = "https://github.com/smithy-lang/smithy-rs"

[features]
http_1x = ["dep:http-1x", "dep:http-body-1x", "aws-smithy-runtime-api/http-1x"]
http_1x = ["dep:http-body-1x", "aws-smithy-runtime-api/http-1x"]
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI, the feature name was updated to http-1x in the main branch, as the updated nightly compiler told us that those who used this feature were specifying http-1x (and we tend to use a hyphen in feature names in our crates).

Copy link

github-actions bot commented Nov 7, 2024

A new generated diff is ready to view.

A new doc preview is ready to view.

Copy link
Contributor

@ysaito1001 ysaito1001 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 going through so many places and updating each site carefully. I know CI is currently failing on server-related tests, but LGTM.

Copy link

github-actions bot commented Nov 7, 2024

A new generated diff is ready to view.

A new doc preview is ready to view.

Copy link

github-actions bot commented Nov 7, 2024

A new generated diff is ready to view.

A new doc preview is ready to view.

Copy link

github-actions bot commented Nov 7, 2024

A new generated diff is ready to view.

A new doc preview is ready to view.

Copy link

github-actions bot commented Nov 8, 2024

A new generated diff is ready to view.

A new doc preview is ready to view.

@aajtodd aajtodd merged commit 83c606f into hyper1 Nov 8, 2024
36 of 44 checks passed
@aajtodd aajtodd deleted the atodd/test-util-switch branch November 8, 2024 18:34
github-merge-queue bot pushed a commit that referenced this pull request Mar 10, 2025
## Motivation and Context
<!--- Why is this change required? What problem does it solve? -->
<!--- If it fixes an open issue, please link to the issue here -->
* #3710
* #1925
* awslabs/aws-sdk-rust#977


## Description
Updates the default HTTPS client to be based off hyper 1.x, rustls, and
aws-lc. See the GitHub discussion:
awslabs/aws-sdk-rust#1257

This PR is a rollup of previously reviewed PRs:

* Refactor the runtime structure and consolidate HTTP client
implementation(s):
[smithy-rs#3866](#3866)
* Migrate HTTP test utils:
[smithy-rs#3888](#3888)
* Update runtime to use new test utils:
[smithy-rs#3898](#3898)
* Backport connection poisoning:
[smithy-rs#3795](#3795)
* Deprecate HTTP 02x presign APIs:
[smithy-rs#3823](#3823)
* Enable hyper1 as default client: [smithy-rs#]()
* Enable hyper1 behind
BMV:[smithy-rs#3973](#3973)
* s2n-tls provider (by Sam):
[smithy-rs#3965](#3965)
* custom TLS config:
[smithy-rs#4032](#4032)

## Testing
<!--- Please describe in detail how you tested your changes -->
<!--- Include details of your testing environment, and the tests you ran
to -->
<!--- see how your change affects other areas of the code, etc. -->

## Checklist
<!--- If a checkbox below is not applicable, then please DELETE it
rather than leaving it unchecked -->
- [x] For changes to the smithy-rs codegen or runtime crates, I have
created a changelog entry Markdown file in the `.changelog` directory,
specifying "client," "server," or both in the `applies_to` key.
- [x] For changes to the AWS SDK, generated SDK code, or SDK runtime
crates, I have created a changelog entry Markdown file in the
`.changelog` directory, specifying "aws-sdk-rust" in the `applies_to`
key.

----

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

---------

Co-authored-by: Sam Clark <[email protected]>
Co-authored-by: Landon James <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants