Don't ignore ImportError when importing setuptools plugins #1564
Conversation
The-Compiler
changed the title
|
It might be nice to add a more user friendly message to the user. If you let Python print the stack trace like I think you are doing now, will it be clear to a user which entry point failed to load? Adding a try/catch/warn/re-raise bit to the plugin manager is probably needed to do that. |
|
I think the filenames in the stacktrace should make it clear what plugin failed - and either way, that'd be a separate PR to pluggy. |
|
why do we actually notsee those warnings? i'd rather like to propose we always show |
|
As mentioned in the message, because pytest bails out earlier when e.g. a commandline option is used but not defined because the import failed. I'd like to ask a different question: Why is failing to import a plugin (which could invoke any unwanted/undefined behaviour) a warning and not an error? |
|
hmm, i'm not sure, but i'd happily do a major release to change that bit i think |
I also wondered this myself. FWIW I would also like for this to be a hard error. |
|
Similar to the added error when no tests were collected, I don't think stopping to hide an error and replacing it with a very confusing different error ("unrecognized arguments") should wait until a major release. At least 5 people did a lot of digging because of this behaviour now... let's stop hiding errors already. If a test environment has a plugin installed which raises an |
👍 |
|
this is a incompatible external behavior change, so in my book this warrants a immediate major release to cope ^^ |
IMHO I consider this a bug. While I understand some CI setup with a broken plugin might start failing when upgrading pytest, I don't think there's anybody counting on this behavior. The current behavior is really undesirable IMO, myself having spent sometime trying to figure out why some fixture was missing from a run ( If you feel strongly about this, a compromise would be change the target of this PR to |
|
hmm, i think my point is not getting across, i'm fine with immediately releasing master as 3.0 as soon a fix for this one merged the version change is simply to be in with line with semver i'm also fine with any other way to get this fix out asap, i'm just unhappy that we'd not follow semver to the best of our knowledge |
|
as for the rationale, i am reasonably certain this one will break some random cooperate testsuites simply due to neglect that will happen, breaking changes get major numbers |
Hmm I see.
That might happen, true. Well, we will have a chance to discuss this in person in a few weeks. 😁 |
|
I see where you're coming from, but if we increment the major version for every version which introduces a behavior change in some obscure way, we'll end up with That being said, I'm okay with delaying this until the sprint and releasing a 3.0 during or shortly after the sprint. |
|
i don't see a problem with high version numbers - and its preferable to go for small handle-able changes per major iterations over completely breaking the world |
|
This seems to be something to target for pytest 3.0, right? |
|
Yes |
|
Agreed, could you rebase it @The-Compiler? |
|
What's the rebase needed for? Just so the tests rerun with the latest code? Anyways, rebased on latest |
Add changelog entry for #1564
This was added in b2d66b9 but is a bad idea. When a plugin can't be imported, commandline options (optionally set in pytest.ini) could be undefined, which means pytest bails out much earlier before showing the warning, which is hard to debug.
Fixes #1479, also see #1307 and #1497