Skip to content

add test that checks for empty body on HEAD req #179

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

Merged
merged 4 commits into from
Jun 5, 2019
Merged

add test that checks for empty body on HEAD req #179

merged 4 commits into from
Jun 5, 2019

Conversation

fairingrey
Copy link
Contributor

@fairingrey fairingrey commented Apr 21, 2019

Attempts to address #45.

Description

This adds a test that checks if a response with an empty body is returned on a HEAD request to a resource where GET behavior exists.

For some reason it fails, so there might be something in http_service_mock that needs to be fixed? Attempting an HTTP request to a live server via curl/Insomnia exhibits the normal behavior e.g. #45 (comment), but a non-empty body is returned on server.simulate(req).unwrap(), as if it was making a GET request normally.

EDIT: See my comment below #179 (comment)

Motivation and Context

How Has This Been Tested?

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.

@fairingrey
Copy link
Contributor Author

Looking at it more in detail, I think I know why this doesn't work. Also related to #45 (comment).

My intuition was somehow that the http service and http service mock crates were using Hyper underneath -- this is wrong. Instead, the app server just has an implementation for HttpService via https://github.com/rustasync/tide/blob/master/src/app.rs#L246-L278 which http-service-mock emulates, but doesn't properly simulate the behavior that http-service-hyper exhibits when the app is probably served.

I'll probably start figuring out more on how to do this, but hmm...

use tide::{error::ResultExt, Context};

async fn ok(_cx: Context<()>) -> String {
String::from("this shouldn't exist in the body of a HEAD response")
Copy link
Member

Choose a reason for hiding this comment

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

I think if we want to make this a persistent test, just having this panic might be reasonable to signify it should never be executed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could do that... Maybe something like this, I'm guessing?

Suggested change
String::from("this shouldn't exist in the body of a HEAD response")
panic!("should not go here");
String::from("this shouldn't exist in the body of a HEAD response")

@yoshuawuyts
Copy link
Member

@fairingrey hmm, not sure either. I guess the obvious choice here would be to expand http-service-mock to include this behavior. But that does kind of feel like a band-aid.

Hmm, we could also fix this at the tide-level, where if the request is a HEAD we abort early?

Not sure which is better here.

@yoshuawuyts
Copy link
Member

Ping @fairingrey -- do you have an idea how to move forward here?

@fairingrey
Copy link
Contributor Author

fairingrey commented May 19, 2019

Mmm... I think I would rather just change http-service-mock to fit the test here. It is a bandaid, admittedly -- but since we already know how hyper behaves, it should be fine.

That said, I do think that approach won't necessitate the panic as you suggested originally here: #179 (comment) (The GET response shouldn't panic, but the HEAD response body should be empty)

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.

Let's merge this (:

@yoshuawuyts yoshuawuyts merged commit 7c2046a into http-rs:master Jun 5, 2019
@fairingrey fairingrey deleted the head_response_empty_body_test branch June 6, 2019 01:22
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.

2 participants