(#378) unit tests mostly working

pyproject.toml

@@ -90,6 +90,7 @@ ignore_missing_imports = true # Ignore missing stubs in imported modules
 asyncio_mode = "auto"
 markers = [
     "s03: marks tests as requiring the s03 simulator running (deselect with '-m \"not s03\"')",
+    "skip_in_pycharm: marks test as not working in pycharm testrunner"
 ]
 addopts = """
     --cov=dodal --cov-report term --cov-report xml:cov.xml

src/dodal/devices/robot.py

@@ -1,5 +1,5 @@
 import asyncio
-from asyncio import FIRST_COMPLETED, Task
+from asyncio import FIRST_COMPLETED, CancelledError, Task
 from dataclasses import dataclass
 from enum import Enum
 
@@ -73,19 +73,28 @@ async def raise_if_no_pin():
             await wait_for_value(self.error_code, self.NO_PIN_ERROR_CODE, None)
             raise RobotLoadFailed(self.NO_PIN_ERROR_CODE, "Pin was not detected")
 
-        finished, unfinished = await asyncio.wait(
-            [
-                Task(raise_if_no_pin()),
-                Task(
-                    wait_for_value(self.gonio_pin_sensor, PinMounted.PIN_MOUNTED, None)
-                ),
-            ],
-            return_when=FIRST_COMPLETED,
-        )
-        for task in unfinished:
-            task.cancel()
-        for task in finished:
-            await task
+        async def wfv():
+            await wait_for_value(self.gonio_pin_sensor, PinMounted.PIN_MOUNTED, None)
+
+        tasks = [
+            (Task(raise_if_no_pin())),
+            (Task(wfv())),
+        ]
+        try:
+            finished, unfinished = await asyncio.wait(
+                tasks,
+                return_when=FIRST_COMPLETED,
+            )
+            for task in unfinished:
+                task.cancel()
+            for task in finished:
+                await task
+        except CancelledError:
+            # If the outer enclosing task cancels after LOAD_TIMEOUT, this causes CancelledError to be raised
+            # in the current task, when it propagates to here we should cancel all pending tasks before bubbling up
+            for task in tasks:
+                task.cancel()
+            raise
 
     async def _load_pin_and_puck(self, sample_location: SampleLocation):
         LOGGER.info(f"Loading pin {sample_location}")

src/dodal/devices/webcam.py

@@ -4,7 +4,7 @@
 import aiofiles
 from aiohttp import ClientSession
 from bluesky.protocols import Triggerable
-from ophyd_async.core import AsyncStatus, StandardReadable, soft_signal_rw
+from ophyd_async.core import AsyncStatus, HintedSignal, StandardReadable, soft_signal_rw
 from PIL import Image
 
 from dodal.devices.oav.utils import save_thumbnail
@@ -18,7 +18,7 @@ def __init__(self, name, prefix, url):
         self.directory = soft_signal_rw(str, name="directory")
         self.last_saved_path = soft_signal_rw(str, name="last_saved_path")
 
-        self.set_readable_signals([self.last_saved_path])
+        self.add_readables([self.last_saved_path], wrapper=HintedSignal)
         super().__init__(name=name)
 
     async def _write_image(self, file_path: str):

src/dodal/devices/zocalo/zocalo_results.py

@@ -10,7 +10,7 @@
 import workflows.transport
 from bluesky.protocols import Descriptor, Triggerable
 from numpy.typing import NDArray
-from ophyd_async.core import StandardReadable, soft_signal_r_and_setter
+from ophyd_async.core import HintedSignal, StandardReadable, soft_signal_r_and_setter
 from ophyd_async.core.async_status import AsyncStatus
 from workflows.transport.common_transport import CommonTransport
 
@@ -88,14 +88,15 @@ def __init__(
         )
         self.ispyb_dcid, _ = soft_signal_r_and_setter(int, name="ispyb_dcid")
         self.ispyb_dcgid, _ = soft_signal_r_and_setter(int, name="ispyb_dcgid")
-        self.set_readable_signals(
-            read=[
+        self.add_readables(
+            [
                 self.results,
                 self.centres_of_mass,
                 self.bbox_sizes,
                 self.ispyb_dcid,
                 self.ispyb_dcgid,
-            ]
+            ],
+            wrapper=HintedSignal,
         )
         super().__init__(name)
 

tests/common/beamlines/test_beamline_utils.py

@@ -1,4 +1,5 @@
-from unittest.mock import ANY, MagicMock, patch
+import asyncio
+from unittest.mock import ANY, AsyncMock, MagicMock, patch
 
 import pytest
 from bluesky.run_engine import RunEngine as RE
@@ -13,11 +14,22 @@
 from dodal.devices.aperturescatterguard import ApertureScatterguard
 from dodal.devices.smargon import Smargon
 from dodal.devices.zebra import Zebra
+from dodal.log import LOGGER
 from dodal.utils import make_all_devices
 
 from ...conftest import mock_beamline_module_filepaths
 
 
+@pytest.fixture(autouse=True)
+def flush_event_loop_on_finish(event_loop):
+    # wait for the test function to complete
+    yield None
+
+    if pending_tasks := asyncio.all_tasks(event_loop):
+        LOGGER.warning(f"Waiting for pending tasks to complete {pending_tasks}")
+        event_loop.run_until_complete(asyncio.gather(*pending_tasks))
+
+
 @pytest.fixture(autouse=True)
 def setup():
     beamline_utils.clear_devices()
@@ -106,10 +118,11 @@ def test_wait_for_v1_device_connection_passes_through_timeout(kwargs, expected_t
 @pytest.mark.parametrize(
     "kwargs,expected_timeout", [({}, 5.0), ({"timeout": 15.0}, 15.0)]
 )
-@patch.object(beamline_utils, "call_in_bluesky_event_loop", spec=callable)
-def test_wait_for_v2_device_connection_passes_through_timeout(
-    call_in_bluesky_el, kwargs, expected_timeout
-):
+@patch(
+    "dodal.common.beamlines.beamline_utils.v2_device_wait_for_connection",
+    new=AsyncMock(),
+)
+def test_wait_for_v2_device_connection_passes_through_timeout(kwargs, expected_timeout):
     RE()
     device = OphydV2Device()
     device.connect = MagicMock()

tests/devices/unit_tests/test_eiger.py

@@ -7,14 +7,15 @@
 from ophyd.status import Status
 from ophyd.utils import UnknownStatusFailure
 
-from conftest import failed_status
 from dodal.devices.detector import DetectorParams, TriggerMode
 from dodal.devices.detector.det_dim_constants import EIGER2_X_16M_SIZE
 from dodal.devices.eiger import TEST_1169_FIX, EigerDetector
 from dodal.devices.status import await_value
 from dodal.devices.util.epics_util import run_functions_without_blocking
 from dodal.log import LOGGER
 
+from ...conftest import failed_status
+
 TEST_DETECTOR_SIZE_CONSTANTS = EIGER2_X_16M_SIZE
 
 TEST_EXPECTED_ENERGY = 100.0

tests/devices/unit_tests/test_focusing_mirror.py

@@ -6,7 +6,6 @@
 from ophyd.status import StatusBase
 from ophyd.utils import UnknownStatusFailure
 
-from conftest import failed_status
 from dodal.devices.focusing_mirror import (
     FocusingMirrorWithStripes,
     MirrorStripe,
@@ -15,6 +14,8 @@
     VFMMirrorVoltages,
 )
 
+from ...conftest import failed_status
+
 
 @pytest.fixture
 def vfm_mirror_voltages_not_ok(vfm_mirror_voltages) -> VFMMirrorVoltages:

tests/devices/unit_tests/test_webcam.py

@@ -39,6 +39,8 @@ async def test_given_filename_and_directory_when_trigger_and_read_then_returns_e
     webcam: Webcam,
 ):
     mock_get.return_value.__aenter__.return_value = (mock_response := AsyncMock())
+    # raise_for_status should be MagicMock() not AsyncMock()
+    mock_response.raise_for_status = MagicMock()
     mock_response.read.return_value = b"TEST"
     await webcam.filename.set(filename)
     await webcam.directory.set(directory)
@@ -54,6 +56,8 @@ async def test_given_data_returned_from_url_when_trigger_then_data_written(
     mock_image, mock_get: MagicMock, mock_aiofiles, webcam: Webcam
 ):
     mock_get.return_value.__aenter__.return_value = (mock_response := AsyncMock())
+    # raise_for_status should be MagicMock() not AsyncMock()
+    mock_response.raise_for_status = MagicMock()
     mock_response.read.return_value = (test_web_data := b"TEST")
     mock_open = mock_aiofiles.open
     mock_open.return_value.__aenter__.return_value = (mock_file := AsyncMock())

tests/test_cli.py

@@ -1,9 +1,12 @@
 import subprocess
 import sys
 
+import pytest
+
 from dodal import __version__
 
 
+@pytest.mark.skip_in_pycharm(reason="subprocess returns tty escape sequences")
 def test_cli_version():
     cmd = [sys.executable, "-m", "dodal", "--version"]
     assert subprocess.check_output(cmd).decode().strip() == __version__

tests/unit_tests/test_log.py

@@ -66,8 +66,12 @@ def test_dev_mode_sets_correct_graypy_handler(
     mock_logger: MagicMock,
 ):
     mock_GELFTCPHandler.return_value.level = logging.INFO
-    set_up_all_logging_handlers(mock_logger, Path("tmp/dev"), "dodal.log", True, 10000)
+    handler_config = set_up_all_logging_handlers(
+        mock_logger, Path("tmp/dev"), "dodal.log", True, 10000
+    )
     mock_GELFTCPHandler.assert_called_once_with("localhost", 5555)
+    for handler in handler_config.values():
+        handler.close()
 
 
 @patch("dodal.log.GELFTCPHandler", autospec=True)
@@ -76,10 +80,14 @@ def test_prod_mode_sets_correct_graypy_handler(
     mock_logger: MagicMock,
 ):
     mock_GELFTCPHandler.return_value.level = logging.INFO
-    set_up_all_logging_handlers(mock_logger, Path("tmp/dev"), "dodal.log", False, 10000)
+    handler_config = set_up_all_logging_handlers(
+        mock_logger, Path("tmp/dev"), "dodal.log", False, 10000
+    )
     mock_GELFTCPHandler.assert_called_once_with(
         "graylog-log-target.diamond.ac.uk", 12231
     )
+    for handler in handler_config.values():
+        handler.close()
 
 
 @patch("dodal.log.GELFTCPHandler", autospec=True)
@@ -95,7 +103,7 @@ def test_no_env_variable_sets_correct_file_handler(
     mock_file_handler.return_value.level = logging.INFO
     mock_GELFTCPHandler.return_value.level = logging.INFO
     clear_all_loggers_and_handlers()
-    _ = set_up_all_logging_handlers(
+    handler_config = set_up_all_logging_handlers(
         LOGGER, get_logging_file_path(), "dodal.log", True, ERROR_LOG_BUFFER_LINES
     )
     integrate_bluesky_and_ophyd_logging(LOGGER)
@@ -106,6 +114,8 @@ def test_no_env_variable_sets_correct_file_handler(
     ]
 
     mock_file_handler.assert_has_calls(expected_calls, any_order=True)
+    for handler in handler_config.values():
+        handler.close()
 
 
 def test_beamline_filter_adds_dev_if_no_beamline():