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

Handle first result with None results #69

Merged
merged 2 commits into from
Aug 29, 2017

Conversation

goodboy
Copy link
Contributor

@goodboy goodboy commented Aug 29, 2017

Addresses #68...

If you guys are happy with this I'll push up a patch release 0.5.1...

@RonnyPfannschmidt
Copy link
Member

at first glance i wonder if that test should trigger the behaviour, i was under the impression lack of plug-ins should trigger it

@goodboy
Copy link
Contributor Author

goodboy commented Aug 29, 2017

That test definitely gets the exact same error so the fix should handle both cases.
I can add another test for the no-plugin case as well.

@RonnyPfannschmidt
Copy link
Member

indeed https://github.com/pytest-dev/pluggy/blob/master/pluggy/callers.py#L92

so firstresult shortcuts, and there should always be zero or one results

@goodboy
Copy link
Contributor Author

goodboy commented Aug 29, 2017

@RonnyPfannschmidt added that second test as well.

@goodboy
Copy link
Contributor Author

goodboy commented Aug 29, 2017

haha wow totally broke it..

When calling a firstresult hook, if no implementation returns a non-None
value a bug is exposed in the new `_MultiCall` which assumes there will
be a least one result.
Only return the first result when at least one result has been returned
by underlying hook implementations.

Fixes pytest-dev#68
@goodboy
Copy link
Contributor Author

goodboy commented Aug 29, 2017

there finally..

@nicoddemus just waiting on your stamp of approval 😄

@goodboy goodboy self-assigned this Aug 29, 2017
Copy link
Member

@asottile asottile left a comment

Choose a reason for hiding this comment

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

lg2m, fixes tox with missing interpreters

@goodboy goodboy merged commit 1672800 into pytest-dev:master Aug 29, 2017
goodboy pushed a commit to goodboy/pluggy that referenced this pull request Aug 29, 2017
Fixes pytest-dev#68 and get's pytest-dev#69 into the wild stat!
@goodboy goodboy mentioned this pull request Aug 29, 2017
@goodboy
Copy link
Contributor Author

goodboy commented Aug 29, 2017

Thanks @asottile!

Once #70 is reviewed I'll push up the release 👍

Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

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

Thanks @tgoodlet for working on this so quickly!

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.

None yet

4 participants