Fix TypeError when importing pytest on Python 3.5.0 and 3.5.1 by bluetech · Pull Request #5752 · pytest-dev/pytest

bluetech · 2019-08-16T07:33:55Z

The typing module on these versions have these issues:

typing.Pattern cannot appear in a Union since it is not considered a
class.
@overload is not supported in runtime. (On the other hand, mypy
doesn't support putting it under if False, so we need some runtime
hack).

nicoddemus

Awesome @bluetech, thanks a lot for the quick fix.

In the future, I suggest you add "Fix ###" instead of "Refs: ###" as this will close the issue automatically when merged. 😉

changelog/5751.bugfix.rst

src/_pytest/_code/code.py

src/_pytest/python_api.py

bluetech · 2019-08-16T12:38:37Z

Thanks for the review @nicoddemus.

The reason I didn't close the issue is that I thought it'd be best to fix this regression quickly for 5.1.x, and then use the issue to discuss how to proceed. The options as I see them are:

Keep Python 3.5.{0,1} support - in this case we should probably add it to CI (or change the 3.5 job to use it), move the hacks to _compat.py and also update flake8-typing-imports to detect these edge cases. Otherwise it is bound to regress again.
Drop support for Python 3.5.{0,1} - would make typing easier since it's missing a lot of things.

For selfish reasons, my preference is obviously 2 :)

src/_pytest/_code/code.py

