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

Remove duplicate 'firstresult' test #62

Merged
merged 1 commit into from
Aug 24, 2017

Conversation

goodboy
Copy link
Contributor

@goodboy goodboy commented Aug 24, 2017

Remove test_method_ordering.test_decorator_functional as it is
duplicate of an already incomplete test.

Fix up test_firstresult_definition to be more more complete and actually test
that when multiple hooks are registered for a spec marked with firstresult
the first result is only ever returned.

Fixes #61

@goodboy
Copy link
Contributor Author

goodboy commented Aug 24, 2017

woa weird.. Is my assertion wrong there?
I thought the first result should come from the first registered plugin?

Ope, no, it comes from the last registered.

Fixing...

Remove `test_method_ordering.test_decorator_functional` as it is
duplicate of an already incomplete test. Fix up
`test_firstresult_definition` to be more more complete and actually test
that when multiple hooks are registered for a spec marked with `firstresult`
the first result is only ever returned.

Fixes pytest-dev#61
@RonnyPfannschmidt
Copy link
Member

now thats a really interesting issue - this shouldn't happen - i was under the impression we would at least use registration order

man seriously - every time i think there cant be a worse horror pytests legacy has one jsut ever more so "interesting" one

@nicoddemus
Copy link
Member

I think the logic is that core hooks are registered first, user hooks are registered last. So they are called in reverse order so user hooks can override core hooks.

But I agree it is slightly confusing.

@RonnyPfannschmidt RonnyPfannschmidt merged commit d53ff72 into pytest-dev:master Aug 24, 2017
@RonnyPfannschmidt
Copy link
Member

thanks for fixing the test - given the comments/properties i wonder if there is a more nice way to express the behaviour

@goodboy
Copy link
Contributor Author

goodboy commented Aug 24, 2017

@RonnyPfannschmidt the label firstresult you mean? Yeah I think it could be communicated a little more clearly for sure; not sure what better name would be though.

It's funny cause if you think about registration order it's more like lastresult lolz.

@nicoddemus
Copy link
Member

The label firstresult is fine IMHO, what is perhaps not clear is the order in which plugins are called. Perhaps we should add a "plugin call order" section to the docs?

@goodboy
Copy link
Contributor Author

goodboy commented Aug 24, 2017

@nicoddemus yeah good call. Do you mind making an issue?

@nicoddemus
Copy link
Member

Definitely, done: #64

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

Successfully merging this pull request may close these issues.

3 participants