Surface filter-promoted unraisable warnings directly (#14263)#14499
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 |
Zac-HD
left a comment
There was a problem hiding this comment.
Adding the gc logic to pytest-unconfigure looks good to me, but I'd make two tweaks:
- keep the gc logic in
cleanup()too, in case of resources which are (partly) freed between those two locations - drop the magic warnings handling. While tempting, we can't fully handle e.g. the tracebacks which should be present, and so I think continuing to raise
PytestUnraisableExceptionWarningis the right approach here.
…rnings Reviewer feedback on pytest-dev#14499: - Keep the GC + queue drain in cleanup() as well as pytest_unconfigure, so objects freed while the cleanup stack unwinds still surface. - Drop the direct-raise path (_warning_class_has_error_filter plus the unwrap branch in collect_unraisable). Re-raising the inner warning lost the traceback the hook captures as a string, and the filter match was only approximate. collect_unraisable now always wraps in PytestUnraisableExceptionWarning and lets the warnings machinery decide. Tests dropped or reworked: - Removed test_refcycle_resource_warning_filter: with only error::ResourceWarning the leak no longer fails the suite, which was the dropped direct-raise. - Removed test_refcycle_userwarning_filter: redundant with test_refcycle_unraisable_warning_filter once the wrapper is always raised. - Removed test_unraisable_warning_filter_add_note_dedups: covered the deleted add_note code. - Reworked test_unraisable_decouples_from_cleanup_stack_order to raise ValueError from __del__ and filter on error::pytest.PytestUnraisableExceptionWarning. A ResourceWarning leak only enters the unraisable pipeline through an error::ResourceWarning filter, which no longer promotes the wrapper; a raised exception is wrapped unconditionally, so the timing fix stays observable. Changelog reworded to describe the pytest_unconfigure timing fix instead of the dropped direct-raise behavior.
Summary
Refs #14263.
At session end,
unraisableexceptionforces agc.collect()so leaked objects finalize and their warnings reachsys.unraisablehook. That collection used to run from aconfig.add_cleanupcallback. Those callbacks pop in reverse order, so another plugin's cleanup could tear down a warning filter before the collection ran. The leaked warning then fired with no filter active, so pytest dropped it with no output.This moves the collection into
pytest_unconfigure, which runs before pytest closes the cleanup stack. The warning filters andsys.unraisablehookare both still in place there, so the leak surfaces no matter what order plugins clean up in. That's approach 2 from the issue. I left the collection incleanup()too, as a final sweep for anything freed while the cleanup stack unwinds.A surfaced unraisable is still wrapped in
PytestUnraisableExceptionWarning. To fail a run on these, use a filter that matches the wrapper: bareerror,-W error, orerror::pytest.PytestUnraisableExceptionWarning. A class-only filter likeerror::ResourceWarningsurfaces the warning but doesn't fail the run on its own (see Scope).Testing
Automated
New/changed tests:
test_unraisable_decouples_from_cleanup_stack_order, the regression test. Aconftest.pywith@hookimpl(trylast=True)registers awarnings.resetwarningscleanup that pops beforeunraisableexception's own cleanup, recreating the bad order that apply warnings filter as soon as possible, and remove it as late as possible #13057's plugin reorder doesn't cover. A__del__raisesValueErrorunder anerror::pytest.PytestUnraisableExceptionWarningfilter. Red onmain(filter gone before the collection runs, exit 0); green here (collection runs inpytest_unconfigure, filter still active, exit 1).test_refcycle_resource_warning_filter,test_refcycle_userwarning_filter,test_unraisable_warning_filter_add_note_dedups), since that branch is gone.Manual. Save as
test_del.py:pytest -W error test_del.pyexits 1 (wrapper promoted, leak fails the run).pytest test_del.pyexits 0 (wrapper logged, no failure).To see the cleanup-order fix by hand, swap
src/_pytest/unraisableexception.pybetweenmainand this branch and reruntest_unraisable_decouples_from_cleanup_stack_order:mainexits 0, this branch exits 1.If you run from inside the pytest source tree, the repo's
pyproject.tomlsetsfilterwarnings = ['error', ...], which promotes every warning. Comment out the bare'error'entry or run the snippet from outside the repo.Reproducer from the issue
My adapted version of the original gist. The original references an undefined
stderr_linesin itsrun_scenariohelper; mine applies a one-line fix.Scope
collect_unraisablewraps every queued unraisable inPytestUnraisableExceptionWarningand emits it throughwarnings.warn. An earlier revision of this PR added an unwrap branch: when an activeerror::<class>filter matched the inner warning, it re-raised that warning directly soerror::ResourceWarningfailed the test. Per reviewer feedback I dropped that branch. Re-raising lost the traceback the hook captures as a string, and the filter check (action == "error"plusissubclass) only approximated Python's real filter matching. So pytest keeps wrapping and lets thewarningsmachinery decide.The issue's exact
error::ResourceWarningconfig now surfaces the leak (logged asPytestUnraisableExceptionWarning) instead of dropping it silently, but doesn't fail the run on its own. Suites using bareerror, including this repo, do fail, sinceerrormatches the wrapper.Branch structure
The branch builds the original direct-raise approach across red→green commits, then a final commit drops it and keeps the timing fix.
pytest_unconfigure, and guardedpytest_unconfigureagainst an unset stash when another plugin'spytest_configureraisesUsageError.cd7e0432emovedgc.collect()and queue processing intopytest_unconfigure._warning_class_has_error_filter, keeps the GC incleanup()as well aspytest_unconfigure, removes the tests that only covered the dropped behavior, and reworks the cleanup-order test to raiseValueErrorfrom__del__under anerror::pytest.PytestUnraisableExceptionWarningfilter.Relationship to #14273
#14273 attempted approach 2 (move GC to
pytest_unconfigure). Maintainers closed it unmerged: #13057 (Dec 2024) had already shipped approach 1 by reorderingdefault_plugins, so the move alone showed no behavior difference under the built-in plugin order. A maintainer asked for "a regression test which demonstrates that the unraisable hook is now subject to warnings."test_unraisable_decouples_from_cleanup_stack_orderanswers that. The@hookimpl(trylast=True)conftest registers awarnings.resetwarningscleanup that pops beforeunraisableexception's own, defeating the staticdefault_pluginsorder. Onmainthe run exits 0 (filter gone before the collection runs); here it exits 1 (collection runs inpytest_unconfigure, filter still active).AI disclosure
sys.unraisablehook, and manually running the regression test before and after changes.Changelog
changelog/14263.bugfix.rst.