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.

Sign up for GitHub

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

test(api): Replace pytest-aiohttp with pytest-asyncio for running async tests #9981

Merged
merged 7 commits into from
Apr 19, 2022
Merged

test(api): Replace pytest-aiohttp with pytest-asyncio for running async tests #9981

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
443f61d
Replace pytest-aiohttp dependency with pytest-asyncio.
SyntaxColoring Apr 15, 2022
8ec1607
Configure pytest-aiohttp for auto mode.
SyntaxColoring Apr 16, 2022
37402db
Delete `asyncio_loop_exception_handler` fixture.
SyntaxColoring Apr 16, 2022
985ba55
Remove `loop` arguments and replace with `get_running_loop()` as nece…
SyntaxColoring Apr 16, 2022
2a9efef
Make additional stuff async to ensure it gets run in an event loop.
SyntaxColoring Apr 15, 2022
fa4de8e
Format and lint.
SyntaxColoring Apr 16, 2022
ac53e0d
fix(api): Less magical async tests fix (#9990)
Apr 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion api/Pipfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ coverage = "==5.1"
mypy = "==0.910"
numpydoc = "==0.9.1"
pytest = "==7.0.1"
pytest-aiohttp = "==0.3.0"
pytest-asyncio = "~=0.18"
pytest-cov = "==2.10.1"
pytest-lazy-fixture = "==0.6.3"
pytest-xdist = "~=2.2.1"
Expand Down
440 changes: 70 additions & 370 deletions api/Pipfile.lock
View file
Open in desktop

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions api/pytest.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,4 @@ markers =
ot2_only: Test only functions using the OT2 hardware
ot3_only: Test only functions the OT3 hardware
addopts = --color=yes
asyncio_mode = auto
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

10 changes: 4 additions & 6 deletions api/tests/opentrons/config/test_advanced_settings.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,6 @@ def test_get_all_adv_settings_empty(clear_cache, mock_read_settings_file):


async def test_set_adv_setting(
loop,
mock_read_settings_file,
mock_settings_values,
mock_write_settings_file,
Expand All @@ -95,15 +94,15 @@ async def test_set_adv_setting(


async def test_set_adv_setting_unknown(
loop, mock_read_settings_file, mock_write_settings_file
mock_read_settings_file, mock_write_settings_file
):
mock_read_settings_file.return_value = advanced_settings.SettingsData({}, 1)
with pytest.raises(ValueError, match="is not recognized"):
await advanced_settings.set_adv_setting("no", False)


async def test_get_all_adv_settings_lru_cache(
loop, clear_cache, mock_read_settings_file, mock_write_settings_file
clear_cache, mock_read_settings_file, mock_write_settings_file
):
# Cache should not be used.
advanced_settings.get_all_adv_settings()
Expand All @@ -126,7 +125,6 @@ async def test_get_all_adv_settings_lru_cache(


async def test_restart_required(
loop,
restore_restart_required,
mock_read_settings_file,
mock_write_settings_file,
Expand Down Expand Up @@ -161,7 +159,7 @@ async def test_restart_required(
[False, "info"],
],
)
async def test_disable_log_integration_side_effect(loop, v, expected_level):
async def test_disable_log_integration_side_effect(v, expected_level):
with patch("opentrons.config.advanced_settings.log_control") as mock_log_control:

async def set_syslog_level(level):
Expand All @@ -177,7 +175,7 @@ async def set_syslog_level(level):
mock_log_control.set_syslog_level.assert_called_once_with(expected_level)


async def test_disable_log_integration_side_effect_error(loop):
async def test_disable_log_integration_side_effect_error():
with patch("opentrons.config.advanced_settings.log_control") as mock_log_control:

async def set_syslog_level(level):
Expand Down
39 changes: 20 additions & 19 deletions api/tests/opentrons/conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import aionotify # type: ignore[import]
except (OSError, ModuleNotFoundError):
aionotify = None
import asyncio
import os
import io
import json
Expand All @@ -36,16 +37,6 @@
Protocol = namedtuple("Protocol", ["text", "filename", "filelike"])


@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)


Comment on lines -39 to -48
Copy link
Contributor Author

@SyntaxColoring SyntaxColoring Apr 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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:

  1. 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.
  2. 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.

@pytest.fixture
def ot_config_tempdir(tmp_path: pathlib.Path) -> Generator[pathlib.Path, None, None]:
os.environ["OT_API_CONFIG_DIR"] = str(tmp_path)
Expand Down Expand Up @@ -153,7 +144,7 @@ def virtual_smoothie_env(monkeypatch):
@pytest.fixture(
params=["ot2", "ot3"],
)
async def machine_variant_ffs(request, loop):
async def machine_variant_ffs(request):
if request.node.get_closest_marker("ot3_only") and request.param == "ot2":
pytest.skip()
if request.node.get_closest_marker("ot2_only") and request.param == "ot3":
Expand All @@ -179,12 +170,14 @@ async def _build_ot2_hw() -> AsyncIterator[ThreadManager[HardwareControlAPI]]:
yield hw_sim
finally:
config.robot_configs.clear()
for m in hw_sim.attached_modules:
await m.cleanup()
hw_sim.set_config(old_config)
hw_sim.clean_up()


@pytest.fixture
async def ot2_hardware(request, loop, virtual_smoothie_env):
async def ot2_hardware(request, virtual_smoothie_env):
async for hw in _build_ot2_hw():
yield hw

Expand All @@ -198,12 +191,14 @@ async def _build_ot3_hw() -> AsyncIterator[ThreadManager[HardwareControlAPI]]:
yield hw_sim
finally:
config.robot_configs.clear()
for m in hw_sim.attached_modules:
await m.cleanup()
hw_sim.set_config(old_config)
hw_sim.clean_up()


@pytest.fixture
async def ot3_hardware(request, loop, enable_ot3_hardware_controller):
async def ot3_hardware(request, enable_ot3_hardware_controller):
# this is from the command line parameters added in root conftest
if request.config.getoption("--ot2-only"):
pytest.skip("testing only ot2")
Expand All @@ -217,7 +212,7 @@ async def ot3_hardware(request, loop, enable_ot3_hardware_controller):
params=[lambda: _build_ot2_hw, lambda: _build_ot3_hw],
ids=["ot2", "ot3"],
)
async def hardware(request, loop, virtual_smoothie_env):
async def hardware(request, virtual_smoothie_env):
if request.node.get_closest_marker("ot2_only") and request.param() == _build_ot3_hw:
pytest.skip()
if request.node.get_closest_marker("ot3_only") and request.param() == _build_ot2_hw:
Expand All @@ -239,12 +234,18 @@ async def hardware(request, loop, virtual_smoothie_env):
)


# Async because ProtocolContext.__init__() needs an event loop,
# so this fixture needs to run in an event loop.
@pytest.fixture
async def ctx(loop, hardware) -> ProtocolContext:
return ProtocolContext(
async def ctx(hardware) -> AsyncIterator[ProtocolContext]:
c = ProtocolContext(
implementation=ProtocolContextImplementation(sync_hardware=hardware.sync),
loop=loop,
loop=asyncio.get_running_loop(),
Copy link
Contributor

Choose a reason for hiding this comment

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

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

)
yield c
# Manually clean up all the modules.
for m in c.loaded_modules.items():
m[1]._module.cleanup()


@pytest.fixture
Expand Down Expand Up @@ -298,8 +299,8 @@ async def mock_connect(obj, port=None):


@pytest.fixture
async def hardware_api(loop, is_robot):
hw_api = await API.build_hardware_simulator(loop=loop)
async def hardware_api(is_robot):
hw_api = await API.build_hardware_simulator(loop=asyncio.get_running_loop())
return hw_api


Expand Down
6 changes: 2 additions & 4 deletions api/tests/opentrons/drivers/asyncio/communication/test_async_serial.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,14 +30,12 @@ def mock_serial(


@pytest.fixture
async def subject(
loop: asyncio.AbstractEventLoop, mock_serial: MagicMock
) -> AsyncSerial:
async def subject(mock_serial: MagicMock) -> AsyncSerial:
"""The test subject."""
return AsyncSerial(
serial=mock_serial,
executor=ThreadPoolExecutor(),
loop=loop,
loop=asyncio.get_running_loop(),
Copy link
Contributor

Choose a reason for hiding this comment

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

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

reset_buffer_before_write=False,
)

Expand Down
4 changes: 3 additions & 1 deletion api/tests/opentrons/drivers/asyncio/communication/test_serial_connection.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,10 @@ def ack() -> str:
return "ack"


# Async because SerialConnection.__init__() needs an event loop,
# so this fixture needs to run in an event loop.
@pytest.fixture
def subject(mock_serial_port: AsyncMock, ack: str) -> SerialConnection:
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(
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Expand Down
4 changes: 3 additions & 1 deletion api/tests/opentrons/drivers/smoothie_drivers/test_connection.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,10 @@ def mock_serial_connection() -> AsyncMock:
return AsyncMock(spec=AsyncSerial)


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

Choose a reason for hiding this comment

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

Same comment about using create, if possible

# so this fixture needs to run in an event loop.
@pytest.fixture
def subject(mock_serial_connection: AsyncMock) -> SmoothieConnection:
async def subject(mock_serial_connection: AsyncMock) -> SmoothieConnection:
"""The test subject."""
return SmoothieConnection(
serial=mock_serial_connection,
Expand Down
3 changes: 1 addition & 2 deletions api/tests/opentrons/hardware_control/emulation/test_proxy.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,13 +42,12 @@ def proxy_listener() -> SimpleProxyListener:

@pytest.fixture
async def subject(
loop: asyncio.AbstractEventLoop,
setting: ProxySettings,
proxy_listener: SimpleProxyListener,
) -> AsyncIterator[Proxy]:
"""Test subject."""
p = Proxy("proxy", proxy_listener, setting)
task = loop.create_task(p.run())
task = asyncio.get_running_loop().create_task(p.run())
yield p
task.cancel()
try:
Expand Down
2 changes: 0 additions & 2 deletions api/tests/opentrons/hardware_control/integration/test_controller.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import asyncio
from typing import Iterator

import pytest
Expand All @@ -11,7 +10,6 @@

@pytest.fixture
async def subject(
loop: asyncio.BaseEventLoop,
emulation_app: Iterator[None],
emulator_settings: Settings,
) -> Controller:
Expand Down
3 changes: 1 addition & 2 deletions api/tests/opentrons/hardware_control/integration/test_magdeck.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,14 @@

@pytest.fixture
async def magdeck(
loop: asyncio.BaseEventLoop,
emulation_app: Iterator[None],
emulator_settings: Settings,
) -> AsyncIterator[MagDeck]:
module = await MagDeck.build(
port=f"socket://127.0.0.1:{emulator_settings.magdeck_proxy.driver_port}",
execution_manager=AsyncMock(),
usb_port=USBPort(name="", port_number=1, device_path="", hub=1),
loop=loop,
loop=asyncio.get_running_loop(),
Copy link
Contributor

Choose a reason for hiding this comment

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

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

)
yield module
await module.cleanup()
Expand Down
3 changes: 1 addition & 2 deletions api/tests/opentrons/hardware_control/integration/test_tempdeck.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@

@pytest.fixture
async def tempdeck(
loop: asyncio.BaseEventLoop,
emulator_settings: Settings,
emulation_app: Iterator[None],
) -> TempDeck:
Expand All @@ -19,7 +18,7 @@ async def tempdeck(
port=f"socket://127.0.0.1:{emulator_settings.temperature_proxy.driver_port}",
execution_manager=execution_manager,
usb_port=USBPort(name="", port_number=1, device_path="", hub=1),
loop=loop,
loop=asyncio.get_running_loop(),
polling_frequency=0.01,
)
yield module
Expand Down
3 changes: 1 addition & 2 deletions api/tests/opentrons/hardware_control/integration/test_thermocycler.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@

@pytest.fixture
async def thermocycler(
loop: asyncio.BaseEventLoop,
emulation_app: Iterator[None],
emulator_settings: Settings,
) -> Thermocycler:
Expand All @@ -19,7 +18,7 @@ async def thermocycler(
port=f"socket://127.0.0.1:{emulator_settings.thermocycler_proxy.driver_port}",
execution_manager=execution_manager,
usb_port=USBPort(name="", port_number=1, device_path="", hub=1),
loop=loop,
loop=asyncio.get_running_loop(),
polling_frequency=0.01,
)
yield module
Expand Down
5 changes: 3 additions & 2 deletions api/tests/opentrons/hardware_control/modules/test_hc_heatershaker.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import asyncio
import pytest
from opentrons.hardware_control import modules, ExecutionManager
from opentrons.hardware_control.modules.types import (
Expand All @@ -20,13 +21,13 @@ def usb_port():


@pytest.fixture
async def simulating_module(usb_port, loop):
async def simulating_module(usb_port):
module = await modules.build(
port=usb_port.device_path,
usb_port=usb_port,
which="heatershaker",
simulating=True,
loop=loop,
loop=asyncio.get_running_loop(),
execution_manager=ExecutionManager(),
)
assert isinstance(module, modules.AbstractModule)
Expand Down
20 changes: 11 additions & 9 deletions api/tests/opentrons/hardware_control/modules/test_hc_magdeck.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import asyncio

import pytest
from opentrons.hardware_control import modules, ExecutionManager

from opentrons.hardware_control import modules, ExecutionManager
from opentrons.drivers.rpi_drivers.types import USBPort


Expand All @@ -14,25 +16,25 @@ def usb_port():
)


async def test_sim_initialization(loop, usb_port):
async def test_sim_initialization(usb_port):
mag = await modules.build(
port="/dev/ot_module_sim_magdeck0",
usb_port=usb_port,
which="magdeck",
simulating=True,
loop=loop,
loop=asyncio.get_running_loop(),
execution_manager=ExecutionManager(),
)
assert isinstance(mag, modules.AbstractModule)


async def test_sim_data(loop, usb_port):
async def test_sim_data(usb_port):
mag = await modules.build(
port="/dev/ot_module_sim_magdeck0",
usb_port=usb_port,
which="magdeck",
simulating=True,
loop=loop,
loop=asyncio.get_running_loop(),
execution_manager=ExecutionManager(),
)
assert mag.status == "disengaged"
Expand All @@ -44,13 +46,13 @@ async def test_sim_data(loop, usb_port):
assert "data" in mag.live_data


async def test_sim_state_update(loop, usb_port):
async def test_sim_state_update(usb_port):
mag = await modules.build(
port="/dev/ot_module_sim_magdeck0",
usb_port=usb_port,
which="magdeck",
simulating=True,
loop=loop,
loop=asyncio.get_running_loop(),
execution_manager=ExecutionManager(),
)
await mag.calibrate()
Expand All @@ -61,13 +63,13 @@ async def test_sim_state_update(loop, usb_port):
assert mag.status == "disengaged"


async def test_revision_model_parsing(loop, usb_port):
async def test_revision_model_parsing(usb_port):
mag = await modules.build(
"",
"magdeck",
True,
usb_port,
loop=loop,
loop=asyncio.get_running_loop(),
execution_manager=ExecutionManager(),
)
mag._device_info["model"] = "mag_deck_v1.1"
Expand Down
Loading