Fix breakage introduced by 8f591682

[Lukasa]

Turns out we broke chunked uploads in 8f59168. I discovered this while investigating #2215.

While we were here, I added a test that would at least have caught errors as egregious as this one.

[Lukasa]

To note: this breakage did not make it into any shipped version of requests, so we don't need to schedule a patch release for this.

[sigmavirus24]

Why is the build failing?

[Lukasa]

Good question. The test behaves fine on my
Mac, but it's a tricky one because chunked encoding and WSGI don't necessarily play nice.

On 5 Nov 2015, at 19:02, Ian Cordasco notifications@github.com wrote:

Why is the build failing?

—
Reply to this email directly or view it on GitHub.

[Lukasa]

Hmm, so this failure is intermittent on my machine.

[Lukasa]

(my machine == Linux box I found)

[Lukasa]

I wonder if this is actually a problem with the WSGI server being used by pytest-httpbin. Specifically, I think it might be related to the speed of the server.

[Lukasa]

Specifically, I think what's happening here is that the chunked-encoding header is being stripped, and so the server is sending the 200 right away and then killing the connection. This works sometimes because the socket sends happen before the connection is closed, but sometimes the server closes the connection before we can get there.

This is pretty annoying, because it means it's likely very hard to find a good test-case here.

[Lukasa]

More importantly, it means we can't really have tests at all for chunked transfer encoding.

[Lukasa]

I honestly don't know what to do about this. To get a test that confirms we don't break this we need either to change the WSGI server used by pytest-httpbin (the only options are ones that are really heavyweight and probably not suitable) or start writing new tests that don't use a httpbin at all.

The second is plenty do-able, it just requires some py.test test fixtures and some socket work. But it makes me a bit nervous given @kennethreitz's attitude to changing our test layout in the past: I'd want to move all the tests to a test/ subdirectory so that I can add the necessary conf.py file, and also start writing new tests that explicitly use sockets rather than a web server.

@kennethreitz, do you have any objection to me adding some more thorough testing around here? The reality is that our chunked upload logic is entirely untested, and has been since it was originally written. That is a recipe for accidental breakage, and it makes me really nervous.

[Lukasa]

Ok, so I pushed a new commit that doesn't contain the test. This should pass, and the fix is right, but I don't want us to lose track of the need for better testing here so I'm going to open a separate issue for that.

[Lukasa]

@sigmavirus24 This should be good to merge.

[sigmavirus24]

@Lukasa I'm 👍 but I think we should work on fixing our test suite. If Kenneth comes along and decimates it again, then we at least had test coverage for some period of time there.