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

Verify hooks after collection completes #2292

Merged
merged 5 commits into from
Apr 19, 2017

Conversation

nicoddemus
Copy link
Member

Fix #1821

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 92.829% when pulling d965620 on nicoddemus:defer-hook-checking into 0c94f51 on pytest-dev:features.

@nicoddemus
Copy link
Member Author

Strange, Linux is breaking because pytester installs LsofFdLeakChecker and for some reason it is failing validation... will have to investigate some other time. Meanwhile if someone has any tips, I'm all ears!

@nicoddemus nicoddemus force-pushed the defer-hook-checking branch from d965620 to 7317767 Compare April 12, 2017 00:55
@nicoddemus
Copy link
Member Author

nicoddemus commented Apr 12, 2017

Oh wow, so the error was that LsofFdLeakChecker was declaring pytest_runtest_item instead of pytest_runtest_protocol. I guess this plugin has not been doing anything for a long time now. 😅

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 92.621% when pulling 7317767 on nicoddemus:defer-hook-checking into 78ac1bf on pytest-dev:features.

@nicoddemus
Copy link
Member Author

And not surprisingly some tests now fail because we are now detecting fd leaks again. 😅

That hookwrapper doesn't seem right as well, it is breaking the entire run instead of failing the offending test.

@@ -54,7 +54,6 @@ def main(args=None, plugins=None):
return 4
else:
try:
config.pluginmanager.check_pending()
Copy link
Member

Choose a reason for hiding this comment

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

should we remove this check just because we added a new one at a different time in execution?

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 so, that was the main motivation of this PR (at least for me): to delay hook checking until we have loaded all plugins, included those delayed by the pytest_plugins mechanism; this avoid having to write [deferred installation](https://docs.pytest.org/en/latest/writing_plugins.html#optionally-using-hooks-from-3rd-party-plugins mechanisms) if the hooks declared by other plugins are required by yours.

Also see #1821 for a detailed example.

Copy link
Member

Choose a reason for hiding this comment

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

@nicoddemus oh right, thanks for the reference, my understanding had been wrong

Copy link
Member Author

Choose a reason for hiding this comment

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

No worries!

Do you or anybody else have any pointers on how to fix the tests leaking FDs?

Copy link
Member

Choose a reason for hiding this comment

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

unfortunately not, does it hapen locally as well

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably, but I don't have a Linux box with me to check this now

Copy link
Member Author

Choose a reason for hiding this comment

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

Since this is unrelated to this PR (the PR actually fixed the plugin and it is working again), should we create an issue for it and disable the plugin internally?

Copy link
Member

Choose a reason for hiding this comment

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

sounds good

@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 92.633% when pulling d9a2e70 on nicoddemus:defer-hook-checking into 78ac1bf on pytest-dev:features.

@nicoddemus nicoddemus force-pushed the defer-hook-checking branch from 2951c59 to 5df340f Compare April 13, 2017 21:35
@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 92.636% when pulling 5df340f on nicoddemus:defer-hook-checking into 78ac1bf on pytest-dev:features.

Just pass fslocation forward and let the hook implementer decide what to do with the parameter
@nicoddemus nicoddemus force-pushed the defer-hook-checking branch from 5df340f to cac82e7 Compare April 13, 2017 22:05
@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 92.632% when pulling cac82e7 on nicoddemus:defer-hook-checking into 78ac1bf on pytest-dev:features.

@nicoddemus
Copy link
Member Author

I think if nobody else wants to chime in, we can close this now.

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