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

Restructure pytest plugin hooks #91

Merged
merged 37 commits into from
May 20, 2020
Merged

Restructure pytest plugin hooks #91

merged 37 commits into from
May 20, 2020

Conversation

altendky
Copy link
Member

@altendky altendky commented Mar 1, 2020

These changes enable handling of nested async fixtures and non-function scope async fixtures.

WIP for:

  • More self review for trivial cleanup
  • Self review of architecture
  • Matching fixture teardown order
    • Presently this PR is coded for concurrent teardown which sounds nice with async but some fixtures will not only have startup dependencies on other fixtures but also teardown dependencies. This should be able to be documented etc but for now we should probably just skip concurrent teardown. That said, there's a test for it so... hmm.
    • In another PR add config for this and deprecate the default concurrent teardown. Or maybe leave it and allow fixture teardown dependency specification. Or default non-concurrent and ... do it somewhere else. Proper ordering of async fixture setup and teardown including inter-fixture dependencies #57
  • Attempt to get 'external' feedback
  • Review and update readme
  • Fix outcome assertion Consider all outcomes in assert_outcomes() #98
  • Address loss of concurrent fixture teardown caused by 50983ee and masked by the bad outcome assertions

@altendky altendky changed the title Rework for nested async fixtures [WIP] Rework for nested async fixtures Mar 1, 2020
@altendky
Copy link
Member Author

altendky commented Mar 2, 2020

@cdunklau, I haven't made it all the way through my basic self-review let alone a reconsideration of where these changes evolved too... but I think this is may be worth looking at at this point and trying out. I'm thinking that it might be worth testing non-function scope fixtures as well. Should be easy enough with a fixture that yields itertools.count() or somesuch but I'm not quite going to get to that tonight. I just think it is worth looking at this as a restructure that implements a few missing pieces rather than just fixing your original issue.

@altendky altendky changed the title [WIP] Rework for nested async fixtures [WIP] Restructure pytest plugin hooks Mar 4, 2020
def pytest_fixture_setup(fixturedef, request):
"""Interface pytest to async setup for async and async yield fixtures."""
# TODO: what about inlineCallbacks fixtures?
maybe_mark = getattr(fixturedef.func, _mark_attribute_name, None)
Copy link

@meejah meejah Mar 4, 2020

Choose a reason for hiding this comment

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

Maybe I'm not following the code exactly, but the maybeDeferred in in_reactor later on should handle that, I think? (referring to the "TODO" comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

And this is why I need these todo's so I come back and get a clue... :| I was thinking there were inlineCallbacks fixtures and they ought to be handled here too. There aren't. *smh* and such. Thanks.

Though, seems like they could be implemented (after 30 seconds of thought anyways). Add a decorator to mark them and intercept them here for setup before pytest gets confused by them.

@altendky
Copy link
Member Author

Hi @vtitor and @schmir. After doing this PR I am starting to feel like I actually have some grasp on how pytest-twisted works overall but it is only reasonable to assume there is still plenty of history and reasons that I still don't know (or straight up errors I've made as well...). If you have time I would appreciate any feedback on these changes, even if just some quick questions or such about something that looks fishy. If not, no worries. I get it being busy with other things. :] As always, thanks for the work you already donated to this project.

@schmir
Copy link
Contributor

schmir commented Mar 31, 2020

Sorry, but I don't have time to look into this.

@altendky altendky mentioned this pull request May 10, 2020
3 tasks
@altendky altendky changed the title [WIP] Restructure pytest plugin hooks Restructure pytest plugin hooks May 10, 2020
@altendky altendky requested a review from cdunklau May 10, 2020 03:18
@altendky
Copy link
Member Author

To have it in the record here: pytest-dev/pytest#3261 (comment)

@altendky altendky mentioned this pull request May 14, 2020
1 task
@altendky altendky merged commit a487615 into pytest-dev:master May 20, 2020
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.

4 participants