Importing pytest_* modules should not trigger PluginValidationError

[The-Compiler]

In pytest-dev/pytest-bdd#224 @soulrebel had an issue when importing pytest_bdd in his conftest.py file:

_pytest.vendored_packages.pluggy.PluginValidationError: unknown hook 'pytest_bdd' in plugin <module 'project_qa.tests.functional.conftest' (<_pytest.assertion.rewrite.AssertionRewritingHook object at 0x7f035b2d4f28>)>

This seems odd - importing a pytest_* module should work normally, no? Maybe pluggy should at least check that unknown hooks aren't module objects?

[nicoddemus]

Agreed, thanks

[goodboy]

@The-Compiler looked at this and indeed this should never happen as PluginManager.register() always rejects non-routines.

I'm not sure how this occurs but it does re-enforce my my thinking we should not allow registering hookspecs based on project names alone and instead should require that all hooks and specs are explicitly marked with decorators.

I'll try to see if I can simulate this with a test.

[RonnyPfannschmidt]

pluggy/pluggy/__init__.py

Lines 241 to 256 in 01d3589

	for name in dir(plugin):
	hookimpl_opts = self.parse_hookimpl_opts(plugin, name)
	if hookimpl_opts is not None:
	normalize_hookimpl_opts(hookimpl_opts)
	method = getattr(plugin, name)
	hookimpl = HookImpl(plugin, plugin_name, method, hookimpl_opts)
	hook = getattr(self.hook, name, None)
	if hook is None:
	hook = _HookCaller(name, self._hookexec)
	setattr(self.hook, name, hook)
	elif hook.has_spec():
	self._verify_hook(hook, hookimpl)
	hook._maybe_apply_history(hookimpl)
	hook._add_hookimpl(hookimpl)
	hookcallers.append(hook)
	return plugin_name

is responsible for the issue - if we at that bit reject modules/other non-callables while issuing a warning, everything will be fine

[RonnyPfannschmidt]

going for the future we should turn the hook discovery into policies so we can change them more reasonably

[goodboy]

@RonnyPfannschmidt not sure I agree with that diagnosis ;)

Like I mentioned above the spot where the registration would be rejected is inside the hookimpl_opts = self.parse_hookimpl_opts(plugin, name) call at 242 which should reject the opts check with the inspect.isroutine() branch. The problem is you guys still haven't de-vendored pluggy in mainline pytest and this code doesn't exist in the old vendored version...

My favourite part is that you @RonnyPfannschmidt wrote that code probably to fix this last year around this time hehe.

I've added a test to prove this.

Really like the policy idea btw 👍

[RonnyPfannschmidt]

#17 lol yes i did that

[goodboy]

@RonnyPfannschmidt @nicoddemus if this isn't motivation to get that de-vendoring done I don't know what is ;)

Also can I close this since it's obviously not a problem with pluggy (anymore)?

[RonnyPfannschmidt]

@tgoodlet the next pytest feature release will

[nicoddemus]

Definitely we can close this now, thanks @tgoodlet