Skip to content
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

Merged
merged 1 commit into from
Oct 3, 2020

Conversation

jbr
Copy link
Member

@jbr jbr commented Sep 14, 2020

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:

#[cfg(test)]
mod tests {
    use super::*;

    pub fn test_client<State>(server: Server<State>) -> Client
    where
        State: Debug + Unpin + Sync + Send + Clone + 'static,
    {
        let mut client = Client::with_http_client(Arc::new(server));
        client.set_base_url(Url::parse("http://example.com/").unwrap());
        client
    }

    #[async_std::test]
    async fn test() -> tide::Result<()> {
        let app = build_app().await;
        let body = test_client(app).get("/").recv_string().await?;
        Ok(())
    }
}

If http-rs/surf#229 is merged, this would allow:

let app = tide::new();
use surf::ClientExt;
app.get("http://example.com/").recv_string()?;

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

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Copy link
Member

@yoshuawuyts yoshuawuyts left a 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.

@yoshuawuyts
Copy link
Member

Also cc/ @LukeMathWalker; author of wiremock -- curious to get your input on this!

@jbr
Copy link
Member Author

jbr commented Sep 19, 2020

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 testing feature or published in a very small http-rs/tide-testing crate

Copy link
Member

@Fishrock123 Fishrock123 left a 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 }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
http-client = {version = "4.0.0", default-features = false }
http-client = { version = "4.0.0", default-features = false }

@jbr
Copy link
Member Author

jbr commented Sep 20, 2020

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)

@LukeMathWalker
Copy link
Contributor

LukeMathWalker commented Sep 20, 2020

This looks really cool 😁

As a web framework user, it's awesome to have a high-level API to interact with an app instance.
Generally speaking though, I prefer to have black-box API tests that actually hit the network. It would be cool to have an app.spawn() method that launches the app in the background and returns me its address to perform HTTP calls.
You could then build on top of it (and have this very same API) by providing an app.client() which spawns an app in the background and returns me a HttpClient instance whose base uri has already been set to match the one from the application.

Copy link
Member

@Fishrock123 Fishrock123 left a 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.

@jbr jbr mentioned this pull request Sep 21, 2020
@jbr
Copy link
Member Author

jbr commented Oct 3, 2020

@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 test_utils::ServerTestingExt

@jbr
Copy link
Member Author

jbr commented Oct 3, 2020

Merging this because of previous approvals and minimal public api (just an impl, no exports)

@jbr jbr merged commit 904a45e into http-rs:main Oct 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants