-
Notifications
You must be signed in to change notification settings - Fork 218
apply us-east-1
as default region if not set by user in with_test_defaults()
#4312
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
…defaults()` for all clients supporting region allowing `aws-smithy-mocks` to work for non AWS SDK generated clients
A new generated diff is ready to view.
A new doc preview is ready to view. |
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 questions on the README, no blockers
[dependencies] | ||
aws-sdk-s3 = "1" | ||
|
||
[test-dependencies] |
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.
[dev-dependencies]
?
## Prerequisites | ||
|
||
<div class="warning"> | ||
You must enable the `test-util` feature of the service client crate in order to use the `mock_client` macro. |
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.
I know its kind of implicit in the example below that test-util
should only be enabled for [dev-dependencies]
, but should we make that explicit and warn users only to use those features for testing?
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 happens so frequently that I wonder if we should design a really nice error where the mock_client invokes a macro we define in the crates and if test-util isn't enabled it uses compile_fail! To give a precise error.
Can't do it here obviously since we'd need to add that macro first in the generated crates
A new generated diff is ready to view.
A new doc preview is ready to view. |
rustTemplate( | ||
""" | ||
if ${section.configBuilderRef}.config.load::<#{Region}>().is_none() { | ||
self.set_region(#{Some}(#{Region}::new("us-east-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.
Is it worth a BMV for this or other opt-out? I feel like there is a very high probability that someones unit test will be broken by this and although the fix won't be hard they might appreciate a way to hold back to keep their tests working
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.
Our own unit tests are broken by this and I'm not sure whether we want to ship this right now as we have introduced a precedence problem for defaults between with_test_defaults
and endpoint builtins.
Generated test code:
// Code generated by software.amazon.smithy.rust.codegen.smithy-rs. DO NOT EDIT.
#![cfg(feature = "test-util")]
#[::tokio::test]
async fn operation_input_test_test_operation_1() {
/* documentation: region should fallback to the default */
/* builtIns: {} */
/* clientParams: {} */
let (http_client, rcvr) = ::aws_smithy_http_client::test_util::capture_request(None);
let conf = {
#[allow(unused_mut)]
let mut builder = endpoint_test_service::Config::builder().with_test_defaults().http_client(http_client);
builder.build()
};
let client = endpoint_test_service::Client::from_conf(conf);
let _result = dbg!(
client
.test_operation()
.set_bar(::std::option::Option::Some(
endpoint_test_service::types::Bar::builder()
.set_f(::std::option::Option::Some("blah".to_owned()))
.build()
))
.send()
.await
);
let req = rcvr.expect_request();
let uri = req.uri().to_string();
assert!(
uri.starts_with("https://prod.us-east-2.api.myservice.aws.dev/"),
"expected URI to start with `https://prod.us-east-2.api.myservice.aws.dev/` but it was `{}`",
uri
);
}
failure:
running 1 test
test operation_input_test_test_operation_1 ... FAILED
failures:
---- operation_input_test_test_operation_1 stdout ----
[endpoint-test-service/rust-client-codegen/tests/endpoint_tests.rs:15:19] client.test_operation().set_bar(::std::option::Option::Some(endpoint_test_service::types::Bar::builder().set_f(::std::option::Option::Some("blah".to_owned())).build())).send().await = Ok(
TestOperationOutput {
_request_id: None,
},
)
thread 'operation_input_test_test_operation_1' panicked at endpoint-test-service/rust-client-codegen/tests/endpoint_tests.rs:28:5:
expected URI to start with `https://prod.us-east-2.api.myservice.aws.dev/` but it was `https://prod.us-east-1.api.myservice.aws.dev/foo`
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
failures:
operation_input_test_test_operation_1
test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
this test is generated by: https://github.com/smithy-lang/smithy-rs/blob/2016a67af3beeb4c976da9fb3d867f15822e[…]/amazon/smithy/rustsdk/endpoints/OperationInputTestGenerator.kt
Interestingly it shows nothing for builtins
but effectively it's relying on the default builtin value of us-east-2which it would not be now because with_test_defaults sets a different default when unset
Motivation and Context
mock_client
does not support clients that don't have a region #4265test-util
feature, provide sampleCargo.toml
#4189Description
us-east-1
as default region if not set by user inwith_test_defaults()
for all clients supporting region allowingaws-smithy-mocks
to work for non AWS SDK generated clients. This is only set as the default if a user has not already supplied one.test-util
feature requirement when usingaws-smithy-mocks
.Checklist
.changelog
directory, specifying "client," "server," or both in theapplies_to
key..changelog
directory, specifying "aws-sdk-rust" in theapplies_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.