Skip to content
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.

Sign up for GitHub

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

Add Windows to CI (plus associated fixes) #287

Merged
merged 19 commits into from
Dec 7, 2020
Merged

Add Windows to CI (plus associated fixes) #287

Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
e97c46e
Matrix against Linux/macOS/Windows
altendky Dec 3, 2020
1759685
Use -m twisted.trial in tox.ini for Windows
altendky Dec 4, 2020
ff26db8
fix for Windows
altendky Dec 4, 2020
01a210e
use io.open() for encoding= for py2 compatibility
altendky Dec 4, 2020
d7dc7e2
os.path.normpath instead of os.path.sep
altendky Dec 4, 2020
224d1e8
macOS will be added separately now
altendky Dec 4, 2020
00f3c0c
Merge branch 'master' into test_more_operating_systems
altendky Dec 5, 2020
c974f05
Add 287.misc.rst
altendky Dec 6, 2020
0a2d65c
Merge branch 'master' into test_more_operating_systems
altendky Dec 6, 2020
51b43c5
Explain git checkout -b master
altendky Dec 6, 2020
4314093
Explain coverage run --module twisted.trial
altendky Dec 6, 2020
5d3d920
Update test_check.py
altendky Dec 6, 2020
96d490f
simplify
altendky Dec 6, 2020
411dcd2
exclude 3.9 on windows due to trial (twisted)
altendky Dec 6, 2020
daf3d01
comment on why windows/cp39 are excluded from ci
altendky Dec 6, 2020
9fb95e4
Expound upon comment about 3.9 on windows
altendky Dec 6, 2020
0f049d6
Remove explicit setting of branch as master
altendky Dec 6, 2020
e20a790
adjust coverage run call and comment
altendky Dec 7, 2020
55f4966
Update tox.ini
altendky Dec 7, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ jobs:
runs-on: ubuntu-latest
- name: macOS
runs-on: macos-latest
- name: Windows
runs-on: windows-latest
python:
- name: CPython 2.7
tox: py27
Expand Down Expand Up @@ -46,6 +48,12 @@ jobs:
task:
- name: Test
tox: tests
exclude:
# Twisted and thus trial don't work on Windows with CPython 3.9
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True. Thanks for the link.

- python:
tox: py39
os:
name: Windows

steps:
- uses: actions/checkout@v2
Expand Down
5 changes: 3 additions & 2 deletions src/towncrier/_writer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

from __future__ import absolute_import, division

import io
import os


Expand All @@ -21,8 +22,8 @@ def append_to_newsfile(
if not os.path.exists(news_file):
existing_content = u""
else:
with open(news_file, "rb") as f:
existing_content = f.read().decode("utf8")
with io.open(news_file, "r", encoding="utf8") as f:
existing_content = f.read()
existing_content = existing_content.split(start_line, 1)
else:
existing_content = [u""]
Expand Down
17 changes: 8 additions & 9 deletions src/towncrier/check.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,12 +48,10 @@ def __main(comparewith, directory, config):
click.echo("On trunk, or no diffs, so no newsfragment required.")
sys.exit(0)

files = set(
map(
lambda x: os.path.join(base_directory, x),
files_changed.strip().split(os.linesep),
)
)
files = {
os.path.normpath(os.path.join(base_directory, path))
for path in files_changed.strip().splitlines()
}

click.echo("Looking at these files:")
click.echo("----")
Expand All @@ -72,14 +70,15 @@ def __main(comparewith, directory, config):
)
fragment_directory = "newsfragments"

fragments = set(
find_fragments(
fragments = {
os.path.normpath(path)
for path in find_fragments(
fragment_base_directory,
config["sections"],
fragment_directory,
config["types"],
)[1]
)
}
fragments_in_branch = fragments & files

if not fragments_in_branch:
Expand Down
Empty file added src/towncrier/newsfragments/287.misc.rst
View file
Open in desktop
Empty file.
3 changes: 3 additions & 0 deletions src/towncrier/test/test_check.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@ def create_project(pyproject_path):
f.write("Adds levitation")

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

#294

call(["git", "config", "user.name", "user"])
call(["git", "config", "user.email", "[email protected]"])
call(["git", "add", "."])
Expand Down
5 changes: 4 additions & 1 deletion src/towncrier/test/test_settings.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,10 @@ def test_missing_template(self):
load_config(temp)

self.assertEqual(
str(e.exception), "The template file '%s/foo.rst' does not exist." % (temp,)
str(e.exception),
"The template file '{}' does not exist.".format(
os.path.normpath(os.path.join(temp, "foo.rst")),
)
)

def test_missing_template_in_towncrier(self):
Expand Down
6 changes: 4 additions & 2 deletions tox.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,11 @@ deps =
commands =
python -V
coverage --version
{envbindir}/trial --version
python -m twisted.trial --version
coverage erase
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}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://tox.readthedocs.io/en/latest/config.html#conf-allowlist_externals

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.

coverage combine -a

[testenv:cov-report]
Expand Down