-
Notifications
You must be signed in to change notification settings - Fork 586
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
WIP: Add async testing within stateful rules #4147
base: master
Are you sure you want to change the base?
Conversation
Nice! Definitely want a single nursery for each run, and only import libraries when needed; I look forward to seeing where this goes! |
@Zac-HD Mind if I get a review on direction for this? I think it's mainly just figuring how how the |
|
||
|
||
async def get_async_result_with_time(func, stateful_run_times, *args, **kwargs): | ||
"""This is mainly a copy of the sync version of the method, but plays nicely with python async/await""" |
There was a problem hiding this comment.
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 possible to move most of the duplicated logic into a (sync-only) context manager
with recorded_time(stateful_run_times, label=f"execute:rule:{func.__name__}"):
return await func(*args, **kwargs)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh that's much cleaner, thanks @jobh!
@@ -17,15 +17,19 @@ | |||
""" | |||
import collections | |||
import inspect | |||
import trio |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be a conditional import, so we only actually import trio
when the user explicitly opts in to some Trio-specific functionality.
I suggest starting with the asyncio
version for now.
@@ -10,6 +10,8 @@ | |||
|
|||
import base64 | |||
import re | |||
import inspect | |||
import trio |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'd want to make this an optional dependency, and thus put the tests in e.g. tests/trio/test_trio_stateful.py
; as above I suggest starting with asyncio and putting those tests in tests/cover/test_asyncio_stateful.py
.
Gives a start to having concurrency within each rule outlined in #4107.
Eventually it seems like a good idea to have a single nursery for the entire run, but this will give a good start. It's gonna need a bit more plumbing, but putting it out there in case if there's any comments in the PR direction!