Surface filter-promoted unraisable warnings directly (#14263)#14499
Surface filter-promoted unraisable warnings directly (#14263)#14499paulzuradzki wants to merge 9 commits into
Conversation
476e4f5 to
4cf46d4
Compare
FuzzysTodd
left a comment
There was a problem hiding this comment.
pytest_unconfigure fires before _cleanup_stack.close(), so warning
filters managed via the cleanup stack (the warnings plugin's
catch_warnings context, in particular) are guaranteed active when GC
63a5109 to
56f71b7
Compare
filterwarnings = error::ResourceWarning does not fail tests that leak resources through a reference cycle. collect_unraisable wraps the captured ResourceWarning in PytestUnraisableExceptionWarning, a class the user has no filter for, so the run exits 0. This test pins that contract: on a refcycle-leaking test with error::ResourceWarning configured, pytest should exit non-zero and the output should show the inner ResourceWarning rather than the wrapping PytestUnraisableExceptionWarning. Fails at this commit; the next commit ships the fix that turns it green. Refs pytest-dev#14263.
When sys.unraisablehook captures a Warning subclass instance and the user has an active ``error::<that class>`` filter, raise the original warning rather than wrapping in PytestUnraisableExceptionWarning. The wrap path remains for any case where no matching error filter is set, so suites that don't use ``error::<warning>`` filters see no change. Filter matching is approximate: category only, not message/module/lineno. The check errs toward false negatives, never false positives. The regression test added in the previous commit now passes. Additional coverage: - test_refcycle_userwarning_filter: locks the contract for a non-builtin Warning subclass. - test_unraisable_warning_without_filter_still_wraps: scope guard. A Warning raised from __del__ without a matching error filter must still be wrapped, not raised directly. - test_unraisable_warning_filter_add_note_dedups: covers the duplicate- note guard in the unwrap path for singleton/cached Warning instances. Tightens the ``errors`` list type from list[Exception] to list[Warning | RuntimeError]. Adds Paul Zuradzki to AUTHORS. Notes in test_create_task_raises_unraisable_warning_filter that the propagated class is now bare RuntimeWarning rather than the wrapping PytestUnraisableExceptionWarning (because -Werror activates the new unwrap path). Closes pytest-dev#14263.
Forces the bad LIFO order with a conftest that registers a warnings.resetwarnings cleanup via @hookimpl(trylast=True): it pops before unraisableexception's cleanup and clears the user's error::ResourceWarning filter before GC runs, so the leak exits 0. The next commit moves GC into pytest_unconfigure (runs before the cleanup stack closes), making the test pass regardless of plugin order. Refs pytest-dev#14263.
Register only the hook-restore + stash-cleanup as the config.add_cleanup callback. Move the GC pump and collect_unraisable call into a new pytest_unconfigure(config) hook. pytest_unconfigure fires before _cleanup_stack.close(), so warning filters managed via the cleanup stack (the warnings plugin's catch_warnings context, in particular) are guaranteed active when GC runs. This decouples the unraisable step from plugin registration order in default_plugins. The previous arrangement worked only because of LIFO ordering on _cleanup_stack; pytest-dev#13057 (Dec 2024) reordered default_plugins to make that ordering correct, but the structural fragility remained. No observable behavior change. The 141 existing tests in test_unraisableexception + test_warnings + test_recwarn + test_threadexception still pass. The previous commit's test_refcycle_resource_warning_filter continues to fail on main and pass here. This is the structural side of the issue's two proposed fixes; the user-visible side shipped in the previous commit.
After GC moved into pytest_unconfigure, a plugin whose pytest_configure raises UsageError leaves the stash key unset while pytest_unconfigure still runs. Without a presence check it hits KeyError and reports INTERNALERROR instead of USAGE_ERROR. The next commit adds the guard. Refs pytest-dev#14263.
When another plugin's pytest_configure raises (e.g. pytest.UsageError in testing/acceptance_test.py::test_config_error), pluggy skips remaining configure hooks. unraisableexception.pytest_configure never runs, config.stash[unraisable_exceptions] is never set. The previous config.add_cleanup callback wasn't registered in that case either, so cleanup was a no-op. The pytest_unconfigure hook introduced in the previous commit ran unconditionally and hit KeyError on the unset stash, surfacing as INTERNAL_ERROR where pytest should exit with USAGE_ERROR. Guard with a stash-presence check at the top of pytest_unconfigure. test_config_error catches the regression direction.
…gure pytest-dev#14441 reduced the default gc_collect_harder passes to 1 on CPython (5 on PyPy, where __del__ can resurrect objects). That change lived in cleanup(), which this branch emptied when it moved GC into pytest_unconfigure. Carry the same default into the new location so the relocation does not silently revert pytest-dev#14441. CPython still collects the refcycle regression tests in a single pass.
30ff861 to
8f303fc
Compare
|
|
||
| msg = meta.msg | ||
| try: | ||
| warnings.warn(pytest.PytestUnraisableExceptionWarning(msg)) |
There was a problem hiding this comment.
Explainer:
- Builds a PytestUnraisableExceptionWarning wrapper and emits that via warnings.warn. The user's filter is error::ResourceWarning.
- PytestUnraisableExceptionWarning is a pytest.PytestWarning subclass; no relationship to Python ResourceWarning in class hierarchy.
- So the user's filter doesn't match the wrapper. The wrapper gets emitted as a regular warning (just a stderr line in the warnings summary), not promoted to an error, and nothing fails the test.
- For the user's error::ResourceWarning filter to fail the suite on main, they'd have to also write error::PytestUnraisableExceptionWarning. This would be filtering on pytest's internal wrapping class. Leaky abstraction. They want to express "fail if a ResourceWarning leaks from a finalizer," not "fail if pytest's internal wrapper class fires."
There was a problem hiding this comment.
Commit 4cf46 says, "if user said error::ResourceWarning, an unraisable ResourceWarning becomes a real raised ResourceWarning"
| errors.append(hook_error) | ||
| continue | ||
|
|
||
| if isinstance(meta.exc_value, Warning) and _warning_class_has_error_filter( |
There was a problem hiding this comment.
When collect_unraisable sees an unraisable whose exc_value is a Warning subclass instance, re-raise it directly instead of wrapping w/ pytest.PytestUnraisableExceptionWarning
| if sys.version_info >= (3, 11): | ||
| if meta.cause_msg not in getattr(meta.exc_value, "__notes__", []): | ||
| meta.exc_value.add_note(meta.cause_msg) | ||
| errors.append(meta.exc_value) |
There was a problem hiding this comment.
Append meta.exc_value (the actual ResourceWarning instance) to errors (what eventually gets raise)
| ): | ||
| # Honor the user's error filter on the inner warning class | ||
| # rather than wrapping in PytestUnraisableExceptionWarning. See #14263. | ||
| if sys.version_info >= (3, 11): |
There was a problem hiding this comment.
For Python 3.11+, attach meta.cause_msg as a PEP 678 note
| # still active. This decouples the GC step from plugin registration order. | ||
| # A single collection doesn't necessarily collect everything; the | ||
| # iteration count was determined experimentally by the Trio project. | ||
| if unraisable_exceptions not in config.stash: |
There was a problem hiding this comment.
Bug made possible by -- this PR -- moving GC and collect_unraisable from cleanup callback into pytest_unconfigure hook.
Guard checks: if our configure never ran, stash key is not there, and there's nothing to drain deque, so early return.
Else, this bug was possile:
- configure step crashes before
def pytest_configure(config): ... config.stash[unraisable_exceptions] = deque - stash never created
- unconfigure runs anyway
- -> tries to read missing stash
- -> KeyError
|
|
||
| # TODO: should be a test failure or error. Currently the exception | ||
| # propagates all the way to the top resulting in exit code 1. | ||
| assert result.ret == 1 |
There was a problem hiding this comment.
I noticed similar TODOs elsewhere.
AI explainer:
The exit code 1 comes from pytest's session-end unraisable handling, not from runpytest_subprocess() (that helper just reports the child's exit code). The leaked warning fires during session-end GC in pytest_unconfigure, after test_it has already been reported as passed, so it propagates to the top of the run instead of failing a specific test. Ideally we'd get failed=1 attributed to test_it; that needs allocation tracking and is out of scope here.
|
cc: @Zac-HD - this is the PR/GH Issue we chatted about at PyCon US sprints last Monday |
Summary
Closes #14263.
filterwarnings = error::ResourceWarningdoes not fail tests that leak resources through a reference cycle.collect_unraisablewraps the captured warning inPytestUnraisableExceptionWarning, a class the user has not filtered.collect_unraisablenow checkswarnings.filtersbefore wrapping. When an activeerrorfilter targets a class that the captured warning is an instance of,collect_unraisablere-raises that warning directly; the resulting exception fails the test.Visual
Behavior on
main: this test passes despite user configuring for ResourceWarning to trigger an error. Picturedtest_leak.pytriggers ResourceWarning.After PR: raises error as expected
Scope
collect_unraisablechooses between two branches when it processes a queued unraisable:error::<class>filter fails the test as written. Applies only when an activeerror::<class>filter matches that class.PytestUnraisableExceptionWarningand emit it viawarnings.warn. A__del__that raises aWarningwithout a matchingerror::<class>filter still hits this branch.Suites without
error::<warning>filters see no change.The unwrap-branch filter check is
action == "error"plusissubclass(warning_class, filter_category). It does not re-run Python'swarningsmodule's full match logic, which additionally tests the filter'smessage_regex,module_regex, andlinenofields. Consequence: for users with narrowly-scoped filters likeerror:some_msg:ResourceWarning::42, this branch can fail a test on aResourceWarningwhose message wouldn't have matched the regex. For broad filters likeerror::ResourceWarningthe behavior matches user intent.Branch structure
Three red→green commit pairs plus a comment-cleanup commit. Each red commit fails the named test; the next commit makes it pass.
7aea99ab0(red) →7cfadea30(fix):test_refcycle_resource_warning_filterexercises the user-visible wrap-vs-unwrap behavior covered in Summary.485f342fb(red) →cd7e0432e(fix): the move-GC hardening from Fix session-end gc in unraisableexception plugin to respect warning filters #14273.test_unraisable_decouples_from_cleanup_stack_orderuses@hookimpl(trylast=True)to force a LIFO cleanup-stack order that PR apply warnings filter as soon as possible, and remove it as late as possible #13057 incidentally avoided. The fix movesgc.collect()and queue processing intopytest_unconfigure, which runs before_cleanup_stack.close().d22468f2b(red) →fb50e17dc(fix):test_pytest_unconfigure_survives_failed_pytest_configureraisesUsageErrorfrom a conftest'spytest_configure, which exposed aKeyErrorafter step 2 moved the GC call out ofconfig.add_cleanup. The fix adds a stash-presence check at the top ofpytest_unconfigure.Follow-ups:
f5311b7a7tightens the new comments inunraisableexception.py;6e8ca5ba8adds failure messages to the new tests'result.retassertions.Rebased on current
main:8f303fc58reconciles with #14441 (which reduced the defaultgc_collect_harderpasses to 1 on CPython, 5 on PyPy). That tweak lived incleanup(), which this branch empties, so the commit carries the same default into the newpytest_unconfigurelocation rather than silently reverting #14441.Relationship to #14273
#14273 attempted approach 2 from the issue (move GC to
pytest_unconfigure). Maintainers closed it unmerged: #13057 (Dec 2024) had already shipped approach 1, so the move-GC change alone produced no observable behavior difference and no fails-then-passes regression test was possible. @Zac-HD asked: "we'd need to see a regression test which demonstrates that the unraisable hook is now subject to warnings."Step 2 in the branch structure addresses that question. The red test forces the LIFO ordering that #13057 incidentally avoided, demonstrating the structural fragility was load-bearing on plugin registration order rather than benign.
Testing
Reproducer
My adapted version of the original gist from the issue. The original references an undefined
stderr_linesin itsrun_scenariohelper; the adapted version applies a one-line fix.For a one-file check, save this as
test_leak.py:Then run
pytest -W error::ResourceWarning test_leak.py. The-Wflag drives the same code path as thepytest.inifilterwarningssetting. Onmainthe test passes (bug); on this branch it exits 1 with the innerResourceWarningraised directly, noPytestUnraisableExceptionWarningwrapper.If you run from inside the pytest source tree, the repo's
pyproject.toml[tool.pytest]filterwarnings = ['error', ...]promotes every warning to an error. Comment out the bare'error'entry, or run the snippet from outside the repo.mainmain, install (pip install -e .), run the adapted reproducer. Scenario 1 exits 0 (bug confirmed). Scenario 2 exits 1.ResourceWarningraised. Scenario 2 still exits 1.7aea99ab0,485f342fb,d22468f2b); the named test fails. Check out the next commit; it passes.Snippets
External suite tests
(time-permitting; I don't think this needs to block review)
Warningfrom__del__without a matchingerrorfilter against this branch.error::DeprecationWarningorerror::UserWarningfilters against this branch. Nothing previously-passing should now fail.AI disclosure
Side note: open to feedback to pull some of the PR comments into the code if helpful
Changelog
changelog/14263.bugfix.rst.