From 0fdacb6db5806dcc2c01d80e6dfe0e5fcd24034e Mon Sep 17 00:00:00 2001 From: Ronny Pfannschmidt Date: Sun, 3 Oct 2021 16:01:40 +0200 Subject: [PATCH] deprecate hook configuration via marks/attributes fixes #4562 --- changelog/4562.deprecation.rst | 4 +++ doc/en/deprecations.rst | 44 ++++++++++++++++++++++++ pyproject.toml | 3 ++ src/_pytest/config/__init__.py | 61 ++++++++++++++++++++++------------ src/_pytest/deprecated.py | 8 +++++ src/_pytest/warning_types.py | 19 +++++++++++ testing/deprecated_test.py | 48 ++++++++++++++++++++++++++ 7 files changed, 165 insertions(+), 22 deletions(-) create mode 100644 changelog/4562.deprecation.rst diff --git a/changelog/4562.deprecation.rst b/changelog/4562.deprecation.rst new file mode 100644 index 000000000..d459801d4 --- /dev/null +++ b/changelog/4562.deprecation.rst @@ -0,0 +1,4 @@ +Deprecate configuring hook specs/impls using attributes/marks. + +Instead use :py:func:`pytest.hookimpl` and :py:func:`pytest.hookspec`. +For more details, see the :ref:`docs `. diff --git a/doc/en/deprecations.rst b/doc/en/deprecations.rst index 3bbd29bb5..a18d9d713 100644 --- a/doc/en/deprecations.rst +++ b/doc/en/deprecations.rst @@ -78,6 +78,50 @@ no matter what argument was used in the constructor. We expect to deprecate the .. _legacy-path-hooks-deprecated: +Configuring hook specs/impls using markers +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Before pluggy, pytest's plugin library, was its own package and had a clear API, +pytest just used ``pytest.mark`` to configure hooks. + +The :py:func:`pytest.hookimpl` and :py:func:`pytest.hookspec` decorators +have been available since years and should be used instead. + +.. code-block:: python + + @pytest.mark.tryfirst + def pytest_runtest_call(): + ... + + + # or + def pytest_runtest_call(): + ... + + + pytest_runtest_call.tryfirst = True + +should be changed to: + +.. code-block:: python + + @pytest.hookimpl(tryfirst=True) + def pytest_runtest_call(): + ... + +Changed ``hookimpl`` attributes: + +* ``tryfirst`` +* ``trylast`` +* ``optionalhook`` +* ``hookwrapper`` + +Changed ``hookwrapper`` attributes: + +* ``firstresult`` +* ``historic`` + + ``py.path.local`` arguments for hooks replaced with ``pathlib.Path`` ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ diff --git a/pyproject.toml b/pyproject.toml index a66f90017..fc9a119f6 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -37,6 +37,9 @@ filterwarnings = [ # Those are caught/handled by pyupgrade, and not easy to filter with the # module being the filename (with .py removed). "default:invalid escape sequence:DeprecationWarning", + # ignore not yet fixed warnings for hook markers + "default:.*not marked using pytest.hook.*", + "ignore:.*not marked using pytest.hook.*::xdist.*", # ignore use of unregistered marks, because we use many to test the implementation "ignore::_pytest.warning_types.PytestUnknownMarkWarning", # https://github.com/benjaminp/six/issues/341 diff --git a/src/_pytest/config/__init__.py b/src/_pytest/config/__init__.py index d753a9054..e1ceb7737 100644 --- a/src/_pytest/config/__init__.py +++ b/src/_pytest/config/__init__.py @@ -14,6 +14,7 @@ import warnings from functools import lru_cache from pathlib import Path from textwrap import dedent +from types import FunctionType from types import TracebackType from typing import Any from typing import Callable @@ -58,6 +59,7 @@ from _pytest.pathlib import ImportMode from _pytest.pathlib import resolve_package_path from _pytest.stash import Stash from _pytest.warning_types import PytestConfigWarning +from _pytest.warning_types import warn_explicit_for if TYPE_CHECKING: @@ -341,6 +343,32 @@ def _get_directory(path: Path) -> Path: return path +def _get_legacy_hook_marks( + method: object, # using object to avoid function type excess + hook_type: str, + opt_names: Tuple[str, ...], +) -> Dict[str, bool]: + known_marks = {m.name for m in getattr(method, "pytestmark", [])} + must_warn = False + opts = {} + for opt_name in opt_names: + if hasattr(method, opt_name) or opt_name in known_marks: + opts[opt_name] = True + must_warn = True + else: + opts[opt_name] = False + if must_warn: + + hook_opts = ", ".join(f"{name}=True" for name, val in opts.items() if val) + message = _pytest.deprecated.HOOK_LEGACY_MARKING.format( + type=hook_type, + fullname=method.__qualname__, # type: ignore + hook_opts=hook_opts, + ) + warn_explicit_for(cast(FunctionType, method), message) + return opts + + @final class PytestPluginManager(PluginManager): """A :py:class:`pluggy.PluginManager ` with @@ -414,40 +442,29 @@ class PytestPluginManager(PluginManager): if name == "pytest_plugins": return - method = getattr(plugin, name) opts = super().parse_hookimpl_opts(plugin, name) + if opts is not None: + return opts + method = getattr(plugin, name) # Consider only actual functions for hooks (#3775). if not inspect.isroutine(method): return - # Collect unmarked hooks as long as they have the `pytest_' prefix. - if opts is None and name.startswith("pytest_"): - opts = {} - if opts is not None: - # TODO: DeprecationWarning, people should use hookimpl - # https://github.com/pytest-dev/pytest/issues/4562 - known_marks = {m.name for m in getattr(method, "pytestmark", [])} - - for name in ("tryfirst", "trylast", "optionalhook", "hookwrapper"): - opts.setdefault(name, hasattr(method, name) or name in known_marks) - return opts + return _get_legacy_hook_marks( + method, "impl", ("tryfirst", "trylast", "optionalhook", "hookwrapper") + ) def parse_hookspec_opts(self, module_or_class, name: str): opts = super().parse_hookspec_opts(module_or_class, name) if opts is None: method = getattr(module_or_class, name) - if name.startswith("pytest_"): - # todo: deprecate hookspec hacks - # https://github.com/pytest-dev/pytest/issues/4562 - known_marks = {m.name for m in getattr(method, "pytestmark", [])} - opts = { - "firstresult": hasattr(method, "firstresult") - or "firstresult" in known_marks, - "historic": hasattr(method, "historic") - or "historic" in known_marks, - } + opts = _get_legacy_hook_marks( + method, + "spec", + ("firstresult", "historic"), + ) return opts def register( diff --git a/src/_pytest/deprecated.py b/src/_pytest/deprecated.py index f2d79760a..623bb0236 100644 --- a/src/_pytest/deprecated.py +++ b/src/_pytest/deprecated.py @@ -98,6 +98,14 @@ INSTANCE_COLLECTOR = PytestRemovedIn8Warning( "The pytest.Instance collector type is deprecated and is no longer used. " "See https://docs.pytest.org/en/latest/deprecations.html#the-pytest-instance-collector", ) +HOOK_LEGACY_MARKING = UnformattedWarning( + PytestDeprecationWarning, + "The hook{type} {fullname} uses old-style configuration options (marks or attributes).\n" + "Please use the pytest.hook{type}({hook_opts}) decorator instead\n" + " to configure the hooks.\n" + " See https://docs.pytest.org/en/latest/deprecations.html" + "#configuring-hook-specs-impls-using-markers", +) # You want to make some `__init__` or function "private". # diff --git a/src/_pytest/warning_types.py b/src/_pytest/warning_types.py index c32ce80cc..fd4b68e18 100644 --- a/src/_pytest/warning_types.py +++ b/src/_pytest/warning_types.py @@ -1,3 +1,6 @@ +import inspect +import warnings +from types import FunctionType from typing import Any from typing import Generic from typing import Type @@ -142,3 +145,19 @@ class UnformattedWarning(Generic[_W]): def format(self, **kwargs: Any) -> _W: """Return an instance of the warning category, formatted with given kwargs.""" return self.category(self.template.format(**kwargs)) + + +def warn_explicit_for(method: FunctionType, message: PytestWarning) -> None: + lineno = method.__code__.co_firstlineno + filename = inspect.getfile(method) + module = method.__module__ + mod_globals = method.__globals__ + + warnings.warn_explicit( + message, + type(message), + filename=filename, + module=module, + registry=mod_globals.setdefault("__warningregistry__", {}), + lineno=lineno, + ) diff --git a/testing/deprecated_test.py b/testing/deprecated_test.py index db649841a..8847695a7 100644 --- a/testing/deprecated_test.py +++ b/testing/deprecated_test.py @@ -20,6 +20,54 @@ def test_external_plugins_integrated(pytester: Pytester, plugin) -> None: pytester.parseconfig("-p", plugin) +def test_hookspec_via_function_attributes_are_deprecated(): + from _pytest.config import PytestPluginManager + + pm = PytestPluginManager() + + class DeprecatedHookMarkerSpec: + def pytest_bad_hook(self): + pass + + pytest_bad_hook.historic = True # type: ignore[attr-defined] + + with pytest.warns( + PytestDeprecationWarning, + match=r"Please use the pytest\.hookspec\(historic=True\) decorator", + ) as recorder: + pm.add_hookspecs(DeprecatedHookMarkerSpec) + (record,) = recorder + assert ( + record.lineno + == DeprecatedHookMarkerSpec.pytest_bad_hook.__code__.co_firstlineno + ) + assert record.filename == __file__ + + +def test_hookimpl_via_function_attributes_are_deprecated(): + from _pytest.config import PytestPluginManager + + pm = PytestPluginManager() + + class DeprecatedMarkImplPlugin: + def pytest_runtest_call(self): + pass + + pytest_runtest_call.tryfirst = True # type: ignore[attr-defined] + + with pytest.warns( + PytestDeprecationWarning, + match=r"Please use the pytest.hookimpl\(tryfirst=True\)", + ) as recorder: + pm.register(DeprecatedMarkImplPlugin()) + (record,) = recorder + assert ( + record.lineno + == DeprecatedMarkImplPlugin.pytest_runtest_call.__code__.co_firstlineno + ) + assert record.filename == __file__ + + def test_fscollector_gethookproxy_isinitpath(pytester: Pytester) -> None: module = pytester.getmodulecol( """