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.

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

Remove py and pytest-forked test dependencies #15501

Merged
merged 2 commits into from
Jun 23, 2023

Conversation

AlexWaygood
Copy link
Member

@AlexWaygood AlexWaygood commented Jun 23, 2023

We don't use either of these in our own code at all. Neither of them should be needed in 2023.

py used to be a dependency of pytest but no longer is. It was added to test-requirements.txt 6 years ago in order to pin a specific version to fix an issue where some tests were failing: 5168c25 and ad6dd4e.

pytest-forked used to be a dependency of pytest-xdist, but no longer is. It was added to test-requirements.txt` 4 years ago in order to pin a specific version to fix builds on macOS: e006b94.

@github-actions

This comment has been minimized.

test-requirements.txt Outdated Show resolved Hide resolved
@AlexWaygood AlexWaygood changed the title Remove py test dependency Remove py and pytest-forked test dependencies Jun 23, 2023
@github-actions
Copy link
Contributor

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅

@sobolevn sobolevn merged commit a47279b into python:master Jun 23, 2023
19 checks passed
@AlexWaygood AlexWaygood deleted the ditch-py branch June 23, 2023 11:53
@AlexWaygood AlexWaygood restored the ditch-py branch June 23, 2023 11:54
@sobolevn
Copy link
Member

For some reason master branch failed: https://github.com/python/mypy/actions/runs/5355970406/jobs/9714896673

@AlexWaygood
Copy link
Member Author

AlexWaygood commented Jun 23, 2023

For some reason master branch failed: https://github.com/python/mypy/actions/runs/5355970406/jobs/9714896673

I saw. I don't understand why that happened, given that all the tests passed on this PR. Looking into it now to see if the environment somehow changed between the PR runs of the tests and the runs of the tests on master.

@bluetech
Copy link
Contributor

It looks like it's due to pytest 7.4.0 which I had just released. Looks like it's using a private API which broke (probably by me...).

@AlexWaygood
Copy link
Member Author

It looks like it's due to pytest 7.4.0 which I had just released. Looks like it's using a private API which broke (probably by me...).

I reached the same conclusion simultaneously :) For now we can pin to pytest <7.4.0, and figure out how to migrate off the private API later.

We can see in the freeze step that the PR run used pytest 7.3.2: https://github.com/python/mypy/actions/runs/5355631169/jobs/9714141396

and the master run used pytest 7.4.0: https://github.com/python/mypy/actions/runs/5355970406/jobs/9714895421

@AlexWaygood AlexWaygood deleted the ditch-py branch June 23, 2023 11:59
@AlexWaygood
Copy link
Member Author

#15505 should hopefully get things green again! 🤞

@bluetech
Copy link
Contributor

Alternatively, a quick fix (untested) would be to change

self.parent._prunetraceback(excinfo)

to

if pytest.version_info >= (7, 4):
    excinfo.traceback = self.parent._traceback_filter(excinfo)
else:
    self.parent._prunetraceback(excinfo)

I don't have time ATM to provide a more proper solution which doesn't use private API, but I can look into it later.

@AlexWaygood AlexWaygood mentioned this pull request Jun 23, 2023
@AlexWaygood
Copy link
Member Author

Alternatively, a quick fix (untested)

I think I favour just adding a pin and a TODO for now, until we can come up with a more principled fix, but thank you!!

sobolevn pushed a commit that referenced this pull request Jun 23, 2023
pytest 7.4.0 was just released, and our CI on `master` is very red as a
result. Looks like we're using a private API that we shouldn't be:
#15501 (comment). For
now let's just pin to pytest <7.4.0.

https://pypi.org/project/pytest/7.4.0/
hauntsaninja pushed a commit that referenced this pull request Jun 24, 2023
pytest 7.4.0 was just released, and our CI on `master` is very red as a
result. Looks like we're using a private API that we shouldn't be:
#15501 (comment). For
now let's just pin to pytest <7.4.0.

https://pypi.org/project/pytest/7.4.0/
@AlexWaygood
Copy link
Member Author

AlexWaygood commented Jun 25, 2023

(Basically just a note to myself): this is the pytest commit that removed the _prunetraceback method: pytest-dev/pytest@fcada1e -- PR pytest-dev/pytest#10921.

Alternatively, a quick fix (untested) would be to change

self.parent._prunetraceback(excinfo)

to

if pytest.version_info >= (7, 4):
    excinfo.traceback = self.parent._traceback_filter(excinfo)
else:
    self.parent._prunetraceback(excinfo)

I don't think this exact snippet will work for mypy, as the second branch won't type check if pytest >=7.4 is installed. There are ways round that (type: ignore; use hasattr() instead of pytest.version_info; etc.), but we could also just bump our pytest lower bound to >=7.4, I think.

But the best solution would be if we could do our tests while using only pytest's public API, of course :)

@bluetech
Copy link
Contributor

But the best solution would be if we could do our tests while using only pytest's public API, of course :)

I guess the first thing to understand is if there's anything wrong with doing

-            self.parent._prunetraceback(excinfo)
-            excrepr = excinfo.getrepr(style="short")
+            excrepr = super().repr_failure(excinfo, style)

BTW, since this is mypy 😄 , I'd mention that if you bump the pytest req to >=7.0.0 then you can type the excinfo parameter as pytest.ExceptionInfo[BaseException].

@AlexWaygood
Copy link
Member Author

I guess the first thing to understand is if there's anything wrong with doing

-            self.parent._prunetraceback(excinfo)
-            excrepr = excinfo.getrepr(style="short")
+            excrepr = super().repr_failure(excinfo, style)

Pfff, no idea really! I dug through git blame, and it looks like we've been using this private API since we first adopted pytest in 2016: #1944. I don't know much about this area of mypy's codebase or much about pytest internals, I'm afraid!

@ikonst, you've been doing some great work improving mypy's test infrastructure recently -- any thoughts on this from you? :)

@mtelka
Copy link
Contributor

mtelka commented Jun 27, 2023

-            self.parent._prunetraceback(excinfo)
-            excrepr = excinfo.getrepr(style="short")
+            excrepr = super().repr_failure(excinfo, style)

I tested this on OpenIndiana with pytest 7.4.0 and both Python 3.7.16 and 3.9.16 and all tests passed.

@sobolevn
Copy link
Member

@mtelka please send a PR :)

@JukkaL
Copy link
Collaborator

JukkaL commented Jun 27, 2023

I don't remember why we are using the private API, but I suspect it may be about ensuring tracebacks on test failure use a specific style or do/don't include certain entries. Tests passing may not be enough to validate things work. Manually checking the contents of a traceback when a test fails would be useful.

@mtelka
Copy link
Contributor

mtelka commented Jun 27, 2023

@mtelka please send a PR :)

Sorry, I'm neither author of that change nor I do fully understand all consequences of the change (see #15501 (comment)), so I won't create a PR. I just wanted to report testing success.

JukkaL pushed a commit that referenced this pull request Jul 12, 2023
The 7.4.0 release of pytest broke mypy because we were using some
undocumented, private API that was removed. Ideally we'd stop using the
private API, but nobody seems to remember why we started using the
private API in the first place (see
#15501 (comment), and
following comments). For now it (unfortunately) seems safer to just
migrate to the new private API rather than try to figure out an
alternative using public API.

I also took @bluetech's advice in
#15501 (comment) to
improve the type annotations in the method in question.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants