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

BUG: fix an edge case where ExceptionInfo._stringify_exception could crash pytest.raises #11879

Merged
merged 7 commits into from Jan 30, 2024

Conversation

neutrinoceros
Copy link
Contributor

@neutrinoceros neutrinoceros commented Jan 29, 2024

Closes #11872
Thanks @bluetech for your guidance !

from urllib.error import HTTPError

with pytest.raises(HTTPError, match="Not Found"):
raise HTTPError(code=404, msg="Not Found", fp=None, hdrs=None, url="") # type: ignore [arg-type]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I resorted to turning off mypy here because it reported that hdrs is supposed to be of type Message, but I have no idea where that type comes from (and it's not just an alias for str).

@neutrinoceros neutrinoceros marked this pull request as ready for review January 29, 2024 08:33
@RonnyPfannschmidt
Copy link
Member

wow, the inheritance tree of that exception is absolutely WILD, terrifying - i'd consider this a cpython upstream bug as well

@neutrinoceros
Copy link
Contributor Author

Agreed, but I have limited time on my hands right now. I can't escalate this to CPython folks until Wednesday, so anyone is welcome to beat me to it.

@bluetech
Copy link
Member

@Zac-HD had fixed a similar bug in #10797, though there @Zac-HD opted to specifically check for urllib (IIUC) instead of safe getattr. So @Zac-HD WDYT?

@Zac-HD
Copy link
Member

Zac-HD commented Jan 30, 2024

I think of this as implementing a workaround for a bug in old versions of CPython, and would argue against hiding bugs in exception types more generally. Better for such bugs to be noticed and fixed as quickly as possible, meaning that having pytest fail on this is actually pretty helpful!

I'd suggest taking a tightly-scoped approach, which you could crib from my implementation in agronholm/exceptiongroup#58. Notably this means we'll be able to take the workaround out again when we drop support for the affected Python versions, and easily know to do so by grepping for the version comparison, without introducing new failures for other broken exception classes.

Copy link
Member

@bluetech bluetech left a comment

Choose a reason for hiding this comment

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

I updated to use @Zac-HD's approach, good to go now.

@bluetech bluetech merged commit 407d984 into pytest-dev:main Jan 30, 2024
23 of 24 checks passed
@neutrinoceros neutrinoceros deleted the hotfix/11872 branch January 30, 2024 16:09
@neutrinoceros
Copy link
Contributor Author

Thanks for picking this up !

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.

BUG: pytest.raises(HTTPError, match="...") breaks on Python 3.9 + pytest 8.0
5 participants