Skip to content
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

Update to not require use of the pytest_namespace() hook #18

Merged
merged 3 commits into from
Feb 27, 2018

Conversation

altendky
Copy link
Member

pytest 3.3.0 deprecated the use of the pytest_namespace() hook.

https://docs.pytest.org/en/latest/changelog.html#id63

@@ -108,12 +109,13 @@ def test_MAIN():
def test_blocon_in_hook(testdir):
testdir.makeconftest("""
import pytest
import pytest_twisted as pt
Copy link
Member Author

@altendky altendky Feb 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like the as pt part of this but without it pytest_twisted is detected as an unknown pytest hook... :|

INTERNALERROR> PluginValidationError: unknown hook 'pytest_twisted' in plugin <module 'conftest' from '/tmp/pytest-of-altendky/pytest-199/testdir/test_blocon_in_hook0/conftest.py'>

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @altendky. It's ok. I think so.
But if you don't like this, you can try to do smth like:

def pytest_addhooks(pluginmanager):
    pytest.blockon = blockon
    ...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the point is they don't want that. I thought it was kind of weird when I first saw it, trying to mash all these things into the one scope... But, what I didn't like was specifically that importing pytest_twisted (without as pt) caused the above error. But that's just an issue with the module name that happened to become apparent now. Maybe I should raise that question with pytest itself. Might make sense to ignore unknown hooks that are module names of plugins.

Copy link
Member

@nicoddemus nicoddemus Feb 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But, what I didn't like was specifically that importing pytest_twisted (without as pt) caused the above error. But that's just an issue with the module name that happened to become apparent now. Maybe I should raise that question with pytest itself.

Hi guys dropping in real quick. 😁

Which pytest and pluggy versions are you using? This issue was fixed in pytest-dev/pluggy#17 IIRC.

@vtitor vtitor merged commit 7830268 into pytest-dev:master Feb 27, 2018
@nicoddemus
Copy link
Member

Guys sorry for jumping in to late, but an approach that can be used to keep backward compatibility is this:

@pytest.hookimpl(hookwrapper=True)
def pytest_load_initial_conftests():
    import pytest
    import pytest_twisted
    pytest.inlineCallbacks = pytest_twisted.inlineCallbacks
    yield

@altendky
Copy link
Member Author

@nicoddemus I think my change retains the backwards compatibility (admittedly without storing tests, but I did run unmodified tests to check) so long as pytest_namespace() continues to be called. Would we want to continue to infect the pytest scope even after they have removed official support for doing so?

@nicoddemus
Copy link
Member

nicoddemus commented Feb 27, 2018

(oops hit the button too soon)

Would we want to continue to infect the pytest scope even after they have removed official support for doing so?

@altendky this is up to you guys, I just wanted to present an alternative solution that's all. I didn't want to leave you guys with the impression that we are deprecating pytest_namespace() and there are no other alternatives to do the same thing and without breaking compatibility with existing suites. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants