hooks: also verify kwargs are per spec in call_historic

Author: bluetech

src/pluggy/_hooks.py

@@ -346,27 +346,32 @@ def _add_hookimpl(self, hookimpl: "HookImpl") -> None:
     def __repr__(self) -> str:
         return f"<_HookCaller {self.name!r}>"
 
-    def __call__(self, *args: object, **kwargs: object) -> Any:
-        if args:
-            raise TypeError("hook calling supports only keyword arguments")
-        assert not self.is_historic()
-
+    def _verify_all_args_are_provided(self, kwargs: Mapping[str, object]) -> None:
         # This is written to avoid expensive operations when not needed.
         if self.spec:
             for argname in self.spec.argnames:
                 if argname not in kwargs:
-                    notincall = tuple(set(self.spec.argnames) - kwargs.keys())
+                    notincall = ", ".join(
+                        repr(argname)
+                        for argname in self.spec.argnames
+                        # Avoid self.spec.argnames - kwargs.keys() - doesn't preserve order.
+                        if argname not in kwargs.keys()
+                    )
                     warnings.warn(
                         "Argument(s) {} which are declared in the hookspec "
-                        "can not be found in this hook call".format(notincall),
+                        "cannot be found in this hook call".format(notincall),
                         stacklevel=2,
                     )
                     break
 
-            firstresult = self.spec.opts.get("firstresult", False)
-        else:
-            firstresult = False
-
+    def __call__(self, *args: object, **kwargs: object) -> Any:
+        if args:
+            raise TypeError("hook calling supports only keyword arguments")
+        assert (
+            not self.is_historic()
+        ), "Cannot directly call a historic hook - use call_historic instead."
+        self._verify_all_args_are_provided(kwargs)
+        firstresult = self.spec.opts.get("firstresult", False) if self.spec else False
         return self._hookexec(self.name, self.get_hookimpls(), kwargs, firstresult)
 
     def call_historic(
@@ -382,6 +387,7 @@ def call_historic(
         """
         assert self._call_history is not None
         kwargs = kwargs or {}
+        self._verify_all_args_are_provided(kwargs)
         self._call_history.append((kwargs, result_callback))
         # Historizing hooks don't return results.
         # Remember firstresult isn't compatible with historic.

testing/test_details.py

@@ -1,4 +1,3 @@
-import warnings
 import pytest
 from pluggy import PluginManager, HookimplMarker, HookspecMarker
 
@@ -89,36 +88,33 @@ class Module:
     assert pm.get_plugin("donttouch") is module
 
 
-def test_warning_on_call_vs_hookspec_arg_mismatch() -> None:
-    """Verify that is a hook is called with less arguments then defined in the
-    spec that a warning is emitted.
-    """
+def test_not_all_arguments_are_provided_issues_a_warning(pm: PluginManager) -> None:
+    """Calling a hook without providing all arguments specified in
+    the hook spec issues a warning."""
 
     class Spec:
         @hookspec
-        def myhook(self, arg1, arg2):
+        def hello(self, arg1, arg2):
             pass
 
-    class Plugin:
-        @hookimpl
-        def myhook(self, arg1):
+        @hookspec(historic=True)
+        def herstory(self, arg1, arg2):
             pass
 
-    pm = PluginManager(hookspec.project_name)
-    pm.register(Plugin())
     pm.add_hookspecs(Spec)
 
-    with warnings.catch_warnings(record=True) as warns:
-        warnings.simplefilter("always")
+    with pytest.warns(UserWarning, match=r"'arg1', 'arg2'.*cannot be found.*$"):
+        pm.hook.hello()
+    with pytest.warns(UserWarning, match=r"'arg2'.*cannot be found.*$"):
+        pm.hook.hello(arg1=1)
+    with pytest.warns(UserWarning, match=r"'arg1'.*cannot be found.*$"):
+        pm.hook.hello(arg2=2)
 
-        # calling should trigger a warning
-        pm.hook.myhook(arg1=1)
+    with pytest.warns(UserWarning, match=r"'arg1', 'arg2'.*cannot be found.*$"):
+        pm.hook.hello.call_extra([], kwargs=dict())
 
-        assert warns is not None
-        assert len(warns) == 1
-        warning = warns[-1]
-        assert issubclass(warning.category, Warning)
-        assert "Argument(s) ('arg2',)" in str(warning.message)
+    with pytest.warns(UserWarning, match=r"'arg1', 'arg2'.*cannot be found.*$"):
+        pm.hook.herstory.call_historic(kwargs=dict())
 
 
 def test_repr() -> None: