-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
gh-74956: Add tests for async generator behaviour #111212
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
base: main
Are you sure you want to change the base?
Conversation
f9bb106
to
6656265
Compare
Lib/test/test_asyncgen.py
Outdated
# try different combinations of asend, athrow, aclose | ||
# which are clashing with an asend which is already running | ||
# (and awaiting sleep(0)) | ||
for op, args in [("asend", ["A"]), ("athrow", [EOFError]), ("aclose", [])]: | ||
agenerator = consumer() | ||
await agenerator.asend(None) # start it | ||
# fa will hit sleep and then fb will run | ||
fa = asyncio.create_task(agenerator.asend("A")) | ||
coro = getattr(agenerator, op)(*args) |
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 part is a bit hard to grasp.
I wonder if there's perhaps a better way to write this test or maybe have separate tests for send/throw/close?
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.
sure, I can unroll this.
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've updated the test to make it clearer, but unrolling it into different functions would be a lot of code duplication. I can also do that, or simply drop the extra tests for the clashing "athrow" and "aclose", although that would be a pity.
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.
it's more readable now, thank you
# The following behaviour may be undefined: | ||
r = await g.athrow(EOFError) | ||
assert r is None |
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.
Who can validate that behaviour is expected?
Maybe @1st1 ?
ef4737c
to
94fb0e3
Compare
# first asend should succeed | ||
await fa | ||
|
||
# second operation should fail | ||
with self.assertRaises(RuntimeError) as err: | ||
await fb | ||
assert "already running" in str(err.exception) |
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 wonder if this order is well-defined.
I guess that asend is internally synchronous send + await, so maybe?
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.
With the current event loop, yes it is well defined. Tasks are scheduled in the order they are created. First task runs until it hits sleep()
, at which point the second task runs. It is a strictly FIFO system.
asend is slightly more complex than send and await hooked together. It needs to have its own "await" implementation.
If you are interested, my implementation of the async generator protocol using my Monitor
pattern is here:
These tests are adapted from tests in my library.
This PR adds tests for issues bpo-30773 / gh-74956.
It appears that no tests were specifically added when this issue was resolved. This PR adds test showing
the behaviour of
ag_running
and how we expect simultaneousasend()
calls to behave.An additional tests verifies the behaviour of an exhausted async generator, particularly the odd
case of calling
athrow
on it. This may be undefined behaviour.This PR was prompted by the fact that
PYPY
exhibits different behaviour in this area from CPython.In particular, it appears that the fix for 74956 was never implemented there, since there were no failing
tests for it.