New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pytest verifies if hooks are valid before processing "pytest_plugins" variable #1821

Closed
nicoddemus opened this Issue Aug 19, 2016 · 11 comments

Comments

Projects
None yet
4 participants
@nicoddemus
Member

nicoddemus commented Aug 19, 2016

Consider:

# content of plugin.py
class Hooks:
    def pytest_my_hook(self, config):
        pass

def pytest_configure(config):
    config.pluginmanager.add_hookspec(Hooks)

# content of conftest.py
pytest_plugins = ['plugin']

def pytest_my_hook(self, config):
    pass

# content of test_foo.py
def test():
    pass

This produces this error:

pytest test_foo.py
Traceback (most recent call last):
  File "E:\Miniconda\envs\pytest\Scripts\pytest-script.py", line 9, in <module>
    load_entry_point('pytest==3.0.0.dev1', 'console_scripts', 'pytest')()
  File "c:\users\bruno\pytest\_pytest\config.py", line 56, in main
    config.pluginmanager.check_pending()
  File "c:\users\bruno\pytest\_pytest\vendored_packages\pluggy.py", line 490, in check_pending
    (name, hookimpl.plugin))
_pytest.vendored_packages.pluggy.PluginValidationError: unknown hook 'pytest_my_hook' in plugin <module 'conftest' from 'C:\Users\bruno\pytest\tmp\conftest.py'>

It seems pytest is validating hooks before actually processing all hooks.

This is using the features branch just pre-release of 3.0 (be9356a).

@nicoddemus nicoddemus self-assigned this Aug 19, 2016

@leifurhauks

This comment has been minimized.

leifurhauks commented Feb 15, 2017

I think I'm seeing the same issue when using pytest-factoryboy (with a django project):

# conftest.py
from factory import django as factory_django
import pytest_factoryboy


@pytest_factoryboy.register
class UserFactory(factory_django.DjangoModelFactory):
    class Meta:
        model = 'auth.User'

    username = 'test_user'

I get the following error:

$ pytest           
Traceback (most recent call last):
  File "/home/leifur/.virtualenvs/eboxtvapi/bin/pytest", line 11, in <module>
    sys.exit(main())
  File "/home/leifur/.virtualenvs/eboxtvapi/lib/python3.6/site-packages/_pytest/config.py", line 56, in main
    config.pluginmanager.check_pending()
  File "/home/leifur/.virtualenvs/eboxtvapi/lib/python3.6/site-packages/_pytest/vendored_packages/pluggy.py", line 498, in check_pending
    (name, hookimpl.plugin))
_pytest.vendored_packages.pluggy.PluginValidationError: unknown hook 'pytest_factoryboy' in plugin <module 'conftest' (<_pytest.assertion.rewrite.AssertionRewritingHook object at 0x7f1c13129828>)>

The simple workaround is to use from pytest_factoryboy import register instead of importing pytest_factoryboy directly.

I don't get this error when I import pytest_factoryboy in conftest files in inline testing directories in my app; it only happens with the conftest at the top of the project.

I'm using pytest 3.0.6, pytest-factoryboy 1.3.0 (although I confirmed that the same error happened with several prior releases) and pytest-django 3.1.2.

@RonnyPfannschmidt

This comment has been minimized.

Member

RonnyPfannschmidt commented Feb 15, 2017

oh - @nicoddemus the behaviour as seen is correct, hooks that may not be declared in plugin order must be marked as optional

@tgoodlet

This comment has been minimized.

Member

tgoodlet commented Feb 27, 2017

@nicoddemus @RonnyPfannschmidt I just ran into this as well with packaging our new project pytestlab.

I'm wondering is maybe check_pending() is being called slightly too early?
Would it not make more sense for it be called at least after the first historic call to pytest_addhooks() (or better yet after collection altogether)?
I'm realizing now it is called after pytest_addhooks().
My issue is actually to do with assuming a pytest_plugins declaration in an entrypoints registered plugin works the same as in conftest.py files, which it doesn't.

I'm stuck at the moment marking nearly all pytestlab hooks as optionalhook=True in pytestlab plugins which are loaded using pytest_plugins .
I'm wondering if it makes sense to have a recursive loading mechanism such as pytest_plugins supported for entry points plugins in pluggy?

