diff --git a/AUTHORS b/AUTHORS index 106d9b8f7f2..27c0b3ac408 100644 --- a/AUTHORS +++ b/AUTHORS @@ -370,6 +370,7 @@ Patrick Hayes Patrick Lannigan Paul Müller Paul Reece +Paul Zuradzki Pauli Virtanen Pavel Karateev Pavel Zhukov diff --git a/changelog/14263.bugfix.rst b/changelog/14263.bugfix.rst new file mode 100644 index 00000000000..56c027d3380 --- /dev/null +++ b/changelog/14263.bugfix.rst @@ -0,0 +1 @@ +An unraisable exception from a finalizer is now re-raised as the original warning class when an active error filter would have promoted it, instead of being wrapped in :class:`pytest.PytestUnraisableExceptionWarning`. This lets a user's ``filterwarnings = error::ResourceWarning`` (or any warning-class error filter) fail tests on resource leaks without a separate filter on the wrapping class. diff --git a/src/_pytest/unraisableexception.py b/src/_pytest/unraisableexception.py index 259f0d8e7f4..63cf872c316 100644 --- a/src/_pytest/unraisableexception.py +++ b/src/_pytest/unraisableexception.py @@ -33,6 +33,17 @@ def gc_collect_harder(iterations: int) -> None: gc.collect() +def _warning_class_has_error_filter(category: type[Warning]) -> bool: + """Return True if an active ``error`` filter matches ``category`` by class. + + Approximate match: ``message``/``module``/``lineno`` filter fields are ignored. + """ + for action, _msg, filt_category, _mod, _lineno in warnings.filters: + if action == "error" and issubclass(category, filt_category): + return True + return False + + class UnraisableMeta(NamedTuple): msg: str cause_msg: str @@ -46,7 +57,7 @@ class UnraisableMeta(NamedTuple): def collect_unraisable(config: Config) -> None: pop_unraisable = config.stash[unraisable_exceptions].pop - errors: list[pytest.PytestUnraisableExceptionWarning | RuntimeError] = [] + errors: list[Warning | RuntimeError] = [] meta = None hook_error = None try: @@ -62,6 +73,17 @@ def collect_unraisable(config: Config) -> None: errors.append(hook_error) continue + if isinstance(meta.exc_value, Warning) and _warning_class_has_error_filter( + type(meta.exc_value) + ): + # Honor the user's error filter on the inner warning class + # rather than wrapping in PytestUnraisableExceptionWarning. See #14263. + 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) + continue + msg = meta.msg try: warnings.warn(pytest.PytestUnraisableExceptionWarning(msg)) @@ -86,22 +108,8 @@ def collect_unraisable(config: Config) -> None: def cleanup( *, config: Config, prev_hook: Callable[[sys.UnraisableHookArgs], object] ) -> None: - # On PyPy, objects (e.g. coroutines) can survive GC rounds because executing - # their __del__ can resurrect them. The Trio project determined experimentally - # that 5 passes are needed on PyPy to flush everything. On CPython, reference - # counting handles most cleanup immediately, so 1 pass is sufficient. - _default_gc_collect_iterations = 5 if sys.implementation.name == "pypy" else 1 - gc_collect_iterations = config.stash.get( - gc_collect_iterations_key, _default_gc_collect_iterations - ) - try: - try: - gc_collect_harder(gc_collect_iterations) - collect_unraisable(config) - finally: - sys.unraisablehook = prev_hook - finally: - del config.stash[unraisable_exceptions] + sys.unraisablehook = prev_hook + del config.stash[unraisable_exceptions] def unraisable_hook( @@ -153,6 +161,26 @@ def pytest_configure(config: Config) -> None: sys.unraisablehook = functools.partial(unraisable_hook, append=deque.append) +def pytest_unconfigure(config: Config) -> None: + # Runs before ``_cleanup_stack.close()``, so warning filters from + # cleanup-stack-managed contexts (notably the ``warnings`` plugin's + # ``catch_warnings``) are still installed when garbage-collected + # finalizers fire. A ``config.add_cleanup`` callback would instead + # couple correctness to LIFO pop order across plugins' cleanups. + if unraisable_exceptions not in config.stash: + # ``pytest_configure`` did not complete (e.g. a usage error raised + # in another plugin's configure), so the queue stash was never set. + return + # PyPy can resurrect objects in __del__, so it needs several GC passes + # (5, per the Trio project); CPython frees cycles in one pass. See #14441. + _default_gc_collect_iterations = 5 if sys.implementation.name == "pypy" else 1 + gc_collect_iterations = config.stash.get( + gc_collect_iterations_key, _default_gc_collect_iterations + ) + gc_collect_harder(gc_collect_iterations) + collect_unraisable(config) + + @pytest.hookimpl(trylast=True) def pytest_runtest_setup(item: Item) -> None: collect_unraisable(item.config) diff --git a/testing/test_unraisableexception.py b/testing/test_unraisableexception.py index a6a4d6f35e8..a2091892f99 100644 --- a/testing/test_unraisableexception.py +++ b/testing/test_unraisableexception.py @@ -317,6 +317,10 @@ def test_scheduler_must_be_created_within_running_loop() -> None: # TODO: Should be a test failure or error. Currently the exception # propagates all the way to the top resulting in exit code 1. + # Note: since #14263, the propagated class is the bare RuntimeWarning + # rather than the wrapping PytestUnraisableExceptionWarning, because + # -Werror activates an error filter that matches RuntimeWarning's + # category and the new unwrap path in collect_unraisable fires. assert result.ret == 1 result.assert_outcomes(passed=1) @@ -359,6 +363,267 @@ def test_it(): result.stderr.fnmatch_lines("ValueError: del is broken") +def test_refcycle_resource_warning_filter(pytester: Pytester) -> None: + # Regression test for https://github.com/pytest-dev/pytest/issues/14263. + # A reference cycle holds a file alive past test return; only the cyclic + # GC at session end frees it. The file finalizer emits ResourceWarning. + # With ``filterwarnings = error::ResourceWarning`` the user has expressed + # intent that resource leaks should fail tests. Before the fix, the + # ResourceWarning was captured by sys.unraisablehook (the timing piece + # was already correct since #13057), but ``collect_unraisable`` wrapped + # it in a ``PytestUnraisableExceptionWarning``. Since the user had no + # error filter on the wrapping class, the failure was silently logged + # as a warning and the test passed. + pytester.makeini( + """ + [pytest] + filterwarnings = + error::ResourceWarning + """ + ) + pytester.makepyfile( + test_it=""" + # Disable gc so the cycle survives until session-end gc_collect_harder. + import gc; gc.disable() + + def test_it(): + f = open(__file__) + cycle = [f] + cycle.append(cycle) + """ + ) + + result = pytester.runpytest_subprocess() + + # 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, ( + "surfaced ResourceWarning should propagate to a non-zero session exit" + ) + result.assert_outcomes(passed=1) + # The unwrap path: stderr shows the ResourceWarning directly, NOT wrapped + # in PytestUnraisableExceptionWarning. The negative assertion is what + # makes this a contract test for #14263 rather than an exit-code check. + result.stderr.fnmatch_lines("*ResourceWarning: unclosed file*") + result.stderr.no_fnmatch_line("*PytestUnraisableExceptionWarning*") + + +def test_refcycle_userwarning_filter(pytester: Pytester) -> None: + # Companion to test_refcycle_resource_warning_filter. Covers the unwrap + # path for a non-built-in Warning subclass (UserWarning here) and a + # finalizer that calls ``warnings.warn(...)`` directly rather than + # leaking a resource. Confirms the fix is not specific to ResourceWarning. + pytester.makeini( + """ + [pytest] + filterwarnings = + error::UserWarning + """ + ) + pytester.makepyfile( + test_it=""" + import gc; gc.disable() + import warnings + + class Leaky: + def __init__(self): + self.self = self # cycle so __del__ defers to session-end gc + + def __del__(self): + warnings.warn("leak detected", UserWarning) + + def test_it(): + Leaky() + """ + ) + + result = pytester.runpytest_subprocess() + + # 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, ( + "surfaced UserWarning should propagate to a non-zero session exit" + ) + result.assert_outcomes(passed=1) + result.stderr.fnmatch_lines("*UserWarning: leak detected*") + result.stderr.no_fnmatch_line("*PytestUnraisableExceptionWarning*") + + +def test_unraisable_warning_without_filter_still_wraps(pytester: Pytester) -> None: + # Regression guard for the scope of the #14263 fix. A Warning raised + # from ``__del__`` *without* a matching error filter must still be + # wrapped in PytestUnraisableExceptionWarning rather than propagated + # directly. Otherwise the fix would change behavior for users who + # haven't set any filter (suites that previously logged would start + # failing unconditionally). + pytester.makepyfile( + test_it=""" + class RaisingDel: + def __del__(self): + raise UserWarning("oops") + + def test_it(): + obj = RaisingDel() + del obj + """ + ) + + # Subprocess so we don't inherit the outer pytest's filterwarnings + # (which is ``error`` in pyproject.toml; that would falsely trigger + # the unwrap path). ``-Wdefault::pytest.PytestUnraisableExceptionWarning`` + # makes the wrapping warning visible on stderr regardless of whether + # ``__del__`` fires inside the test or during later GC (PyPy). + result = pytester.runpytest_subprocess( + "-Wdefault::pytest.PytestUnraisableExceptionWarning" + ) + + assert result.ret == 0 + result.assert_outcomes(passed=1) + # Wrap path fired: the unraisable hook emitted + # PytestUnraisableExceptionWarning rather than propagating UserWarning. + # The warning lands in the warnings-summary section of stdout on + # CPython (where ``__del__`` fires inside the test) and on stderr on + # PyPy (where it fires during later GC). Check both rather than the + # outcomes counter, which is timing-dependent. + combined = "\n".join(result.outlines + result.errlines) + assert "PytestUnraisableExceptionWarning" in combined + + +@pytest.mark.skipif(sys.version_info < (3, 11), reason="add_note requires Python 3.11+") +def test_unraisable_warning_filter_add_note_dedups(pytester: Pytester) -> None: + # Covers the duplicate-note guard in the unwrap path. When the same + # Warning instance reaches sys.unraisablehook twice (which happens + # for singleton/cached warnings), the cause note must be appended + # once, not duplicated. + pytester.makeini( + """ + [pytest] + filterwarnings = + error::UserWarning + """ + ) + pytester.makepyfile( + test_it=""" + import sys + import types + + cached = UserWarning("cached") + + def test_emit_same_instance_twice(): + for _ in range(2): + sys.unraisablehook( + types.SimpleNamespace( + exc_type=UserWarning, + exc_value=cached, + exc_traceback=None, + err_msg=None, + object=None, + ) + ) + """ + ) + result = pytester.runpytest_subprocess() + # The unwrap path raises the UserWarning, so the test fails. + assert result.ret == 1, "re-raised UserWarning should make the run exit non-zero" + # Two errors land in the ExceptionGroup (one per hook call). With the + # dedup guard, ``cached.__notes__`` holds the cause note once, so the + # formatted group prints it twice (once per entry). Without the + # guard it would print four times. + note_count = sum( + 1 for ln in result.outlines + result.errlines if "Exception ignored in" in ln + ) + assert note_count == 2, f"expected 2 cause-note lines, saw {note_count}" + + +@pytest.mark.skipif(PYPY, reason="garbage-collection differences make this flaky") +def test_unraisable_decouples_from_cleanup_stack_order(pytester: Pytester) -> None: + # Regression test for the structural fix. The garbage-collection step + # that surfaces queued unraisables must run before _cleanup_stack.close() + # so warning filters installed via cleanup-stack-managed contexts are + # still in effect when finalizers fire. Otherwise correctness depends + # on the order in which plugins register their cleanups under LIFO. + # + # The conftest uses ``@hookimpl(trylast=True)`` so its pytest_configure + # runs after all built-in pytest_configures. Its + # ``warnings.resetwarnings`` cleanup then lands on the cleanup stack + # last and pops first under LIFO. Under the pre-fix layout, where + # garbage collection runs inside unraisableexception's own cleanup + # callback, that pop clears the user's ``error::ResourceWarning`` + # filter before the leaking finalizer fires; ``warnings.warn`` emits + # the ResourceWarning silently rather than promoting it, and the suite + # exits 0. Under the post-fix layout, ``pytest_unconfigure`` does the + # garbage collection and queue processing before the cleanup stack + # starts closing, so the conftest's reset has no effect. + pytester.makeini( + """ + [pytest] + filterwarnings = + error::ResourceWarning + """ + ) + pytester.makeconftest( + """ + import warnings + import pytest + + @pytest.hookimpl(trylast=True) + def pytest_configure(config): + config.add_cleanup(warnings.resetwarnings) + """ + ) + pytester.makepyfile( + test_it=""" + import gc; gc.disable() + + def test_it(): + f = open(__file__) + cycle = [f] + cycle.append(cycle) + """ + ) + + result = pytester.runpytest_subprocess() + + assert result.ret == 1, ( + "filter is still installed at GC time, so the leak exits non-zero " + "regardless of cleanup-stack pop order" + ) + result.assert_outcomes(passed=1) + result.stderr.fnmatch_lines("*ResourceWarning: unclosed file*") + result.stderr.no_fnmatch_line("*PytestUnraisableExceptionWarning*") + + +def test_pytest_unconfigure_survives_failed_pytest_configure( + pytester: Pytester, +) -> None: + # Regression test for the guard against an unset stash key. When + # another plugin's pytest_configure raises pytest.UsageError, pluggy + # stops calling the remaining configure hooks; the unraisable plugin's + # pytest_configure never runs and config.stash[unraisable_exceptions] + # stays unset. pytest_unconfigure still fires for partially-configured + # runs, so without a presence check the unraisable plugin's + # pytest_unconfigure raises KeyError and pytest reports INTERNALERROR + # instead of USAGE_ERROR. + pytester.makeconftest( + """ + import pytest + + def pytest_configure(config): + raise pytest.UsageError("simulated bad config") + """ + ) + pytester.makepyfile(test_it="def test_it(): pass") + + result = pytester.runpytest_subprocess() + + assert result.ret == pytest.ExitCode.USAGE_ERROR, ( + "the UsageError must surface as USAGE_ERROR, not a KeyError INTERNALERROR" + ) + result.stderr.fnmatch_lines("*ERROR: simulated bad config*") + result.stderr.no_fnmatch_line("*INTERNALERROR*") + result.stderr.no_fnmatch_line("*KeyError*") + + @pytest.mark.filterwarnings("error::pytest.PytestUnraisableExceptionWarning") def test_possibly_none_excinfo(pytester: Pytester) -> None: pytester.makepyfile(