-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Run test_requesting_large_resources_via_ssl separately #3181
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
Conversation
b8177d9
to
a6f2149
Compare
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.
This is excellent and I love the result for CI times! Now I'm inspired to look into the macOS times 🚀
Thanks @pquentin, this is a great idea! Based on the following docstring, the library code relevant to the test may work differently in CPython 3.8, 3.9, and in any version with the pyOpenSSL injection. urllib3/src/urllib3/response.py Lines 763 to 767 in f7cd7f3
What do you think about running the test in CPython 3.8, 3.9, and the latest one to make sure that no changes to |
Excellent point, thank you. I'm wondering if I should not instead try to monkeypatch the calls that CPython is making to avoid decrypting multiple gigabytes of actual SSL data. If it proves difficult, then I'll run the integration tests on 3.8, 3.9 and latest. How does that sound? |
@pquentin I'm curious to see what can be patched to avoid that. IIRC there are three approaches CPython uses in different versions:
They have different consequences for urllib3. Also, when pyOpenSSL is used, the library is responsible for the decryption. |
I'm liking the idea to run it on 3.8, 3.9, and latest if only to speed up every other PRs CI time for now? Quite the improvement, would like to take advantage 👀 |
I still think the mocking idea could work, but I'd prefer focusing my energy on more useful things! Added 3.8 and 3.9. |
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.
Thanks!
We have many timeouts on GitHub Actions due to those tests that can take minutes to run on overloaded CI runners. This is compounded by the fact they run six times (3 parametrizations * 2 SSL configuration - pyopenssl and standard library SSL). Running on multiple platforms isn't useful. We found a "Windows-specific" bug once but only because the Windows worker had less memory, and we have
limit_memory
tests now for the memory aspects.