-
-
Notifications
You must be signed in to change notification settings - Fork 22
Conversation
This test is about timeouts, we don't need to test _make_request specifically here.
It's used throughout the test so it's better to explain that it's a short timeout.
We had an interesting test failure in CI where this test failed twice in a row: we were expecting a timeout in 0.001 seconds, but both times it took more than one second! How can this happen? Well, when we ask a request to timeout after N seconds, here's what we know: * the request will time out, eventually * it won't timeout before those N seconds * it could timeout a long time after those N seconds, especially on a busy CI service Let's take advantage of those facts by specifying a request-specific timeout that's longer than the pool-level timeout and making sure that the timeout was long indeed. We still test that the request-specific timeout overrides the pool-level timeout, but will stop failing on busy CI services.
Both files are Python so it makes sense to merge them, and it will help with CI configuration.
Keep testing 2.7 with AppVeyor as GitHub Actions makes it hard to test Python 2.7 on Windows and Nox.
e81495e
to
7a31c47
Compare
Codecov Report
@@ Coverage Diff @@
## bleach-spike #153 +/- ##
============================================
Coverage 99.3% 99.3%
============================================
Files 29 29
Lines 2009 2009
============================================
Hits 1995 1995
Misses 14 14 |
codecov is lying, src/urllib3/util/connection.py is fully covered, see https://codecov.io/gh/python-trio/hip/pull/153/src/src/urllib3/util/connection.py |
if sys.version_info >= (3, 8) and platform.system() == "Windows": | ||
import asyncio | ||
|
||
asyncio.set_event_loop_policy(asyncio.WindowsSelectorEventLoopPolicy()) |
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 going to be a problem at some point – hip itself will need to support, and be tested on, the default event loop :-/. (Not something for this PR to worry about of course.)
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, opened #154 so that we don't forget about it
I added the CODECOV_TOKEN |
I used the work-around from wntrblm/nox#233 (comment) (and copied stuff from their configuration file) to fully move to GitHub Actions for Windows tests. Now we can really remove AppVeyor. :) Thanks @njsmith for the help here! |
Woohoo! I edited the branch protection to remove appveyor as a required check. Can you replace |
@njsmith Thanks, done! |
Hmm, I meant replacing the |
But we don't know what the future will look like :) Maybe we'll have multiple OSes in the same "build", or maybe we'll have different files. I still made the change, and it does look better, thanks! |
Thank you for the review! |
This cherry-picks a few commits from urllib3, fixes Python 3.8 on Windows, moves cleancov.py inside noxfile.py (the previous setup was failing), and uses GitHub Actions to test Python 3.5 to 3.8.
I think GitHub Actions is going to pick up the configuration file, however @njsmith will need to copy the CODECOV_TOKEN secret from https://codecov.io/gh/python-trio/hip/settings to an encrypted variable in the hip repo. Coverage upload wasn't working on AppVeyor and is still not working: I intend to work on it later.