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

[MRG+1] Add test for webclient with POST method and no body given #1089

Merged
merged 4 commits into from Mar 31, 2015

Conversation

@persiyanov
Copy link
Contributor

@persiyanov persiyanov commented Mar 21, 2015

Discussed in #823

…or POST request with no given body
@kmike
Copy link
Member

@kmike kmike commented Mar 21, 2015

👍

Could you please move this check to a separate method and mark it as expectedFailure? Use https://docs.python.org/2/library/unittest.html#unittest.expectedFailure if it works (I'm not sure it'll work here), something pytest-specific if it doesn't work.

@persiyanov persiyanov force-pushed the persiyanov:master branch from deb5bb5 to 5498825 Mar 22, 2015
@persiyanov
Copy link
Contributor Author

@persiyanov persiyanov commented Mar 22, 2015

@kmike I have added few lines to ScrapyHTTPClientFactory implementation and now it passes the test. Could you check this?

persiyanov added 2 commits Mar 22, 2015
…Factory for POST request with no given body
…Factory for POST request with no given body
@kmike
Copy link
Member

@kmike kmike commented Mar 23, 2015

LGTM.

I think we shouldn't set Content-Length header if it is already set, but it is not related to this issue because when body is not None scrapy also overrides the header.

@kmike kmike changed the title Add test for webclient with POST method and no body given [MRG+1] Add test for webclient with POST method and no body given Mar 23, 2015
@persiyanov
Copy link
Contributor Author

@persiyanov persiyanov commented Mar 23, 2015

Thanks for feedback, should I correct something?

@kmike
Copy link
Member

@kmike kmike commented Mar 23, 2015

I think this PR is fine as-is.

We usually require 2 team members to review the PR before merging (unless the PR is obvious); let's wait for someone else to check & merge it :)

@persiyanov
Copy link
Contributor Author

@persiyanov persiyanov commented Mar 23, 2015

Ok!

@nyov
Copy link
Contributor

@nyov nyov commented Mar 25, 2015

@drack3800, I know i'm nitpicking but please don't add whitespace on empty lines? same in your referenced bugfix. Thanks.

@persiyanov
Copy link
Contributor Author

@persiyanov persiyanov commented Mar 25, 2015

@nyov fixed.

dangra added a commit that referenced this pull request Mar 31, 2015
[MRG+1] Add test for webclient with POST method and no body given
@dangra dangra merged commit ec4251a into scrapy:master Mar 31, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
redapple added a commit to redapple/scrapy that referenced this pull request Feb 20, 2016
redapple added a commit that referenced this pull request Feb 24, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.