Add Windows to CI (plus associated fixes)#287
Conversation
Otherwise coverage tries to run `trial` but it is `trial.exe`.
Codecov Report
@@ Coverage Diff @@
## master #287 +/- ##
==========================================
+ Coverage 89.93% 95.20% +5.27%
==========================================
Files 11 20 +9
Lines 457 1001 +544
Branches 91 102 +11
==========================================
+ Hits 411 953 +542
- Misses 25 27 +2
Partials 21 21
Continue to review full report at Codecov.
|
.github/workflows/ci.yml
Outdated
| - name: Test | ||
| tox: tests | ||
| exclude: | ||
| # Twisted and thus trial don't work on Windows with CPython 3.9 |
There was a problem hiding this comment.
Maybe link to a Trac ticket for Twisted... I am sure this will be fixed ... or maybe is already fixed and just waiting for a new release.
But maybe it is worth migrating towcrier tests to stdlib unit test. In this way we will not depend on twisted for running the tests.
There was a problem hiding this comment.
Trial really has a bunch of nice facilities compared to stdlib unittest. There are other options, of course, like pytest and testtools, but you're going to run into exactly the same issues with anything that's not part of the stdlib itself.
As far as Twisted's 3.9 release goes, there's https://twistedmatrix.com/trac/ticket/10027 at least
src/towncrier/test/test_check.py
Outdated
| call(["git", "init"]) | ||
| # The default git branch name may not be master, let's make sure we | ||
| # are consistent here. | ||
| call(["git", "checkout", "-b", "master"]) |
There was a problem hiding this comment.
Do we have to pick master here? It wouldn't surprise me if the answer is yes because other things still assume/use master... But if it works to use some other name like "main" then maybe that would be a better choice.
There was a problem hiding this comment.
I can switch it over. I think there was one test that explicitly passed master to towncrier as the reference branch. I could also just make the test check the primary branch before creating the 'PR' branch and then specify no name anywhere. I'll do one of the two unless you make a specific request.
There was a problem hiding this comment.
So I just removed this all together for now. This wasn't actually a Windows related issue and only showed up because I happened to install an updated Git in my Windows VM and allowed it to default to main instead of master. So, it's a real issue in regards to an assumption made by the tests about external configuration, but not a Windows issue nor one that CI is presently forcing us to fix.
| coverage run -p {envbindir}/trial {posargs:towncrier} | ||
| # coverage fails to run trial on Windows since it is trial.exe | ||
| # so just use --module. | ||
| coverage run -p --module twisted.trial {posargs:towncrier} |
There was a problem hiding this comment.
May not make any difference but this could also be python -m coverage .... I expect the right command is on PATH thanks to tox ... but a few lines above it looks like you thought python -m twisted.trial ... was a good idea, so maybe it's a good idea for coverage too?
There was a problem hiding this comment.
The -m twisted.trial was for consistency with this line which switched as explained in the comment. But sure, could do it for everything. I'll look, or maybe another PR to switch everything if there are several unrelated places where -m would make sense.
There was a problem hiding this comment.
Hmm, so tox only warns about outside access and doesn't, right there anyways, list an option to make that an error. Anyways, there are several other places this would apply in general so I won't change them here. I did update the comment to better represent coverage's perspective on the run subcommand.
4 participants
No description provided.