The typing module on these versions have these issues: - `typing.Pattern` cannot appear in a Union since it is not considered a class. - `@overload` is not supported in runtime. (On the other hand, mypy doesn't support putting it under `if False`, so we need some runtime hack). Refs pytest-dev#5751.

bluetech · 2019-08-17T12:43:56Z

Updated per comments, except for moving to compat, as explained above.

asottile · 2019-08-17T12:47:30Z

Updated per comments, except for moving to compat, as explained above.

looks like the comment about compat didn't make it through -- I assume it's because mypy won't understand it

nicoddemus · 2019-08-17T14:18:40Z

looks like the comment about compat didn't make it through -- I assume it's because mypy won't understand it

Even for overload?

Also @bluetech, would it be possible to add 3.5.0 to CI?

nicoddemus · 2019-08-17T14:19:35Z

Here's how trio does it:

https://github.com/python-trio/trio/blob/4512ae3ca199f0a99d02f6ff2a6f8e94dd08f1ef/.travis.yml#L17-L18

I think we can only add it to Travis as well, skipping Azure. 👍

asottile · 2019-08-17T16:46:45Z

I should probably also make flake8-typing-imports smarter about this -- we should really be able to figure most of this out statically 🤔

Ref: pytest-dev#5752 (comment)

blueyed · 2019-08-17T20:44:05Z

I am not a fan of supporting 3.5.0 etc.
It turns out to even be non-trivial to test this on CI: #5757

I can see that Python 3.5 is still supported in general (until September 2020), but would vote for dropping support of it soon in pytest, especially given that we can benefit a lot from type annotations, and py35 make this much more unpleasant.

asottile · 2019-08-17T20:47:20Z

I don't think we can drop py35 quite yet, maybe 3.5.0 / 3.5.1 though -- but in order to do that, we need to have a latest release that works correctly with those versions and then we can exclude them with python_requires -- so either way we need to fix this

that said, I think if I fixed flake8-typing-imports to recognize these cases then we can ~reasonably rely on that without having to actually test 3.5.0 / 3.5.1

blueyed · 2019-08-17T20:50:20Z

@asottile
Would that also cover #5751 then?

asottile · 2019-08-17T20:54:26Z

yes that would be the goal

Ref: pytest-dev#5752 (comment)

blueyed · 2019-08-17T21:14:16Z

Pulled in the testing with 3.5.0 on Travis.

src/_pytest/python_api.py

asottile

blueyed · 2019-08-19T00:00:24Z

TestRaises.test_raises_cyclic_reference[with] fails on 3.5.0: https://travis-ci.org/pytest-dev/pytest/jobs/573541272#L5590

Zac-HD · 2019-08-20T04:48:37Z

.travis.yml

    - env: TOXENV=py35-xdist
-      python: '3.5'
+      dist: trusty
+      python: '3.5.0'


Based on my experience with Hypothesis, we need tests to ensure that Pytest's typing-related things don't regress on 3.5.0 or 3.5.x, meaning an additional environment rather than pinning to .0.

It's kinda annoying, but there we are - it's stuff like this that makes me worried about the exotic proposals for CPython release cadence and sympathetic to the "batteries are leaking" argument.

We have 3.5.x in Azure, no?
btw: Django only supports the latest patch releases of Python versions.

Comments were addressed.

bluetech · 2019-08-20T09:30:10Z

I added a workaround for the TestRaises.test_raises_cyclic_reference[with] test. CI looks happy now. I think we can merge it now.

RonnyPfannschmidt · 2019-08-20T09:59:16Z

i just catched up on prs, it looks like this one also sorts out #5770 as i

bluetech · 2019-08-20T10:12:19Z

@RonnyPfannschmidt yes, since flake8-typing-imports was updated to catch the overload thing, and the flake8-typing-imports dependency wasn't pinned, it started to fail. This PR pins it and also fixes the lint.

RonnyPfannschmidt · 2019-08-20T10:18:12Z

@bluetech thanks for the update, great job

nicoddemus

LGTM! Please consider my suggestions for improved coverage. 👍

src/_pytest/compat.py

testing/python/raises.py

bluetech · 2019-08-20T13:17:45Z

@nicoddemus Updated, but the coverage check failed again. It is quite inscrutable, do you have any other ideas how to please it?

nicoddemus · 2019-08-20T13:20:30Z

Thanks @bluetech, again excellent work!

It is quite inscrutable, do you have any other ideas how to please it?

I never seen it myself, but I wouldn't worry about it given that patch is covered. 👍

@blueyed might have a suggestion here.

asottile

blueyed · 2019-08-20T15:22:16Z

pragma: no cover

I recommend just being honest and not excluding this.
After all it is not covered, and makes it easy to find dead (uncovered) code
etc later.

I agree that we do not have to run coverage for 3.5.0 though - which then
should also have made it covered. In my humble opinion a better workaround than
adding the comment (but with more overhead).

@asottile
Animated gifs are distracting when you are writing a comment.

asottile · 2019-08-20T15:26:56Z

@asottile
Animated gifs are distracting when you are writing a comment.

I'm sorry you hate fun :P, consider blocking them

nicoddemus · 2019-08-20T15:32:14Z

I recommend just being honest and not excluding this.
After all it is not covered, and makes it easy to find dead (uncovered) code
etc later.

I see it differently: a "pragma no cover", to me, is clear a indication on code about this shared agreement:

I agree that we do not have to run coverage for 3.5.0

This way people in the future don't have to worry about covering those lines if we ever do a "let's get to 100% coverage" movement or such.

But other than that I don't feel too strongly about it either way. 😁

blueyed · 2019-08-20T15:38:47Z

@asottile

I'm sorry you hate fun :P,

That's not my kind of fun.. ;)

consider blocking them

How? Using an ad block for their URL?
Everything from https://camo.githubusercontent.com/ then? (feels a bit exaggerated)
Unfortunately you cannot just stop them using escape like e.g. in Firefox (IIRC) in qutebrowser (https://github.com/qutebrowser/qutebrowser/issues?q=is%3Aissue+animated+gifs+is%3Aclosed).

@nicoddemus

This way people in the future don't have to worry about covering those lines if we ever do a "let's get to 100% coverage" movement or such.

If that ever would be a goal then we should just run the coverage for 3.5.0 also.

I do not feel too strong about it myself, but not adding the comment there would have been the easier (and slightly clearer) way. Just ignore the "hint" from codecov then.

asottile · 2019-08-20T15:41:42Z

probably something like

##img[canonical-src*=".gif"]

nicoddemus previously requested changes Aug 16, 2019

View reviewed changes

changelog/5751.bugfix.rst Outdated Show resolved Hide resolved

src/_pytest/_code/code.py Outdated Show resolved Hide resolved

src/_pytest/python_api.py Outdated Show resolved Hide resolved

nicoddemus mentioned this pull request Aug 16, 2019

latest pytest crashes on import on python 3.5.0 #5751

Closed

asottile reviewed Aug 16, 2019

View reviewed changes

src/_pytest/_code/code.py Outdated Show resolved Hide resolved

bluetech force-pushed the typing-py350-fix branch from 809f11e to 9f3bfe8 Compare August 17, 2019 12:42

blueyed added a commit to blueyed/pytest that referenced this pull request Aug 17, 2019

ci: Travis: use 3.5.0

44d6a12

Ref: pytest-dev#5752 (comment)

blueyed mentioned this pull request Aug 17, 2019

ci: Travis: use Python 3.5.0 #5757

Closed

ci: Travis: use 3.5.0

1e3205e

Ref: pytest-dev#5752 (comment)

asottile reviewed Aug 18, 2019

View reviewed changes

src/_pytest/python_api.py Outdated Show resolved Hide resolved

Move @overload to compat

a7ede64

asottile approved these changes Aug 18, 2019

View reviewed changes

Zac-HD reviewed Aug 20, 2019

View reviewed changes

bluetech added 2 commits August 20, 2019 11:19

Add workaround for test_raises_cyclic_reference in Python 3.5.{0,1}

cec2183

Pypy doesn't have sys.getrefcount(), so go back to gc

a7c2357

nicoddemus approved these changes Aug 20, 2019

View reviewed changes

src/_pytest/compat.py Outdated Show resolved Hide resolved

testing/python/raises.py Show resolved Hide resolved

Fix coverage

43eab91

asottile approved these changes Aug 20, 2019

View reviewed changes

asottile merged commit daff906 into pytest-dev:master Aug 20, 2019

blueyed mentioned this pull request Aug 20, 2019

Coverage for 3.5.0 #5772

Closed

bluetech deleted the typing-py350-fix branch February 28, 2020 12:09

The-Compiler mentioned this pull request Sep 28, 2020

dropping python3.5? #7808

Closed

Uh oh!

Conversation

bluetech commented Aug 16, 2019 • edited by RonnyPfannschmidt Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicoddemus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bluetech commented Aug 16, 2019

Uh oh!

Uh oh!

bluetech commented Aug 17, 2019

Uh oh!

asottile commented Aug 17, 2019

Uh oh!

nicoddemus commented Aug 17, 2019

Uh oh!

nicoddemus commented Aug 17, 2019

Uh oh!

asottile commented Aug 17, 2019

Uh oh!

blueyed commented Aug 17, 2019

Uh oh!

asottile commented Aug 17, 2019

Uh oh!

blueyed commented Aug 17, 2019

Uh oh!

asottile commented Aug 17, 2019

Uh oh!

blueyed commented Aug 17, 2019

Uh oh!

Uh oh!

asottile left a comment

Choose a reason for hiding this comment

Uh oh!

blueyed commented Aug 19, 2019

Uh oh!

Zac-HD Aug 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blueyed Aug 20, 2019

Choose a reason for hiding this comment

Uh oh!

bluetech commented Aug 20, 2019

Uh oh!

RonnyPfannschmidt commented Aug 20, 2019

Uh oh!

bluetech commented Aug 20, 2019

Uh oh!

RonnyPfannschmidt commented Aug 20, 2019

Uh oh!

nicoddemus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bluetech commented Aug 20, 2019

Uh oh!

nicoddemus commented Aug 20, 2019

Uh oh!

asottile left a comment

Choose a reason for hiding this comment

Uh oh!

blueyed commented Aug 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asottile commented Aug 20, 2019

Uh oh!

nicoddemus commented Aug 20, 2019

Uh oh!

blueyed commented Aug 20, 2019

Uh oh!

asottile commented Aug 20, 2019

Uh oh!

Reviewers

Assignees

bluetech commented Aug 16, 2019 •

edited by RonnyPfannschmidt

Loading

Zac-HD Aug 20, 2019 •

edited

Loading

blueyed commented Aug 20, 2019 •

edited

Loading