deprecate hook configuration via marks/attributes

fixes #4562

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 <configuring-hook-specs-impls-using-markers>`.

doc/en/deprecations.rst

@@ -19,6 +19,38 @@ Below is a complete list of all pytest features which are considered deprecated.
 :class:`PytestWarning` or subclasses, which can be filtered using :ref:`standard warning filters <warnings>`.
 
 
+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():
+        ...
+
+
 ``py.path.local`` arguments for hooks replaced with ``pathlib.Path``
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 

pyproject.toml

@@ -40,6 +40,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

src/_pytest/config/__init__.py

@@ -13,6 +13,7 @@
 import warnings
 from functools import lru_cache
 from pathlib import Path
+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:
 
@@ -329,6 +331,32 @@ def _prepareconfig(
         raise
 
 
+def _get_legacy_hook_marks(
+    method: FunctionType,
+    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__,
+            hook_opts=hook_opts,
+        )
+        warn_explicit_for(method, message)
+    return opts
+
+
 @final
 class PytestPluginManager(PluginManager):
     """A :py:class:`pluggy.PluginManager <pluggy.PluginManager>` with
@@ -392,40 +420,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/deprecated.py

@@ -106,6 +106,16 @@
     " Replace pytest.warns(None) by simply pytest.warns()."
 )
 
+
+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".
 #
 #   def my_private_function(some, args):

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
@@ -130,3 +133,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,
+    )

testing/deprecated_test.py

@@ -51,6 +51,52 @@ def test_fillfixtures_is_deprecated() -> None:
         _pytest.fixtures.fillfixtures(mock.Mock())
 
 
+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="instead of pytest.mark"
+    ) 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="instead of pytest.mark"
+    ) as recorder:
+        pm.register(DeprecatedMarkImplPlugin())
+    (record,) = recorder
+    assert (
+        record.lineno
+        == DeprecatedMarkImplPlugin.pytest_runtest_call.__code__.co_firstlineno
+    )
+    assert record.filename == __file__
+
+
 def test_minus_k_dash_is_deprecated(pytester: Pytester) -> None:
     threepass = pytester.makepyfile(
         test_threepass="""