From 3bc3aab681d8c17536fe0a952ad571e3fac5c27e Mon Sep 17 00:00:00 2001 From: Ran Benita Date: Thu, 21 Dec 2023 12:32:27 +0200 Subject: [PATCH] Warn when old-style hookwrapper raises during teardown Fix #463. --- changelog/463.feature.rst | 2 ++ docs/api_reference.rst | 12 ++++++++++ src/pluggy/__init__.py | 6 +++++ src/pluggy/_callers.py | 24 +++++++++++++++---- src/pluggy/_warnings.py | 27 +++++++++++++++++++++ testing/test_warnings.py | 49 +++++++++++++++++++++++++++++++++++++++ 6 files changed, 116 insertions(+), 4 deletions(-) create mode 100644 changelog/463.feature.rst create mode 100644 src/pluggy/_warnings.py create mode 100644 testing/test_warnings.py diff --git a/changelog/463.feature.rst b/changelog/463.feature.rst new file mode 100644 index 00000000..8340cc94 --- /dev/null +++ b/changelog/463.feature.rst @@ -0,0 +1,2 @@ +A warning :class:`~pluggy.PluggyTeardownRaisedWarning` is now issued when an old-style hookwrapper raises an exception during teardown. +See the warning documentation for more details. diff --git a/docs/api_reference.rst b/docs/api_reference.rst index 40b27bfe..b14d725d 100644 --- a/docs/api_reference.rst +++ b/docs/api_reference.rst @@ -47,3 +47,15 @@ API Reference .. autoclass:: pluggy.HookimplOpts() :show-inheritance: :members: + + +Warnings +-------- + +Custom warnings generated in some situations such as improper usage or deprecated features. + +.. autoclass:: pluggy.PluggyWarning() + :show-inheritance: + +.. autoclass:: pluggy.PluggyTeardownRaisedWarning() + :show-inheritance: diff --git a/src/pluggy/__init__.py b/src/pluggy/__init__.py index 9d9e873b..2adf9454 100644 --- a/src/pluggy/__init__.py +++ b/src/pluggy/__init__.py @@ -18,6 +18,8 @@ "HookspecMarker", "HookimplMarker", "Result", + "PluggyWarning", + "PluggyTeardownRaisedWarning", ] from ._manager import PluginManager, PluginValidationError @@ -31,3 +33,7 @@ HookimplOpts, HookImpl, ) +from ._warnings import ( + PluggyWarning, + PluggyTeardownRaisedWarning, +) diff --git a/src/pluggy/_callers.py b/src/pluggy/_callers.py index f9bad2d2..787f56ba 100644 --- a/src/pluggy/_callers.py +++ b/src/pluggy/_callers.py @@ -3,6 +3,7 @@ """ from __future__ import annotations +import warnings from typing import cast from typing import Generator from typing import Mapping @@ -14,12 +15,13 @@ from ._hooks import HookImpl from ._result import HookCallError from ._result import Result +from ._warnings import PluggyTeardownRaisedWarning # Need to distinguish between old- and new-style hook wrappers. -# Wrapping one a singleton tuple is the fastest type-safe way I found to do it. +# Wrapping with a tuple is the fastest type-safe way I found to do it. Teardown = Union[ - Tuple[Generator[None, Result[object], None]], + Tuple[Generator[None, Result[object], None], HookImpl], Generator[None, object, object], ] @@ -37,6 +39,16 @@ def _raise_wrapfail( ) +def _warn_teardown_exception( + hook_name: str, hook_impl: HookImpl, e: BaseException +) -> None: + msg = "A plugin raised an exception during an old-style hookwrapper teardown.\n" + msg += f"Plugin: {hook_impl.plugin_name}, Hook: {hook_name}\n" + msg += f"{type(e).__name__}: {e}\n" + msg += "For more information see https://pluggy.readthedocs.io/en/stable/api_reference.html#pluggy.PluggyTeardownRaisedWarning" # noqa: E501 + warnings.warn(PluggyTeardownRaisedWarning(msg), stacklevel=5) + + def _multicall( hook_name: str, hook_impls: Sequence[HookImpl], @@ -73,7 +85,7 @@ def _multicall( res = hook_impl.function(*args) wrapper_gen = cast(Generator[None, Result[object], None], res) next(wrapper_gen) # first yield - teardowns.append((wrapper_gen,)) + teardowns.append((wrapper_gen, hook_impl)) except StopIteration: _raise_wrapfail(wrapper_gen, "did not yield") elif hook_impl.wrapper: @@ -141,9 +153,13 @@ def _multicall( if isinstance(teardown, tuple): try: teardown[0].send(outcome) - _raise_wrapfail(teardown[0], "has second yield") except StopIteration: pass + except BaseException as e: + _warn_teardown_exception(hook_name, teardown[1], e) + raise + else: + _raise_wrapfail(teardown[0], "has second yield") else: try: if outcome._exception is not None: diff --git a/src/pluggy/_warnings.py b/src/pluggy/_warnings.py new file mode 100644 index 00000000..6356c770 --- /dev/null +++ b/src/pluggy/_warnings.py @@ -0,0 +1,27 @@ +from typing import final + + +class PluggyWarning(UserWarning): + """Base class for all warnings emitted by pluggy.""" + + __module__ = "pluggy" + + +@final +class PluggyTeardownRaisedWarning(PluggyWarning): + """A plugin raised an exception during an :ref:`old-style hookwrapper + ` teardown. + + Such exceptions are not handled by pluggy, and may cause subsequent + teardowns to be executed at unexpected times, or be skipped entirely. + + This is an issue in the plugin implementation. + + If the exception is unintended, fix the underlying cause. + + If the exception is intended, switch to :ref:`new-style hook wrappers + `, or use :func:`result.force_exception() + ` to set the exception instead of raising. + """ + + __module__ = "pluggy" diff --git a/testing/test_warnings.py b/testing/test_warnings.py new file mode 100644 index 00000000..4f5454be --- /dev/null +++ b/testing/test_warnings.py @@ -0,0 +1,49 @@ +from pathlib import Path + +import pytest + +from pluggy import HookimplMarker +from pluggy import HookspecMarker +from pluggy import PluggyTeardownRaisedWarning +from pluggy import PluginManager + + +hookspec = HookspecMarker("example") +hookimpl = HookimplMarker("example") + + +def test_teardown_raised_warning(pm: PluginManager) -> None: + class Api: + @hookspec + def my_hook(self): + raise NotImplementedError() + + pm.add_hookspecs(Api) + + class Plugin1: + @hookimpl + def my_hook(self): + pass + + class Plugin2: + @hookimpl(hookwrapper=True) + def my_hook(self): + yield + 1 / 0 + + class Plugin3: + @hookimpl(hookwrapper=True) + def my_hook(self): + yield + + pm.register(Plugin1(), "plugin1") + pm.register(Plugin2(), "plugin2") + pm.register(Plugin3(), "plugin3") + with pytest.warns( + PluggyTeardownRaisedWarning, + match=r"\bplugin2\b.*\bmy_hook\b.*\n.*ZeroDivisionError", + ) as wc: + with pytest.raises(ZeroDivisionError): + pm.hook.my_hook() + assert len(wc.list) == 1 + assert Path(wc.list[0].filename).name == "test_warnings.py"