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: warnings are re-emitted with wrong module #11933

Open
bluetech opened this issue Feb 5, 2024 · 3 comments
Open

recwarn: warnings are re-emitted with wrong module #11933

bluetech opened this issue Feb 5, 2024 · 3 comments
Labels
plugin: warnings related to the warnings builtin plugin type: bug problem that needs to be addressed

Comments

@bluetech
Copy link
Member

bluetech commented Feb 5, 2024

While reviewing #11917 I noticed a problem with the warning re-emitting code added in pytest 8.0. I think the module=w.__module__ line below is wrong:

warnings.warn_explicit(
str(w.message),
w.message.__class__, # type: ignore[arg-type]
w.filename,
w.lineno,
module=w.__module__,
source=w.source,
)

w here is a warnings.WarningMessage so w.__module__ is always "warnings". But the warning.warn_explicit say this should rather be the module of the warning that is used for filtering.

If I'm reading the warnings code correctly, the module originally passed by the user is not preserved, so the warning cannot be re-emitted faithfully in this regard, but we can probably do something better than the current situation.

cc @reaganjlee @Zac-HD

@bluetech bluetech added type: bug problem that needs to be addressed plugin: warnings related to the warnings builtin plugin labels Feb 5, 2024
@Zac-HD
Copy link
Member

Zac-HD commented Feb 5, 2024

Ah, yeah, we'll need to recover the module name from w.filename and inspecting sys.modules.

@reaganjlee
Copy link
Contributor

reaganjlee commented Feb 6, 2024

Apologies if I'm missing something. The warn_explicit() docs say

The module name defaults to the filename with .py stripped

w.filename lists the full path (e.g. '/path/to/error-file.py'). Is it possible to leave module to this default? Or does the actual module get lost somehow

@Zac-HD
Copy link
Member

Zac-HD commented Feb 8, 2024

OK, here's what I think is going on - there are several things with too-similar names and we're getting them mixed up.

  • In warn_explicit( ..., module=, ...), the module argument should be the importable name of the module - e.g. "random" or "urllib.parse". These can be found as keys in the sys.modules dict; not to be confused with the module objects which are the values in that dict.
    • Our bug was that w.__module__ is warnings.WarningMessage.__class__.__module__, i.e. always "warnings", rather than the location from which the instance-of-Warning (aka message) was emitted. Oops!
  • "The module name defaults to the filename with .py stripped" - my interpretation of this is that /path/to/error_file.py would use "error_file" as the module= argument, though I'm not confident without running some experiments.
  • The original module= value doesn't seem to be tracked anywhere - it's used to find the relevant warnings registry, but not stored. I think we can recover it, at least in most cases, with next(k for k, v in sys.modules.items() if getattr(v, "__file__", None) == m.filename), handling the not-found case - or maybe create and cache a filename: modulename dict.

So it's going to be imperfect, but we can still improve somewhat on the status quo 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plugin: warnings related to the warnings builtin plugin type: bug problem that needs to be addressed
Projects
None yet
Development

No branches or pull requests

3 participants