test(api): Replace pytest-aiohttp with pytest-asyncio for running async tests by SyntaxColoring · Pull Request #9981 · Opentrons/opentrons

SyntaxColoring · 2022-04-16T05:12:34Z

Overview

Our test framework, pytest, does not natively support async test functions or fixtures. Various 3rd-party plugins take up that responsibility. For historical reasons, the plugin that we've been using is pytest-aiohttp, which isn't a good fit for us because we're not even using the aiohttp web server in this project.

This PR replaces pytest-aiohttp with pytest-asyncio, a much smaller plugin from the pytest devs that's specifically meant for running async tests.

Benefits to this include:

No longer needing confusing loop boilerplate arguments for async fixtures.
Replacing a big dev dependency with a much smaller one. (pytest-aiohttp pulled in the whole aiohttp web server library.)
Speeding up the test suite from 4m50s to 1m30s, on my machine.

This addresses half of #8176, with the other half being robot-server. See https://github.com/Opentrons/opentrons/issues/8176#issuecomment-1100568533 for why I went with pytest-asyncio instead of anyio.

Changelog

Remove the pytest-aiohttp dev dependency and replace it with pytest-asyncio.
pytest-aiohttp's magic loop fixture now no longer exists. If a fixture or test function was requesting it, remove it. If it was actually using it, replace that usage with asyncio.get_running_loop().
Some def things secretly needed to be running in an event loop, even though they never await anything. Make them async def to help pytest-asyncio pick them up.
Remove the asyncio_loop_exception_handler fixture, which I couldn't port.

Review requests

Skim the diff and make sure nothing snuck in. The easiest way to scan through the changes might be commit-by-commit.
Pull this branch, run make -C api teardown && make -C api setup, and then run some pytest commands. Make sure they keep reporting what they were reporting before. In particular, make sure results don't change between running the whole suite and running individual files or functions.

Risk assessment

I think low. Async stuff in pytest is inherently a huge mess, so there's some risk because of that, but, I mean...can this be any worse than pytest-aiohttp?

…ssary.

SyntaxColoring · 2022-04-16T05:15:47Z

        ot2_only: Test only functions using the OT2 hardware
        ot3_only: Test only functions the OT3 hardware
 addopts = --color=yes
+asyncio_mode = auto


See https://github.com/pytest-dev/pytest-asyncio#modes.

We alternatively could have used strict mode, but it would have created a lot of boilerplate.

codecov · 2022-04-16T05:16:13Z

Codecov Report

Merging #9981 (ac53e0d) into edge (b44dbfb) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             edge    #9981   +/-   ##
=======================================
  Coverage   74.93%   74.93%           
=======================================
  Files        2077     2077           
  Lines       54807    54807           
  Branches     5527     5527           
=======================================
  Hits        41069    41069           
  Misses      12642    12642           
  Partials     1096     1096

Flag	Coverage Δ
notify-server	`89.17% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

SyntaxColoring · 2022-04-16T05:36:54Z

-@pytest.fixture(autouse=True)
-def asyncio_loop_exception_handler(loop):
-    def exception_handler(loop, context):
-        pytest.fail(str(context))
-
-    loop.set_exception_handler(exception_handler)
-    yield
-    loop.set_exception_handler(None)
-
-


I believe the intent of this fixture was to fail any test when the subject accidentally created an orphan task, and that task raised an uncaught exception.

I couldn't figure out a robust way to reimplement this, since we no longer have the loop fixture.

Note that it would not be sufficient to do:

@pytest.fixture(autouse=True) async def asyncio_loop_exception_handler(): loop = asyncio.get_running_loop() def exception_handler(loop, context): pytest.fail(str(context)) loop.set_exception_handler(exception_handler) yield loop.set_exception_handler(None)

For 2 reasons:

We can't choose exactly when this fixture runs (as far as I can tell). So, if it happens to run after an orphan task raises an exception, this exception handler won't catch it.

This fixture only makes sense if we restore the old exception handler after the event loop closes. Otherwise, we can't be sure our exception handler has been around to catch everything there is to catch. So, inherently, this can't run inside the event loop. It needs to be something supervisory, "around" the event loop.

(Actually, now that I'm thinking about this, these might have been problems with the fixture under pytest-aiohttp, all along.)

Honestly, I'm not too bummed about removing this? It's nice to catch these errors if we can, but really, we should never be creating orphan tasks in the first place. And the easiest way to enforce that is often by enforcing good structural practices, rather than tests. Sort of like how we know it's important to use with open(...) as f: to avoid leaking file descriptors, because the tests can't always catch that kind of bug easily. We can also look into cranking up the warnings, though.

mcous

Holy crap, the test speedup is wild. I had no idea this plugin was causing such harm to our test suite. My make -C api test times are:

edge - 3m42s
This branch - 1m35s (more than twice as fast)

A couple minor comments below, but the reasoning for going with pytest-asyncio over anyio all checks out, and is consistent with our actual needs (which do not include attempting to run on a trio loop or anything like that). The auto mode is nice and seems cleanest for test suite maintenance.

There's a few places where we could use the event_loop fixture instead of asyncio.get_running_loop(). I think I'm more in favor of the latter (as this PR is written) but I'm unaware of the implications one way or another. There are enough open issues about event_loop in the pytest-asyncio repo to make me nervous about using it.

Reading through this PR, I also think at some point we should re-evaluate all these explicit loop parameters spread across our codebase and remove most, if not all, of them

mcous · 2022-04-16T16:28:23Z

    return ProtocolContext(
        implementation=ProtocolContextImplementation(sync_hardware=hardware.sync),
-        loop=loop,
+        loop=asyncio.get_running_loop(),


Not sure how I feel about this, but seems like we could use the event_loop fixture instead

mcous · 2022-04-16T16:31:19Z

        serial=mock_serial,
        executor=ThreadPoolExecutor(),
-        loop=loop,
+        loop=asyncio.get_running_loop(),


Could use event_loop (but still, not sure if we should)

mcous · 2022-04-16T16:33:06Z

+async def subject(mock_serial_port: AsyncMock, ack: str) -> SerialConnection:
    """Create the test subject."""
    SerialConnection.RETRY_WAIT_TIME = 0  # type: ignore[attr-defined]
    return SerialConnection(


Perhaps we should use the async SerialConnection.create factory method, instead, to make this more obvious? The creation of an asyncio.Lock in __init__ seems a little problematic

mcous · 2022-04-16T16:33:21Z

    return AsyncMock(spec=AsyncSerial)


+# Async because SmoothieConnection.__init__() needs an event loop,


Same comment about using create, if possible

mcous · 2022-04-17T14:30:14Z

        execution_manager=AsyncMock(),
        usb_port=USBPort(name="", port_number=1, device_path="", hub=1),
-        loop=loop,
+        loop=asyncio.get_running_loop(),


I was going to suggest omitting the loop parameter, but it looks like AbstractModule.__init__ will call asyncio.get_event_loop instead of get_running_loop. So the code as you've written it seems like the best move here.

Calling this out since get_event_loop may spin up a new event loop, which seems bad and is probably worth a TODO or ticket at some point. See src/opentrons/hardware_control/util.py::use_or_initialize_loop for the offending code

mcous · 2022-04-17T16:13:45Z

+# Async because ProtocolContext.__init__() needs an event loop,
+# so this fixture needs to run in an event loop.


I wonder if we should make this more obvious by passing in loop=asyncio.get_running_loop()

Yeah this is an interesting question that I struggled with. And it applies to async API design in general, not just tests.

I’ll ask about this in Slack.

sfoster1

Love the speedup, makes total sense.

SyntaxColoring · 2022-04-18T13:48:18Z

Holy crap, the test speedup is wild. I had no idea this plugin was causing such harm to our test suite.

Yeah, this was a happy surprise for me, too. :)

There's a few places where we could use the event_loop fixture instead of asyncio.get_running_loop(). I think I'm more in favor of the latter (as this PR is written) but I'm unaware of the implications one way or another. There are enough open issues about event_loop in the pytest-asyncio repo to make me nervous about using it.

I totally missed that that fixture even existed. Thanks for the pointer. I also don’t have an informed opinion on whether we should be using it or get_running_loop(). In the absence of a reason to prefer the fixture, I’ll keep us using get_running_loop() for now, since it’s a bit less magical.

Reading through this PR, I also think at some point we should re-evaluate all these explicit loop parameters spread across our codebase and remove most, if not all, of them

💯

sanni-t

🙌

* module instances clean themselves up

SyntaxColoring added 6 commits April 15, 2022 23:26

Replace pytest-aiohttp dependency with pytest-asyncio.

443f61d

Configure pytest-aiohttp for auto mode.

8ec1607

Delete asyncio_loop_exception_handler fixture.

37402db

Remove loop arguments and replace with get_running_loop() as nece…

985ba55

…ssary.

Make additional stuff async to ensure it gets run in an event loop.

2a9efef

Format and lint.

fa4de8e

SyntaxColoring added api Affects the `api` project refactor robot-svcs Falls under the purview of the Robot Services squad (formerly CPX, Core Platform Experience). python General Python tickets and PRs; e.g. tech debt or Python guild activities labels Apr 16, 2022

SyntaxColoring requested a review from a team as a code owner April 16, 2022 05:12

SyntaxColoring commented Apr 16, 2022

View reviewed changes

mcous approved these changes Apr 17, 2022

View reviewed changes

sfoster1 approved these changes Apr 18, 2022

View reviewed changes

sanni-t reviewed Apr 18, 2022

View reviewed changes

fix(api): Less magical async tests fix (#9990)

ac53e0d

* module instances clean themselves up

SyntaxColoring merged commit ef2b9f9 into edge Apr 19, 2022

SyntaxColoring deleted the less_magical_async_tests branch April 19, 2022 16:07

mcous mentioned this pull request Apr 19, 2022

test(robot-server): replace pytest-aiohttp with pytest-asyncio #10012

Merged

2 tasks

amitlissack mentioned this pull request Apr 20, 2022

test(hardware): replace pytest-aiohttp with pytest-asyncio #10022

Merged

		return AsyncMock(spec=AsyncSerial)


		# Async because SmoothieConnection.__init__() needs an event loop,

		# Async because ProtocolContext.__init__() needs an event loop,
		# so this fixture needs to run in an event loop.

Conversation

SyntaxColoring commented Apr 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Changelog

Review requests

Risk assessment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Apr 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

SyntaxColoring Apr 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcous left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sfoster1 left a comment

Choose a reason for hiding this comment

Uh oh!

SyntaxColoring commented Apr 18, 2022

Uh oh!

sanni-t left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

SyntaxColoring commented Apr 16, 2022 •

edited

Loading

codecov bot commented Apr 16, 2022 •

edited

Loading

SyntaxColoring Apr 16, 2022 •

edited

Loading

mcous left a comment •

edited

Loading