-
Notifications
You must be signed in to change notification settings - Fork 207
promote aws-smithy-mocks out of experimental #4098
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
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.
I am really excited about this!
@@ -3,11 +3,13 @@ | |||
* SPDX-License-Identifier: Apache-2.0 | |||
*/ | |||
|
|||
//! This crate allows mocking of smithy clients. | |||
//! This crate has been deprecated. Please migrate to the `aws-smithy-mocks` crate. |
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.
We should link a migration guide from here
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.
Right now the API should be compatible so I would hope/think that simply changing out the crate would be enough. I didn't force users to go through the new sequence()
builder API, they can still do:
mock!(Client::get_object)
.then_output(|| ...);
If we wanted a breaking change then I'd probably consider whether we even want to keep these since a sequence of 1 is the same:
mock!(Client::get_object)
.sequence()
.output(|| ...)
.build()
I see mild utility in keeping the existing API for convenience but mostly I kept it so that existing code should be easy to migrate and still work.
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.
The migration guide isn't as much as a "here's how you NEED to migrate" as much as its "here's all the cool things you can do with the new API"
/// | ||
#[macro_export] | ||
macro_rules! mock { | ||
($operation: expr) => { |
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.
There is apparently a slightly less cursed way to do this @compiler-errors came up with
} | ||
|
||
/// Interceptor which produces mock responses based on a list of rules | ||
pub struct MockResponseInterceptor { |
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 guess this has to be pub
since the macro expands to add a MockResponseInterceptor::new()
, but it feels like it should just be pub(crate)
since we don't really intend for it to be used outside of the context of the macro. Could we update the macro to instead use a pub
function like add_mock_interceptor(&mut config_builder)
that takes in the config builder and adds the interceptor so we can keep it pub(crate)
?
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.
Interesting. For an HTTP client, it does have a pub
free function create_mock_http_client
that returns SharedHttpClient
, used by the macro. We could have a similar pub
free function returning SharedInterceptor
, used by the 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.
It was pub before though and I believe there are people wiring it up that way today (internally anyway). I was trying to keep migration issues to a minimum but if we all feel this is the right direction I'm open to it.
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.
Personally, for APIs required by macros, but that I don't want to expose publicly, I put them in a doc(hidden) module named __macro_impl
or something like that.
Historically, we've seen that you need to eject out of the macros, and I think we should maintain that as a possibility.
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.
The only thing is this needs the HTTP client to be the one created by create_mock_http_client
. I made that function public to allow people to construct what the mock is doing manually but I'm not sure how we can validate the interceptor is being used in conjunction with the right HTTP client. I left this public and allowed the possibility though simply because people are already using this without the 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 love the work, especially documentation. Looks great, leaving minor nits and comments on top of Landon's.
.key("foo") | ||
.send() | ||
.await | ||
.expect_err("404"); // async fn mock_client() { |
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.
nit: should we remove comment?
} | ||
|
||
/// Interceptor which produces mock responses based on a list of rules | ||
pub struct MockResponseInterceptor { |
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.
Interesting. For an HTTP client, it does have a pub
free function create_mock_http_client
that returns SharedHttpClient
, used by the macro. We could have a similar pub
free function returning SharedInterceptor
, used by the macro.
} | ||
|
||
/// Repeat the last added response multiple times (total count) | ||
pub fn times(mut self, count: usize) -> Self { |
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.
What would happen if we specified .times(0)
? I've worked with google mocks for C++ where TIMES(0)
means that no call is expected for the mock method.
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.
Right now anything less than 1 has no additional effect.
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.
Got it, we should probably call out in the doc stating anything less than 1 has no effect or leave TODO
.
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.
In addition to documenting, I would probably panic on 0, this will be run in tests anyway and we want to make it as obvious as possible.
A new generated diff is ready to view.
A new doc preview is ready to view. |
I think probably yes and yes, it's low-cost and avoids potential confusion. |
let retry_rule = mock!(aws_sdk_s3::Client::get_object) | ||
.sequence() | ||
.http_status(503, None) | ||
.times(2) // Return 503 HTTP status twice |
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.
Nice, I like this API that you landed on.
} | ||
|
||
/// Repeat the last added response multiple times (total count) | ||
pub fn times(mut self, count: usize) -> Self { |
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.
In addition to documenting, I would probably panic on 0, this will be run in tests anyway and we want to make it as obvious as possible.
A new generated diff is ready to view.
A new doc preview is ready to view. |
Motivation and Context
aws_smithy_mocks_experimental
#3926Description
aws-smithy-mocks-experimental
toaws-smithy-mocks
and reset version to0.1.0
mock_client!
macro always set an HTTP client. By default it returns a dummy I'm a teapot response to aid in debugging and be easy to tell where that response is coming from. Also when a rule sets an HTTP response it is not set as an extension in the request and returned by the HTTP client. This allows for more thorough and accurate integration testing as the client/runtime will see the response coming from the HTTP client instead of being ignored and then replaced as it was beforeQuestions
aws-smithy-mocks-experimental
toaws-smithy-mocks
. Do we want or need to publish one last version ofaws-smithy-mocks-experimental
with a deprecated API that instructs users it has moved? Alternatively we could publish an informational advisory notice about the rename. We don't currently promote the crate anywhere and the dev guide will get updated so I'm on the fence for what we want to do.Testing
Operation
APIs.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.