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

Pkg resources bugfix #1854

Merged
merged 5 commits into from
Sep 19, 2016
Merged

Conversation

raquelalegre
Copy link
Contributor

Related to #1853.

@RonnyPfannschmidt
Copy link
Member

unfortunately by design the import will have to happen in any case (pythe plugins load that way)

so i dont see how your patch helps,
does it behave different on jython?

@@ -942,6 +941,7 @@ def _consider_importhook(self, args, entrypoint_name):
and find all the installed plugins to mark them for re-writing
by the importhook.
"""
import pkg_resources
ns, unknown_args = self._parser.parse_known_and_unknown_args(args)
mode = ns.assertmode
if mode == 'rewrite':
Copy link
Member

Choose a reason for hiding this comment

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

@nicoddemus do we have a potential behaviour bug wrt plugin loading there

it seems to work as intended it shouldnt be in the else branch below

Copy link
Member

Choose a reason for hiding this comment

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

I think it would be OK to move this to the else clause below, this would at least allow people to use plain if for some reason they don't have pkg_resources. OTOH if we really want to support running py.test without pkg_resources, we probably would need to write a test for it to ensure we don't break it again by accident.

@raquel-ucl is moving the import enough for you in Jython? It seems _consider_importhook function will be called regardless of the value of the --assert= option, so I think it will still break for you.

Btw, are you using --assert=plain?

Copy link
Contributor Author

@raquelalegre raquelalegre Aug 24, 2016

Choose a reason for hiding this comment

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

Sorry, I haven't been able to properly test this on my code yet because the Maven plugin is a bit of a black box for me still. But you are right, the import needs to be moved to the else clause, this change won't be enough. I'll do that. I'd like to help with the test as well, but I think I will need your help :)

About assert=plain I didn't know about it! I'll modify my code to use it. Thanks!

I'll have a closer look at the plugin I'm using to install the Python modules and see if I can make any sense of it.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 93.025% when pulling d3f4b3d on raquel-ucl:pkg_resources_bugfix into 847067f on pytest-dev:master.

@The-Compiler
Copy link
Member

I don't still don't get how pytest did work for you before v3 without pkg_resources though... Doesn't pluginmanager.load_setuptools_entrypoint unconditionally import pkg_resources (inside pluggy) and gets run on start?

@RonnyPfannschmidt
Copy link
Member

@raquel-ucl is everything ok on your side and do you plan to follow up on this PR ?

@flub flub merged commit 8f516d2 into pytest-dev:master Sep 19, 2016
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.

6 participants