Skip to content

Update testing etc for Python 3.8 #75

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

Merged
merged 18 commits into from
Feb 19, 2020
Merged

Update testing etc for Python 3.8 #75

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
6cd9667
Update testing etc for Python 3.8
altendky Jan 6, 2020
57003d6
Merge branch 'master' into py38
altendky Jan 18, 2020
0a11d88
Merge branch 'master' into py38
altendky Jan 18, 2020
33a1d1a
Update appveyor.yml
altendky Feb 18, 2020
c6eff3d
Use asyncio.WindowsSelectorEventLoopPolicy for 3.8+
altendky Feb 18, 2020
adb46b6
Update pytest_twisted.py
altendky Feb 18, 2020
d5f5bf8
Update pytest_twisted.py
altendky Feb 18, 2020
84fd407
Update pytest_twisted.py
altendky Feb 18, 2020
54bfe45
Merge branch 'master' into py38
altendky Feb 18, 2020
08bc017
remove asyncio.WindowsSelectorEventLoopPolicy attempt
altendky Feb 18, 2020
277a4e9
Use asyncio.WindowsSelectorEventLoopPolicy() for applicable tests
altendky Feb 18, 2020
c0c9a15
Trying again for asyncio on windows on py38
altendky Feb 19, 2020
5e84a28
add missing trailing newline
altendky Feb 19, 2020
0bdf552
Add notes about py3.8/asyncio/proactor to readme
altendky Feb 19, 2020
41ba6e2
Make the proactor helper _private for now
altendky Feb 19, 2020
39f9fbb
Merge branch 'master' into py38
altendky Feb 19, 2020
22fd7c5
Add CPython 3.8 to GitHub Actions
altendky Feb 19, 2020
44130c8
remove whitespace from blank lines
altendky Feb 19, 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
3 changes: 3 additions & 0 deletions .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@ matrix:
- python: 3.7
dist: xenial
sudo: true
- python: 3.8
dist: xenial
sudo: true

addons:
apt:
Expand Down
6 changes: 6 additions & 0 deletions appveyor.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,12 @@ environment:
- TOXENV: py37-defaultreactor, win-py37-qt5reactor, py37-asyncioreactor
PYTHON: "C:\\Python37-x64"

- TOXENV: py38-defaultreactor, win-py38-qt5reactor, py38-asyncioreactor
PYTHON: "C:\\Python38"

- TOXENV: py38-defaultreactor, win-py38-qt5reactor, py38-asyncioreactor
PYTHON: "C:\\Python38-x64"

install:
# https://github.com/pypa/virtualenv/issues/1050
- pip install -U git+https://github.com/pypa/virtualenv@e8163e83a92c9098f51d390289323232ece15e3b
Expand Down
10 changes: 10 additions & 0 deletions pytest_twisted.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import functools
import inspect
import sys
import warnings

import decorator
Expand Down Expand Up @@ -272,6 +273,15 @@ def init_qt5_reactor():


def init_asyncio_reactor():
if sys.platform == 'win32':
if sys.version_info >= (3, 8):
# If twisted releases a fix/workaround we can check that version too
# https://twistedmatrix.com/trac/ticket/9766
import asyncio

selector_policy = asyncio.WindowsSelectorEventLoopPolicy()
asyncio.set_event_loop_policy(selector_policy)
Copy link
Collaborator

@cdunklau cdunklau Feb 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This is a bit extreme, don't you think? There's gotta be a nicer way of dealing with this... Come to think of it, this isn't really pytest_twisted's job at all, and it should probably not try to be clever.

Perhaps get_event_loop, check if it's a selector event loop, then issue a warning otherwise? I think a special case to make the tests pass would be reasonable, but not something in the lib like this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, I just realize the NotImplementedError happens when we install the reactor, so maybe just trap it and raise a contextual exception? On py3 it would just look like:

def init_asyncio_reactor():
    from twisted.internet import asyncioreactor
    try:
        _install_reactor(
            reactor_installer=asyncioreactor.install,
            reactor_type=asyncioreactor.AsyncioSelectorReactor,
        )
    except NotImplementedError as e:
        raise RuntimeError(
            "Failed to install AsyncioSelectorReactor. "
            "If you're on windows and python 3.8+, you might need to change "
            "the event loop policy to WindowsSelectorEventLoopPolicy."
        ) from e

...but that's invalid syntax in py2. On py2, NotImplementedError will still show up in the traceback, so it's not too bad, and to get syntax compat six.raise_from would work, or we could just borrow its MIT-licensed implementation as it would be kinda silly to involve six for one case. IANAL but the phrasing "all copies or substantial portions of the Software" does not seem like it would apply to 11 lines (we would still credit them accordingly anyway, 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.

As long as we support py2, a six dependency seems reasonable. It's not like we need to put a restrictive version requirement on it, I would assume. I'm not totally clear why doing a thing that works in a case where it otherwise won't is particularly bad. When either twisted or asyncio address this in a new version then a version check is added here to limit the application of this. I expected to add docs notes and perhaps a warning if this was actually run. And I suppose a check to see if it's needed.

But sure, just failing is simpler and simple is good. (with a good message on how to solve it) But, it just makes everyone else implement this code themselves. So, I dunno how to balance that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It is my passionate and not-entirely-uninformed opinion that a library should not muck around with global state, except where it's either somehow scoped to the lib (say, logging.getLogger(__name__)) or really necessary.

This situation is clearly not the former, and IMO not really the latter... the failure case is both easily detectable by pytest-twisted, and has a clear, easy (enough) to describe resolution. The burden to the end user is a matter of two lines at module scope in conftest.py (I think? We should probably test that...), and the benefit is not needing to hunt through other probably-nonsensical errors caused by the global fiddling.

OTOH, my feeling is that the intersection between the userbases of asyncio, twisted, pytest-twisted, and windows is small as it is, and might just vanish entirely when you intersect in Python 3.8 and people who depend on ProactorEventLoop. I wish it would be feasible to get some actual data on that 😐. Then again, the number will probably only increase.

Personal ideology and guessing aside, I still prefer the except NotImplementedError approach over "look at versions" for a more practical reason: version detection sucks. We don't actually care what Python version we're running under for this check, only if the event loop policy will give Twisted an incompatible event loop... and that can happen on pre-3.8.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe the best thing to do for this PR is to not attempt to handle it in the lib at all, and just special-case the tests. We can take another look, and with a working py38 CI, do some experiments in another PR to get a clearer picture of the various ways this issue could be triggered.

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 think I'm convinced. In the meantime we see that 3.8 works otherwise. NotImplementedError isn't very specific. :[ I'll see about special casing the tests.


from twisted.internet import asyncioreactor

_install_reactor(
Expand Down
1 change: 1 addition & 0 deletions setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
"Programming Language :: Python :: 3.5",
"Programming Language :: Python :: 3.6",
"Programming Language :: Python :: 3.7",
"Programming Language :: Python :: 3.8",
],
entry_points={"pytest11": ["twisted = pytest_twisted"]},
)
4 changes: 2 additions & 2 deletions tox.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
[tox]
envlist=
py{27,35}-defaultreactor
py{35,36,37}-{default,qt5,asyncio}reactor
win-py{35,36,37}-qt5reactor
py{35,36,37,38}-{default,qt5,asyncio}reactor
win-py{35,36,37,38}-qt5reactor
linting

[testenv]
Expand Down