Repository: pytest-dev/pluggy (python)

Issue:
pytest won't run root conftest.py's pytest_configure in special circumstances
Wanted to cross post https://github.com/pytest-dev/pytest/issues/11365 here, in case it might be on the pluggy side:

> Consider the following project: https://github.com/Redoubts/_pytest_bug_report
> 
> There's a root level conftest.py, and 3 plugins (two loaded in the conftest, and a third loaded by plugin 2). All define a `pytest_configure` in some way.
> 
> with `platform darwin -- Python 3.11.4, pytest-7.4.0, pluggy-1.0.0`, we see
> 
> ```
> % pytest test1.py
> mypackage.plugin2 Configured
> mypackage.plugin3 Configured
> conftest Configured
> mypackage.plugin Configured
> ```
> 
> Which is somewhat expected, each `pytest_configure` gets called.
> 
> If you update pluggy to 1.2+, you see
> 
> ```
> % pytest test1.py           
> mypackage.plugin2 Configured
> mypackage.plugin3 Configured
> mypackage.plugin Configured
> mypackage.plugin3 Configured
> ============================================================================================ test session starts =============================================================================================
> platform darwin -- Python 3.11.4, pytest-7.4.0, pluggy-1.2.0
> ...
> ```
> 
> Where the third plugin's `pytest_configure` is called twice, but the root `pytest_configure` is never called.
> 
> I think this is more specifically due to `mypackage/plugin3.py`'s `@pytest.hookimpl` wrapper. Maybe it's ill advised to do that, but I found this bizarre scenario when testing package updates in a much larger codebase.



Pull request description:
hooks: fix plugins registering other plugins in a hook
when the other plugins implement the same hook itself.

Fix #438.
Regressed in 63b7e908b4b22c30d86cd2cff240b3b7aa6da596 - pluggy 1.1.0.

Went with the simple solution described in the issue for now, basically undoing most of the rationale for 63b7e908b4b22c30d86cd2cf (though I think it's still better to have a single `_hookimpls` list).

Patch:
diff --git a/src/pluggy/_hooks.py b/src/pluggy/_hooks.py
index 6623c14..7c8420f 100644
--- a/src/pluggy/_hooks.py
+++ b/src/pluggy/_hooks.py
@@ -497,7 +497,8 @@ class HookCaller:
         ), "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._hookimpls, kwargs, firstresult)
+        # Copy because plugins may register other plugins during iteration (#438).
+        return self._hookexec(self.name, self._hookimpls.copy(), kwargs, firstresult)
 
     def call_historic(
         self,
@@ -518,7 +519,8 @@ class HookCaller:
         self._call_history.append((kwargs, result_callback))
         # Historizing hooks don't return results.
         # Remember firstresult isn't compatible with historic.
-        res = self._hookexec(self.name, self._hookimpls, kwargs, False)
+        # Copy because plugins may register other plugins during iteration (#438).
+        res = self._hookexec(self.name, self._hookimpls.copy(), kwargs, False)
         if result_callback is None:
             return
         if isinstance(res, list):


Test patch:
diff --git a/testing/test_pluginmanager.py b/testing/test_pluginmanager.py
index 81b86b6..08e2f37 100644
--- a/testing/test_pluginmanager.py
+++ b/testing/test_pluginmanager.py
@@ -675,3 +675,79 @@ def test_hook_tracing(he_pm: PluginManager) -> None:
         assert saveindent[0] > indent
     finally:
         undo()
+
+
+@pytest.mark.parametrize("historic", [False, True])
+def test_register_while_calling(
+    pm: PluginManager,
+    historic: bool,
+) -> None:
+    """Test that registering an impl of a hook while it is being called does
+    not affect the call itself, only later calls.
+
+    For historic hooks however, the hook is called immediately on registration.
+
+    Regression test for #438.
+    """
+    hookspec = HookspecMarker("example")
+
+    class Hooks:
+        @hookspec(historic=historic)
+        def configure(self) -> int:
+            raise NotImplementedError()
+
+    class Plugin1:
+        @hookimpl
+        def configure(self) -> int:
+            return 1
+
+    class Plugin2:
+        def __init__(self) -> None:
+            self.already_registered = False
+
+        @hookimpl
+        def configure(self) -> int:
+            if not self.already_registered:
+                pm.register(Plugin4())
+                pm.register(Plugin5())
+                pm.register(Plugin6())
+                self.already_registered = True
+            return 2
+
+    class Plugin3:
+        @hookimpl
+        def configure(self) -> int:
+            return 3
+
+    class Plugin4:
+        @hookimpl(tryfirst=True)
+        def configure(self) -> int:
+            return 4
+
+    class Plugin5:
+        @hookimpl()
+        def configure(self) -> int:
+            return 5
+
+    class Plugin6:
+        @hookimpl(trylast=True)
+        def configure(self) -> int:
+            return 6
+
+    pm.add_hookspecs(Hooks)
+    pm.register(Plugin1())
+    pm.register(Plugin2())
+    pm.register(Plugin3())
+
+    if not historic:
+        result = pm.hook.configure()
+        assert result == [3, 2, 1]
+        result = pm.hook.configure()
+        assert result == [4, 5, 3, 2, 1, 6]
+    else:
+        result = []
+        pm.hook.configure.call_historic(result.append)
+        assert result == [4, 5, 6, 3, 2, 1]
+        result = []
+        pm.hook.configure.call_historic(result.append)
+        assert result == [4, 5, 3, 2, 1, 6]