8000 Add ContentLength property in mocked http.Response by browny · Pull Request #20 · jarcoal/httpmock · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Add ContentLength property in mocked http.Response #20

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 17, 2019
Merged

Add ContentLength property in mocked http.Response #20

merged 1 commit into from
Mar 17, 2019

Conversation

browny
Copy link
@browny browny commented Sep 22, 2016

When using httpmock with grequests http client, the grequests.Response.String() method will fail to read http response body because the property ContentLength of the mocked http.Response is 0. (When ContentLength == 0, grequests doesn't copy http response body to its own Response, refer to here)

According to the godoc of http.Response, I think make it as -1 would be good.

// ContentLength records the length of the associated content. The
// value -1 indicates that the length is unknown. Unless Request.Method
// is "HEAD", values >= 0 indicate that the given number of bytes may
// be read from Body.

thanks :)

@dlebech
Copy link
Collaborator
dlebech commented Dec 10, 2016

@browny Thanks for the PR! I have two requests:

  1. Given that you have access to the actual response body when setting this value, I think it would be better to set the correct ContentLength based on the length of the string and the byte slice.
  2. If you could also add a test that asserts the correct length and possibly test this with either something like io.CopyN() or simply resp.Body.Len() == resp.ContentLength. Something along those lines would be awesome 😃

@dlebech dlebech mentioned this pull request Dec 10, 2016
Closed
@dlebech dlebech changed the base branch from master to v1 December 10, 2016 15:20
@maxatome maxatome merged commit b43b330 into jarcoal:v1 Mar 17, 2019
@vrnvu vrnvu mentioned this pull request Dec 20, 2022
Closed
@maxatome maxatome mentioned this pull request Jan 16, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@browny @dlebech @maxatome @ikala-browny
0