Skip to content
This repository has been archived by the owner on Apr 14, 2022. It is now read-only.

Run Windows tests on GitHub Actions #153

Merged
merged 10 commits into from
Nov 26, 2019

Conversation

pquentin
Copy link
Member

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.

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.
@codecov
Copy link

codecov bot commented Nov 26, 2019

Codecov Report

Merging #153 into bleach-spike will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff              @@
##           bleach-spike    #153   +/-   ##
============================================
  Coverage          99.3%   99.3%           
============================================
  Files                29      29           
  Lines              2009    2009           
============================================
  Hits               1995    1995           
  Misses               14      14

@pquentin pquentin changed the title Test Windows 3 on GitHub Actions Test Windows Python 3 on GitHub Actions Nov 26, 2019
@pquentin
Copy link
Member Author

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())
Copy link
Member

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.)

Copy link
Member Author

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

@njsmith
Copy link
Member

njsmith commented Nov 26, 2019

I added the CODECOV_TOKEN

@pquentin pquentin changed the title Test Windows Python 3 on GitHub Actions Run Windows tests on GitHub Actions Nov 26, 2019
@pquentin
Copy link
Member Author

pquentin commented Nov 26, 2019

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!

@njsmith
Copy link
Member

njsmith commented Nov 26, 2019

Woohoo! I edited the branch protection to remove appveyor as a required check.

Can you replace build: with Windows:, so the check names are more meaningful?

@pquentin
Copy link
Member Author

@njsmith Thanks, done!

@njsmith
Copy link
Member

njsmith commented Nov 26, 2019

Hmm, I meant replacing the build: line with Windows:, so that the "job" is named Windows instead of the whole CI run. I guess this works too for now, but my way is more future-proof in case we start moving more runs from travis to github actions :-)

@pquentin
Copy link
Member Author

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!

@njsmith
Copy link
Member

njsmith commented Nov 26, 2019

It turns out that only the "job name" shows up in the branch protection UI:

image

So this is definitely nicer in that respect :-)

@njsmith njsmith merged commit 056c309 into python-trio:bleach-spike Nov 26, 2019
@pquentin
Copy link
Member Author

Thank you for the review!

@pquentin pquentin deleted the github-actions branch November 26, 2019 10:33
@pquentin pquentin mentioned this pull request Nov 27, 2019
5 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants