-
Notifications
You must be signed in to change notification settings - Fork 325
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
add a http_client::HttpClient implementation for tide::Server #697
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.
This PR is fantastic and absolutely something we should do. I'm very much sold on the end-user API this provides, and I think it'll make Tide's testing workflow second to none.
Also cc/ @LukeMathWalker; author of wiremock -- curious to get your input on this! |
I've been thinking a bit more about this, and if there's hesitation about the surf PR, we could achieve the same api by just publishing a tide trait behind a |
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 seems fine to me, pending the Surf PR, which should probably be finalized first.
Cargo.toml
Outdated
@@ -47,6 +47,7 @@ pin-project-lite = "0.1.7" | |||
route-recognizer = "0.2.0" | |||
serde = "1.0.102" | |||
serde_json = "1.0.41" | |||
http-client = {version = "4.0.0", default-features = false } |
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.
http-client = {version = "4.0.0", default-features = false } | |
http-client = { version = "4.0.0", default-features = false } |
This PR doesn't depend on the surf PR at all, and in fact we'll want another tide PR to use the surf pr instead of the test_utils::TestingExt trait (which is currently used in this pr, in order to make them independent) |
This looks really cool 😁 As a web framework user, it's awesome to have a high-level API to interact with an app instance. |
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.
After looking again I agree in http-rs/surf#229 (comment) that the Surf PR is in no way fundamental.
32d81cb
to
91ef8b0
Compare
@yoshuawuyts @Fishrock123 I revisited this PR and scaled it back. The only public change as of the latest commit is the addition of the HttpClient impl for tide::Server, which hopefully will make it less controversial. We can iron out the rest of the details in further PRs, but this will at least allow applications to build their own testing extensions along the lines of |
eb089b0
to
f960b9d
Compare
Merging this because of previous approvals and minimal public api (just an impl, no exports) |
In order to facilitate testing, it is convenient for tide::Server to implement http_client::HttpClient. This allows surf to be used for testing, with only the following shim:
If http-rs/surf#229 is merged, this would allow:
this is tested by using it throughout the tide testing suite as the basis for test_utils::ServerTesting
Drawbacks/Limitations
This adds a dependency on http-client, but that should be pretty lightweight without default features.
Types of changes
Checklist: