Switched to a trampoline-style test runner in the pytest plugin #497

agronholm · 2022-11-09T23:16:20Z

This allows async fixtures to provide context variables for tests.

zanieb

Sweet! I have some questions and minor comments. It looks like there's pretty good prior test coverage for different sorts of fixtures / cancellation / etc.

This change also makes task groups created in fixtures available in tests, right?

Are there any expected negative side-effects to running everything in a single task?

zanieb · 2022-11-11T21:26:54Z

src/anyio/_backends/_asyncio.py

@@ -1674,6 +1676,8 @@ def _create_task_info(task: asyncio.Task) -> TaskInfo:


 class TestRunner(abc.TestRunner):
+ _send_stream: MemoryObjectSendStream[tuple[Awaitable[Any], asyncio.Future[Any]]]


Any particular reason this is defined at the class level instead of in __init__ alongside _runner_task?

I see you've defined it in __init__ for the trio backend.

In the trio implementation, I do a null check against it so I needed to set a value. It felt natural to do that in __init__() since I don't have any other class annotations there. Just a matter of taste I suppose.

I have a preference for keeping instance variables in __init__ and declaring class variables at the top level, but I think whatever your preference is here is still plenty clear.

zanieb · 2022-11-11T21:29:16Z

src/anyio/_backends/_asyncio.py

+
+ coro = func(*args, **kwargs)
+ future: asyncio.Future[T_Retval] = self._loop.create_future()
+ self._send_stream.send_nowait((coro, future))


Here you are relying on the null check for self._runner_task to ensure that self._send_stream exists. I'm surprised mypy doesn't complain about this, but I'm guessing it's because it's not declared as an optional type.

I don't know why mypy would complain about it. I never set _send_stream to None, and as long as _runner_task is around, _send_stream is as well.

Makes sense. If you set it to None at close (#497 (comment)) then the type will change and then mypy may have qualms.

zanieb · 2022-11-11T21:30:36Z

src/anyio/_backends/_asyncio.py

+ future.set_result(retval)
+
+ async def _call_in_runner_task(
+ self, func: Callable[..., Awaitable[T_Retval]], *args: object, **kwargs: object


I'm curious about the use of object as the annotation here instead of Any

It's to ensure that we're not accidentally trying to use them for anything.

zanieb · 2022-11-11T21:31:46Z

src/anyio/_backends/_asyncio.py

+ self._runner_task = None



Should self._send_stream be released here too?

Hm, I suppose that would be good form, even if not strictly necessary.

Agreed, seems nice for bookkeeping but I doubt it's critical for it to be garbage collected.

I've added code that cleanly closes, and then deletes, the send stream.

Nice! Seems great to include the aclose call there.

zanieb · 2022-11-11T22:23:43Z

src/anyio/_backends/_asyncio.py

+ except StopAsyncIteration:
+ self._raise_async_exceptions()


Why is this reraised now? It wasn't previously and is not in the trio implementation.

Trio does not have callbacks like asyncio does. Here we raise any errors from those callbacks that occurred during the run. I'm unsure if we should do this in the setup phase too.

Now that I look at the previous code, we did that in the setup phase but not teardown. I think it might make sense to do it in both.

Ah I see it's raising exceptions that occurred in teardown callbacks not reraising the captured iteration error. Might be nice to include a brief comment noting that.

Raising exceptions during setup and teardown makes sense right? Is there a reason you'd ever want them to pass silently? I wish it was easier to see the expected behavior in these tests https://github.com/agronholm/anyio/blob/trampoline-test/tests/test_pytest_plugin.py#L155-L159

I think a clarification is necessary. What's being raised here are errors caught by the asyncio exception handler, for callbacks scheduled via loop.call_soon(), loop.call_later() and loop.call_at(), not pytest's setup/teardown callbacks.

And as for reraising these in setup/teardown, I forgot that I'm already raising them in both the setup/teardown phases, as we should be doing.

Great thanks for taking the time to explain! I should have just read the implementation of _raise_async_exceptions in the first place but this was helpful.

agronholm · 2022-11-11T23:04:44Z

This change also makes task groups created in fixtures available in tests, right?

They were available to tests before this.

Are there any expected negative side-effects to running everything in a single task?

The reason I marked it as backwards incompatible is because users might have depended on contextvars set in fixtures to be visible only within those fixtures, and this approach might cause fixtures and tests to interfere with each other if they assume the previous behavior. In particular, context variables set in tests will carry on to subsequent tests if the same runner is used (which it would be if the next test shares a higher scoped async fixture with the current one).

agronholm · 2022-11-11T23:07:39Z

For the record, setting a context variable and not resetting it at the end of the fixture/test is just plain irresponsible, and the regular pytest machinery doesn't protect against that either.

zanieb · 2022-11-11T23:19:43Z

This change also makes task groups created in fixtures available in tests, right?

They were available to tests before this.

Ah in pytest-asyncio I get RuntimeError: Attempted to exit cancel scope in a different task than it was entered in so I presumed the separate tasks caused problems:

@pytest.fixture
async def task_group():
    async with create_task_group() as tg:
        yield tg


async def test_foo(task_group):
    pass
    # You can also use the task group to run something but it's not needed for the teardown error

Separately from this pull request, it may be worth adding test coverage for using a task group yielded in a fixture from a test — I don't see one like that right now.

In particular, context variables set in tests will carry on to subsequent tests if the same runner is used (which it would be if the next test shares a higher scoped async fixture with the current one).

This may be a cause of confusion and worth highlighting in documentation.

For the record, setting a context variable and not resetting it at the end of the fixture/test is just plain irresponsible

Agreed!

agronholm · 2022-11-11T23:22:30Z

Ah in pytest-asyncio I get RuntimeError: Attempted to exit cancel scope in a different task than it was entered in so I presumed the separate tasks caused problems:

That was already made possible in v3.6.0 by running the setup and teardown phases of each asyncgen fixture in the same task (this was mentioned in the version history).

agronholm · 2022-11-12T10:19:35Z

Separately from this pull request, it may be worth adding test coverage for using a task group yielded in a fixture from a test — I don't see one like that right now.

What's this then? https://github.com/agronholm/anyio/blob/master/tests/test_pytest_plugin.py#L242-L245

zanieb · 2022-11-13T21:25:50Z

That test uses a stream fixture that uses the task group fixture but the test itself does not use the task group's methods. I'm guessing it covers the relevant code paths, but it's not clear to an outsider that a task group yielded in a fixture could be consumed directly in a test. I'm not actually sure why you'd want to do that though — it'd leave a task running outside the test's context which seems like pretty bad practice. I think the existing test is great :)

agronholm · 2022-11-13T21:27:57Z

What I intend to use this for is higher scoped fixtures that start some heavy duty server like a complex web app. Such fixtures tend to be costly to set up or tear down.

agronholm · 2022-11-13T21:43:45Z

So, what's left? Documenting that context variables now work the same way in async tests/fixtures as with synchronous tests/fixtures?

zanieb

Documenting that context variables now work the same way in async tests/fixtures as with synchronous tests/fixtures?

Yeah I'd point out the potential pitfall there otherwise this looks great to me :)

agronholm · 2022-11-13T22:12:03Z

I did some testing and turns out it's slightly more complex than I originally thought. I'll write down the semantics in the testing docs.

agronholm · 2022-11-20T20:25:13Z

@madkinsz Ok, I've documented the logic and added a small section for comparison to pytest-asyncio and pytest-trio. Are we good now? 🙂

zanieb · 2022-11-23T22:05:57Z

@agronholm that reads clearly to me!

agronholm · 2022-11-23T22:07:50Z

Thanks for the review!

Switched to a trampoline-style test runner in the pytest plugin

9fd9959

agronholm requested a review from graingert November 9, 2022 23:23

zanieb reviewed Nov 11, 2022

View reviewed changes

Cleanly shut down the send stream on close()

d6af1cc

zanieb approved these changes Nov 13, 2022

View reviewed changes

agronholm added 2 commits November 13, 2022 23:59

Merge branch 'master' into trampoline-test

ff35baa

Fixed new mypy errors

9250d9f

agronholm marked this pull request as draft November 14, 2022 06:59

agronholm marked this pull request as ready for review November 14, 2022 06:59

Documented the context variable propagation login

9b9be81

agronholm merged commit 1b57189 into master Nov 23, 2022

agronholm deleted the trampoline-test branch November 23, 2022 22:07

zanieb mentioned this pull request Apr 17, 2024

IndexError on force_rollback of Transaction in tests. encode/databases#570

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switched to a trampoline-style test runner in the pytest plugin #497

Switched to a trampoline-style test runner in the pytest plugin #497

agronholm commented Nov 9, 2022

zanieb left a comment

zanieb Nov 11, 2022

zanieb Nov 11, 2022

agronholm Nov 11, 2022

zanieb Nov 11, 2022

zanieb Nov 11, 2022

agronholm Nov 11, 2022

zanieb Nov 11, 2022 •

edited

Loading

zanieb Nov 11, 2022

agronholm Nov 11, 2022

zanieb Nov 11, 2022

agronholm Nov 11, 2022

zanieb Nov 11, 2022

agronholm Nov 12, 2022

zanieb Nov 13, 2022

zanieb Nov 11, 2022

agronholm Nov 11, 2022

agronholm Nov 11, 2022

zanieb Nov 11, 2022

agronholm Nov 12, 2022 •

edited

Loading

zanieb Nov 13, 2022

agronholm commented Nov 11, 2022

agronholm commented Nov 11, 2022

zanieb commented Nov 11, 2022

agronholm commented Nov 11, 2022 •

edited

Loading

agronholm commented Nov 12, 2022

zanieb commented Nov 13, 2022

agronholm commented Nov 13, 2022

agronholm commented Nov 13, 2022

zanieb left a comment

agronholm commented Nov 13, 2022

agronholm commented Nov 20, 2022

zanieb commented Nov 23, 2022

agronholm commented Nov 23, 2022

		@@ -1674,6 +1676,8 @@ def _create_task_info(task: asyncio.Task) -> TaskInfo:


		class TestRunner(abc.TestRunner):
		_send_stream: MemoryObjectSendStream[tuple[Awaitable[Any], asyncio.Future[Any]]]

Switched to a trampoline-style test runner in the pytest plugin #497

Switched to a trampoline-style test runner in the pytest plugin #497

Conversation

agronholm commented Nov 9, 2022

zanieb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zanieb Nov 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agronholm Nov 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agronholm commented Nov 11, 2022

agronholm commented Nov 11, 2022

zanieb commented Nov 11, 2022

agronholm commented Nov 11, 2022 • edited Loading

agronholm commented Nov 12, 2022

zanieb commented Nov 13, 2022

agronholm commented Nov 13, 2022

agronholm commented Nov 13, 2022

zanieb left a comment

Choose a reason for hiding this comment

agronholm commented Nov 13, 2022

agronholm commented Nov 20, 2022

zanieb commented Nov 23, 2022

agronholm commented Nov 23, 2022

zanieb Nov 11, 2022 •

edited

Loading

agronholm Nov 12, 2022 •

edited

Loading

agronholm commented Nov 11, 2022 •

edited

Loading