- 
                Notifications
    You must be signed in to change notification settings 
- Fork 24
Add testing utilities to smithy-http #588
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: functional-testing-framework
Are you sure you want to change the base?
Add testing utilities to smithy-http #588
Conversation
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.
Thanks Alessandra! This looks great. Just left some minor comments about simplification and other nits.
| client_config: HTTPClientConfiguration | None = None, | ||
| ) -> None: | ||
| """ | ||
| :param client_config: Configuration that applies to all requests made with this | 
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.
FYI, I think we'll be using Google style docstrings moving forward (see #564). I think for now that would cause the CI to fail so this is more of a heads up.
| def add_response( | ||
| self, | ||
| status: int = 200, | ||
| headers: list[tuple[str, str]] | None = None, | 
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 line 49 we set fields to headers if it exists or to an empty list if it doesn't exist. Can we simplify this by defaulting headers to an empty list instead of making it an optional parameter with a default of None?
| method: str = "GET", | ||
| host: str = "test.aws.dev", | ||
| path: str | None = None, | ||
| headers: list[tuple[str, str]] | None = None, | 
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.
Same comment as above.
| response = await mock_client.send(request) | ||
|  | ||
| assert response.status == 201 | ||
| assert "Content-Type" in response.fields | 
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.
tiny nit: if we're checking Content-Type we should also check X-Amz-Custom
| from smithy_http.interfaces import HTTPClientConfiguration, HTTPRequestConfiguration | ||
|  | ||
|  | ||
| class MockHTTPClient(HTTPClient): | 
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.
General question, not a suggestion: Do we expect users to create a new MockHTTPClient if they need an empty client instead of exposing APIs to clean up the responses and requests?
Either works for me but wanted to know if that was considered.
| # smithy-http | ||
|  | ||
| This package provides primitives and interfaces for http functionality in tooling generated by Smithy. | ||
|  | 
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.
Thanks for adding usage documentation! Once we have documentation for Smithy Python, we should add a top-level section for testing that displays this information. Right now users have to stumble on smithy-http's README to find this information. It should be more direct and easy to find when we have docs set up.
| @@ -0,0 +1,4 @@ | |||
| { | |||
| "type": "feature", | |||
| "description": "Added support for minimal components required for SDK functional 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.
nit: I think we can be more specific in describing what exactly was added. We should mention that MockHTTPClient was added by name.
| from .mockhttp import MockHTTPClient, MockHTTPClientError | ||
| from .utils import create_test_request | ||
|  | ||
| __version__ = "0.0.0" | 
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 may have been left over from the smithy-testing package, but I don't think we should include a version if this is now part of smithy-http.
Issue #, if available:
Description of changes:
MockHTTPClientto test generated clients without hitting live endpoints.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.