-
Notifications
You must be signed in to change notification settings - Fork 361
enable unit testing of http handlers by way of test cfg-only mock helpers #85
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
Conversation
where | ||
Q: Into<StrMap>, | ||
{ | ||
let mut s = 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.
I would have used mut self
here except that syntax is being phased out. See this post for more details on why
/// Configures instance with query string parameters under #[cfg(test)] configurations | ||
/// | ||
/// This is intended for use in mock testing contexts. | ||
#[cfg(test)] |
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.
notes the cfg(test)
attributes. the net effect is that these methods are only available for tests and keeps the surface for the runtime interface focused on read only access
/cc @wbprice |
the travis error doesn't seem related to these changes |
No, the travis error's related to toolstate. I think these values can and should be constructed in non-test targets as well. Do you think it makes sense to provide |
These are extensions to the standard http types specific to aws alb/api gateway. There isn't really a corollary in the http crate to path parameters/query string parameters/stage variables. The reason why I don't want to expose these for non test targets is because there's not actually a usecase for that. Its the trigger that provides these so the current interfaces expose just that, a read only interface. The only use case where'd you want to populate these your self is in a test context. |
I'm starting to see the same travis error in my other projects. I learned in my journey theres a few rust project links that are helpful to keep an eye on listed here rust-lang/rustfmt#3404 (comment) |
@davidbarsky just checking to see if anyone still reviewing this. the travis error is unrelated to the changes https://travis-ci.org/awslabs/aws-lambda-rust-runtime/jobs/493590501 but does look like the rustfmt gone missing thats trolling everyone's nightly builds. I could open a separate pull to fix that for this projects build if you want |
@softprops Apologies for the delay. I think this is a good PR and conservatively-scoped—I'll merge this one in. |
Issue #, if available:
Description of changes:
In a discussion on unit testing in softprops/serverless-aws-rust-multi#1 (comment) it came to mind that one drawback of the current interface for the http lambda crate was that there is no way to manually create a request to test a handler that depends on the data returned from one of the
RequestExt
methods. This is partially intentional as the extension types are a private impl detail.I'm deeply sympathetic the limitations of this drawback this because I'm a big believer in tests. This pull represents an interface that still respects that and should only be exported in
cfg(test)
builds making them available in unit test contexts only.Here's a sketch of what this pull makes possible
By submitting this pull request