-
Notifications
You must be signed in to change notification settings - Fork 15
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
patched requests at lower level #12
Conversation
Oh actually I added tests for this also, and github magic to run them which might well start to deal with #1 I need to head out, so I've pushed this PR before the continuous integration has run the tests, will fix if needed when I'm back later |
🤩🤩 Awesome work Joe! I was struggling to setup those tests. Really appreciate your effort! |
pip install pyodide-build pytest-pyodide | ||
# install chromedriver stuff for selenium to control chrome | ||
pip install selenium webdriver-manager | ||
pip install chromedriver-binary-auto |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When installing them via GH actions as in https://github.com/pyodide/micropip/blob/32117636152a253fddd041273a2c21cd00c449f1/.github/workflows/main.yml#L59 it's probably easier to make sure the chrome and chrome-driver versions match.
But this is already a very large improvement compared to when there were no tests. Great to see work on this package:
@rth I did see that - I think for this package for now, using chromedriver-binary-auto to ensure sync is good enough here, so I didn't do the full work on that. The sensible place to do work is on pytest-pyodide, which should have a github action template so that this wheel doesn't need reinventing for each project. pyodide/pytest-pyodide#38 |
(and fixed a bug that they found in the process)
@koenvo I fixed the tests - the way it was loading the web-server it wasn't ever testing streaming properly, because the I also implemented tests for parallel streaming etc., which is what exposed the problem with the web-server, which is neat and a sort of vindication of test-driven development. Oh and I fixed a bug with headers and added a test for that. I can't think of much else that needs a test now. |
👏fixes #13 |
I've just published a new version to PyPi: 0.1.0. Thanks for this great work @joemarshall and review @rth ! |
I changed how this patches requests, because the old one was missing out some stuff, like if you called
requests.Session
to create a session it was bypassed. It also lost quite a lot of the per-session logic of requests.I made a transport class instead, and patched that into the session class. This means it is almost using the official method that requests supports to override transports, the only dodgy monkeypatch is that requests has no support for changing the default transport class for all new sessions, so I patch it onto init