-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Conversation
This comment has been minimized.
This comment has been minimized.
py
test dependencypy
and pytest-forked
test dependencies
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
For some reason |
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 |
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 and the |
#15505 should hopefully get things green again! 🤞 |
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. |
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!! |
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/
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/
(Basically just a note to myself): this is the pytest commit that removed the
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 ( 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 |
Pfff, no idea really! I dug through @ikonst, you've been doing some great work improving mypy's test infrastructure recently -- any thoughts on this from you? :) |
I tested this on OpenIndiana with |
@mtelka please send a PR :) |
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. |
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. |
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.
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.