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

Try to make integration tests pass #1

Merged
merged 1 commit into from
Jul 16, 2022

Conversation

sethmlarson
Copy link

Trying to make integration tests pass for now while we work towards a way of handling the HTTP lifecycle.

@sethmlarson
Copy link
Author

@shadycuz Btw you'll have to approve running workflows since this is the first time I'm contributing to your repository ;)

@shadycuz
Copy link
Owner

@sethmlarson let me know when you want this merged.

I'll try to look through it tomorrow.

@shadycuz
Copy link
Owner

@sethmlarson Is there something I can do to speed this along? The boto tests are fixed, but not requests.

I see your new config block:

if self._response_options is None:
    raise ResponseNotReady()

Is killing the request tests, it seems they call getresponse() directly and not the urlib3.request mixin. I'm not sure what I can do to help, but I will look and see how they are passing the "response_kwargs", but it's likely they were just taking advantage of the default values and possibly not passing any?

@shadycuz
Copy link
Owner

Okay so if its not chunked, then they call conn.urlopen() and if it is chunked they seem to be doing all the work manually?

https://github.com/psf/requests/blob/bda7f0171f8bba17989d3a2c28dfa9a9261b1b65/requests/adapters.py#L488-L528

But even if we didn't break getresponse(), they very next call is HTTPResponse.from_httplib() which has been removed 🤔

https://github.com/psf/requests/blob/bda7f0171f8bba17989d3a2c28dfa9a9261b1b65/requests/adapters.py#L531-L539

@sethmlarson
Copy link
Author

@shadycuz Oh dang, forgot that Requests was using all those low-level APIs. Going to work with @nateprewitt to see if we can move Requests away from using these since they'll be going away in v2.0 anyways.

@sethmlarson
Copy link
Author

An update on this: chatted with @nateprewitt and he agreed to change the function to use our chunking instead of custom code in Requests. Once that is changed this PR should begin working.

Are you okay with reviewing or merging this PR @shadycuz?

@shadycuz
Copy link
Owner

@sethmlarson I have already reviewed, I didn't understand everything, but I understood most.

@shadycuz shadycuz merged commit 67df385 into shadycuz:refactor_http_response Jul 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants