pytest icon indicating copy to clipboard operation
pytest copied to clipboard

recwarn: warnings are re-emitted with wrong module

Open bluetech opened this issue 1 year ago • 6 comments

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:

https://github.com/pytest-dev/pytest/blob/aaa9ca7327de16ca07e5d169e4cf1ad5d810d1da/src/_pytest/recwarn.py#L324-L331

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 avatar Feb 05 '24 20:02 bluetech

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

Zac-HD avatar Feb 05 '24 21:02 Zac-HD

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

reaganjlee avatar Feb 06 '24 00:02 reaganjlee

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 🙂

Zac-HD avatar Feb 08 '24 01:02 Zac-HD