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

Recwarn: Fix module when re-emitting unmatched warnings #12898

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

reaganjlee
Copy link
Contributor

@reaganjlee reaganjlee commented Oct 18, 2024

Fixes #11933

Credit to @Zac-HD for fix!

@psf-chronographer psf-chronographer bot added the bot:chronographer:provided (automation) changelog entry is part of PR label Oct 18, 2024
Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

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

Thanks @reaganjlee, we appreciate the PR!

Left a comment, but other than that we need a test to ensure this fix does not regress in the future. 👍

Comment on lines 331 to 335
module = next(
k
for k, v in sys.modules.items()
if getattr(v, "__file__", None) == w.filename
)
Copy link
Member

Choose a reason for hiding this comment

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

We should probably fallback to some string just in case something is very wrong and we cannot match the filename:

Suggested change
module = next(
k
for k, v in sys.modules.items()
if getattr(v, "__file__", None) == w.filename
)
module = next(
k
for k, v in sys.modules.items()
if getattr(v, "__file__", None) == w.filename,
None
)

I'm assuming None is a valid argument for module=.

@@ -0,0 +1 @@
Fix regression with :func:`pytest.warns` using wrong module when re-emitting unmatched warnings.
Copy link
Member

Choose a reason for hiding this comment

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

Nice! 👍


def test_ignore_warning_from_module_a():
with pytest.raises(pytest.fail.Exception, match="DID NOT WARN"):
with pytest.warns(UserWarning, match="module A.") as outer:
Copy link
Member

Choose a reason for hiding this comment

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

The test is not really veryfing the new module attribute... if I go in the code and set module = None in warnings.warn_explicit, the test still passes, which makes it unsuitable for a regression test... can you improve the test to check the actual module attribute of the warnings?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot:chronographer:provided (automation) changelog entry is part of PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

recwarn: warnings are re-emitted with wrong module
2 participants