Merge pull request #9118 from RonnyPfannschmidt/fix-4562-deprecate-hookmarkers

deprecate hook configuration via marks/attributes

Author: RonnyPfannschmidt

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 <legacy-path-hooks-deprecated>`.

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``
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 

extra/setup-py.test/setup.py

@@ -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

pyproject.toml

@@ -14,6 +14,7 @@
 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 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,38 @@ def _get_directory(path: Path) -> Path:
         return path
 
 
+def _get_legacy_hook_marks(
+    method: Any,
+    hook_type: str,
+    opt_names: Tuple[str, ...],
+) -> Dict[str, bool]:
+    if TYPE_CHECKING:
+        # abuse typeguard from importlib to avoid massive method type union thats lacking a alias
+        assert inspect.isroutine(method)
+    known_marks: set[str] = {m.name for m in getattr(method, "pytestmark", [])}
+    must_warn: list[str] = []
+    opts: dict[str, bool] = {}
+    for opt_name in opt_names:
+        opt_attr = getattr(method, opt_name, AttributeError)
+        if opt_attr is not AttributeError:
+            must_warn.append(f"{opt_name}={opt_attr}")
+            opts[opt_name] = True
+        elif opt_name in known_marks:
+            must_warn.append(f"{opt_name}=True")
+            opts[opt_name] = True
+        else:
+            opts[opt_name] = False
+    if must_warn:
+        hook_opts = ", ".join(must_warn)
+        message = _pytest.deprecated.HOOK_LEGACY_MARKING.format(
+            type=hook_type,
+            fullname=method.__qualname__,
+            hook_opts=hook_opts,
+        )
+        warn_explicit_for(cast(FunctionType, method), message)
+    return opts
+
+
 @final
 class PytestPluginManager(PluginManager):
     """A :py:class:`pluggy.PluginManager <pluggy.PluginManager>` with
@@ -414,40 +448,29 @@ def parse_hookimpl_opts(self, plugin: _PluggyPlugin, name: str):
         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(

src/_pytest/config/__init__.py

@@ -98,6 +98,14 @@
     "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".
 #

src/_pytest/deprecated.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,25 @@ 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:
+    """
+    Issue the warning :param:`message` for the definition of the given :param:`method`
+
+    this helps to log warnigns for functions defined prior to finding an issue with them
+    (like hook wrappers being marked in a legacy mechanism)
+    """
+    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,
+    )

src/_pytest/warning_types.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 = False  # type: ignore[attr-defined]
+
+    with pytest.warns(
+        PytestDeprecationWarning,
+        match=r"Please use the pytest\.hookspec\(historic=False\) 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(
         """

testing/deprecated_test.py

@@ -2,7 +2,7 @@ anyio[curio,trio]==3.6.1
 django==4.1.1
 pytest-asyncio==0.19.0
 pytest-bdd==6.0.1
-pytest-cov==3.0.0
+pytest-cov==4.0.0
 pytest-django==4.5.2
 pytest-flakes==4.0.5
 pytest-html==3.1.1