multi hook registration does not unregister

[RonnyPfannschmidt]

#251 added having multiple hooks of the same name to a plugin

https://github.com/pytest-dev/pluggy/blob/475c4aa99200bfa3e30d12458ab2a3593a36dd56/src/pluggy/_hooks.py#L417-L422C57 does not unregister them

[wahuneke]

Hi @RonnyPfannschmidt . I added the following test case in test_hookcaller.py but this did not result in test failure. Am I misunderstanding the (brief) description on this bug?

def test_hook_multi_impl(pm: PluginManager) -> None:
    """Since plugins' impls are able to (optionally) specify a spec name, it is possible for a plugin to implement
    the same spec multiple times."""

    class Api:
        @hookspec
        def hello(self, arg: object) -> None:
            "api hook 1"

    pm.add_hookspecs(Api)
    hook = pm.hook

    class Plugin:
        @hookimpl
        def hello(self, arg):
            return arg + 1

        @hookimpl(specname="hello")
        def hello_again(self, arg):
            return arg + 100

    plugin = Plugin()
    pm.register(plugin)
    out = hook.hello(arg=3)
    assert out == [103, 4]
    pm.unregister(plugin)
    assert hook.hello(arg=3) == []

I'm new to this project and hoping to help out somewhere. This looked like a spot to jump in ...

[RonnyPfannschmidt]

Please validate both hooks are actually registered,I believe there's a second issue at Hand

[wahuneke]

Ok. I will compare your feedback to the code and see what else I can check in order to confirm that both implementations are actually registered.

The test case does already confirm that:

• both implementations get run by the hook caller
• and, confirms that neither implementations remain after we deregister

I will take a look to see what other aspects I can confirm (perhaps something in tracing or other utils).

[wahuneke]

@RonnyPfannschmidt - I've added additional testing but still do not see any test failures.

I've added:

• checks of other methods on the plugin manager and the hook caller, confirming sane results
• a simple test for tracing, both for standard plugin usage and for this special case plugin

Rather than paste new code here for you to look at, I've created a Draft PR #446

[RonnyPfannschmidt]

PS, I finally figured why it's working,

Unregister collects hook callers for plugins multiple times if there's multiple registration, thus in turn unregister is called multiple times

[wahuneke]

Yes! before I wrote the test cases, I wrote a change for that for loop so it would remove all matches instead of just the first. I was disappointed that it fixed nothing (and actually broke deregistration since now the outer loop would still run twice and second time would fail due to the overzealous first remove pass). :) Good code around here. Enjoying the project so far. Cheers!

…

On Fri, Sept 22, 2023, 10:55 a.m. Ronny Pfannschmidt < ***@***.***> wrote: PS, I finally figured why it's working, Unregister collects hook callers for plugins multiple times if there's multiple registration, thus in turn unregister is called multiple times — Reply to this email directly, view it on GitHub <#431 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABFYUNXXVGVH5YS25XQEGU3X3WRF7ANCNFSM6AAAAAA3PSRJ7Y> . You are receiving this because you commented.Message ID: ***@***.***>