@tgoodlet

This comment has been minimized.

Member

tgoodlet commented Feb 27, 2017

Guys unless I'm misunderstanding this might be actually kind of bad.

Currently check_pending() is only called once through the entire code base. That means there's no hook validation after pytest_cmdline_main() is entered.

Therefore hook validation is never done except for the initial plugin set loaded by _prepareconfig().

This also makes me wonder if hook validation is something that should be done on every plugin loaded with pluggy.

@RonnyPfannschmidt

This comment has been minimized.

Member

RonnyPfannschmidt commented Feb 28, 2017

@tgoodlet potentially yes, this may need a discussion/brainstorm

@tgoodlet

This comment has been minimized.

Member

tgoodlet commented Feb 28, 2017

@RonnyPfannschmidt I've opened pytest-dev/pluggy#48 for a follow up regarding the validation question.

What do you think about the recursive loading suggestion I made?

@nicoddemus

This comment has been minimized.

Member

nicoddemus commented Mar 3, 2017

Here are the updated files:

# content of conftest.py
pytest_plugins = ['plugin']

def pytest_my_hook(config):
    assert 0

# content of test_foo.py
def test(request):
    request.config.hook.pytest_my_hook(config=request.config)

# content of plugin.py
class Hooks:
    def pytest_my_hook(self, config):
        pass

def pytest_configure(config):
    config.pluginmanager.add_hookspecs(Hooks)

I think this can be fixed by calling check_pending as late as possible in the process, instead of at pytest_cmdline_main. I propose calling it right after collection:

diff --git a/_pytest/config.py b/_pytest/config.py
index c73416f..8af3908 100644
--- a/_pytest/config.py
+++ b/_pytest/config.py
@@ -53,7 +53,7 @@ def main(args=None, plugins=None):
             return 4
         else:
             try:
-                config.pluginmanager.check_pending()
+
                 return config.hook.pytest_cmdline_main(config=config)
             finally:
                 config._ensure_unconfigure()
diff --git a/_pytest/main.py b/_pytest/main.py
index b66b661..1bc80b2 100644
--- a/_pytest/main.py
+++ b/_pytest/main.py
@@ -598,6 +598,7 @@ class Session(FSCollector):
         hook = self.config.hook
         try:
             items = self._perform_collect(args, genitems)
+            self.config.pluginmanager.check_pending()
             hook.pytest_collection_modifyitems(session=self,
                 config=self.config, items=items)
         finally:

This gives a chance for all plugins to be loaded and validated properly. We then don't need the slighly hackish way of deferring the use of hooks.

@nicoddemus

This comment has been minimized.

Member

nicoddemus commented Mar 3, 2017

With the above patch, my example works (the assertion inside pytest_my_hook is triggered) and if I don't register the spec by commenting out pytest_configure I get the same validation error.

@tgoodlet

This comment has been minimized.

Member

tgoodlet commented Mar 4, 2017

@nicoddemus nice, yes this is the fix I wanted to see and I think it addresses the bulk of the issue :)

I still think it's not necessarily great that validation only ever happens once.
There could be cases where plugins are registered after collection no? An example might be choosing an appropriate reporting hook depending on a fixtures calculation of the target platform.

I feel like we should at least allow the plugin provider the option to specify how "strict" (or maybe how often?) validations should be.

@nicoddemus

This comment has been minimized.

Member

nicoddemus commented Mar 4, 2017

nice, yes this is the fix I wanted to see and I think it addresses the bulk of the issue :)

Great, then unless @RonnyPfannschmidt has any objections I will put together a PR.

There could be cases where plugins are registered after collection no?

Not that I'm aware of, but I'm not 100% sure either.

I feel like we should at least allow the plugin provider the option to specify how "strict" (or maybe how often?) validations should be.

I see, thanks. Do you have an idea on how pluggy can provide that functionality though? It seems like it depends completely on how pluggy is being used. As an example, check_pending was being called in pytest's main, and my code above proposes it to be called after collection, both which are outside of pluggy's control.

@tgoodlet

This comment has been minimized.

Member

tgoodlet commented Mar 4, 2017

@nicoddemus see my comments in pytest-dev/pluggy#48 for a deeper explanation :)

This was referenced Mar 6, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment