From 9c8036964fcdefb357fd1d9d92f5b33f51c7787e Mon Sep 17 00:00:00 2001 From: Noemi Frisina Date: Mon, 1 Jun 2026 16:26:03 +0100 Subject: [PATCH 01/35] Add first version of an access controlled device to change the beamline energy --- .../i19/access_controlled/energy_device.py | 92 +++++++++++++++++++ 1 file changed, 92 insertions(+) create mode 100644 src/dodal/devices/beamlines/i19/access_controlled/energy_device.py diff --git a/src/dodal/devices/beamlines/i19/access_controlled/energy_device.py b/src/dodal/devices/beamlines/i19/access_controlled/energy_device.py new file mode 100644 index 00000000000..adb6db42ba2 --- /dev/null +++ b/src/dodal/devices/beamlines/i19/access_controlled/energy_device.py @@ -0,0 +1,92 @@ +from enum import StrEnum + +from ophyd_async.core import AsyncStatus, StandardReadableFormat +from ophyd_async.epics.core import epics_signal_r + +from dodal.devices.beamlines.i19.access_controlled.blueapi_device import ( + HutchState, + OpticsBlueAPIDevice, +) +from dodal.devices.beamlines.i19.access_controlled.hutch_access import ( + ACCESS_DEVICE_NAME, +) +from dodal.devices.beamlines.i19.mirror_stripes import StripeChoice + + +class OutOfRangeEnergyRequestError(Exception): + pass + + +class Stripes(StrEnum): + RH = "Rh" + SI = "Si" + PT = "Pt" + + +CHANGE_ENERGY_PLAN_NAME = "change_energy_plan" + + +class AccessControlledEnergyComposite(OpticsBlueAPIDevice): + """I19-specific device to change the beamline energy. + + This device will send a REST call to the blueapi instance controlling the optics + hutch running on the I19 cluster, which will evaluate the current hutch in use vs + the hutch sending the request and decide if the plan will be run or not. + As the two hutches are located in series, checking the hutch in use is necessary to + avoid accidentally operating the common optics devices (dcm, undulator, focusing + mirrors and stripes) from one hutch while the other has beamtime. + + The name of the hutch that wants to change the energy, as well as a commissioning + directory to act as a placeholder for the instrument_session, should be passed to the + device upon instantiation. + + For details see the architecture described in + https://diamondlightsource.github.io/i19-bluesky/main/explanations/decisions/0004-optics-blueapi-architecture.html + """ + + def __init__( + self, + dcm_prefix: str, + hutch: HutchState, + instrument_session: str = "", + name: str = "", + ) -> None: + with self.add_children_as_readables(StandardReadableFormat.HINTED_SIGNAL): + self.energy_in_kev = epics_signal_r(float, f"{dcm_prefix}ENERGY") + self.wavelength_in_a = epics_signal_r(float, f"{dcm_prefix}WAVELENGTH") + super().__init__(hutch, instrument_session, name) + + def _get_stripe_choice_from_energy_request( + self, energy_in_kev: float + ) -> StripeChoice: + """Returns the correct value to set to the mirror stripes, considering the + energy request and the invoking hutch. + """ + if 5 <= energy_in_kev < 10: + stripe = Stripes.SI + elif 10 <= energy_in_kev < 20: + stripe = Stripes.RH + elif 20 <= energy_in_kev < 30: + stripe = Stripes.PT + else: + raise OutOfRangeEnergyRequestError( + f"The requested energy at {energy_in_kev} KeV is out of range" + ) + _choice = f"{stripe}-{self._invoking_hutch}" + return StripeChoice(_choice) + + @AsyncStatus.wrap + async def set(self, value: float): + # Given value, get stripe type from energy range and get correct enum + selected_stripe = self._get_stripe_choice_from_energy_request(value) + request_params = { + "name": CHANGE_ENERGY_PLAN_NAME, + "params": { + "experiment_hutch": self._invoking_hutch, + "access_device": ACCESS_DEVICE_NAME, + "energy_in_kev": value, + "stripe_choice": selected_stripe.value, + }, + "instrument_session": self.instrument_session, + } + await super().set(request_params) From 7348647ab54c0ceb7d9f4a7338b69ca19c468091 Mon Sep 17 00:00:00 2001 From: Noemi Frisina Date: Mon, 1 Jun 2026 17:18:45 +0100 Subject: [PATCH 02/35] Instantiate device on i19-1 and i19-2 and tidy up a bit --- src/dodal/beamlines/i19_1.py | 21 ++++++++++++++------- src/dodal/beamlines/i19_2.py | 19 +++++++++++++------ 2 files changed, 27 insertions(+), 13 deletions(-) diff --git a/src/dodal/beamlines/i19_1.py b/src/dodal/beamlines/i19_1.py index 309e719a2f6..280e548c9e7 100644 --- a/src/dodal/beamlines/i19_1.py +++ b/src/dodal/beamlines/i19_1.py @@ -13,6 +13,9 @@ AttenuatorMotorSquad, ) from dodal.devices.beamlines.i19.access_controlled.blueapi_device import HutchState +from dodal.devices.beamlines.i19.access_controlled.energy_device import ( + AccessControlledEnergyComposite, +) from dodal.devices.beamlines.i19.access_controlled.piezo_control import ( AccessControlledPiezoActuator, FocusingMirrorName, @@ -43,7 +46,7 @@ set_utils_beamline(BL) -I19_1_COMMISSIONING_INSTR_SESSION: str = "cm40638-5" +I19_1_COMMISSIONING_INSTR_SESSION: str = "cm44168-3" I19_1_ZEBRA_MAPPING = ZebraMapping( outputs=ZebraTTLOutputs(TTL_PILATUS=1), @@ -85,6 +88,16 @@ def beamstop() -> BeamStop: return BeamStop(prefix=f"{PREFIX.beamline_prefix}-RS-ABSB-01:") +@devices.factory() +def energy_device() -> AccessControlledEnergyComposite: + """Access controlled composite device to enable changing the energy from EH1.""" + return AccessControlledEnergyComposite( + dcm_prefix=f"{PREFIX.beamline_prefix}-MO-DCM-01:", + hutch=HutchState.EH1, + instrument_session=I19_1_COMMISSIONING_INSTR_SESSION, + ) + + @devices.fixture def oav_config() -> OAVConfigBeamCentre: return OAVConfigBeamCentre(ZOOM_PARAMS_FILE, DISPLAY_CONFIG, config_client()) @@ -165,9 +178,6 @@ def zebra() -> Zebra: @devices.factory() def hfm_piezo() -> AccessControlledPiezoActuator: - """Get the i19-1 access controlled hfm piezo device, instantiate it if it hasn't already been. - If this is called when already instantiated, it will return the existing object. - """ return AccessControlledPiezoActuator( prefix=f"{PREFIX.beamline_prefix}-OP-HFM-01:", mirror_type=FocusingMirrorName.HFM, @@ -178,9 +188,6 @@ def hfm_piezo() -> AccessControlledPiezoActuator: @devices.factory() def vfm_piezo() -> AccessControlledPiezoActuator: - """Get the i19-1 access controlled vfm piezo device, instantiate it if it hasn't already been. - If this is called when already instantiated, it will return the existing object. - """ return AccessControlledPiezoActuator( prefix=f"{PREFIX.beamline_prefix}-OP-VFM-01:", mirror_type=FocusingMirrorName.VFM, diff --git a/src/dodal/beamlines/i19_2.py b/src/dodal/beamlines/i19_2.py index daa92f476df..6e4d2e01b07 100644 --- a/src/dodal/beamlines/i19_2.py +++ b/src/dodal/beamlines/i19_2.py @@ -16,6 +16,9 @@ AttenuatorMotorSquad, ) from dodal.devices.beamlines.i19.access_controlled.blueapi_device import HutchState +from dodal.devices.beamlines.i19.access_controlled.energy_device import ( + AccessControlledEnergyComposite, +) from dodal.devices.beamlines.i19.access_controlled.piezo_control import ( AccessControlledPiezoActuator, FocusingMirrorName, @@ -109,6 +112,16 @@ def eiger(path_provider: PathProvider) -> EigerDetector: ) +@devices.factory() +def energy_device() -> AccessControlledEnergyComposite: + """Access controlled composite device to enable changing the energy from EH2.""" + return AccessControlledEnergyComposite( + dcm_prefix=f"{PREFIX.beamline_prefix}-MO-DCM-01:", + hutch=HutchState.EH2, + instrument_session=I19_2_COMMISSIONING_INSTR_SESSION, + ) + + @devices.factory() def panda(path_provider: PathProvider) -> HDFPanda: return HDFPanda( @@ -152,9 +165,6 @@ def zebra() -> Zebra: @devices.factory() def hfm_piezo() -> AccessControlledPiezoActuator: - """Get the i19-2 access controlled hfm piezo device, instantiate it if it hasn't already been. - If this is called when already instantiated, it will return the existing object. - """ return AccessControlledPiezoActuator( prefix=f"{PREFIX.beamline_prefix}-OP-HFM-01:", mirror_type=FocusingMirrorName.HFM, @@ -165,9 +175,6 @@ def hfm_piezo() -> AccessControlledPiezoActuator: @devices.factory() def vfm_piezo() -> AccessControlledPiezoActuator: - """Get the i19-2 access controlled vfm piezo device, instantiate it if it hasn't already been. - If this is called when already instantiated, it will return the existing object. - """ return AccessControlledPiezoActuator( prefix=f"{PREFIX.beamline_prefix}-OP-VFM-01:", mirror_type=FocusingMirrorName.VFM, From 4b9a6d7664877b8ed5e738ae73d519733c004660 Mon Sep 17 00:00:00 2001 From: Noemi Frisina Date: Mon, 1 Jun 2026 17:45:03 +0100 Subject: [PATCH 03/35] Add tests --- .../i19/access_controlled/energy_device.py | 2 +- .../access_controlled/test_energy_device.py | 217 ++++++++++++++++++ 2 files changed, 218 insertions(+), 1 deletion(-) create mode 100644 tests/devices/beamlines/i19/access_controlled/test_energy_device.py diff --git a/src/dodal/devices/beamlines/i19/access_controlled/energy_device.py b/src/dodal/devices/beamlines/i19/access_controlled/energy_device.py index adb6db42ba2..d8456477c6f 100644 --- a/src/dodal/devices/beamlines/i19/access_controlled/energy_device.py +++ b/src/dodal/devices/beamlines/i19/access_controlled/energy_device.py @@ -72,7 +72,7 @@ def _get_stripe_choice_from_energy_request( raise OutOfRangeEnergyRequestError( f"The requested energy at {energy_in_kev} KeV is out of range" ) - _choice = f"{stripe}-{self._invoking_hutch}" + _choice = f"{self._invoking_hutch}-{stripe}" return StripeChoice(_choice) @AsyncStatus.wrap diff --git a/tests/devices/beamlines/i19/access_controlled/test_energy_device.py b/tests/devices/beamlines/i19/access_controlled/test_energy_device.py new file mode 100644 index 00000000000..ecb9f18e8c9 --- /dev/null +++ b/tests/devices/beamlines/i19/access_controlled/test_energy_device.py @@ -0,0 +1,217 @@ +import json +from unittest.mock import AsyncMock, patch + +import pytest +from ophyd_async.core import SignalR, init_devices, set_mock_value +from ophyd_async.testing import assert_reading, partial_reading + +from dodal.devices.beamlines.i19.access_controlled.blueapi_device import ( + HEADERS, + HutchState, +) +from dodal.devices.beamlines.i19.access_controlled.energy_device import ( + AccessControlledEnergyComposite, + OutOfRangeEnergyRequestError, +) +from dodal.devices.beamlines.i19.mirror_stripes import StripeChoice + + +@pytest.fixture +def eh1_energy_device() -> AccessControlledEnergyComposite: + with init_devices(mock=True): + device = AccessControlledEnergyComposite( + "", HutchState.EH1, "cm12345-1", "mock_eh1_energy" + ) + device.url = "http://test.url" + set_mock_value(device.energy_in_kev, 17.9973) + set_mock_value(device.wavelength_in_a, 0.6889) + return device + + +@pytest.fixture +def eh2_energy_device() -> AccessControlledEnergyComposite: + with init_devices(mock=True): + device = AccessControlledEnergyComposite( + "", HutchState.EH2, "cm12345-1", "mock_eh1_energy" + ) + device.url = "http://test.url" + set_mock_value(device.energy_in_kev, 17.9973) + set_mock_value(device.wavelength_in_a, 0.6889) + return device + + +@pytest.mark.parametrize("hutch_name", [HutchState.EH1, HutchState.EH2]) +def test_device_created_without_errors(hutch_name: HutchState): + test_device = AccessControlledEnergyComposite( + "", hutch_name, "cm12345-1", "fake-device" + ) + assert isinstance(test_device, AccessControlledEnergyComposite) + assert isinstance(test_device.energy_in_kev, SignalR) + assert isinstance(test_device.wavelength_in_a, SignalR) + + +async def test_energy_and_wavelength_can_be_read_from_device( + eh1_energy_device: AccessControlledEnergyComposite, +): + await assert_reading( + eh1_energy_device, + { + "mock_eh1_energy-energy_in_kev": partial_reading(17.9973), + "mock_eh1_energy-wavelength_in_a": partial_reading(0.6889), + }, + ) + + +def test_get_stripe_choice_errors_if_energy_out_of_bounds( + eh2_energy_device: AccessControlledEnergyComposite, +): + with pytest.raises(OutOfRangeEnergyRequestError): + eh2_energy_device._get_stripe_choice_from_energy_request(2.0) + + +@pytest.mark.parametrize( + "energy_request, expected_stripe", + [ + (8.4, StripeChoice.EH1_SI), + (17.9, StripeChoice.EH1_RH), + (25.3, StripeChoice.EH1_PT), + ], +) +def test_correct_stripe_selected_for_eh1( + energy_request: float, + expected_stripe: StripeChoice, + eh1_energy_device: AccessControlledEnergyComposite, +): + stripe = eh1_energy_device._get_stripe_choice_from_energy_request(energy_request) + assert stripe == expected_stripe + + +@pytest.mark.parametrize( + "energy_request, expected_stripe", + [ + (6.2, StripeChoice.EH2_SI), + (14.86, StripeChoice.EH2_RH), + (28.7, StripeChoice.EH2_PT), + ], +) +def test_correct_stripe_returned_for_eh2( + energy_request: float, + expected_stripe: StripeChoice, + eh2_energy_device: AccessControlledEnergyComposite, +): + stripe = eh2_energy_device._get_stripe_choice_from_energy_request(energy_request) + assert stripe == expected_stripe + + +@pytest.mark.parametrize( + "energy_request, expected_stripe", + [ + (8.4, StripeChoice.EH1_SI), + (17.9, StripeChoice.EH1_RH), + (25.3, StripeChoice.EH1_PT), + ], +) +async def test_device_makes_correct_rest_call_for_eh1( + energy_request: float, + expected_stripe: StripeChoice, + eh1_energy_device: AccessControlledEnergyComposite, +): + expected_params = { + "name": "change_energy_plan", + "params": { + "experiment_hutch": "EH1", + "access_device": "access_control", + "energy_in_kev": energy_request, + "stripe_choice": expected_stripe.value, + }, + "instrument_session": "cm12345-1", + } + expected_params_json = json.dumps(expected_params) + with ( + patch( + "dodal.devices.beamlines.i19.access_controlled.blueapi_device.ClientSession.post" + ) as mock_post, + patch( + "dodal.devices.beamlines.i19.access_controlled.blueapi_device.ClientSession.put" + ) as mock_put, + patch( + "dodal.devices.beamlines.i19.access_controlled.blueapi_device.ClientSession.get" + ) as mock_get, + ): + mock_post.return_value.__aenter__.return_value = (mock_response := AsyncMock()) + mock_response.ok = True + mock_response.json.return_value = {"task_id": "abc0-3fg8"} + mock_put.return_value.__aenter__.return_value = ( + mock_put_response := AsyncMock() + ) + mock_put_response.ok = True + mock_get.return_value.__aenter__.return_value = ( + mock_get_response := AsyncMock() + ) + mock_get_response.json.return_value = {"is_complete": True, "errors": []} + + await eh1_energy_device.set(energy_request) + + mock_post.assert_called_with( + "/tasks", data=expected_params_json, headers=HEADERS + ) + mock_put.assert_called_with( + "/worker/task", data='{"task_id": "abc0-3fg8"}', headers=HEADERS + ) + + +@pytest.mark.parametrize( + "energy_request, expected_stripe", + [ + (6.2, StripeChoice.EH2_SI), + (14.86, StripeChoice.EH2_RH), + (28.7, StripeChoice.EH2_PT), + ], +) +async def test_device_makes_correct_rest_call_for_eh2( + energy_request: float, + expected_stripe: StripeChoice, + eh2_energy_device: AccessControlledEnergyComposite, +): + expected_params = { + "name": "change_energy_plan", + "params": { + "experiment_hutch": "EH2", + "access_device": "access_control", + "energy_in_kev": energy_request, + "stripe_choice": expected_stripe.value, + }, + "instrument_session": "cm12345-1", + } + expected_params_json = json.dumps(expected_params) + with ( + patch( + "dodal.devices.beamlines.i19.access_controlled.blueapi_device.ClientSession.post" + ) as mock_post, + patch( + "dodal.devices.beamlines.i19.access_controlled.blueapi_device.ClientSession.put" + ) as mock_put, + patch( + "dodal.devices.beamlines.i19.access_controlled.blueapi_device.ClientSession.get" + ) as mock_get, + ): + mock_post.return_value.__aenter__.return_value = (mock_response := AsyncMock()) + mock_response.ok = True + mock_response.json.return_value = {"task_id": "abc0-3fg8"} + mock_put.return_value.__aenter__.return_value = ( + mock_put_response := AsyncMock() + ) + mock_put_response.ok = True + mock_get.return_value.__aenter__.return_value = ( + mock_get_response := AsyncMock() + ) + mock_get_response.json.return_value = {"is_complete": True, "errors": []} + + await eh2_energy_device.set(energy_request) + + mock_post.assert_called_with( + "/tasks", data=expected_params_json, headers=HEADERS + ) + mock_put.assert_called_with( + "/worker/task", data='{"task_id": "abc0-3fg8"}', headers=HEADERS + ) From 43ce6c7797c1ccc7d720e23c4e1a211bf3b719b8 Mon Sep 17 00:00:00 2001 From: Noemi Frisina Date: Mon, 1 Jun 2026 17:48:49 +0100 Subject: [PATCH 04/35] Add energy ranges to comment --- .../devices/beamlines/i19/access_controlled/energy_device.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/dodal/devices/beamlines/i19/access_controlled/energy_device.py b/src/dodal/devices/beamlines/i19/access_controlled/energy_device.py index d8456477c6f..ecc35c805e6 100644 --- a/src/dodal/devices/beamlines/i19/access_controlled/energy_device.py +++ b/src/dodal/devices/beamlines/i19/access_controlled/energy_device.py @@ -61,6 +61,11 @@ def _get_stripe_choice_from_energy_request( ) -> StripeChoice: """Returns the correct value to set to the mirror stripes, considering the energy request and the invoking hutch. + + Energy ranges: + SI: (5KeV, 10KeV] + RH: (10KeV, 20KeV] + PT: (20KeV, 30KeV] """ if 5 <= energy_in_kev < 10: stripe = Stripes.SI From 76f3566b1d69ca900c921fbf64ac8bd35c257f71 Mon Sep 17 00:00:00 2001 From: Noemi Frisina Date: Tue, 2 Jun 2026 11:24:08 +0100 Subject: [PATCH 05/35] Update docstring --- .../beamlines/i19/access_controlled/energy_device.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/dodal/devices/beamlines/i19/access_controlled/energy_device.py b/src/dodal/devices/beamlines/i19/access_controlled/energy_device.py index ecc35c805e6..ed11dccc916 100644 --- a/src/dodal/devices/beamlines/i19/access_controlled/energy_device.py +++ b/src/dodal/devices/beamlines/i19/access_controlled/energy_device.py @@ -40,6 +40,10 @@ class AccessControlledEnergyComposite(OpticsBlueAPIDevice): directory to act as a placeholder for the instrument_session, should be passed to the device upon instantiation. + The plan to change the energy also involves moving the mirror stripes and the stripe + selection is hutch-dependent, so the choice of stripe needs to be done internally to + this device and passed to the plan. + For details see the architecture described in https://diamondlightsource.github.io/i19-bluesky/main/explanations/decisions/0004-optics-blueapi-architecture.html """ @@ -63,9 +67,9 @@ def _get_stripe_choice_from_energy_request( energy request and the invoking hutch. Energy ranges: - SI: (5KeV, 10KeV] - RH: (10KeV, 20KeV] - PT: (20KeV, 30KeV] + SI: [5KeV, 10KeV) + RH: [10KeV, 20KeV) + PT: [20KeV, 30KeV) """ if 5 <= energy_in_kev < 10: stripe = Stripes.SI From d278017ed9793ec7de370478d684d3c9ecb7133e Mon Sep 17 00:00:00 2001 From: Noemi Frisina Date: Fri, 5 Jun 2026 13:19:05 +0100 Subject: [PATCH 06/35] Make the exception a ValueError --- .../devices/beamlines/i19/access_controlled/energy_device.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/dodal/devices/beamlines/i19/access_controlled/energy_device.py b/src/dodal/devices/beamlines/i19/access_controlled/energy_device.py index ed11dccc916..fee064e3379 100644 --- a/src/dodal/devices/beamlines/i19/access_controlled/energy_device.py +++ b/src/dodal/devices/beamlines/i19/access_controlled/energy_device.py @@ -13,7 +13,7 @@ from dodal.devices.beamlines.i19.mirror_stripes import StripeChoice -class OutOfRangeEnergyRequestError(Exception): +class OutOfRangeEnergyRequestError(ValueError): pass From 9200db403081fcdc77e8f9325528c14363c30d5b Mon Sep 17 00:00:00 2001 From: Noemi Frisina Date: Fri, 19 Jun 2026 13:44:43 +0100 Subject: [PATCH 07/35] Read the energy range bounds from a file and use the config server to parse it --- src/dodal/beamlines/i19_1.py | 3 +- src/dodal/beamlines/i19_2.py | 3 +- .../i19/access_controlled/energy_device.py | 55 ++++++++++++++----- .../access_controlled/test_energy_device.py | 41 ++++++++++++-- 4 files changed, 81 insertions(+), 21 deletions(-) diff --git a/src/dodal/beamlines/i19_1.py b/src/dodal/beamlines/i19_1.py index 280e548c9e7..4c5cdc82557 100644 --- a/src/dodal/beamlines/i19_1.py +++ b/src/dodal/beamlines/i19_1.py @@ -89,11 +89,12 @@ def beamstop() -> BeamStop: @devices.factory() -def energy_device() -> AccessControlledEnergyComposite: +def energy_device(config_client: ConfigClient) -> AccessControlledEnergyComposite: """Access controlled composite device to enable changing the energy from EH1.""" return AccessControlledEnergyComposite( dcm_prefix=f"{PREFIX.beamline_prefix}-MO-DCM-01:", hutch=HutchState.EH1, + config_client=config_client, instrument_session=I19_1_COMMISSIONING_INSTR_SESSION, ) diff --git a/src/dodal/beamlines/i19_2.py b/src/dodal/beamlines/i19_2.py index 6e4d2e01b07..525b5319f32 100644 --- a/src/dodal/beamlines/i19_2.py +++ b/src/dodal/beamlines/i19_2.py @@ -113,11 +113,12 @@ def eiger(path_provider: PathProvider) -> EigerDetector: @devices.factory() -def energy_device() -> AccessControlledEnergyComposite: +def energy_device(config_client: ConfigClient) -> AccessControlledEnergyComposite: """Access controlled composite device to enable changing the energy from EH2.""" return AccessControlledEnergyComposite( dcm_prefix=f"{PREFIX.beamline_prefix}-MO-DCM-01:", hutch=HutchState.EH2, + config_client=config_client, instrument_session=I19_2_COMMISSIONING_INSTR_SESSION, ) diff --git a/src/dodal/devices/beamlines/i19/access_controlled/energy_device.py b/src/dodal/devices/beamlines/i19/access_controlled/energy_device.py index fee064e3379..2c48aaa51e5 100644 --- a/src/dodal/devices/beamlines/i19/access_controlled/energy_device.py +++ b/src/dodal/devices/beamlines/i19/access_controlled/energy_device.py @@ -1,7 +1,11 @@ from enum import StrEnum +from pathlib import Path +from daq_config_server import ConfigClient from ophyd_async.core import AsyncStatus, StandardReadableFormat from ophyd_async.epics.core import epics_signal_r +from pydantic import BaseModel +from pydantic.dataclasses import dataclass from dodal.devices.beamlines.i19.access_controlled.blueapi_device import ( HutchState, @@ -12,6 +16,12 @@ ) from dodal.devices.beamlines.i19.mirror_stripes import StripeChoice +MIRROR_ENERGY_FILE_PATH = Path( + "/dls_sw/i19-1/software/i19-acquisition/i19-shared/json/MirrorEnergyRanges.json" +) + +CHANGE_ENERGY_PLAN_NAME = "change_energy_plan" + class OutOfRangeEnergyRequestError(ValueError): pass @@ -23,7 +33,25 @@ class Stripes(StrEnum): PT = "Pt" -CHANGE_ENERGY_PLAN_NAME = "change_energy_plan" +@dataclass +class EnergyRange: + name: Stripes + upper: float + lower: float + + +class MirrorEnergyRanges(BaseModel): + silicon: EnergyRange + rhodium: EnergyRange + platinum: EnergyRange + + def get_stripe_material_from_energy(self, energy_in_kev: float) -> Stripes: + for val in self.model_dump().values(): + if val["lower"] <= energy_in_kev < val["upper"]: + return Stripes(val["name"]) + raise OutOfRangeEnergyRequestError( + f"The requested energy at {energy_in_kev} KeV is out of range" + ) class AccessControlledEnergyComposite(OpticsBlueAPIDevice): @@ -52,9 +80,13 @@ def __init__( self, dcm_prefix: str, hutch: HutchState, + config_client: ConfigClient, instrument_session: str = "", name: str = "", ) -> None: + self.mirror_energies = config_client.get_file_contents( + MIRROR_ENERGY_FILE_PATH, desired_return_type=dict + ) with self.add_children_as_readables(StandardReadableFormat.HINTED_SIGNAL): self.energy_in_kev = epics_signal_r(float, f"{dcm_prefix}ENERGY") self.wavelength_in_a = epics_signal_r(float, f"{dcm_prefix}WAVELENGTH") @@ -67,21 +99,14 @@ def _get_stripe_choice_from_energy_request( energy request and the invoking hutch. Energy ranges: - SI: [5KeV, 10KeV) - RH: [10KeV, 20KeV) - PT: [20KeV, 30KeV) + Si: [5KeV, 10KeV) + Rh: [10KeV, 20KeV) + Pt: [20KeV, 30KeV) """ - if 5 <= energy_in_kev < 10: - stripe = Stripes.SI - elif 10 <= energy_in_kev < 20: - stripe = Stripes.RH - elif 20 <= energy_in_kev < 30: - stripe = Stripes.PT - else: - raise OutOfRangeEnergyRequestError( - f"The requested energy at {energy_in_kev} KeV is out of range" - ) - _choice = f"{self._invoking_hutch}-{stripe}" + _stripe = MirrorEnergyRanges( + **self.mirror_energies + ).get_stripe_material_from_energy(energy_in_kev) + _choice = f"{self._invoking_hutch}-{_stripe}" return StripeChoice(_choice) @AsyncStatus.wrap diff --git a/tests/devices/beamlines/i19/access_controlled/test_energy_device.py b/tests/devices/beamlines/i19/access_controlled/test_energy_device.py index ecb9f18e8c9..bb117e87334 100644 --- a/tests/devices/beamlines/i19/access_controlled/test_energy_device.py +++ b/tests/devices/beamlines/i19/access_controlled/test_energy_device.py @@ -1,7 +1,9 @@ import json -from unittest.mock import AsyncMock, patch +from typing import Any +from unittest.mock import AsyncMock, MagicMock, patch import pytest +from daq_config_server import ConfigClient from ophyd_async.core import SignalR, init_devices, set_mock_value from ophyd_async.testing import assert_reading, partial_reading @@ -11,16 +13,26 @@ ) from dodal.devices.beamlines.i19.access_controlled.energy_device import ( AccessControlledEnergyComposite, + MirrorEnergyRanges, OutOfRangeEnergyRequestError, + Stripes, ) from dodal.devices.beamlines.i19.mirror_stripes import StripeChoice +MIRROR_ENERGIES: dict[str, Any] = { + "silicon": {"name": "Si", "lower": 5.0, "upper": 10.0}, + "rhodium": {"name": "Rh", "lower": 10.0, "upper": 20.0}, + "platinum": {"name": "Pt", "lower": 20.0, "upper": 30.0}, +} + @pytest.fixture def eh1_energy_device() -> AccessControlledEnergyComposite: + mock_client = MagicMock() + mock_client.get_file_contents = MagicMock(return_value=MIRROR_ENERGIES) with init_devices(mock=True): device = AccessControlledEnergyComposite( - "", HutchState.EH1, "cm12345-1", "mock_eh1_energy" + "", HutchState.EH1, mock_client, "cm12345-1", "mock_eh1_energy" ) device.url = "http://test.url" set_mock_value(device.energy_in_kev, 17.9973) @@ -30,9 +42,11 @@ def eh1_energy_device() -> AccessControlledEnergyComposite: @pytest.fixture def eh2_energy_device() -> AccessControlledEnergyComposite: + mock_client = MagicMock() + mock_client.get_file_contents = MagicMock(return_value=MIRROR_ENERGIES) with init_devices(mock=True): device = AccessControlledEnergyComposite( - "", HutchState.EH2, "cm12345-1", "mock_eh1_energy" + "", HutchState.EH2, mock_client, "cm12345-1", "mock_eh1_energy" ) device.url = "http://test.url" set_mock_value(device.energy_in_kev, 17.9973) @@ -40,10 +54,29 @@ def eh2_energy_device() -> AccessControlledEnergyComposite: return device +def test_mirror_energy_ranges_model(): + model = MirrorEnergyRanges(**MIRROR_ENERGIES) + + assert model.silicon.name == "Si" + assert model.rhodium.lower == 10.0 and model.rhodium.upper == 20.0 + + +@pytest.mark.parametrize( + "energy_request, expected_stripe", + [(8, Stripes.SI), (17, Stripes.RH), (25, Stripes.PT)], +) +def test_get_stripe_from_energy_model(energy_request: float, expected_stripe: Stripes): + stripe = MirrorEnergyRanges(**MIRROR_ENERGIES).get_stripe_material_from_energy( + energy_request + ) + + assert stripe == expected_stripe + + @pytest.mark.parametrize("hutch_name", [HutchState.EH1, HutchState.EH2]) def test_device_created_without_errors(hutch_name: HutchState): test_device = AccessControlledEnergyComposite( - "", hutch_name, "cm12345-1", "fake-device" + "", hutch_name, ConfigClient(""), "cm12345-1", "fake-device" ) assert isinstance(test_device, AccessControlledEnergyComposite) assert isinstance(test_device.energy_in_kev, SignalR) From 1f808fd5d37e383eb805f7da3f11a26d2dfdc345 Mon Sep 17 00:00:00 2001 From: Noemi Frisina Date: Fri, 19 Jun 2026 13:48:29 +0100 Subject: [PATCH 08/35] Add small bit to test --- .../beamlines/i19/access_controlled/test_energy_device.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/devices/beamlines/i19/access_controlled/test_energy_device.py b/tests/devices/beamlines/i19/access_controlled/test_energy_device.py index bb117e87334..fa70b0315d8 100644 --- a/tests/devices/beamlines/i19/access_controlled/test_energy_device.py +++ b/tests/devices/beamlines/i19/access_controlled/test_energy_device.py @@ -95,11 +95,13 @@ async def test_energy_and_wavelength_can_be_read_from_device( ) +@pytest.mark.parametrize("out_of_bounds_energy", [2.0, 31.0]) def test_get_stripe_choice_errors_if_energy_out_of_bounds( + out_of_bounds_energy: float, eh2_energy_device: AccessControlledEnergyComposite, ): with pytest.raises(OutOfRangeEnergyRequestError): - eh2_energy_device._get_stripe_choice_from_energy_request(2.0) + eh2_energy_device._get_stripe_choice_from_energy_request(out_of_bounds_energy) @pytest.mark.parametrize( From 1d368ebd1e3d5d0a64bbc8de2bff6894fa86f78d Mon Sep 17 00:00:00 2001 From: Noemi Frisina Date: Fri, 19 Jun 2026 13:49:53 +0100 Subject: [PATCH 09/35] And fix failing one --- .../beamlines/i19/access_controlled/test_energy_device.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/devices/beamlines/i19/access_controlled/test_energy_device.py b/tests/devices/beamlines/i19/access_controlled/test_energy_device.py index fa70b0315d8..55fd3b96721 100644 --- a/tests/devices/beamlines/i19/access_controlled/test_energy_device.py +++ b/tests/devices/beamlines/i19/access_controlled/test_energy_device.py @@ -3,7 +3,6 @@ from unittest.mock import AsyncMock, MagicMock, patch import pytest -from daq_config_server import ConfigClient from ophyd_async.core import SignalR, init_devices, set_mock_value from ophyd_async.testing import assert_reading, partial_reading @@ -76,7 +75,7 @@ def test_get_stripe_from_energy_model(energy_request: float, expected_stripe: St @pytest.mark.parametrize("hutch_name", [HutchState.EH1, HutchState.EH2]) def test_device_created_without_errors(hutch_name: HutchState): test_device = AccessControlledEnergyComposite( - "", hutch_name, ConfigClient(""), "cm12345-1", "fake-device" + "", hutch_name, MagicMock(), "cm12345-1", "fake-device" ) assert isinstance(test_device, AccessControlledEnergyComposite) assert isinstance(test_device.energy_in_kev, SignalR) From b91cbcf2ea301582dc6d312be95d7676430ea9ec Mon Sep 17 00:00:00 2001 From: Noemi Frisina Date: Fri, 19 Jun 2026 14:16:29 +0100 Subject: [PATCH 10/35] Try to fix failure in device instantiation test --- src/dodal/beamlines/i19_1.py | 4 ++++ src/dodal/beamlines/i19_2.py | 5 +++++ .../beamlines/i19/access_controlled/energy_device.py | 8 ++------ tests/conftest.py | 3 +++ 4 files changed, 14 insertions(+), 6 deletions(-) diff --git a/src/dodal/beamlines/i19_1.py b/src/dodal/beamlines/i19_1.py index 4c5cdc82557..1c1211785ef 100644 --- a/src/dodal/beamlines/i19_1.py +++ b/src/dodal/beamlines/i19_1.py @@ -60,6 +60,9 @@ ) DISPLAY_CONFIG = f"{DAQ_CONFIGURATION_PATH}/domain/display.configuration" +SHARED_CONFIG_PATH = "/dls_sw/i19-1/software/i19-acquisition/i19-shared" +MIRROR_ENERGY_FILE_PATH = f"{SHARED_CONFIG_PATH}/json/MirrorEnergyRanges.json" + devices = DeviceManager() @@ -94,6 +97,7 @@ def energy_device(config_client: ConfigClient) -> AccessControlledEnergyComposit return AccessControlledEnergyComposite( dcm_prefix=f"{PREFIX.beamline_prefix}-MO-DCM-01:", hutch=HutchState.EH1, + mirror_energy_config=MIRROR_ENERGY_FILE_PATH, config_client=config_client, instrument_session=I19_1_COMMISSIONING_INSTR_SESSION, ) diff --git a/src/dodal/beamlines/i19_2.py b/src/dodal/beamlines/i19_2.py index 525b5319f32..62f0513a508 100644 --- a/src/dodal/beamlines/i19_2.py +++ b/src/dodal/beamlines/i19_2.py @@ -57,6 +57,10 @@ sources=ZebraSources(), ) +SHARED_CONFIG_PATH = "/dls_sw/i19-1/software/i19-acquisition/i19-shared" +MIRROR_ENERGY_FILE_PATH = f"{SHARED_CONFIG_PATH}/json/MirrorEnergyRanges.json" + + devices = DeviceManager() @@ -118,6 +122,7 @@ def energy_device(config_client: ConfigClient) -> AccessControlledEnergyComposit return AccessControlledEnergyComposite( dcm_prefix=f"{PREFIX.beamline_prefix}-MO-DCM-01:", hutch=HutchState.EH2, + mirror_energy_config=MIRROR_ENERGY_FILE_PATH, config_client=config_client, instrument_session=I19_2_COMMISSIONING_INSTR_SESSION, ) diff --git a/src/dodal/devices/beamlines/i19/access_controlled/energy_device.py b/src/dodal/devices/beamlines/i19/access_controlled/energy_device.py index 2c48aaa51e5..19c36466327 100644 --- a/src/dodal/devices/beamlines/i19/access_controlled/energy_device.py +++ b/src/dodal/devices/beamlines/i19/access_controlled/energy_device.py @@ -1,5 +1,4 @@ from enum import StrEnum -from pathlib import Path from daq_config_server import ConfigClient from ophyd_async.core import AsyncStatus, StandardReadableFormat @@ -16,10 +15,6 @@ ) from dodal.devices.beamlines.i19.mirror_stripes import StripeChoice -MIRROR_ENERGY_FILE_PATH = Path( - "/dls_sw/i19-1/software/i19-acquisition/i19-shared/json/MirrorEnergyRanges.json" -) - CHANGE_ENERGY_PLAN_NAME = "change_energy_plan" @@ -80,12 +75,13 @@ def __init__( self, dcm_prefix: str, hutch: HutchState, + mirror_energy_config: str, config_client: ConfigClient, instrument_session: str = "", name: str = "", ) -> None: self.mirror_energies = config_client.get_file_contents( - MIRROR_ENERGY_FILE_PATH, desired_return_type=dict + mirror_energy_config, desired_return_type=dict ) with self.add_children_as_readables(StandardReadableFormat.HINTED_SIGNAL): self.energy_in_kev = epics_signal_r(float, f"{dcm_prefix}ENERGY") diff --git a/tests/conftest.py b/tests/conftest.py index 6c058e2ea3a..7f4c6a77ce3 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -42,12 +42,15 @@ ("ZOOM_PARAMS_FILE", TEST_OAV_ZOOM_LEVELS), ("DISPLAY_CONFIG", TEST_DISPLAY_CONFIG), ("LOOK_UPTABLE_DIR", LOOKUP_TABLE_PATH), + ("SHARED_CONFIG_PATH", MOCK_DAQ_CONFIG_PATH), ] MOCK_ATTRIBUTES_TABLE = { "i03": MOCK_PATHS, "i10_optics": MOCK_PATHS, "i04": MOCK_PATHS, "i19_1": MOCK_PATHS, + "i19_2": MOCK_PATHS, + "i19_optics": MOCK_PATHS, "i23": MOCK_PATHS, "i24": MOCK_PATHS, "aithre": MOCK_PATHS, From c38e7c9107be90f9704a130a170f7fb8861b88b0 Mon Sep 17 00:00:00 2001 From: Noemi Frisina Date: Fri, 19 Jun 2026 14:18:54 +0100 Subject: [PATCH 11/35] Use correct config path for optics as daq-config-server released --- src/dodal/beamlines/i19_optics.py | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/dodal/beamlines/i19_optics.py b/src/dodal/beamlines/i19_optics.py index 833ea6a91b5..a265d31f0ef 100644 --- a/src/dodal/beamlines/i19_optics.py +++ b/src/dodal/beamlines/i19_optics.py @@ -34,12 +34,7 @@ set_log_beamline(BL) set_utils_beamline(BL) -# For the moment pointing to the daq_configuration path in i19-1 which has links to the -# common optics configuration, as it's already present in the daq-config-server. -# The correct path will have to wait for this PR -# https://github.com/DiamondLightSource/daq-config-server/pull/186 to be merged and -# a subsequent release -DAQ_CONFIGURATION_PATH = "/dls_sw/i19-1/software/daq_configuration" +DAQ_CONFIGURATION_PATH = "/dls_sw/i19-1/software/i19-acquisition/i19-shared" ID_GAP_LOOKUP = ( f"{DAQ_CONFIGURATION_PATH}/lookup-shared/energy_to_id_gap_look_up_table.txt" ) From 0e32204b4e955640a49dd56e7ab6cdf11f027f8e Mon Sep 17 00:00:00 2001 From: Noemi Frisina Date: Fri, 19 Jun 2026 14:20:58 +0100 Subject: [PATCH 12/35] And update all other tests by consequence maybe --- .../i19/access_controlled/test_energy_device.py | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/tests/devices/beamlines/i19/access_controlled/test_energy_device.py b/tests/devices/beamlines/i19/access_controlled/test_energy_device.py index 55fd3b96721..676170595ac 100644 --- a/tests/devices/beamlines/i19/access_controlled/test_energy_device.py +++ b/tests/devices/beamlines/i19/access_controlled/test_energy_device.py @@ -31,7 +31,12 @@ def eh1_energy_device() -> AccessControlledEnergyComposite: mock_client.get_file_contents = MagicMock(return_value=MIRROR_ENERGIES) with init_devices(mock=True): device = AccessControlledEnergyComposite( - "", HutchState.EH1, mock_client, "cm12345-1", "mock_eh1_energy" + "", + HutchState.EH1, + "/path/to/config", + mock_client, + "cm12345-1", + "mock_eh1_energy", ) device.url = "http://test.url" set_mock_value(device.energy_in_kev, 17.9973) @@ -45,7 +50,12 @@ def eh2_energy_device() -> AccessControlledEnergyComposite: mock_client.get_file_contents = MagicMock(return_value=MIRROR_ENERGIES) with init_devices(mock=True): device = AccessControlledEnergyComposite( - "", HutchState.EH2, mock_client, "cm12345-1", "mock_eh1_energy" + "", + HutchState.EH2, + "/path/to/config", + mock_client, + "cm12345-1", + "mock_eh1_energy", ) device.url = "http://test.url" set_mock_value(device.energy_in_kev, 17.9973) @@ -75,7 +85,7 @@ def test_get_stripe_from_energy_model(energy_request: float, expected_stripe: St @pytest.mark.parametrize("hutch_name", [HutchState.EH1, HutchState.EH2]) def test_device_created_without_errors(hutch_name: HutchState): test_device = AccessControlledEnergyComposite( - "", hutch_name, MagicMock(), "cm12345-1", "fake-device" + "", hutch_name, "/path/to/config", MagicMock(), "cm12345-1", "fake-device" ) assert isinstance(test_device, AccessControlledEnergyComposite) assert isinstance(test_device.energy_in_kev, SignalR) From 9dd1171ac06c72345c7e5d6f0c34856c8693d398 Mon Sep 17 00:00:00 2001 From: Oli Wenman Date: Mon, 22 Jun 2026 13:14:50 +0000 Subject: [PATCH 13/35] Standardise Daq config client in tests --- src/dodal/testing/fixtures/config_server.py | 120 +++++++++--------- system_tests/conftest.py | 5 +- system_tests/test_oav_system.py | 8 +- tests/beamlines/test_i24.py | 13 -- .../beamlines/test_beamline_parameters.py | 4 +- .../beamlines/test_device_instantiation.py | 7 +- tests/conftest.py | 94 ++++++++++++++ .../beamlines/i03/test_undulator_dcm.py | 10 +- .../beamlines/i09_2_shared/test_i09_apple2.py | 2 +- .../devices/beamlines/i10/test_i10_apple2.py | 2 +- tests/devices/beamlines/i15_1/test_blower.py | 6 +- tests/devices/beamlines/i15_1/test_cobra.py | 6 +- .../beamlines/i15_1/test_cryostream.py | 6 +- .../i15_1/test_laue_monochrometer.py | 4 +- .../devices/beamlines/i17/test_i17_apple2.py | 2 +- tests/devices/beamlines/i24/test_vgonio.py | 12 +- tests/devices/detector/test_det_resolution.py | 4 +- tests/devices/detector/test_detector.py | 13 +- .../test_apple_knot_path_finder.py | 2 +- tests/devices/mx_phase1/test_beamstop.py | 6 +- tests/devices/oav/conftest.py | 12 +- tests/devices/oav/test_oav.py | 12 +- tests/devices/oav/test_oav_parameters.py | 14 +- tests/devices/test_beam_converter.py | 4 +- tests/devices/test_eiger.py | 5 +- tests/devices/test_focusing_mirror.py | 4 +- tests/devices/test_undulator.py | 14 +- tests/devices/util/test_lookup_tables.py | 7 +- tests/plan_stubs/test_topup_plan.py | 4 +- tests/plans/conftest.py | 6 +- .../device_setup_plans/test_setup_pin_tip.py | 4 +- ...nfigure_arm_trigger_and_disarm_detector.py | 7 +- 32 files changed, 262 insertions(+), 157 deletions(-) delete mode 100644 tests/beamlines/test_i24.py diff --git a/src/dodal/testing/fixtures/config_server.py b/src/dodal/testing/fixtures/config_server.py index ba84eb050ca..6aa3a5bd1df 100644 --- a/src/dodal/testing/fixtures/config_server.py +++ b/src/dodal/testing/fixtures/config_server.py @@ -1,60 +1,60 @@ -import json -from collections.abc import Callable -from pathlib import Path -from typing import Any, TypeVar -from unittest.mock import patch - -import pytest -from daq_config_server.models import ConfigModel -from pydantic import TypeAdapter - -T = TypeVar("T", str, dict, ConfigModel) - - -def fake_config_server_get_file_contents( - filepath: str | Path, - desired_return_type: type[T] = str, - reset_cached_result: bool = True, - force_parser: Callable[[str], Any] | None = None, -) -> T: - """Fakes getting a file from the config server by reading it directly. - - Args: - filepath (str | Path): Filepath of the file to read - desired_return_type (type[T], optional): Type to convert the file to. Defaults to str. - reset_cached_result (bool, optional): Whether or not to use the config server's cached result. - Does nothing here as we don't cache. Defaults to True. - force_parser (Callable[[str], Any] | None, optional): Use a certain converter function. - Only needed for the interim where the converter exists but the config server has not - been redeployed. Defaults to None. - - Raises: - ValueError: Raised if an invalid type is requested - - Returns: - T: The contents of the config file. - """ - filepath = Path(filepath) - # Minimal logic required for unit tests - with filepath.open("r") as f: - contents = f.read() - if force_parser: - return TypeAdapter(desired_return_type).validate_python(force_parser(contents)) - if desired_return_type is str: - return contents # type: ignore - if desired_return_type is dict: - return json.loads(contents) - if issubclass(desired_return_type, ConfigModel): - return desired_return_type.model_validate(json.loads(contents)) - raise ValueError("Invalid return type requested") - - -@pytest.fixture(autouse=True) -def mock_config_server(): - # Don't actually talk to central service during unit tests, and reset caches between test - - with patch( - "daq_config_server.app.client.ConfigClient.get_file_contents", - side_effect=fake_config_server_get_file_contents, - ): - yield +# import json +# from collections.abc import Callable +# from pathlib import Path +# from typing import Any, TypeVar +# from unittest.mock import patch + +# import pytest +# from daq_config_server.models import ConfigModel +# from pydantic import TypeAdapter + +# T = TypeVar("T", str, dict, ConfigModel) + + +# def fake_config_server_get_file_contents( +# filepath: str | Path, +# desired_return_type: type[T] = str, +# reset_cached_result: bool = True, +# force_parser: Callable[[str], Any] | None = None, +# ) -> T: +# """Fakes getting a file from the config server by reading it directly. + +# Args: +# filepath (str | Path): Filepath of the file to read +# desired_return_type (type[T], optional): Type to convert the file to. Defaults to str. +# reset_cached_result (bool, optional): Whether or not to use the config server's cached result. +# Does nothing here as we don't cache. Defaults to True. +# force_parser (Callable[[str], Any] | None, optional): Use a certain converter function. +# Only needed for the interim where the converter exists but the config server has not +# been redeployed. Defaults to None. + +# Raises: +# ValueError: Raised if an invalid type is requested + +# Returns: +# T: The contents of the config file. +# """ +# filepath = Path(filepath) +# # Minimal logic required for unit tests +# with filepath.open("r") as f: +# contents = f.read() +# if force_parser: +# return TypeAdapter(desired_return_type).validate_python(force_parser(contents)) +# if desired_return_type is str: +# return contents # type: ignore +# if desired_return_type is dict: +# return json.loads(contents) +# if issubclass(desired_return_type, ConfigModel): +# return desired_return_type.model_validate(json.loads(contents)) +# raise ValueError("Invalid return type requested") + + +# @pytest.fixture(autouse=True) +# def mock_config_client(): +# # Don't actually talk to central service during unit tests, and reset caches between test + +# with patch( +# "daq_config_server.app.client.ConfigClient.get_file_contents", +# side_effect=fake_config_server_get_file_contents, +# ): +# yield diff --git a/system_tests/conftest.py b/system_tests/conftest.py index 06fee26e905..d3df62cbd5d 100644 --- a/system_tests/conftest.py +++ b/system_tests/conftest.py @@ -1,5 +1,2 @@ # Add run_engine to be used in tests -pytest_plugins = [ - "dodal.testing.fixtures.run_engine", - "dodal.testing.fixtures.config_server", -] +pytest_plugins = ["dodal.testing.fixtures.run_engine"] diff --git a/system_tests/test_oav_system.py b/system_tests/test_oav_system.py index 30f04a2fc0a..9438c72d665 100644 --- a/system_tests/test_oav_system.py +++ b/system_tests/test_oav_system.py @@ -15,8 +15,8 @@ @pytest.fixture -async def oav() -> OAV: - oav_config = OAVConfig(TEST_OAV_ZOOM_LEVELS, ConfigClient("")) +async def oav(mock_config_client: ConfigClient) -> OAV: + oav_config = OAVConfig(TEST_OAV_ZOOM_LEVELS, mock_config_client) async with init_devices(connect=True): oav = OAV("", config=oav_config, name="oav") return oav @@ -35,9 +35,9 @@ def take_snapshot_with_grid(oav: OAV, snapshot_filename, snapshot_directory): # We need to find a better way of integrating this, see https://github.com/DiamondLightSource/mx-bluesky/issues/183 @pytest.mark.skip(reason="Don't want to actually take snapshots during testing.") -def test_grid_overlay(run_engine: RunEngine): +def test_grid_overlay(run_engine: RunEngine, mock_config_client: ConfigClient): beamline = "BL03I" - oav_params = OAVConfig(TEST_OAV_ZOOM_LEVELS, ConfigClient("")) + oav_params = OAVConfig(TEST_OAV_ZOOM_LEVELS, mock_config_client) oav = OAV(name="oav", prefix=f"{beamline}", config=oav_params) snapshot_filename = "snapshot" snapshot_directory = "." diff --git a/tests/beamlines/test_i24.py b/tests/beamlines/test_i24.py deleted file mode 100644 index e32bb47c072..00000000000 --- a/tests/beamlines/test_i24.py +++ /dev/null @@ -1,13 +0,0 @@ -import pytest - -from dodal.devices.beamlines.i24.vgonio import VerticalGoniometer - - -@pytest.mark.parametrize("module_and_devices_for_beamline", ["i24"], indirect=True) -def test_device_creation(module_and_devices_for_beamline): - _, devices, exceptions = module_and_devices_for_beamline - assert not exceptions - vgonio: VerticalGoniometer = devices["vgonio"] # type: ignore - - assert vgonio._name == "vgonio" - assert vgonio.omega._name == "vgonio-omega" diff --git a/tests/common/beamlines/test_beamline_parameters.py b/tests/common/beamlines/test_beamline_parameters.py index 42e7f88d69d..5a0faeb28dd 100644 --- a/tests/common/beamlines/test_beamline_parameters.py +++ b/tests/common/beamlines/test_beamline_parameters.py @@ -27,8 +27,8 @@ def patch_beamline_parameter_paths(): @pytest.fixture(autouse=True) -def always_set_config_client(): - set_config_client(ConfigClient("test")) +def always_set_config_client(mock_config_client: ConfigClient): + set_config_client(mock_config_client) def test_beamline_parameters(): diff --git a/tests/common/beamlines/test_device_instantiation.py b/tests/common/beamlines/test_device_instantiation.py index e9ce27fd437..97d17591c94 100644 --- a/tests/common/beamlines/test_device_instantiation.py +++ b/tests/common/beamlines/test_device_instantiation.py @@ -28,10 +28,13 @@ def patch_config_paths(monkeypatch): @pytest.fixture(autouse=True) -def reset_config_client(): - set_config_client(ConfigClient("")) +def reset_config_client(mock_config_client: ConfigClient): + original = ConfigClient.get_file_contents + ConfigClient.get_file_contents = mock_config_client.get_file_contents # type: ignore + set_config_client(mock_config_client) yield clear_config_client() + ConfigClient.get_file_contents = original @pytest.mark.parametrize( diff --git a/tests/conftest.py b/tests/conftest.py index 7f4c6a77ce3..6c032e1d636 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,14 +1,20 @@ import importlib +import json import logging import os import sys +from collections.abc import Callable from os import environ from pathlib import Path from types import ModuleType +from typing import Any, TypeVar from unittest.mock import MagicMock, patch import pytest +from daq_config_server import ConfigClient +from daq_config_server.models import ConfigModel from ophyd_async.core import PathProvider +from pydantic import TypeAdapter from dodal.common.beamlines import beamline_parameters, beamline_utils from dodal.common.beamlines.beamline_utils import ( @@ -91,6 +97,36 @@ def patched_open(*args, **kwargs): yield [] +def _is_banned(path: Path) -> bool: + try: + resolved = path.resolve() + except Exception: + resolved = path + + return resolved.is_absolute() and any( + resolved.is_relative_to(banned) for banned in BANNED_PATHS + ) + + +@pytest.fixture(autouse=True) +def block_dls_access_in_config_client(): + + original = ConfigClient.get_file_contents + + def guarded(self, file_path, *args, **kwargs): + # --- normalize path safely --- + path = Path(file_path) + + if _is_banned(path): + raise AssertionError(f"Forbidden config access blocked in test: {path}") + + # --- delegate to whatever is currently installed --- + return original(self, file_path, *args, **kwargs) + + with patch.object(ConfigClient, "get_file_contents", new=guarded): + yield + + def pytest_runtest_setup(item): beamline_utils.clear_devices() if LOGGER.handlers == []: @@ -177,6 +213,64 @@ def eiger_params(tmp_path: Path) -> DetectorParams: ) +T = TypeVar("T", str, dict, ConfigModel) + + +def fake_config_server_get_file_contents( + file_path: str | Path, + desired_return_type: type[T] = str, + reset_cached_result: bool = True, + force_parser: Callable[[str], Any] | None = None, +) -> T: + """Fakes getting a file from the config server by reading it directly. + + Args: + file_path (str | Path): Filepath of the file to read + desired_return_type (type[T], optional): Type to convert the file to. Defaults to str. + reset_cached_result (bool, optional): Whether or not to use the config server's cached result. + Does nothing here as we don't cache. Defaults to True. + force_parser (Callable[[str], Any] | None, optional): Use a certain converter function. + Only needed for the interim where the converter exists but the config server has not + been redeployed. Defaults to None. + + Raises: + ValueError: Raised if an invalid type is requested + + Returns: + T: The contents of the config file. + """ + filepath = Path(file_path) + if filepath.is_absolute(): + for p in BANNED_PATHS: + assert not filepath.is_relative_to(p), ( + f"Attempt to open {filepath} from inside a unit test" + ) + # Minimal logic required for unit tests + with filepath.open("r") as f: + contents = f.read() + if force_parser: + return TypeAdapter(desired_return_type).validate_python(force_parser(contents)) + if desired_return_type is str: + return contents # type: ignore + if desired_return_type is dict: + return json.loads(contents) + if issubclass(desired_return_type, ConfigModel): + return desired_return_type.model_validate(json.loads(contents)) + raise ValueError("Invalid return type requested") + + +@pytest.fixture +def mock_config_client() -> ConfigClient: + # Don't actually talk to central service during unit tests, and reset caches between test + mock_config_client = ConfigClient() + mock_config_client.get_file_contents = MagicMock(spec=["get_file_contents"]) + + mock_config_client.get_file_contents.side_effect = ( + fake_config_server_get_file_contents + ) + return mock_config_client + + @pytest.fixture(autouse=True) def reset_config_client(): yield diff --git a/tests/devices/beamlines/i03/test_undulator_dcm.py b/tests/devices/beamlines/i03/test_undulator_dcm.py index f94093dc46e..28c47e01fa8 100644 --- a/tests/devices/beamlines/i03/test_undulator_dcm.py +++ b/tests/devices/beamlines/i03/test_undulator_dcm.py @@ -48,17 +48,17 @@ def flush_event_loop_on_finish(): @pytest.fixture(autouse=True) -def always_set_config_client(): - set_config_client(ConfigClient("test")) +def always_set_config_client(mock_config_client: ConfigClient): + set_config_client(mock_config_client) @pytest.fixture -async def fake_undulator_dcm() -> UndulatorDCM: +async def fake_undulator_dcm(mock_config_client: ConfigClient) -> UndulatorDCM: async with init_devices(mock=True): baton = Baton("BATON-01:") undulator = UndulatorInKeV( "UND-01", - ConfigClient(""), + mock_config_client, name="undulator", poles=80, id_gap_lookup_table_path=TEST_BEAMLINE_UNDULATOR_TO_GAP_LUT, @@ -70,7 +70,7 @@ async def fake_undulator_dcm() -> UndulatorDCM: undulator, dcm, daq_configuration_path=MOCK_DAQ_CONFIG_PATH, - config_client=ConfigClient(""), + config_client=mock_config_client, name="undulator_dcm", ) return undulator_dcm diff --git a/tests/devices/beamlines/i09_2_shared/test_i09_apple2.py b/tests/devices/beamlines/i09_2_shared/test_i09_apple2.py index 1fc0592867d..215abeb5818 100644 --- a/tests/devices/beamlines/i09_2_shared/test_i09_apple2.py +++ b/tests/devices/beamlines/i09_2_shared/test_i09_apple2.py @@ -42,7 +42,7 @@ assert_expected_lut_file_equals_config_server_energy_motor_update_lookup_table, ) -# add mock_config_client, mock_id_gap, mock_phase and mock_jaw_phase_axes to pytest. +# mock_id_gap, mock_phase and mock_jaw_phase_axes to pytest. pytest_plugins = ["dodal.testing.fixtures.devices.apple2"] diff --git a/tests/devices/beamlines/i10/test_i10_apple2.py b/tests/devices/beamlines/i10/test_i10_apple2.py index 35a2465cb87..8d11a44cb92 100644 --- a/tests/devices/beamlines/i10/test_i10_apple2.py +++ b/tests/devices/beamlines/i10/test_i10_apple2.py @@ -57,7 +57,7 @@ assert_expected_lut_file_equals_config_server_energy_motor_update_lookup_table, ) -# add mock_config_client, mock_id_gap, mock_phase and mock_jaw_phase_axes to pytest. +# mock_id_gap, mock_phase and mock_jaw_phase_axes to pytest. pytest_plugins = ["dodal.testing.fixtures.devices.apple2"] diff --git a/tests/devices/beamlines/i15_1/test_blower.py b/tests/devices/beamlines/i15_1/test_blower.py index a67c1b3f869..18aaaa6326b 100644 --- a/tests/devices/beamlines/i15_1/test_blower.py +++ b/tests/devices/beamlines/i15_1/test_blower.py @@ -7,8 +7,10 @@ from tests.test_data import TEST_XPDF_LOCAL_PARAMETERS -def test_blower_config_client_reads_config_file_successfully(): - blower = Blower("", ConfigClient(""), TEST_XPDF_LOCAL_PARAMETERS) +def test_blower_config_client_reads_config_file_successfully( + mock_config_client: ConfigClient, +): + blower = Blower("", mock_config_client, TEST_XPDF_LOCAL_PARAMETERS) assert blower.get_config() == TemperatureControllerParams( beam_position=44.7, safe_position=2.0, diff --git a/tests/devices/beamlines/i15_1/test_cobra.py b/tests/devices/beamlines/i15_1/test_cobra.py index 3a647d93ceb..fcf7615ccc0 100644 --- a/tests/devices/beamlines/i15_1/test_cobra.py +++ b/tests/devices/beamlines/i15_1/test_cobra.py @@ -7,8 +7,10 @@ from tests.test_data import TEST_XPDF_LOCAL_PARAMETERS -def test_cobra_config_client_reads_config_file_successfully(): - cobra = Cobra("", ConfigClient(""), TEST_XPDF_LOCAL_PARAMETERS) +def test_cobra_config_client_reads_config_file_successfully( + mock_config_client: ConfigClient, +): + cobra = Cobra("", mock_config_client, TEST_XPDF_LOCAL_PARAMETERS) assert cobra.get_config() == TemperatureControllerParams( beam_position=461.5, safe_position=2.0, diff --git a/tests/devices/beamlines/i15_1/test_cryostream.py b/tests/devices/beamlines/i15_1/test_cryostream.py index 322631f6562..92be15683ef 100644 --- a/tests/devices/beamlines/i15_1/test_cryostream.py +++ b/tests/devices/beamlines/i15_1/test_cryostream.py @@ -7,8 +7,10 @@ from tests.test_data import TEST_XPDF_LOCAL_PARAMETERS -def test_cryostream_config_client_reads_config_file_successfully(): - cryostream = Cryostream("", ConfigClient(""), TEST_XPDF_LOCAL_PARAMETERS) +def test_cryostream_config_client_reads_config_file_successfully( + mock_config_client: ConfigClient, +): + cryostream = Cryostream("", mock_config_client, TEST_XPDF_LOCAL_PARAMETERS) assert cryostream.get_config() == TemperatureControllerParams( beam_position=469.9, safe_position=0.0, diff --git a/tests/devices/beamlines/i15_1/test_laue_monochrometer.py b/tests/devices/beamlines/i15_1/test_laue_monochrometer.py index d9ca2b91c10..91f04ec5a31 100644 --- a/tests/devices/beamlines/i15_1/test_laue_monochrometer.py +++ b/tests/devices/beamlines/i15_1/test_laue_monochrometer.py @@ -9,11 +9,11 @@ @pytest.fixture -def laue_monochrometer(): +def laue_monochrometer(mock_config_client: ConfigClient): with init_devices(mock=True): monocrometer = LaueMonochrometer( prefix="", - config_client=ConfigClient(""), + config_client=mock_config_client, crystal_lut_path=TEST_I15_1_CRYSTAL_LUT, ) return monocrometer diff --git a/tests/devices/beamlines/i17/test_i17_apple2.py b/tests/devices/beamlines/i17/test_i17_apple2.py index 40473b54d5d..b4814e152f8 100644 --- a/tests/devices/beamlines/i17/test_i17_apple2.py +++ b/tests/devices/beamlines/i17/test_i17_apple2.py @@ -14,7 +14,7 @@ ) from dodal.devices.insertion_device.energy_motor_lookup import EnergyMotorLookup -# add mock_config_client, mock_id_gap, mock_phase and mock_jaw_phase_axes to pytest. +# mock_id_gap, mock_phase and mock_jaw_phase_axes to pytest. pytest_plugins = ["dodal.testing.fixtures.devices.apple2"] diff --git a/tests/devices/beamlines/i24/test_vgonio.py b/tests/devices/beamlines/i24/test_vgonio.py index 891181444bb..d5be6ee5c0e 100644 --- a/tests/devices/beamlines/i24/test_vgonio.py +++ b/tests/devices/beamlines/i24/test_vgonio.py @@ -1,14 +1,15 @@ import bluesky.plan_stubs as bps import pytest from bluesky import RunEngine +from ophyd_async.core import init_devices from dodal.devices.beamlines.i24.vgonio import VerticalGoniometer @pytest.fixture -async def vgonio(): - vgonio = VerticalGoniometer("", name="fake_vgonio") - await vgonio.connect(mock=True) +def vgonio() -> VerticalGoniometer: + with init_devices(mock=True): + vgonio = VerticalGoniometer("") return vgonio @@ -22,3 +23,8 @@ async def test_vgonio_motor_move(vgonio: VerticalGoniometer, run_engine: RunEngi assert await vgonio.x.user_readback.get_value() == 1.0 assert await vgonio.yh.user_readback.get_value() == 1.5 + + +def test_device_creation_names(vgonio: VerticalGoniometer): + assert vgonio._name == "vgonio" + assert vgonio.omega._name == "vgonio-omega" diff --git a/tests/devices/detector/test_det_resolution.py b/tests/devices/detector/test_det_resolution.py index b8b75dcea3e..0ce1e5e983b 100644 --- a/tests/devices/detector/test_det_resolution.py +++ b/tests/devices/detector/test_det_resolution.py @@ -17,8 +17,8 @@ @pytest.fixture(autouse=True) -def always_set_config_client(): - set_config_client(ConfigClient("test")) +def always_set_config_client(mock_config_client: ConfigClient): + set_config_client(mock_config_client) @pytest.fixture(scope="function") diff --git a/tests/devices/detector/test_detector.py b/tests/devices/detector/test_detector.py index 80b071aa8a1..653dc7550c1 100644 --- a/tests/devices/detector/test_detector.py +++ b/tests/devices/detector/test_detector.py @@ -4,6 +4,7 @@ import pytest from pydantic import ValidationError +from dodal.common.beamlines.beamline_utils import set_config_client from dodal.devices.detector import DetectorParams from dodal.devices.detector.det_dim_constants import EIGER2_X_16M_SIZE from tests.devices.test_data import TEST_LUT_TXT @@ -45,15 +46,15 @@ def test_if_path_provided_check_is_dir(tmp_path: Path): create_det_params_with_dir_and_prefix(file_path) -@patch( - "dodal.devices.detector.detector.get_config_client", -) +@pytest.fixture(autouse=True) +def always_set_config_client(): + set_config_client(MagicMock()) + + @patch( "dodal.devices.detector.det_dist_to_beam_converter.linear_extrapolation_lut", ) -def test_correct_det_dist_to_beam_converter_path_passed_in( - mock_lut, mock_get_config_client, tmp_path -): +def test_correct_det_dist_to_beam_converter_path_passed_in(mock_lut, tmp_path): params = DetectorParams( expected_energy_ev=100, exposure_time_s=1.0, diff --git a/tests/devices/insertion_device/test_apple_knot_path_finder.py b/tests/devices/insertion_device/test_apple_knot_path_finder.py index 07ebb35c70d..bd33f79b509 100644 --- a/tests/devices/insertion_device/test_apple_knot_path_finder.py +++ b/tests/devices/insertion_device/test_apple_knot_path_finder.py @@ -8,7 +8,7 @@ Apple2Val, ) -# add mock_config_client, mock_id_gap, mock_phase and mock_jaw_phase_axes to pytest. +# mock_id_gap, mock_phase and mock_jaw_phase_axes to pytest. pytest_plugins = [ "dodal.testing.fixtures.devices.apple2", "dodal.testing.fixtures.devices.apple_knot", diff --git a/tests/devices/mx_phase1/test_beamstop.py b/tests/devices/mx_phase1/test_beamstop.py index 599a763678c..83f276c4ef0 100644 --- a/tests/devices/mx_phase1/test_beamstop.py +++ b/tests/devices/mx_phase1/test_beamstop.py @@ -7,16 +7,16 @@ from bluesky import plan_stubs as bps from bluesky.preprocessors import run_decorator from bluesky.run_engine import RunEngine +from daq_config_server import ConfigClient from ophyd_async.core import get_mock, get_mock_put, set_mock_value from dodal.devices.beamlines.i03 import Beamstop, BeamstopPositions -from dodal.testing.fixtures.config_server import fake_config_server_get_file_contents from tests.common.beamlines.test_beamline_parameters import TEST_BEAMLINE_PARAMETERS_TXT @pytest.fixture -def beamline_parameters() -> dict[str, Any]: - return fake_config_server_get_file_contents(TEST_BEAMLINE_PARAMETERS_TXT, dict) +def beamline_parameters(mock_config_client: ConfigClient) -> dict[str, Any]: + return mock_config_client.get_file_contents(TEST_BEAMLINE_PARAMETERS_TXT, dict) @pytest.mark.parametrize( diff --git a/tests/devices/oav/conftest.py b/tests/devices/oav/conftest.py index 8da3a5512f5..78b1dc7617b 100644 --- a/tests/devices/oav/conftest.py +++ b/tests/devices/oav/conftest.py @@ -10,9 +10,9 @@ @pytest.fixture -async def oav() -> OAVBeamCentreFile: +async def oav(mock_config_client: ConfigClient) -> OAVBeamCentreFile: oav_config = OAVConfigBeamCentre( - TEST_OAV_ZOOM_LEVELS, TEST_DISPLAY_CONFIG, ConfigClient("") + TEST_OAV_ZOOM_LEVELS, TEST_DISPLAY_CONFIG, mock_config_client ) async with init_devices(mock=True, connect=True): oav = OAVBeamCentreFile("", config=oav_config, name="oav") @@ -27,8 +27,8 @@ async def oav() -> OAVBeamCentreFile: @pytest.fixture -async def oav_beam_centre_pv_roi() -> OAVBeamCentrePV: - oav_config = OAVConfig(TEST_OAV_ZOOM_LEVELS, ConfigClient("")) +async def oav_beam_centre_pv_roi(mock_config_client: ConfigClient) -> OAVBeamCentrePV: + oav_config = OAVConfig(TEST_OAV_ZOOM_LEVELS, mock_config_client) async with init_devices(mock=True, connect=True): oav = OAVBeamCentrePV("", config=oav_config, name="oav") zoom_levels_list = ["1.0x", "3.0x", "5.0x", "7.5x", "10.0x"] @@ -42,8 +42,8 @@ async def oav_beam_centre_pv_roi() -> OAVBeamCentrePV: @pytest.fixture -async def oav_beam_centre_pv_fs() -> OAVBeamCentrePV: - oav_config = OAVConfig(TEST_OAV_ZOOM_LEVELS, ConfigClient("")) +async def oav_beam_centre_pv_fs(mock_config_client: ConfigClient) -> OAVBeamCentrePV: + oav_config = OAVConfig(TEST_OAV_ZOOM_LEVELS, mock_config_client) async with init_devices(mock=True, connect=True): oav = OAVBeamCentrePV( "", config=oav_config, name="oav", mjpeg_prefix="XTAL", overlay_channel=3 diff --git a/tests/devices/oav/test_oav.py b/tests/devices/oav/test_oav.py index 5c4987e2c63..cb0c525fe1d 100644 --- a/tests/devices/oav/test_oav.py +++ b/tests/devices/oav/test_oav.py @@ -164,9 +164,11 @@ async def test_beam_centre_signals_have_same_names( assert "oav-beam_centre_j" in reading.keys() -async def test_oav_with_null_zoom_controller(null_controller: NullZoomController): +async def test_oav_with_null_zoom_controller( + null_controller: NullZoomController, mock_config_client: ConfigClient +): oav_config = OAVConfigBeamCentre( - TEST_OAV_ZOOM_LEVELS, TEST_DISPLAY_CONFIG, ConfigClient("") + TEST_OAV_ZOOM_LEVELS, TEST_DISPLAY_CONFIG, mock_config_client ) oav = OAVBeamCentreFile("", oav_config, "", zoom_controller=null_controller) @@ -192,9 +194,11 @@ async def test_oav_with_null_zoom_controller_set_zoom_level_other_than_1( "mjpeg_prefix", ["MJPG", "XTAL"], ) -async def test_setting_mjpeg_prefix_changes_stream_url(mjpeg_prefix): +async def test_setting_mjpeg_prefix_changes_stream_url( + mjpeg_prefix, mock_config_client: ConfigClient +): oav_config = OAVConfigBeamCentre( - TEST_OAV_ZOOM_LEVELS, TEST_DISPLAY_CONFIG, ConfigClient("") + TEST_OAV_ZOOM_LEVELS, TEST_DISPLAY_CONFIG, mock_config_client ) async with init_devices(mock=True, connect=True): oav = OAVBeamCentreFile( diff --git a/tests/devices/oav/test_oav_parameters.py b/tests/devices/oav/test_oav_parameters.py index a56419c7b12..ded3c7c30ea 100644 --- a/tests/devices/oav/test_oav_parameters.py +++ b/tests/devices/oav/test_oav_parameters.py @@ -18,19 +18,21 @@ @pytest.fixture -def mock_parameters(): - return OAVParameters(ConfigClient(""), "loopCentring", TEST_OAV_CENTRING_JSON) +def mock_parameters(mock_config_client: ConfigClient): + return OAVParameters(mock_config_client, "loopCentring", TEST_OAV_CENTRING_JSON) @pytest.fixture -def mock_config() -> dict[str, ZoomParams]: - return OAVConfig(TEST_OAV_ZOOM_LEVELS, ConfigClient("")).get_parameters() +def mock_config(mock_config_client: ConfigClient) -> dict[str, ZoomParams]: + return OAVConfig(TEST_OAV_ZOOM_LEVELS, mock_config_client).get_parameters() @pytest.fixture -def mock_config_with_beam_centre() -> dict[str, ZoomParamsCrosshair]: +def mock_config_with_beam_centre( + mock_config_client: ConfigClient, +) -> dict[str, ZoomParamsCrosshair]: config = OAVConfigBeamCentre( - TEST_OAV_ZOOM_LEVELS, TEST_DISPLAY_CONFIG, ConfigClient("") + TEST_OAV_ZOOM_LEVELS, TEST_DISPLAY_CONFIG, mock_config_client ).get_parameters() config = cast(dict[str, ZoomParamsCrosshair], config) return config diff --git a/tests/devices/test_beam_converter.py b/tests/devices/test_beam_converter.py index 4bdbe687ce9..33c92862fab 100644 --- a/tests/devices/test_beam_converter.py +++ b/tests/devices/test_beam_converter.py @@ -17,12 +17,12 @@ @pytest.fixture -def fake_converter(tmp_path): +def fake_converter(tmp_path, mock_config_client: ConfigClient): lookup_table_path = tmp_path / "test.txt" with open(lookup_table_path, "w") as f: f.write(LOOKUP_TABLE_TEST_VALUES.model_dump_json()) - yield DetectorDistanceToBeamXYConverter(lookup_table_path, ConfigClient("")) + yield DetectorDistanceToBeamXYConverter(lookup_table_path, mock_config_client) @pytest.mark.parametrize( diff --git a/tests/devices/test_eiger.py b/tests/devices/test_eiger.py index e42dd11f9d3..8b9aff8cb99 100644 --- a/tests/devices/test_eiger.py +++ b/tests/devices/test_eiger.py @@ -4,7 +4,6 @@ from unittest.mock import ANY, MagicMock, Mock, call, create_autospec, patch import pytest -from daq_config_server import ConfigClient from ophyd.sim import NullStatus, make_fake_device from ophyd.status import Status from ophyd.utils import UnknownStatusFailure @@ -32,8 +31,8 @@ class StatusError(Exception): @pytest.fixture(autouse=True) -def always_set_config_client(): - set_config_client(ConfigClient("test")) +def always_set_config_client(mock_config_client): + set_config_client(mock_config_client) @pytest.fixture diff --git a/tests/devices/test_focusing_mirror.py b/tests/devices/test_focusing_mirror.py index feb79c6f21c..91d75f4316f 100644 --- a/tests/devices/test_focusing_mirror.py +++ b/tests/devices/test_focusing_mirror.py @@ -236,13 +236,13 @@ def test_mirror_set_voltage_returns_immediately_if_voltage_already_demanded( get_mock_put(mirror_voltage_with_set._setpoint_v).assert_not_called() -def test_mirror_populates_voltage_channels(): +def test_mirror_populates_voltage_channels(mock_config_client: ConfigClient): with init_devices(mock=True): mirror_voltages = MirrorVoltages( "", "", daq_configuration_path=MOCK_DAQ_CONFIG_PATH, - config_client=ConfigClient(""), + config_client=mock_config_client, ) assert len(mirror_voltages.horizontal_voltages) == 14 assert len(mirror_voltages.vertical_voltages) == 8 diff --git a/tests/devices/test_undulator.py b/tests/devices/test_undulator.py index c04e1fd4317..b925d5b0d19 100644 --- a/tests/devices/test_undulator.py +++ b/tests/devices/test_undulator.py @@ -33,12 +33,12 @@ @pytest.fixture -async def undulator() -> UndulatorInKeV: +async def undulator(mock_config_client: ConfigClient) -> UndulatorInKeV: async with init_devices(mock=True): baton = Baton("BATON-01") undulator = UndulatorInKeV( "UND-01", - ConfigClient(""), + mock_config_client, name="undulator", poles=80, length=2.0, @@ -113,11 +113,11 @@ async def test_configuration_includes_configuration_fields(undulator: UndulatorI ) -async def test_poles_not_propagated_if_not_supplied(): +async def test_poles_not_propagated_if_not_supplied(mock_config_client: ConfigClient): async with init_devices(mock=True): undulator = UndulatorInKeV( "UND-01", - ConfigClient(""), + mock_config_client, name="undulator", length=2.0, id_gap_lookup_table_path=TEST_BEAMLINE_UNDULATOR_TO_GAP_LUT, @@ -126,11 +126,11 @@ async def test_poles_not_propagated_if_not_supplied(): assert "undulator-poles" not in (await undulator.read_configuration()) -async def test_length_not_propagated_if_not_supplied(): +async def test_length_not_propagated_if_not_supplied(mock_config_client: ConfigClient): async with init_devices(mock=True): undulator = UndulatorInKeV( "UND-01", - ConfigClient(""), + mock_config_client, name="undulator", poles=80, id_gap_lookup_table_path=TEST_BEAMLINE_UNDULATOR_TO_GAP_LUT, @@ -151,7 +151,7 @@ def test_correct_closest_distance_to_energy_from_table(energy, expected_output): async def test_when_gap_access_is_disabled_set_then_error_is_raised( - undulator, + undulator: UndulatorInKeV, ): set_mock_value(undulator.gap_access, EnabledDisabledUpper.DISABLED) with pytest.raises(AccessError): diff --git a/tests/devices/util/test_lookup_tables.py b/tests/devices/util/test_lookup_tables.py index 55313c0b575..5e00e8125ad 100644 --- a/tests/devices/util/test_lookup_tables.py +++ b/tests/devices/util/test_lookup_tables.py @@ -21,10 +21,11 @@ ) -async def test_energy_to_distance_table_correct_format(): - config_server = ConfigClient("") +async def test_energy_to_distance_table_correct_format( + mock_config_client: ConfigClient, +): table = np.array( - config_server.get_file_contents( + mock_config_client.get_file_contents( TEST_BEAMLINE_UNDULATOR_TO_GAP_LUT, UndulatorEnergyGapLookupTable ).rows ) diff --git a/tests/plan_stubs/test_topup_plan.py b/tests/plan_stubs/test_topup_plan.py index 7f845b84cea..007dbab2701 100644 --- a/tests/plan_stubs/test_topup_plan.py +++ b/tests/plan_stubs/test_topup_plan.py @@ -20,8 +20,8 @@ @pytest.fixture(autouse=True) -def always_set_config_client(): - set_config_client(ConfigClient("test")) +def always_set_config_client(mock_config_client: ConfigClient): + set_config_client(mock_config_client) @pytest.fixture diff --git a/tests/plans/conftest.py b/tests/plans/conftest.py index ac002e729e5..bfb6d2f1f47 100644 --- a/tests/plans/conftest.py +++ b/tests/plans/conftest.py @@ -26,11 +26,13 @@ def __init__(self, undulator: UndulatorInKeV, dcm: DoubleCrystalMonochromatorBas @pytest.fixture -async def mock_undulator_and_dcm() -> UndulatorGapCheckDevices: +async def mock_undulator_and_dcm( + mock_config_client: ConfigClient, +) -> UndulatorGapCheckDevices: async with init_devices(mock=True): undulator = UndulatorInKeV( "", - ConfigClient(""), + mock_config_client, id_gap_lookup_table_path=TEST_BEAMLINE_UNDULATOR_TO_GAP_LUT, ) dcm = DCM("") diff --git a/tests/plans/device_setup_plans/test_setup_pin_tip.py b/tests/plans/device_setup_plans/test_setup_pin_tip.py index e94192d10a6..718febe3e68 100644 --- a/tests/plans/device_setup_plans/test_setup_pin_tip.py +++ b/tests/plans/device_setup_plans/test_setup_pin_tip.py @@ -20,8 +20,8 @@ def pin_tip_detection() -> PinTipDetection: @pytest.fixture -def params() -> OAVParameters: - return OAVParameters(ConfigClient(""), "pinTipCentring", TEST_OAV_CENTRING_JSON) +def params(mock_config_client: ConfigClient) -> OAVParameters: + return OAVParameters(mock_config_client, "pinTipCentring", TEST_OAV_CENTRING_JSON) @pytest.mark.parametrize( diff --git a/tests/plans/test_configure_arm_trigger_and_disarm_detector.py b/tests/plans/test_configure_arm_trigger_and_disarm_detector.py index 17c8fdd6f0a..6a859053e28 100644 --- a/tests/plans/test_configure_arm_trigger_and_disarm_detector.py +++ b/tests/plans/test_configure_arm_trigger_and_disarm_detector.py @@ -28,9 +28,12 @@ async def fake_eiger() -> FastEiger: async def test_configure_arm_trigger_and_disarm_detector( - fake_eiger: FastEiger, eiger_params: DetectorParams, run_engine: RunEngine + fake_eiger: FastEiger, + eiger_params: DetectorParams, + run_engine: RunEngine, + mock_config_client: ConfigClient, ): - set_config_client(ConfigClient("test")) + set_config_client(mock_config_client) trigger_info = TriggerInfo( # Manual trigger, so setting number of triggers to 1. number_of_events=1, From 5a22ffdbdf3f49b23ca7ea706297c9629e23b9ee Mon Sep 17 00:00:00 2001 From: Oli Wenman Date: Tue, 23 Jun 2026 13:01:36 +0000 Subject: [PATCH 14/35] Fix all tests --- pyproject.toml | 11 +- src/dodal/beamlines/i09_1_shared.py | 13 +- src/dodal/beamlines/i18.py | 14 +- src/dodal/beamlines/p38.py | 12 +- .../fixtures/devices/hard_undulator.py | 43 --- .../beamlines/test_device_instantiation.py | 25 -- tests/conftest.py | 247 +++++++++--------- .../beamlines/i09_1_shared/conftest.py | 30 +++ .../i09_1_shared/test_hard_energy.py | 10 +- .../i09_1_shared/test_undulator_functions.py | 8 +- 10 files changed, 191 insertions(+), 222 deletions(-) delete mode 100644 src/dodal/testing/fixtures/devices/hard_undulator.py create mode 100644 tests/devices/beamlines/i09_1_shared/conftest.py diff --git a/pyproject.toml b/pyproject.toml index 36212985b73..419049c09db 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -24,7 +24,7 @@ dependencies = [ "requests", "graypy", "pydantic>=2.0", - "opencv-python-headless", # For pin-tip detection. + "opencv-python-headless", # For pin-tip detection. "numpy", "aiofiles", "aiohttp", @@ -32,8 +32,8 @@ dependencies = [ "scanspec>=0.7.3", "pyzmq==27.1.0", "deepdiff", - "daq-config-server >= 1.3.6", # For getting Configuration settings. - "mysql-connector-python == 9.5.0", # Can unpin once https://github.com/DiamondLightSource/ispyb-api/pull/244 is merged and released + "daq-config-server >= 1.3.6", # For getting Configuration settings. + "mysql-connector-python == 9.5.0", # Can unpin once https://github.com/DiamondLightSource/ispyb-api/pull/244 is merged and released ] dynamic = ["version"] @@ -113,9 +113,10 @@ typeCheckingMode = "standard" reportMissingImports = false # Ignore missing stubs in imported modules [tool.pytest.ini_options] -# Run pytest with all our checkers, and don't spam us with massive tracebacks on error +# Run pytest with all our checkers asyncio_mode = "auto" -timeout = 1 +timeout = 1.5 +timeout_method = "signal" # Alllow us to see full stack trace of where it timed out to make debugging easier. markers = [ "requires: marks tests as requiring other infrastructure", "skip_in_pycharm: marks test as not working in pycharm testrunner", diff --git a/src/dodal/beamlines/i09_1_shared.py b/src/dodal/beamlines/i09_1_shared.py index b41a6dd08ba..425b876a332 100644 --- a/src/dodal/beamlines/i09_1_shared.py +++ b/src/dodal/beamlines/i09_1_shared.py @@ -1,6 +1,5 @@ from daq_config_server import ConfigClient -from dodal.common.beamlines.beamline_utils import get_config_client, set_config_client from dodal.device_manager import DeviceManager from dodal.devices.beamlines.i09_1_shared import ( HardEnergy, @@ -30,10 +29,14 @@ devices = DeviceManager() -set_config_client(ConfigClient()) LOOK_UPTABLE_FILE = "/dls_sw/i09-1/software/gda/workspace_git/gda-diamond.git/configurations/i09-1-shared/lookupTables/IIDCalibrationTable.txt" +@devices.fixture +def config_client() -> ConfigClient: + return ConfigClient() + + @devices.factory() def psi1() -> HutchShutter: return HutchShutter(I_PREFIX.beamline_prefix) @@ -60,12 +63,14 @@ def ienergy_order() -> UndulatorOrder: @devices.factory() def iidenergy( - ienergy_order: UndulatorOrder, iid: UndulatorInMm + ienergy_order: UndulatorOrder, + iid: UndulatorInMm, + config_client: ConfigClient, ) -> HardInsertionDeviceEnergy: return HardInsertionDeviceEnergy( undulator_order=ienergy_order, undulator=iid, - config_server=get_config_client(), + config_server=config_client, filepath=LOOK_UPTABLE_FILE, gap_to_energy_func=calculate_energy_i09_hu, energy_to_gap_func=calculate_gap_i09_hu, diff --git a/src/dodal/beamlines/i18.py b/src/dodal/beamlines/i18.py index d662e04a007..b18114c2d89 100644 --- a/src/dodal/beamlines/i18.py +++ b/src/dodal/beamlines/i18.py @@ -5,10 +5,6 @@ from ophyd_async.core import PathProvider from ophyd_async.fastcs.panda import HDFPanda -from dodal.common.beamlines.beamline_utils import ( - get_config_client, - set_config_client, -) from dodal.common.beamlines.beamline_utils import set_beamline as set_utils_beamline from dodal.common.visit import ( LocalDirectoryServiceClient, @@ -53,7 +49,9 @@ def path_provider() -> PathProvider: ) -set_config_client(ConfigClient()) +@devices.fixture +def config_client() -> ConfigClient: + return ConfigClient() @devices.factory() @@ -62,10 +60,8 @@ def synchrotron() -> Synchrotron: @devices.factory() -def undulator() -> UndulatorInKeV: - return UndulatorInKeV( - f"{PREFIX.insertion_prefix}-MO-SERVC-01:", get_config_client() - ) +def undulator(config_client: ConfigClient) -> UndulatorInKeV: + return UndulatorInKeV(f"{PREFIX.insertion_prefix}-MO-SERVC-01:", config_client) # See https://github.com/DiamondLightSource/dodal/issues/1180 diff --git a/src/dodal/beamlines/p38.py b/src/dodal/beamlines/p38.py index 7e7800fc312..e2c0ccde6bb 100644 --- a/src/dodal/beamlines/p38.py +++ b/src/dodal/beamlines/p38.py @@ -7,10 +7,6 @@ from ophyd_async.epics.adcore import ADWriterFactory from ophyd_async.fastcs.panda import HDFPanda -from dodal.common.beamlines.beamline_utils import ( - get_config_client, - set_config_client, -) from dodal.common.beamlines.beamline_utils import set_beamline as set_utils_beamline from dodal.common.beamlines.device_helpers import HDF5_SUFFIX from dodal.common.crystal_metadata import ( @@ -54,7 +50,9 @@ def path_provider() -> PathProvider: ) -set_config_client(ConfigClient()) +@devices.fixture +def config_client() -> ConfigClient: + return ConfigClient() @devices.factory() @@ -168,10 +166,10 @@ def dcm() -> DCM: @devices.factory(mock=True) -def undulator() -> UndulatorInKeV: +def undulator(config_client: ConfigClient) -> UndulatorInKeV: return UndulatorInKeV( f"{PREFIX.insertion_prefix}-MO-SERVC-01:", - get_config_client(), + config_client, poles=80, length=2.0, ) diff --git a/src/dodal/testing/fixtures/devices/hard_undulator.py b/src/dodal/testing/fixtures/devices/hard_undulator.py deleted file mode 100644 index 4f8ae689b4b..00000000000 --- a/src/dodal/testing/fixtures/devices/hard_undulator.py +++ /dev/null @@ -1,43 +0,0 @@ -from unittest.mock import MagicMock - -import pytest -from daq_config_server import ConfigClient -from daq_config_server.models.lookup_tables import GenericLookupTable - -from dodal.devices.beamlines.i09_1_shared.hard_undulator_functions import ( - I09_HU_UNDULATOR_LUT_COLUMN_NAMES, -) - -lut = GenericLookupTable( - column_names=I09_HU_UNDULATOR_LUT_COLUMN_NAMES, - rows=[ - [1, 3.00089, 0.98928, 2.12, 3.05, 14.265, 23.72, 0.0], - [2, 3.04129, 1.02504, 2.5, 2.8, 5.05165, 8.88007, 0.0], - [3, 3.05798, 1.03065, 2.4, 4.3, 5.2, 8.99036, 0.0], - [4, 3.03635, 1.02332, 3.2, 5.7, 5.26183, 8.9964, 0.0], - [5, 3.06334, 1.03294, 4.0, 7.2, 5.22735, 9.02065, 0.0], - [6, 3.04963, 1.02913, 4.7, 8.6, 5.13939, 9.02527, 0.0], - [7, 3.06515, 1.03339, 5.5, 10.1, 5.12684, 9.02602, 0.0], - [8, 3.05775, 1.03223, 6.3, 11.5, 5.16289, 9.02873, 0.0], - [9, 3.06829, 1.03468, 7.1, 13.0, 5.16357, 9.03049, 0.0], - [10, 3.06164, 1.03328, 7.9, 14.4, 5.17205, 9.02845, 0.0], - [11, 3.07056, 1.03557, 8.6, 15.9, 5.1135, 9.0475, 0.0], - [12, 3.06627, 1.03482, 9.4, 17.3, 5.12051, 9.02826, 0.0], - [13, 3.07176, 1.03623, 10.2, 18.3, 5.13027, 8.8494, 0.0], - [14, 3.06964, 1.03587, 11.0, 18.3, 5.13985, 8.30146, 0.0], - [15, 3.06515, 1.03391, 11.8, 18.3, 5.14643, 7.8238, 0.0], - ], -) - - -@pytest.fixture -def mock_config_client() -> ConfigClient: - mock_config_client = ConfigClient() - mock_config_client.get_file_contents = MagicMock(spec=["get_file_contents"]) - - def my_side_effect(file_path, desired_return_type, reset_cached_result): - assert reset_cached_result is True - return lut - - mock_config_client.get_file_contents.side_effect = my_side_effect - return mock_config_client diff --git a/tests/common/beamlines/test_device_instantiation.py b/tests/common/beamlines/test_device_instantiation.py index 97d17591c94..4f3890b4143 100644 --- a/tests/common/beamlines/test_device_instantiation.py +++ b/tests/common/beamlines/test_device_instantiation.py @@ -1,42 +1,17 @@ from typing import Any import pytest -from daq_config_server import ConfigClient from ophyd_async.core import NotConnectedError from dodal.beamlines import all_beamline_modules -from dodal.common.beamlines.beamline_utils import clear_config_client, set_config_client from dodal.device_manager import DeviceManager from dodal.utils import BLUESKY_PROTOCOLS, make_all_devices -from tests.test_data import I04_BEAMLINE_PARAMETERS, TEST_BEAMLINE_PARAMETERS_TXT def follows_bluesky_protocols(obj: Any) -> bool: return any(isinstance(obj, protocol) for protocol in BLUESKY_PROTOCOLS) -@pytest.fixture(autouse=True) -def patch_config_paths(monkeypatch): - monkeypatch.setattr( - "dodal.beamlines.i03.BEAMLINE_PARAMETERS_PATH", - TEST_BEAMLINE_PARAMETERS_TXT, - ) - monkeypatch.setattr( - "dodal.beamlines.i04.BEAMLINE_PARAMETERS_PATH", - I04_BEAMLINE_PARAMETERS, - ) - - -@pytest.fixture(autouse=True) -def reset_config_client(mock_config_client: ConfigClient): - original = ConfigClient.get_file_contents - ConfigClient.get_file_contents = mock_config_client.get_file_contents # type: ignore - set_config_client(mock_config_client) - yield - clear_config_client() - ConfigClient.get_file_contents = original - - @pytest.mark.parametrize( "module_and_devices_for_beamline", set(all_beamline_modules()), diff --git a/tests/conftest.py b/tests/conftest.py index 6c032e1d636..420e764e8d8 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -16,7 +16,7 @@ from ophyd_async.core import PathProvider from pydantic import TypeAdapter -from dodal.common.beamlines import beamline_parameters, beamline_utils +from dodal.common.beamlines import beamline_utils from dodal.common.beamlines.beamline_utils import ( clear_config_client, clear_path_provider, @@ -35,31 +35,50 @@ collect_factories, make_all_devices, ) -from tests.devices.beamlines.i10.test_data import LOOKUP_TABLE_PATH from tests.devices.oav.test_data import TEST_DISPLAY_CONFIG, TEST_OAV_ZOOM_LEVELS from tests.devices.test_daq_configuration import MOCK_DAQ_CONFIG_PATH from tests.devices.test_data import TEST_LUT_TXT -from tests.test_data import ( - I04_BEAMLINE_PARAMETERS, -) - -MOCK_PATHS = [ - ("DAQ_CONFIGURATION_PATH", MOCK_DAQ_CONFIG_PATH), - ("ZOOM_PARAMS_FILE", TEST_OAV_ZOOM_LEVELS), - ("DISPLAY_CONFIG", TEST_DISPLAY_CONFIG), - ("LOOK_UPTABLE_DIR", LOOKUP_TABLE_PATH), - ("SHARED_CONFIG_PATH", MOCK_DAQ_CONFIG_PATH), -] -MOCK_ATTRIBUTES_TABLE = { - "i03": MOCK_PATHS, - "i10_optics": MOCK_PATHS, - "i04": MOCK_PATHS, - "i19_1": MOCK_PATHS, - "i19_2": MOCK_PATHS, - "i19_optics": MOCK_PATHS, - "i23": MOCK_PATHS, - "i24": MOCK_PATHS, - "aithre": MOCK_PATHS, +from tests.test_data import I04_BEAMLINE_PARAMETERS, TEST_BEAMLINE_PARAMETERS_TXT + +# ToDo - Make documentation so clearer for beamlines on how to fix tests by mocking the +# paths needed. +MOCK_ATTRIBUTES_TABLE: dict[str, dict[str, str]] = { + "i03": { + "BEAMLINE_PARAMETERS_PATH": TEST_BEAMLINE_PARAMETERS_TXT, + "DAQ_CONFIGURATION_PATH": MOCK_DAQ_CONFIG_PATH, + "ZOOM_PARAMS_FILE": TEST_OAV_ZOOM_LEVELS, + "DISPLAY_CONFIG": TEST_DISPLAY_CONFIG, + }, + "i04": { + "BEAMLINE_PARAMETERS_PATH": I04_BEAMLINE_PARAMETERS, + "DAQ_CONFIGURATION_PATH": MOCK_DAQ_CONFIG_PATH, + "ZOOM_PARAMS_FILE": TEST_OAV_ZOOM_LEVELS, + "DISPLAY_CONFIG": TEST_DISPLAY_CONFIG, + }, + "i19_1": { + "SHARED_CONFIG_PATH": MOCK_DAQ_CONFIG_PATH, + "DAQ_CONFIGURATION_PATH": MOCK_DAQ_CONFIG_PATH, + "ZOOM_PARAMS_FILE": TEST_OAV_ZOOM_LEVELS, + "DISPLAY_CONFIG": TEST_DISPLAY_CONFIG, + }, + "i19_2": { + "SHARED_CONFIG_PATH": MOCK_DAQ_CONFIG_PATH, + }, + "i19_optics": { + "DAQ_CONFIGURATION_PATH": MOCK_DAQ_CONFIG_PATH, + }, + "aithre": { + "DISPLAY_CONFIG": TEST_DISPLAY_CONFIG, + "ZOOM_PARAMS_FILE": TEST_OAV_ZOOM_LEVELS, + }, + "i23": { + "ZOOM_PARAMS_FILE": TEST_OAV_ZOOM_LEVELS, + "DISPLAY_CONFIG": TEST_DISPLAY_CONFIG, + }, + "i24": { + "ZOOM_PARAMS_FILE": TEST_OAV_ZOOM_LEVELS, + "DISPLAY_CONFIG": TEST_DISPLAY_CONFIG, + }, } BANNED_PATHS = [Path("/dls"), Path("/dls_sw")] @@ -67,11 +86,7 @@ # Add run_engine and util fixtures to be used in tests -pytest_plugins = [ - "dodal.testing.fixtures.run_engine", - "dodal.testing.fixtures.utils", - "dodal.testing.fixtures.config_server", -] +pytest_plugins = ["dodal.testing.fixtures.run_engine", "dodal.testing.fixtures.utils"] @pytest.fixture(autouse=True) @@ -80,21 +95,62 @@ def reset_path_provider(): clear_path_provider() -@pytest.fixture(autouse=True) -def patch_open_to_prevent_dls_reads_in_tests(): - unpatched_open = open +T = TypeVar("T", str, dict, ConfigModel) - def patched_open(*args, **kwargs): - requested_path = Path(args[0]) - if requested_path.is_absolute(): - for p in BANNED_PATHS: - assert not requested_path.is_relative_to(p), ( - f"Attempt to open {requested_path} from inside a unit test" - ) - return unpatched_open(*args, **kwargs) - with patch("builtins.open", side_effect=patched_open): - yield [] +def fake_config_server_get_file_contents( + file_path: str | Path, + desired_return_type: type[T] = str, + reset_cached_result: bool = True, + force_parser: Callable[[str], Any] | None = None, +) -> T: + """Fakes getting a file from the config server by reading it directly. + + Args: + file_path (str | Path): Filepath of the file to read + desired_return_type (type[T], optional): Type to convert the file to. Defaults to str. + reset_cached_result (bool, optional): Whether or not to use the config server's cached result. + Does nothing here as we don't cache. Defaults to True. + force_parser (Callable[[str], Any] | None, optional): Use a certain converter function. + Only needed for the interim where the converter exists but the config server has not + been redeployed. Defaults to None. + + Raises: + ValueError: Raised if an invalid type is requested + + Returns: + T: The contents of the config file. + """ + filepath = Path(file_path) + if filepath.is_absolute(): + for p in BANNED_PATHS: + assert not filepath.is_relative_to(p), ( + f"Attempt to open {filepath} from inside a unit test" + ) + # Minimal logic required for unit tests + with filepath.open("r") as f: + contents = f.read() + if force_parser: + return TypeAdapter(desired_return_type).validate_python(force_parser(contents)) + if desired_return_type is str: + return contents # type: ignore + if desired_return_type is dict: + return json.loads(contents) + if issubclass(desired_return_type, ConfigModel): + return desired_return_type.model_validate(json.loads(contents)) + raise ValueError("Invalid return type requested") + + +@pytest.fixture +def mock_config_client() -> ConfigClient: + # Don't actually talk to central service during unit tests, and reset caches between test + mock_config_client = ConfigClient() + mock_config_client.get_file_contents = MagicMock(spec=["get_file_contents"]) + + mock_config_client.get_file_contents.side_effect = ( + fake_config_server_get_file_contents + ) + return mock_config_client def _is_banned(path: Path) -> bool: @@ -109,21 +165,31 @@ def _is_banned(path: Path) -> bool: @pytest.fixture(autouse=True) -def block_dls_access_in_config_client(): +def patch_open_to_prevent_dls_reads_in_tests(): + real_open = open + + def patched_open(*args, **kwargs): + requested_path = Path(args[0]) + if _is_banned(requested_path): + raise AssertionError( + f"Attempt to open {requested_path} from inside a unit test" + ) + return real_open(*args, **kwargs) - original = ConfigClient.get_file_contents + with patch("builtins.open", side_effect=patched_open): + yield [] - def guarded(self, file_path, *args, **kwargs): - # --- normalize path safely --- - path = Path(file_path) +@pytest.fixture(autouse=True) +def block_dls_access_in_config_client(): + + def patch_get_file_contents(self, file_path, *args, **kwargs): + path = Path(file_path) if _is_banned(path): raise AssertionError(f"Forbidden config access blocked in test: {path}") + return ConfigClient.get_file_contents(self, file_path, *args, **kwargs) - # --- delegate to whatever is currently installed --- - return original(self, file_path, *args, **kwargs) - - with patch.object(ConfigClient, "get_file_contents", new=guarded): + with patch.object(ConfigClient, "get_file_contents", new=patch_get_file_contents): yield @@ -161,8 +227,16 @@ async def static_path_provider( @pytest.fixture(scope="function") -def module_and_devices_for_beamline(request: pytest.FixtureRequest): +def module_and_devices_for_beamline( + mock_config_client: ConfigClient, request: pytest.FixtureRequest +): + # Swap all ConfigClient instances with the mock one method. + # Prevents dls paths used, open local files rather than from service. + original_config_client = ConfigClient.get_file_contents + ConfigClient.get_file_contents = mock_config_client.get_file_contents # type: ignore + beamline = request.param + with patch.dict(os.environ, {"BEAMLINE": beamline}, clear=True): bl_mod = importlib.import_module("dodal.beamlines." + beamline) mock_beamline_module_filepaths(beamline, bl_mod) @@ -187,11 +261,14 @@ def module_and_devices_for_beamline(request: pytest.FixtureRequest): factory.cache_clear() del bl_mod + clear_config_client() + # Set back the ConfigClient instance with the original one. + ConfigClient.get_file_contents = original_config_client + def mock_beamline_module_filepaths(bl_name: str, bl_module: ModuleType): if mock_attributes := MOCK_ATTRIBUTES_TABLE.get(bl_name): - [bl_module.__setattr__(attr[0], attr[1]) for attr in mock_attributes] - beamline_parameters.BEAMLINE_PARAMETER_PATHS[bl_name] = I04_BEAMLINE_PARAMETERS + [bl_module.__setattr__(attr, value) for attr, value in mock_attributes.items()] @pytest.fixture @@ -211,67 +288,3 @@ def eiger_params(tmp_path: Path) -> DetectorParams: det_dist_to_beam_converter_path=TEST_LUT_TXT, detector_size_constants=EIGER2_X_16M_SIZE.det_type_string, # type: ignore ) - - -T = TypeVar("T", str, dict, ConfigModel) - - -def fake_config_server_get_file_contents( - file_path: str | Path, - desired_return_type: type[T] = str, - reset_cached_result: bool = True, - force_parser: Callable[[str], Any] | None = None, -) -> T: - """Fakes getting a file from the config server by reading it directly. - - Args: - file_path (str | Path): Filepath of the file to read - desired_return_type (type[T], optional): Type to convert the file to. Defaults to str. - reset_cached_result (bool, optional): Whether or not to use the config server's cached result. - Does nothing here as we don't cache. Defaults to True. - force_parser (Callable[[str], Any] | None, optional): Use a certain converter function. - Only needed for the interim where the converter exists but the config server has not - been redeployed. Defaults to None. - - Raises: - ValueError: Raised if an invalid type is requested - - Returns: - T: The contents of the config file. - """ - filepath = Path(file_path) - if filepath.is_absolute(): - for p in BANNED_PATHS: - assert not filepath.is_relative_to(p), ( - f"Attempt to open {filepath} from inside a unit test" - ) - # Minimal logic required for unit tests - with filepath.open("r") as f: - contents = f.read() - if force_parser: - return TypeAdapter(desired_return_type).validate_python(force_parser(contents)) - if desired_return_type is str: - return contents # type: ignore - if desired_return_type is dict: - return json.loads(contents) - if issubclass(desired_return_type, ConfigModel): - return desired_return_type.model_validate(json.loads(contents)) - raise ValueError("Invalid return type requested") - - -@pytest.fixture -def mock_config_client() -> ConfigClient: - # Don't actually talk to central service during unit tests, and reset caches between test - mock_config_client = ConfigClient() - mock_config_client.get_file_contents = MagicMock(spec=["get_file_contents"]) - - mock_config_client.get_file_contents.side_effect = ( - fake_config_server_get_file_contents - ) - return mock_config_client - - -@pytest.fixture(autouse=True) -def reset_config_client(): - yield - clear_config_client() diff --git a/tests/devices/beamlines/i09_1_shared/conftest.py b/tests/devices/beamlines/i09_1_shared/conftest.py new file mode 100644 index 00000000000..722b087b08a --- /dev/null +++ b/tests/devices/beamlines/i09_1_shared/conftest.py @@ -0,0 +1,30 @@ +from collections.abc import Callable +from pathlib import Path +from typing import Any + +import pytest +from daq_config_server import ConfigClient +from daq_config_server.models.lookup_tables import GenericLookupTable +from daq_config_server.models.lookup_tables.insertion_device import ( + parse_i09_hu_undulator_energy_gap_lut, +) + + +@pytest.fixture +def mock_config_client(mock_config_client: ConfigClient) -> ConfigClient: + # To Do, need a more effective way to customise this without removing other + # behaviour like banned paths. Need to chain mock behaviour. + # See https://github.com/DiamondLightSource/daq-config-server/issues/194 + def load_file( + file_path: str | Path, + desired_return_type: type[GenericLookupTable], + reset_cached_result: bool = True, + force_parser: Callable[[str], Any] | None = None, + ) -> GenericLookupTable: + with open(file_path) as f: + contents = f.read() + return parse_i09_hu_undulator_energy_gap_lut(contents) + + mock_config_client.get_file_contents = load_file # type: ignore + + return mock_config_client diff --git a/tests/devices/beamlines/i09_1_shared/test_hard_energy.py b/tests/devices/beamlines/i09_1_shared/test_hard_energy.py index fb4720e71c7..3c15089210e 100644 --- a/tests/devices/beamlines/i09_1_shared/test_hard_energy.py +++ b/tests/devices/beamlines/i09_1_shared/test_hard_energy.py @@ -5,10 +5,7 @@ from bluesky.run_engine import RunEngine from daq_config_server import ConfigClient from ophyd_async.core import init_devices -from ophyd_async.testing import ( - assert_reading, - partial_reading, -) +from ophyd_async.testing import assert_reading, partial_reading from dodal.devices.beamlines.i09_1_shared import ( HardEnergy, @@ -22,8 +19,7 @@ StationaryCrystal, ) from dodal.devices.undulator import UndulatorInMm, UndulatorOrder - -pytest_plugins = ["dodal.testing.fixtures.devices.hard_undulator"] +from tests.devices.beamlines.i09_1_shared.test_data import TEST_HARD_UNDULATOR_LUT @pytest.fixture @@ -60,7 +56,7 @@ async def hu_id_energy( undulator_order=undulator_order, undulator=undulator_in_mm, config_server=mock_config_client, - filepath="path/to/lut", + filepath=TEST_HARD_UNDULATOR_LUT, gap_to_energy_func=calculate_energy_i09_hu, energy_to_gap_func=calculate_gap_i09_hu, ) diff --git a/tests/devices/beamlines/i09_1_shared/test_undulator_functions.py b/tests/devices/beamlines/i09_1_shared/test_undulator_functions.py index 38766b32e4f..d67fb674108 100644 --- a/tests/devices/beamlines/i09_1_shared/test_undulator_functions.py +++ b/tests/devices/beamlines/i09_1_shared/test_undulator_functions.py @@ -9,20 +9,18 @@ calculate_energy_i09_hu, calculate_gap_i09_hu, ) - -pytest_plugins = ["dodal.testing.fixtures.devices.hard_undulator"] +from tests.devices.beamlines.i09_1_shared.test_data import TEST_HARD_UNDULATOR_LUT @pytest.fixture() def lut( mock_config_client: ConfigClient, ) -> GenericLookupTable: - _lut = mock_config_client.get_file_contents( - file_path="path/to/lut", + return mock_config_client.get_file_contents( + file_path=TEST_HARD_UNDULATOR_LUT, desired_return_type=GenericLookupTable, reset_cached_result=True, ) - return _lut @pytest.mark.parametrize( From eb6c0034d59c5d6a88ea3792805fae2bf7c44738 Mon Sep 17 00:00:00 2001 From: Oli Wenman Date: Tue, 23 Jun 2026 13:33:25 +0000 Subject: [PATCH 15/35] Update config_client to be more reusable --- src/dodal/testing/fixtures/config_client.py | 98 ++++++++++++++++++++ src/dodal/testing/fixtures/config_server.py | 60 ------------ src/dodal/testing/fixtures/devices/apple2.py | 18 +--- src/dodal/testing/paths.py | 13 +++ tests/conftest.py | 86 ++--------------- 5 files changed, 120 insertions(+), 155 deletions(-) create mode 100644 src/dodal/testing/fixtures/config_client.py delete mode 100644 src/dodal/testing/fixtures/config_server.py create mode 100644 src/dodal/testing/paths.py diff --git a/src/dodal/testing/fixtures/config_client.py b/src/dodal/testing/fixtures/config_client.py new file mode 100644 index 00000000000..d2955bb54b6 --- /dev/null +++ b/src/dodal/testing/fixtures/config_client.py @@ -0,0 +1,98 @@ +import json +from collections.abc import Callable +from pathlib import Path +from typing import Any, TypeVar +from unittest.mock import MagicMock, patch + +import pytest +from daq_config_server import ConfigClient +from daq_config_server.models import ConfigModel +from pydantic import TypeAdapter + +from dodal.testing.paths import is_path_banned + +T = TypeVar("T", str, dict, ConfigModel) + + +def mock_config_server_get_file_contents( + file_path: str | Path, + desired_return_type: type[T] = str, + reset_cached_result: bool = True, + force_parser: Callable[[str], Any] | None = None, +) -> T: + """Mocks getting a file from the config server by reading it directly. + + Args: + file_path (str | Path): Filepath of the file to read + desired_return_type (type[T], optional): Type to convert the file to. Defaults to str. + reset_cached_result (bool, optional): Whether or not to use the config server's cached result. + Does nothing here as we don't cache. Defaults to True. + force_parser (Callable[[str], Any] | None, optional): Use a certain converter function. + Only needed for the interim where the converter exists but the config server has not + been redeployed. Defaults to None. + + Raises: + ValueError: Raised if an invalid type is requested + + Returns: + T: The contents of the config file. + """ + requested_path = Path(file_path) + if requested_path.is_absolute(): + if is_path_banned(requested_path): + raise AssertionError( + f"Attempt to open {requested_path} from inside a unit test" + ) + # Minimal logic required for unit tests + with requested_path.open("r") as f: + contents = f.read() + if force_parser: + return TypeAdapter(desired_return_type).validate_python(force_parser(contents)) + if desired_return_type is str: + return contents # type: ignore + if desired_return_type is dict: + return json.loads(contents) + if issubclass(desired_return_type, ConfigModel): + return desired_return_type.model_validate(json.loads(contents)) + raise ValueError("Invalid return type requested") + + +@pytest.fixture +def mock_config_client() -> ConfigClient: + # Don't actually talk to central service during unit tests, and reset caches between test + mock_config_client = ConfigClient() + mock_config_client.get_file_contents = MagicMock(spec=["get_file_contents"]) + + mock_config_client.get_file_contents.side_effect = ( + mock_config_server_get_file_contents + ) + return mock_config_client + + +@pytest.fixture(autouse=True) +def patch_open_to_prevent_dls_reads_in_tests(): + real_open = open + + def patched_open(*args, **kwargs): + requested_path = Path(args[0]) + if is_path_banned(requested_path): + raise AssertionError( + f"Attempt to open {requested_path} from inside a unit test" + ) + return real_open(*args, **kwargs) + + with patch("builtins.open", side_effect=patched_open): + yield [] + + +@pytest.fixture(autouse=True) +def block_dls_access_in_config_client(): + + def patch_get_file_contents(self, file_path, *args, **kwargs): + path = Path(file_path) + if is_path_banned(path): + raise AssertionError(f"Forbidden config access blocked in test: {path}") + return ConfigClient.get_file_contents(self, file_path, *args, **kwargs) + + with patch.object(ConfigClient, "get_file_contents", new=patch_get_file_contents): + yield diff --git a/src/dodal/testing/fixtures/config_server.py b/src/dodal/testing/fixtures/config_server.py deleted file mode 100644 index 6aa3a5bd1df..00000000000 --- a/src/dodal/testing/fixtures/config_server.py +++ /dev/null @@ -1,60 +0,0 @@ -# import json -# from collections.abc import Callable -# from pathlib import Path -# from typing import Any, TypeVar -# from unittest.mock import patch - -# import pytest -# from daq_config_server.models import ConfigModel -# from pydantic import TypeAdapter - -# T = TypeVar("T", str, dict, ConfigModel) - - -# def fake_config_server_get_file_contents( -# filepath: str | Path, -# desired_return_type: type[T] = str, -# reset_cached_result: bool = True, -# force_parser: Callable[[str], Any] | None = None, -# ) -> T: -# """Fakes getting a file from the config server by reading it directly. - -# Args: -# filepath (str | Path): Filepath of the file to read -# desired_return_type (type[T], optional): Type to convert the file to. Defaults to str. -# reset_cached_result (bool, optional): Whether or not to use the config server's cached result. -# Does nothing here as we don't cache. Defaults to True. -# force_parser (Callable[[str], Any] | None, optional): Use a certain converter function. -# Only needed for the interim where the converter exists but the config server has not -# been redeployed. Defaults to None. - -# Raises: -# ValueError: Raised if an invalid type is requested - -# Returns: -# T: The contents of the config file. -# """ -# filepath = Path(filepath) -# # Minimal logic required for unit tests -# with filepath.open("r") as f: -# contents = f.read() -# if force_parser: -# return TypeAdapter(desired_return_type).validate_python(force_parser(contents)) -# if desired_return_type is str: -# return contents # type: ignore -# if desired_return_type is dict: -# return json.loads(contents) -# if issubclass(desired_return_type, ConfigModel): -# return desired_return_type.model_validate(json.loads(contents)) -# raise ValueError("Invalid return type requested") - - -# @pytest.fixture(autouse=True) -# def mock_config_client(): -# # Don't actually talk to central service during unit tests, and reset caches between test - -# with patch( -# "daq_config_server.app.client.ConfigClient.get_file_contents", -# side_effect=fake_config_server_get_file_contents, -# ): -# yield diff --git a/src/dodal/testing/fixtures/devices/apple2.py b/src/dodal/testing/fixtures/devices/apple2.py index a8d06cedc14..e5baee50ee7 100644 --- a/src/dodal/testing/fixtures/devices/apple2.py +++ b/src/dodal/testing/fixtures/devices/apple2.py @@ -1,7 +1,4 @@ -from unittest.mock import MagicMock - import pytest -from daq_config_server import ConfigClient from ophyd_async.core import ( init_devices, set_mock_value, @@ -19,20 +16,7 @@ UndulatorLockedPhaseAxes, ) - -@pytest.fixture -def mock_config_client() -> ConfigClient: - mock_config_client = ConfigClient() - - mock_config_client.get_file_contents = MagicMock(spec=["get_file_contents"]) - - def my_side_effect(file_path, reset_cached_result) -> str: - assert reset_cached_result is True - with open(file_path) as f: - return f.read() - - mock_config_client.get_file_contents.side_effect = my_side_effect - return mock_config_client +pytest_plugins = ["dodal.testing.fixtures.config_client"] @pytest.fixture diff --git a/src/dodal/testing/paths.py b/src/dodal/testing/paths.py new file mode 100644 index 00000000000..051e2cb6307 --- /dev/null +++ b/src/dodal/testing/paths.py @@ -0,0 +1,13 @@ +from pathlib import Path + +BANNED_PATHS = [Path("/dls"), Path("/dls_sw")] + + +def is_path_banned(path: Path) -> bool: + try: + resolved = path.resolve() + except Exception: + resolved = path + return resolved.is_absolute() and any( + resolved.is_relative_to(banned) for banned in BANNED_PATHS + ) diff --git a/tests/conftest.py b/tests/conftest.py index 420e764e8d8..20b53c40cd5 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,20 +1,15 @@ import importlib -import json import logging import os import sys -from collections.abc import Callable from os import environ from pathlib import Path from types import ModuleType -from typing import Any, TypeVar from unittest.mock import MagicMock, patch import pytest from daq_config_server import ConfigClient -from daq_config_server.models import ConfigModel from ophyd_async.core import PathProvider -from pydantic import TypeAdapter from dodal.common.beamlines import beamline_utils from dodal.common.beamlines.beamline_utils import ( @@ -30,6 +25,7 @@ from dodal.devices.detector import DetectorParams from dodal.devices.detector.det_dim_constants import EIGER2_X_16M_SIZE from dodal.log import LOGGER, GELFTCPHandler, set_up_all_logging_handlers +from dodal.testing.paths import is_path_banned from dodal.utils import ( DeviceInitializationController, collect_factories, @@ -81,12 +77,15 @@ }, } -BANNED_PATHS = [Path("/dls"), Path("/dls_sw")] environ["DODAL_TEST_MODE"] = "true" # Add run_engine and util fixtures to be used in tests -pytest_plugins = ["dodal.testing.fixtures.run_engine", "dodal.testing.fixtures.utils"] +pytest_plugins = [ + "dodal.testing.fixtures.run_engine", + "dodal.testing.fixtures.utils", + "dodal.testing.fixtures.config_client", +] @pytest.fixture(autouse=True) @@ -95,82 +94,13 @@ def reset_path_provider(): clear_path_provider() -T = TypeVar("T", str, dict, ConfigModel) - - -def fake_config_server_get_file_contents( - file_path: str | Path, - desired_return_type: type[T] = str, - reset_cached_result: bool = True, - force_parser: Callable[[str], Any] | None = None, -) -> T: - """Fakes getting a file from the config server by reading it directly. - - Args: - file_path (str | Path): Filepath of the file to read - desired_return_type (type[T], optional): Type to convert the file to. Defaults to str. - reset_cached_result (bool, optional): Whether or not to use the config server's cached result. - Does nothing here as we don't cache. Defaults to True. - force_parser (Callable[[str], Any] | None, optional): Use a certain converter function. - Only needed for the interim where the converter exists but the config server has not - been redeployed. Defaults to None. - - Raises: - ValueError: Raised if an invalid type is requested - - Returns: - T: The contents of the config file. - """ - filepath = Path(file_path) - if filepath.is_absolute(): - for p in BANNED_PATHS: - assert not filepath.is_relative_to(p), ( - f"Attempt to open {filepath} from inside a unit test" - ) - # Minimal logic required for unit tests - with filepath.open("r") as f: - contents = f.read() - if force_parser: - return TypeAdapter(desired_return_type).validate_python(force_parser(contents)) - if desired_return_type is str: - return contents # type: ignore - if desired_return_type is dict: - return json.loads(contents) - if issubclass(desired_return_type, ConfigModel): - return desired_return_type.model_validate(json.loads(contents)) - raise ValueError("Invalid return type requested") - - -@pytest.fixture -def mock_config_client() -> ConfigClient: - # Don't actually talk to central service during unit tests, and reset caches between test - mock_config_client = ConfigClient() - mock_config_client.get_file_contents = MagicMock(spec=["get_file_contents"]) - - mock_config_client.get_file_contents.side_effect = ( - fake_config_server_get_file_contents - ) - return mock_config_client - - -def _is_banned(path: Path) -> bool: - try: - resolved = path.resolve() - except Exception: - resolved = path - - return resolved.is_absolute() and any( - resolved.is_relative_to(banned) for banned in BANNED_PATHS - ) - - @pytest.fixture(autouse=True) def patch_open_to_prevent_dls_reads_in_tests(): real_open = open def patched_open(*args, **kwargs): requested_path = Path(args[0]) - if _is_banned(requested_path): + if is_path_banned(requested_path): raise AssertionError( f"Attempt to open {requested_path} from inside a unit test" ) @@ -185,7 +115,7 @@ def block_dls_access_in_config_client(): def patch_get_file_contents(self, file_path, *args, **kwargs): path = Path(file_path) - if _is_banned(path): + if is_path_banned(path): raise AssertionError(f"Forbidden config access blocked in test: {path}") return ConfigClient.get_file_contents(self, file_path, *args, **kwargs) From f25e97233a6f7f7953e838e424f92971f23ac813 Mon Sep 17 00:00:00 2001 From: Oli Wenman Date: Tue, 23 Jun 2026 13:35:31 +0000 Subject: [PATCH 16/35] Add doc comment --- src/dodal/testing/fixtures/config_client.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/dodal/testing/fixtures/config_client.py b/src/dodal/testing/fixtures/config_client.py index d2955bb54b6..f286daba954 100644 --- a/src/dodal/testing/fixtures/config_client.py +++ b/src/dodal/testing/fixtures/config_client.py @@ -57,6 +57,7 @@ def mock_config_server_get_file_contents( raise ValueError("Invalid return type requested") +# ToDo - Add documentation for tests to reuse this fixture. @pytest.fixture def mock_config_client() -> ConfigClient: # Don't actually talk to central service during unit tests, and reset caches between test From d9659326a843b1aa91281ed503c55004261999f7 Mon Sep 17 00:00:00 2001 From: Oli Wenman Date: Tue, 23 Jun 2026 13:41:51 +0000 Subject: [PATCH 17/35] Add mock_config_client to system tests --- system_tests/conftest.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/system_tests/conftest.py b/system_tests/conftest.py index d3df62cbd5d..6d5c5dfb24b 100644 --- a/system_tests/conftest.py +++ b/system_tests/conftest.py @@ -1,2 +1,5 @@ -# Add run_engine to be used in tests -pytest_plugins = ["dodal.testing.fixtures.run_engine"] +# Add run_engine and mock_config_client to be used in tests +pytest_plugins = [ + "dodal.testing.fixtures.run_engine", + "dodal.testing.fixtures.config_client", +] From a486444b4dd882c20f6b07522efed033318d3275 Mon Sep 17 00:00:00 2001 From: Oli Wenman Date: Tue, 23 Jun 2026 13:42:14 +0000 Subject: [PATCH 18/35] Update comment --- system_tests/conftest.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/system_tests/conftest.py b/system_tests/conftest.py index 6d5c5dfb24b..c0b76377f5a 100644 --- a/system_tests/conftest.py +++ b/system_tests/conftest.py @@ -1,4 +1,4 @@ -# Add run_engine and mock_config_client to be used in tests +# Add run_engine and mock_config_client to be used in system tests pytest_plugins = [ "dodal.testing.fixtures.run_engine", "dodal.testing.fixtures.config_client", From ea4ef4d13d546ddb026226c186c903ec5bea8d4f Mon Sep 17 00:00:00 2001 From: Oli Wenman Date: Tue, 23 Jun 2026 13:45:45 +0000 Subject: [PATCH 19/35] Remove duplicate code --- src/dodal/testing/fixtures/config_client.py | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/src/dodal/testing/fixtures/config_client.py b/src/dodal/testing/fixtures/config_client.py index f286daba954..e9032ba8315 100644 --- a/src/dodal/testing/fixtures/config_client.py +++ b/src/dodal/testing/fixtures/config_client.py @@ -84,16 +84,3 @@ def patched_open(*args, **kwargs): with patch("builtins.open", side_effect=patched_open): yield [] - - -@pytest.fixture(autouse=True) -def block_dls_access_in_config_client(): - - def patch_get_file_contents(self, file_path, *args, **kwargs): - path = Path(file_path) - if is_path_banned(path): - raise AssertionError(f"Forbidden config access blocked in test: {path}") - return ConfigClient.get_file_contents(self, file_path, *args, **kwargs) - - with patch.object(ConfigClient, "get_file_contents", new=patch_get_file_contents): - yield From 2a77061eacfebe5afcf09a390d37eb68b3f8316f Mon Sep 17 00:00:00 2001 From: Oli Wenman Date: Tue, 23 Jun 2026 14:08:17 +0000 Subject: [PATCH 20/35] Add missing test --- tests/testing/test_paths.py | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) create mode 100644 tests/testing/test_paths.py diff --git a/tests/testing/test_paths.py b/tests/testing/test_paths.py new file mode 100644 index 00000000000..d1b8dcc3b57 --- /dev/null +++ b/tests/testing/test_paths.py @@ -0,0 +1,18 @@ +from pathlib import Path + +import pytest + +from dodal.testing.paths import is_path_banned + + +@pytest.mark.parametrize( + "path, expected_result", + [ + (Path("/dls/iXX"), True), + (Path("/dls_sw/iXX"), True), + (Path("relative/path"), False), + (Path("/absolute/path"), False), + ], +) +def test_is_path_banned(path: Path, expected_result: bool) -> None: + assert is_path_banned(path) is expected_result From a4c556a955840b92ca500ff1f442a010ab19c64b Mon Sep 17 00:00:00 2001 From: Oli Wenman Date: Tue, 23 Jun 2026 14:20:55 +0000 Subject: [PATCH 21/35] Simplify is_banned --- src/dodal/testing/paths.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/dodal/testing/paths.py b/src/dodal/testing/paths.py index 051e2cb6307..75105fcc404 100644 --- a/src/dodal/testing/paths.py +++ b/src/dodal/testing/paths.py @@ -4,10 +4,7 @@ def is_path_banned(path: Path) -> bool: - try: - resolved = path.resolve() - except Exception: - resolved = path + resolved = path.resolve() return resolved.is_absolute() and any( resolved.is_relative_to(banned) for banned in BANNED_PATHS ) From 752f779a3896022583397eaf5899717a5aa4de30 Mon Sep 17 00:00:00 2001 From: Oli Wenman Date: Tue, 23 Jun 2026 14:28:07 +0000 Subject: [PATCH 22/35] Remove duplicate function --- src/dodal/testing/fixtures/config_client.py | 18 +----------------- 1 file changed, 1 insertion(+), 17 deletions(-) diff --git a/src/dodal/testing/fixtures/config_client.py b/src/dodal/testing/fixtures/config_client.py index e9032ba8315..1a95cbf9686 100644 --- a/src/dodal/testing/fixtures/config_client.py +++ b/src/dodal/testing/fixtures/config_client.py @@ -2,7 +2,7 @@ from collections.abc import Callable from pathlib import Path from typing import Any, TypeVar -from unittest.mock import MagicMock, patch +from unittest.mock import MagicMock import pytest from daq_config_server import ConfigClient @@ -68,19 +68,3 @@ def mock_config_client() -> ConfigClient: mock_config_server_get_file_contents ) return mock_config_client - - -@pytest.fixture(autouse=True) -def patch_open_to_prevent_dls_reads_in_tests(): - real_open = open - - def patched_open(*args, **kwargs): - requested_path = Path(args[0]) - if is_path_banned(requested_path): - raise AssertionError( - f"Attempt to open {requested_path} from inside a unit test" - ) - return real_open(*args, **kwargs) - - with patch("builtins.open", side_effect=patched_open): - yield [] From 8b2f472252e2168cb85c07d7dbf5fbba4bcbcbeb Mon Sep 17 00:00:00 2001 From: Oli Wenman Date: Tue, 23 Jun 2026 14:57:55 +0000 Subject: [PATCH 23/35] Add a hook for tests to customise return value for daq config server --- src/dodal/testing/fixtures/config_client.py | 11 +++++++ .../beamlines/i09_1_shared/conftest.py | 33 +++++++------------ 2 files changed, 22 insertions(+), 22 deletions(-) diff --git a/src/dodal/testing/fixtures/config_client.py b/src/dodal/testing/fixtures/config_client.py index 1a95cbf9686..15eaf1ccc10 100644 --- a/src/dodal/testing/fixtures/config_client.py +++ b/src/dodal/testing/fixtures/config_client.py @@ -13,6 +13,14 @@ T = TypeVar("T", str, dict, ConfigModel) +TModel = TypeVar("TModel", bound=ConfigModel) + +# Test can register a specific file path with an assoicated file contents to model +# conversion function. +MOCK_CONFIG_CLIENT_PATH_TO_MODEL_CONVERSION: dict[ + str, Callable[[str], ConfigModel] +] = {} + def mock_config_server_get_file_contents( file_path: str | Path, @@ -53,6 +61,9 @@ def mock_config_server_get_file_contents( if desired_return_type is dict: return json.loads(contents) if issubclass(desired_return_type, ConfigModel): + if str(requested_path) in MOCK_CONFIG_CLIENT_PATH_TO_MODEL_CONVERSION: + callable = MOCK_CONFIG_CLIENT_PATH_TO_MODEL_CONVERSION[str(requested_path)] + return callable(contents) # type: ignore return desired_return_type.model_validate(json.loads(contents)) raise ValueError("Invalid return type requested") diff --git a/tests/devices/beamlines/i09_1_shared/conftest.py b/tests/devices/beamlines/i09_1_shared/conftest.py index 722b087b08a..69e0e64d642 100644 --- a/tests/devices/beamlines/i09_1_shared/conftest.py +++ b/tests/devices/beamlines/i09_1_shared/conftest.py @@ -1,30 +1,19 @@ -from collections.abc import Callable -from pathlib import Path -from typing import Any - import pytest from daq_config_server import ConfigClient -from daq_config_server.models.lookup_tables import GenericLookupTable from daq_config_server.models.lookup_tables.insertion_device import ( parse_i09_hu_undulator_energy_gap_lut, ) +from dodal.testing.fixtures.config_client import ( + MOCK_CONFIG_CLIENT_PATH_TO_MODEL_CONVERSION, +) +from tests.devices.beamlines.i09_1_shared.test_data import TEST_HARD_UNDULATOR_LUT -@pytest.fixture -def mock_config_client(mock_config_client: ConfigClient) -> ConfigClient: - # To Do, need a more effective way to customise this without removing other - # behaviour like banned paths. Need to chain mock behaviour. - # See https://github.com/DiamondLightSource/daq-config-server/issues/194 - def load_file( - file_path: str | Path, - desired_return_type: type[GenericLookupTable], - reset_cached_result: bool = True, - force_parser: Callable[[str], Any] | None = None, - ) -> GenericLookupTable: - with open(file_path) as f: - contents = f.read() - return parse_i09_hu_undulator_energy_gap_lut(contents) - - mock_config_client.get_file_contents = load_file # type: ignore - return mock_config_client +@pytest.fixture +def mock_config_client(mock_config_client: ConfigClient): + MOCK_CONFIG_CLIENT_PATH_TO_MODEL_CONVERSION[TEST_HARD_UNDULATOR_LUT] = ( + parse_i09_hu_undulator_energy_gap_lut + ) + yield mock_config_client + MOCK_CONFIG_CLIENT_PATH_TO_MODEL_CONVERSION.pop(TEST_HARD_UNDULATOR_LUT, None) From 7d2e3a39dc33c6ae8d32d5ea1161472ba3e1baf8 Mon Sep 17 00:00:00 2001 From: Oli Wenman Date: Tue, 23 Jun 2026 14:59:38 +0000 Subject: [PATCH 24/35] Update comments --- src/dodal/testing/fixtures/config_client.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/dodal/testing/fixtures/config_client.py b/src/dodal/testing/fixtures/config_client.py index 15eaf1ccc10..3bd2492ccd7 100644 --- a/src/dodal/testing/fixtures/config_client.py +++ b/src/dodal/testing/fixtures/config_client.py @@ -15,8 +15,9 @@ TModel = TypeVar("TModel", bound=ConfigModel) -# Test can register a specific file path with an assoicated file contents to model -# conversion function. +# Tests can register a specific file path with an assoicated file contents to model +# conversion function. This should be moved to daq-config-server. +# See https://github.com/DiamondLightSource/daq-config-server/issues/194 MOCK_CONFIG_CLIENT_PATH_TO_MODEL_CONVERSION: dict[ str, Callable[[str], ConfigModel] ] = {} From 3ff44751b2ed37b83db492975d3c045c6d146a11 Mon Sep 17 00:00:00 2001 From: Oli Wenman Date: Tue, 23 Jun 2026 16:01:33 +0000 Subject: [PATCH 25/35] Update banned_paths to be more flexible --- src/dodal/testing/fixtures/config_client.py | 19 +++++++------- src/dodal/testing/paths.py | 6 ++--- tests/conftest.py | 29 +++++++++------------ tests/devices/test_eiger.py | 3 ++- tests/testing/test_paths.py | 4 ++- 5 files changed, 30 insertions(+), 31 deletions(-) diff --git a/src/dodal/testing/fixtures/config_client.py b/src/dodal/testing/fixtures/config_client.py index 3bd2492ccd7..c7f9e9c9169 100644 --- a/src/dodal/testing/fixtures/config_client.py +++ b/src/dodal/testing/fixtures/config_client.py @@ -22,6 +22,8 @@ str, Callable[[str], ConfigModel] ] = {} +MOCK_CONFIG_CLIENT_BANNED_PATHS: list[Path] = [] + def mock_config_server_get_file_contents( file_path: str | Path, @@ -46,14 +48,13 @@ def mock_config_server_get_file_contents( Returns: T: The contents of the config file. """ - requested_path = Path(file_path) - if requested_path.is_absolute(): - if is_path_banned(requested_path): - raise AssertionError( - f"Attempt to open {requested_path} from inside a unit test" - ) + requested_file = Path(file_path) + if is_path_banned(requested_file, MOCK_CONFIG_CLIENT_BANNED_PATHS): + raise AssertionError( + f"Attempt to open {requested_file} from inside a unit test using ConfigClient" + ) # Minimal logic required for unit tests - with requested_path.open("r") as f: + with requested_file.open("r") as f: contents = f.read() if force_parser: return TypeAdapter(desired_return_type).validate_python(force_parser(contents)) @@ -62,8 +63,8 @@ def mock_config_server_get_file_contents( if desired_return_type is dict: return json.loads(contents) if issubclass(desired_return_type, ConfigModel): - if str(requested_path) in MOCK_CONFIG_CLIENT_PATH_TO_MODEL_CONVERSION: - callable = MOCK_CONFIG_CLIENT_PATH_TO_MODEL_CONVERSION[str(requested_path)] + if str(requested_file) in MOCK_CONFIG_CLIENT_PATH_TO_MODEL_CONVERSION: + callable = MOCK_CONFIG_CLIENT_PATH_TO_MODEL_CONVERSION[str(requested_file)] return callable(contents) # type: ignore return desired_return_type.model_validate(json.loads(contents)) raise ValueError("Invalid return type requested") diff --git a/src/dodal/testing/paths.py b/src/dodal/testing/paths.py index 75105fcc404..3ecde2091e4 100644 --- a/src/dodal/testing/paths.py +++ b/src/dodal/testing/paths.py @@ -1,10 +1,8 @@ from pathlib import Path -BANNED_PATHS = [Path("/dls"), Path("/dls_sw")] - -def is_path_banned(path: Path) -> bool: +def is_path_banned(path: Path, banned_paths: list[Path]) -> bool: resolved = path.resolve() return resolved.is_absolute() and any( - resolved.is_relative_to(banned) for banned in BANNED_PATHS + resolved.is_relative_to(banned) for banned in banned_paths ) diff --git a/tests/conftest.py b/tests/conftest.py index 20b53c40cd5..e85f0e9445b 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -94,35 +94,32 @@ def reset_path_provider(): clear_path_provider() +BANNED_PATHS = [Path("/dls"), Path("/dls_sw")] + + +@pytest.fixture(autouse=True) +def config_client_banned_paths(): + from dodal.testing.fixtures.config_client import MOCK_CONFIG_CLIENT_BANNED_PATHS + + MOCK_CONFIG_CLIENT_BANNED_PATHS.extend(BANNED_PATHS) + + @pytest.fixture(autouse=True) def patch_open_to_prevent_dls_reads_in_tests(): - real_open = open + unpatched_open = open def patched_open(*args, **kwargs): requested_path = Path(args[0]) - if is_path_banned(requested_path): + if is_path_banned(requested_path, BANNED_PATHS): raise AssertionError( f"Attempt to open {requested_path} from inside a unit test" ) - return real_open(*args, **kwargs) + return unpatched_open(*args, **kwargs) with patch("builtins.open", side_effect=patched_open): yield [] -@pytest.fixture(autouse=True) -def block_dls_access_in_config_client(): - - def patch_get_file_contents(self, file_path, *args, **kwargs): - path = Path(file_path) - if is_path_banned(path): - raise AssertionError(f"Forbidden config access blocked in test: {path}") - return ConfigClient.get_file_contents(self, file_path, *args, **kwargs) - - with patch.object(ConfigClient, "get_file_contents", new=patch_get_file_contents): - yield - - def pytest_runtest_setup(item): beamline_utils.clear_devices() if LOGGER.handlers == []: diff --git a/tests/devices/test_eiger.py b/tests/devices/test_eiger.py index 8b9aff8cb99..95a8d47fb79 100644 --- a/tests/devices/test_eiger.py +++ b/tests/devices/test_eiger.py @@ -4,6 +4,7 @@ from unittest.mock import ANY, MagicMock, Mock, call, create_autospec, patch import pytest +from daq_config_server import ConfigClient from ophyd.sim import NullStatus, make_fake_device from ophyd.status import Status from ophyd.utils import UnknownStatusFailure @@ -31,7 +32,7 @@ class StatusError(Exception): @pytest.fixture(autouse=True) -def always_set_config_client(mock_config_client): +def always_set_config_client(mock_config_client: ConfigClient): set_config_client(mock_config_client) diff --git a/tests/testing/test_paths.py b/tests/testing/test_paths.py index d1b8dcc3b57..7e3c7123d53 100644 --- a/tests/testing/test_paths.py +++ b/tests/testing/test_paths.py @@ -4,6 +4,8 @@ from dodal.testing.paths import is_path_banned +BANNED_PATHS = [Path("/dls"), Path("/dls_sw")] + @pytest.mark.parametrize( "path, expected_result", @@ -15,4 +17,4 @@ ], ) def test_is_path_banned(path: Path, expected_result: bool) -> None: - assert is_path_banned(path) is expected_result + assert is_path_banned(path, BANNED_PATHS) is expected_result From b59d756d9de1eb320fe450bf84f1b4460076801d Mon Sep 17 00:00:00 2001 From: Oli Wenman Date: Tue, 23 Jun 2026 16:05:15 +0000 Subject: [PATCH 26/35] Set timeout back to 1, increase timout for device creation test only --- pyproject.toml | 2 +- tests/common/beamlines/test_device_instantiation.py | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/pyproject.toml b/pyproject.toml index 419049c09db..af59ab87c5e 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -115,7 +115,7 @@ reportMissingImports = false # Ignore missing stubs in imported modules [tool.pytest.ini_options] # Run pytest with all our checkers asyncio_mode = "auto" -timeout = 1.5 +timeout = 1 timeout_method = "signal" # Alllow us to see full stack trace of where it timed out to make debugging easier. markers = [ "requires: marks tests as requiring other infrastructure", diff --git a/tests/common/beamlines/test_device_instantiation.py b/tests/common/beamlines/test_device_instantiation.py index 4f3890b4143..9c14a6b21d8 100644 --- a/tests/common/beamlines/test_device_instantiation.py +++ b/tests/common/beamlines/test_device_instantiation.py @@ -17,6 +17,9 @@ def follows_bluesky_protocols(obj: Any) -> bool: set(all_beamline_modules()), indirect=True, ) +# Increase timeout for this specific test as running inividually can take over a second +# to import everything so causes tests to fail. +@pytest.mark.timeout(2) def test_device_creation(module_and_devices_for_beamline): """Ensures that for every beamline all device factories are using valid args and creating types that conform to Bluesky protocols. From 44dc01aa9f52ea53f1fcd4ab566811bbb62e16f2 Mon Sep 17 00:00:00 2001 From: Oli Wenman Date: Wed, 24 Jun 2026 09:06:06 +0000 Subject: [PATCH 27/35] Update dodal banned paths to include Path.open. Now blocks mock ConfigClient --- src/dodal/testing/fixtures/config_client.py | 8 ---- src/dodal/testing/paths.py | 8 ---- tests/conftest.py | 53 +++++++++++++-------- tests/testing/test_paths.py | 20 -------- 4 files changed, 33 insertions(+), 56 deletions(-) delete mode 100644 src/dodal/testing/paths.py delete mode 100644 tests/testing/test_paths.py diff --git a/src/dodal/testing/fixtures/config_client.py b/src/dodal/testing/fixtures/config_client.py index c7f9e9c9169..f9c2a8c326e 100644 --- a/src/dodal/testing/fixtures/config_client.py +++ b/src/dodal/testing/fixtures/config_client.py @@ -9,8 +9,6 @@ from daq_config_server.models import ConfigModel from pydantic import TypeAdapter -from dodal.testing.paths import is_path_banned - T = TypeVar("T", str, dict, ConfigModel) TModel = TypeVar("TModel", bound=ConfigModel) @@ -22,8 +20,6 @@ str, Callable[[str], ConfigModel] ] = {} -MOCK_CONFIG_CLIENT_BANNED_PATHS: list[Path] = [] - def mock_config_server_get_file_contents( file_path: str | Path, @@ -49,10 +45,6 @@ def mock_config_server_get_file_contents( T: The contents of the config file. """ requested_file = Path(file_path) - if is_path_banned(requested_file, MOCK_CONFIG_CLIENT_BANNED_PATHS): - raise AssertionError( - f"Attempt to open {requested_file} from inside a unit test using ConfigClient" - ) # Minimal logic required for unit tests with requested_file.open("r") as f: contents = f.read() diff --git a/src/dodal/testing/paths.py b/src/dodal/testing/paths.py deleted file mode 100644 index 3ecde2091e4..00000000000 --- a/src/dodal/testing/paths.py +++ /dev/null @@ -1,8 +0,0 @@ -from pathlib import Path - - -def is_path_banned(path: Path, banned_paths: list[Path]) -> bool: - resolved = path.resolve() - return resolved.is_absolute() and any( - resolved.is_relative_to(banned) for banned in banned_paths - ) diff --git a/tests/conftest.py b/tests/conftest.py index e85f0e9445b..021494b06c1 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -25,7 +25,6 @@ from dodal.devices.detector import DetectorParams from dodal.devices.detector.det_dim_constants import EIGER2_X_16M_SIZE from dodal.log import LOGGER, GELFTCPHandler, set_up_all_logging_handlers -from dodal.testing.paths import is_path_banned from dodal.utils import ( DeviceInitializationController, collect_factories, @@ -77,10 +76,11 @@ }, } +BANNED_PATHS = [Path("/dls"), Path("/dls_sw")] environ["DODAL_TEST_MODE"] = "true" -# Add run_engine and util fixtures to be used in tests +# Add run_engine, util fixtures, and mock_config_client to be used in tests pytest_plugins = [ "dodal.testing.fixtures.run_engine", "dodal.testing.fixtures.utils", @@ -88,36 +88,44 @@ ] +def is_path_banned(path: Path, banned_paths: list[Path]) -> bool: + resolved = path.resolve() + return resolved.is_absolute() and any( + resolved.is_relative_to(banned) for banned in banned_paths + ) + + @pytest.fixture(autouse=True) def reset_path_provider(): yield clear_path_provider() -BANNED_PATHS = [Path("/dls"), Path("/dls_sw")] - - -@pytest.fixture(autouse=True) -def config_client_banned_paths(): - from dodal.testing.fixtures.config_client import MOCK_CONFIG_CLIENT_BANNED_PATHS - - MOCK_CONFIG_CLIENT_BANNED_PATHS.extend(BANNED_PATHS) - - @pytest.fixture(autouse=True) def patch_open_to_prevent_dls_reads_in_tests(): unpatched_open = open + unpatched_path_open = Path.open - def patched_open(*args, **kwargs): - requested_path = Path(args[0]) + def check_path(path: str | Path): + requested_path = Path(path) if is_path_banned(requested_path, BANNED_PATHS): raise AssertionError( f"Attempt to open {requested_path} from inside a unit test" ) - return unpatched_open(*args, **kwargs) - with patch("builtins.open", side_effect=patched_open): - yield [] + def patched_open(file, *args, **kwargs): + check_path(file) + return unpatched_open(file, *args, **kwargs) + + def patched_path_open(self: Path, *args, **kwargs): + check_path(self) + return unpatched_path_open(self, *args, **kwargs) + + with ( + patch("builtins.open", side_effect=patched_open), + patch.object(Path, "open", patched_path_open), + ): + yield def pytest_runtest_setup(item): @@ -157,8 +165,8 @@ async def static_path_provider( def module_and_devices_for_beamline( mock_config_client: ConfigClient, request: pytest.FixtureRequest ): - # Swap all ConfigClient instances with the mock one method. - # Prevents dls paths used, open local files rather than from service. + # Swap all ConfigClient get_file_contents instances with the mock one method. + # Open local files for tests rather than from service. original_config_client = ConfigClient.get_file_contents ConfigClient.get_file_contents = mock_config_client.get_file_contents # type: ignore @@ -188,7 +196,6 @@ def module_and_devices_for_beamline( factory.cache_clear() del bl_mod - clear_config_client() # Set back the ConfigClient instance with the original one. ConfigClient.get_file_contents = original_config_client @@ -215,3 +222,9 @@ def eiger_params(tmp_path: Path) -> DetectorParams: det_dist_to_beam_converter_path=TEST_LUT_TXT, detector_size_constants=EIGER2_X_16M_SIZE.det_type_string, # type: ignore ) + + +@pytest.fixture(autouse=True) +def reset_config_client(): + yield + clear_config_client() diff --git a/tests/testing/test_paths.py b/tests/testing/test_paths.py deleted file mode 100644 index 7e3c7123d53..00000000000 --- a/tests/testing/test_paths.py +++ /dev/null @@ -1,20 +0,0 @@ -from pathlib import Path - -import pytest - -from dodal.testing.paths import is_path_banned - -BANNED_PATHS = [Path("/dls"), Path("/dls_sw")] - - -@pytest.mark.parametrize( - "path, expected_result", - [ - (Path("/dls/iXX"), True), - (Path("/dls_sw/iXX"), True), - (Path("relative/path"), False), - (Path("/absolute/path"), False), - ], -) -def test_is_path_banned(path: Path, expected_result: bool) -> None: - assert is_path_banned(path, BANNED_PATHS) is expected_result From 031a6989379226092079e3db8d004c066f24828e Mon Sep 17 00:00:00 2001 From: Oli Wenman Date: Wed, 24 Jun 2026 09:13:19 +0000 Subject: [PATCH 28/35] Increase timeout for test_devices_are_identical --- tests/common/beamlines/test_device_instantiation.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/common/beamlines/test_device_instantiation.py b/tests/common/beamlines/test_device_instantiation.py index 9c14a6b21d8..a21715d4979 100644 --- a/tests/common/beamlines/test_device_instantiation.py +++ b/tests/common/beamlines/test_device_instantiation.py @@ -42,6 +42,9 @@ def test_device_creation(module_and_devices_for_beamline): set(all_beamline_modules()), indirect=True, ) +# Increase timeout for this specific test as running inividually can take over a second +# to import everything so causes tests to fail. +@pytest.mark.timeout(2) def test_devices_are_identical(module_and_devices_for_beamline): """Ensures that for every beamline all device functions prevent duplicate instantiation. From 2883b21a644666da8cada82c01dde90bbdaf18ef Mon Sep 17 00:00:00 2001 From: Oli Wenman Date: Wed, 24 Jun 2026 11:04:43 +0000 Subject: [PATCH 29/35] Add documentation for how to use mock_config_client and how to make test_device_creation pass --- docs/how-to/write-tests.md | 209 ++++++++++++++++++++ src/dodal/beamlines/i09_1_shared.py | 6 +- src/dodal/beamlines/i09_2_shared.py | 11 +- src/dodal/testing/fixtures/config_client.py | 1 - tests/conftest.py | 2 - 5 files changed, 220 insertions(+), 9 deletions(-) diff --git a/docs/how-to/write-tests.md b/docs/how-to/write-tests.md index 73023744c9d..bd6f7895cec 100644 --- a/docs/how-to/write-tests.md +++ b/docs/how-to/write-tests.md @@ -121,6 +121,215 @@ async def test_my_device_read(sim_my_device: MyDevice, run_engine: RunEngine) -> ) ``` +## Mocking `ConfigClient` + +Beamlines and devices may depend on the daq-config-server `ConfigClient` to retrieve configuration files. These files are often stored on the DLS filesystem and contain configuration data that determines device behaviour. + +For example, an insertion device may use a calibration file containing polynomial coefficients that convert photon energy into insertion device gap for different polarisation modes. A device may retrieve this information using a `ConfigClient`: + +```python +from pathlib import Path + +from daq_config_server import ConfigClient + +from dodal.device_manager import DeviceManager +from dodal.devices.my_device import MyDevice + +LOOKUPTABLE_DIR = ( + "/dls_sw/iXX/software/gda/workspace_git/" + "gda-diamond.git/configurations/iXX/lookupTables" +) +LOOKUP_FILE_NAME = "JIDEnergy2GapCalibrations.csv" + +devices = DeviceManager() + + +@devices.fixture +def config_client() -> ConfigClient: + return ConfigClient() + + +@devices.factory() +def my_device(config_client: ConfigClient) -> MyDevice: + return MyDevice( + config_client=config_client, + path=Path(LOOKUPTABLE_DIR, LOOKUP_FILE_NAME), + ) +``` + +### Using the default test fixture + +Unit tests should not communicate with the real daq-config-server or read files from the DLS filesystem. + +Dodal provides a `mock_config_client` fixture in `dodal.testing.fixtures.config_client`. This fixture replaces the real service with a lightweight implementation that reads test files directly from the local filesystem. + +Most tests only need to depend on this fixture: + +```python +import pytest +from pathlib import Path + +from daq_config_server import ConfigClient +from dodal.devices.my_device import MyDevice +from ophyd_async.core import init_devices + + +@pytest.fixture +def my_device(mock_config_client: ConfigClient) -> MyDevice: + with init_devices(mock=True): + return MyDevice( + config_client=mock_config_client, + path=Path("tests/test_data/example_lookup_table.json"), + ) +``` + +### Default file parsing behaviour + +The mocked `ConfigClient.get_file_contents()` supports the same return types commonly used by production code: + +| Requested type | Behaviour | +| ---------------------- | ----------------------------------------------------------------- | +| `str` | Returns the raw file contents | +| `dict` | Parses the file as JSON and returns a dictionary | +| `ConfigModel` subclass | Parses the file as JSON and validates it using `model_validate()` | + +For example, if the requested return type is a `ConfigModel` subclass: + +```python +config_client.get_file_contents( + path, + desired_return_type=MyConfigModel, +) +``` + +the mock implementation will perform: + +```python +MyConfigModel.model_validate(json.loads(contents)) +``` + +### Registering a custom parser + +Some configuration file formats are not JSON-based and require custom parsing logic. For these cases, tests can register a parser using `MOCK_CONFIG_CLIENT_PATH_TO_MODEL_CONVERSION`. + +This registry maps a file path to a function that converts the file contents into a `ConfigModel`. + +For example: + +```python +import pytest +from daq_config_server import ConfigClient +from daq_config_server.models.lookup_tables.insertion_device import ( + parse_i09_hu_undulator_energy_gap_lut, +) + +from dodal.testing.fixtures.config_client import ( + MOCK_CONFIG_CLIENT_PATH_TO_MODEL_CONVERSION, +) +from tests.devices.beamlines.i09_1_shared.test_data import ( + TEST_HARD_UNDULATOR_LUT, +) + + +@pytest.fixture +def mock_config_client(mock_config_client: ConfigClient): + MOCK_CONFIG_CLIENT_PATH_TO_MODEL_CONVERSION[ + TEST_HARD_UNDULATOR_LUT + ] = parse_i09_hu_undulator_energy_gap_lut + + yield mock_config_client + + MOCK_CONFIG_CLIENT_PATH_TO_MODEL_CONVERSION.pop( + TEST_HARD_UNDULATOR_LUT, + None, + ) +``` + +When `get_file_contents()` is called for `TEST_HARD_UNDULATOR_LUT`, the registered parser will be used instead of the default JSON parsing behaviour. + +This mechanism exists as a temporary workaround for configuration formats that are not yet directly supported by daq-config-server. + +## Testing beamline module imports and device creation + +The tests in `tests/common/beamlines/test_device_instantiation.py` validate that every beamline module can be imported and that all device factories successfully construct devices in a test environment. + +During import and device creation, many beamlines load configuration files using either: + +* `ConfigClient.get_file_contents()` +* Module-level path constants that point to files on the DLS filesystem + +For example: + +```python +BEAMLINE_PARAMETERS_PATH = ( + "/dls_sw/i03/software/daq_configuration/json/beamlineParameters.json" +) +``` + +These files are not available in unit test environments and must not be accessed during tests. + +To allow beamline import and device creation tests to run without access to the DLS filesystem, the `module_and_devices_for_beamline` fixture performs two forms of mocking: + +1. Replaces `ConfigClient.get_file_contents()` with the `mock_config_client` implementation. +2. Replaces known module-level file path constants with paths to test data files. + +### Mocking `ConfigClient` + +Before importing the beamline module, the fixture replaces the production implementation: + +```python +ConfigClient.get_file_contents = mock_config_client.get_file_contents +``` + +This ensures that any configuration requested through the daq-config-server client is loaded from local test files rather than the real service. + +### Mocking beamline file paths + +Some beamlines contain module-level constants that directly reference files on the DLS filesystem. These constants are replaced after module import using the `MOCK_ATTRIBUTES_TABLE`. + +For example: + +```python +MOCK_ATTRIBUTES_TABLE = { + "i03": { + "BEAMLINE_PARAMETERS_PATH": TEST_BEAMLINE_PARAMETERS_TXT, + "DAQ_CONFIGURATION_PATH": MOCK_DAQ_CONFIG_PATH, + "ZOOM_PARAMS_FILE": TEST_OAV_ZOOM_LEVELS, + "DISPLAY_CONFIG": TEST_DISPLAY_CONFIG, + }, +} +``` + +When the `i03` beamline is tested, these attributes are overwritten with paths to local test files: + +```python +mock_beamline_module_filepaths("i03", beamline_module) +``` + +This allows device factories to load representative configuration data without accessing the real DLS filesystem. + +### Adding support for a new beamline + +If the device creation tests fail because a beamline attempts to access a file under `/dls` or `/dls_sw`, identify the module attribute containing the path and add an entry to `MOCK_ATTRIBUTES_TABLE`. + +For example: + +```python +MOCK_ATTRIBUTES_TABLE["iXX"] = { + "MY_CONFIGURATION_FILE": TEST_MY_CONFIGURATION_FILE, +} +``` + +The replacement file should be committed to the test suite and contain the minimum configuration required for the beamline to initialise successfully. + +As a rule of thumb: + +* Use `mock_config_client` when configuration is loaded through `ConfigClient`. +* Use `MOCK_ATTRIBUTES_TABLE` when a beamline module contains hard-coded filesystem paths. +* If both mechanisms are used by a beamline, both may need to be configured for the tests to pass. + + + ## Test performance and reliability Dodal has well over 1000 unit tests and developers will run the full unit test suite frequently on their local diff --git a/src/dodal/beamlines/i09_1_shared.py b/src/dodal/beamlines/i09_1_shared.py index 425b876a332..8b31b49d37b 100644 --- a/src/dodal/beamlines/i09_1_shared.py +++ b/src/dodal/beamlines/i09_1_shared.py @@ -33,7 +33,7 @@ @devices.fixture -def config_client() -> ConfigClient: +def iconfig_client() -> ConfigClient: return ConfigClient() @@ -65,12 +65,12 @@ def ienergy_order() -> UndulatorOrder: def iidenergy( ienergy_order: UndulatorOrder, iid: UndulatorInMm, - config_client: ConfigClient, + iconfig_client: ConfigClient, ) -> HardInsertionDeviceEnergy: return HardInsertionDeviceEnergy( undulator_order=ienergy_order, undulator=iid, - config_server=config_client, + config_server=iconfig_client, filepath=LOOK_UPTABLE_FILE, gap_to_energy_func=calculate_energy_i09_hu, energy_to_gap_func=calculate_gap_i09_hu, diff --git a/src/dodal/beamlines/i09_2_shared.py b/src/dodal/beamlines/i09_2_shared.py index 930cd54253d..65927b4df01 100644 --- a/src/dodal/beamlines/i09_2_shared.py +++ b/src/dodal/beamlines/i09_2_shared.py @@ -25,7 +25,6 @@ from dodal.devices.pgm import PlaneGratingMonochromator from dodal.utils import BeamlinePrefix, get_beamline_name -J09_CONF_CLIENT = ConfigClient(url="https://daq-config.diamond.ac.uk") LOOK_UPTABLE_DIR = "/dls_sw/i09-2/software/gda/workspace_git/gda-diamond.git/configurations/i09-2-shared/lookupTables/" GAP_LOOKUP_FILE_NAME = "JIDEnergy2GapCalibrations.csv" PHASE_LOOKUP_FILE_NAME = "JIDEnergy2PhaseCalibrations.csv" @@ -37,6 +36,11 @@ devices = DeviceManager() +@devices.fixture +def jconfig_client() -> ConfigClient: + return ConfigClient(url="https://daq-config.diamond.ac.uk") + + @devices.factory() def psk1() -> HutchShutter: return HutchShutter(K_PREFIX.beamline_prefix) @@ -75,18 +79,19 @@ def jid(jgap: UndulatorGap, jphase: UndulatorPhaseAxes) -> Apple2[UndulatorPhase @devices.factory() def jidcontroller( jid: Apple2[UndulatorPhaseAxes], + jconfig_client: ConfigClient, ) -> Apple2EnforceLHMoveController[UndulatorPhaseAxes]: """J09 insertion device controller.""" return Apple2EnforceLHMoveController[UndulatorPhaseAxes]( apple2=jid, gap_energy_motor_lut=ConfigServerEnergyMotorLookup( lut_config=LookupTableColumnConfig(poly_deg=J09_GAP_POLY_DEG_COLUMNS), - config_client=J09_CONF_CLIENT, + config_client=jconfig_client, path=Path(LOOK_UPTABLE_DIR, GAP_LOOKUP_FILE_NAME), ), phase_energy_motor_lut=ConfigServerEnergyMotorLookup( lut_config=LookupTableColumnConfig(poly_deg=J09_PHASE_POLY_DEG_COLUMNS), - config_client=J09_CONF_CLIENT, + config_client=jconfig_client, path=Path(LOOK_UPTABLE_DIR, PHASE_LOOKUP_FILE_NAME), ), units="keV", diff --git a/src/dodal/testing/fixtures/config_client.py b/src/dodal/testing/fixtures/config_client.py index f9c2a8c326e..f80db8609e6 100644 --- a/src/dodal/testing/fixtures/config_client.py +++ b/src/dodal/testing/fixtures/config_client.py @@ -62,7 +62,6 @@ def mock_config_server_get_file_contents( raise ValueError("Invalid return type requested") -# ToDo - Add documentation for tests to reuse this fixture. @pytest.fixture def mock_config_client() -> ConfigClient: # Don't actually talk to central service during unit tests, and reset caches between test diff --git a/tests/conftest.py b/tests/conftest.py index 021494b06c1..b8031503d74 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -35,8 +35,6 @@ from tests.devices.test_data import TEST_LUT_TXT from tests.test_data import I04_BEAMLINE_PARAMETERS, TEST_BEAMLINE_PARAMETERS_TXT -# ToDo - Make documentation so clearer for beamlines on how to fix tests by mocking the -# paths needed. MOCK_ATTRIBUTES_TABLE: dict[str, dict[str, str]] = { "i03": { "BEAMLINE_PARAMETERS_PATH": TEST_BEAMLINE_PARAMETERS_TXT, From 67372c5e6a412027f639277f5ea14b70a061b8bb Mon Sep 17 00:00:00 2001 From: Oli Wenman Date: Wed, 24 Jun 2026 11:06:58 +0000 Subject: [PATCH 30/35] Update doc --- docs/how-to/write-tests.md | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/docs/how-to/write-tests.md b/docs/how-to/write-tests.md index bd6f7895cec..9c7cfb4c617 100644 --- a/docs/how-to/write-tests.md +++ b/docs/how-to/write-tests.md @@ -315,8 +315,12 @@ If the device creation tests fail because a beamline attempts to access a file u For example: ```python -MOCK_ATTRIBUTES_TABLE["iXX"] = { - "MY_CONFIGURATION_FILE": TEST_MY_CONFIGURATION_FILE, +MOCK_ATTRIBUTES_TABLE= { + ... + "iXX: { + "MY_CONFIGURATION_FILE": TEST_MY_CONFIGURATION_FILE + }, + ... } ``` From 322ea11bb590cad577f4e21afc1bbf39913a2e25 Mon Sep 17 00:00:00 2001 From: Oli Wenman Date: Wed, 24 Jun 2026 11:07:27 +0000 Subject: [PATCH 31/35] formatting --- docs/how-to/write-tests.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/how-to/write-tests.md b/docs/how-to/write-tests.md index 9c7cfb4c617..8266e5e6e7d 100644 --- a/docs/how-to/write-tests.md +++ b/docs/how-to/write-tests.md @@ -315,7 +315,7 @@ If the device creation tests fail because a beamline attempts to access a file u For example: ```python -MOCK_ATTRIBUTES_TABLE= { +MOCK_ATTRIBUTES_TABLE = { ... "iXX: { "MY_CONFIGURATION_FILE": TEST_MY_CONFIGURATION_FILE From bafdbf871f52de6f89332c1d942f00f2315b1f78 Mon Sep 17 00:00:00 2001 From: Oli Wenman Date: Wed, 24 Jun 2026 12:27:59 +0000 Subject: [PATCH 32/35] Add light weight example --- docs/how-to/write-tests.md | 36 +++++++++++++++++++++++++++++++++++- 1 file changed, 35 insertions(+), 1 deletion(-) diff --git a/docs/how-to/write-tests.md b/docs/how-to/write-tests.md index 8266e5e6e7d..f7b710e0f71 100644 --- a/docs/how-to/write-tests.md +++ b/docs/how-to/write-tests.md @@ -157,8 +157,42 @@ def my_device(config_client: ConfigClient) -> MyDevice: ) ``` -### Using the default test fixture +### Directly mocking return values +If a test does not need to exercise file parsing logic, it is often simpler to mock the ConfigClient response directly rather than creating a test file and configuring a parser. + +```python +from unittest.mock import MagicMock + +from daq_config_server.models.lookup_tables.insertion_device import ( + UndulatorEnergyGapLookupTable, +) + +@pytest.fixture +def mock_config_client(): + mock_config_client = MagicMock() + mock_config_client.get_file_contents = MagicMock( + return_value=UndulatorEnergyGapLookupTable( + rows=[ + [5700, 5.4606], + [7000, 6.045], + [9700, 6.404], + ], + ) + ) + return mock_config_client +``` +This approach is appropriate when: + +* The test only cares about the behaviour of the device or component consuming the configuration data. +* The contents of the configuration file are not relevant to the test. +* The parsing logic is tested elsewhere. + +In these cases, mocking the return value directly avoids unnecessary coupling to file formats or test data files. + +Use the below method if you need the data to come from a file for a unit test. + +### Using the mock_config_client fixture to read files Unit tests should not communicate with the real daq-config-server or read files from the DLS filesystem. Dodal provides a `mock_config_client` fixture in `dodal.testing.fixtures.config_client`. This fixture replaces the real service with a lightweight implementation that reads test files directly from the local filesystem. From a9be3bb912411ee96dd1c4c51012d315b7964633 Mon Sep 17 00:00:00 2001 From: Oli Wenman Date: Fri, 26 Jun 2026 09:14:21 +0000 Subject: [PATCH 33/35] Fix spelling --- tests/common/beamlines/test_device_instantiation.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/common/beamlines/test_device_instantiation.py b/tests/common/beamlines/test_device_instantiation.py index a21715d4979..6a1cbeaef5c 100644 --- a/tests/common/beamlines/test_device_instantiation.py +++ b/tests/common/beamlines/test_device_instantiation.py @@ -17,7 +17,7 @@ def follows_bluesky_protocols(obj: Any) -> bool: set(all_beamline_modules()), indirect=True, ) -# Increase timeout for this specific test as running inividually can take over a second +# Increase timeout for this specific test as running individually can take over a second # to import everything so causes tests to fail. @pytest.mark.timeout(2) def test_device_creation(module_and_devices_for_beamline): @@ -42,7 +42,7 @@ def test_device_creation(module_and_devices_for_beamline): set(all_beamline_modules()), indirect=True, ) -# Increase timeout for this specific test as running inividually can take over a second +# Increase timeout for this specific test as running individually can take over a second # to import everything so causes tests to fail. @pytest.mark.timeout(2) def test_devices_are_identical(module_and_devices_for_beamline): From c7f8a723be16ccdb5ac92c141f352b7d5658cdfa Mon Sep 17 00:00:00 2001 From: Oli Wenman Date: Fri, 26 Jun 2026 09:14:37 +0000 Subject: [PATCH 34/35] Remove unused fixture --- system_tests/test_oav_system.py | 9 --------- 1 file changed, 9 deletions(-) diff --git a/system_tests/test_oav_system.py b/system_tests/test_oav_system.py index 9438c72d665..5b5885b42bb 100644 --- a/system_tests/test_oav_system.py +++ b/system_tests/test_oav_system.py @@ -2,7 +2,6 @@ import pytest from bluesky.run_engine import RunEngine from daq_config_server import ConfigClient -from ophyd_async.core import init_devices from tests.devices.oav.test_data import TEST_OAV_ZOOM_LEVELS from dodal.devices.oav.oav_detector import OAV, OAVConfig @@ -14,14 +13,6 @@ TEST_GRID_NUM_BOXES_Y = 6 -@pytest.fixture -async def oav(mock_config_client: ConfigClient) -> OAV: - oav_config = OAVConfig(TEST_OAV_ZOOM_LEVELS, mock_config_client) - async with init_devices(connect=True): - oav = OAV("", config=oav_config, name="oav") - return oav - - def take_snapshot_with_grid(oav: OAV, snapshot_filename, snapshot_directory): yield from bps.abs_set(oav.grid_snapshot.top_left_x, TEST_GRID_TOP_LEFT_X) yield from bps.abs_set(oav.grid_snapshot.top_left_y, TEST_GRID_TOP_LEFT_Y) From 6dae5e3f3c58219a47404fbad36e55ba4a5dec84 Mon Sep 17 00:00:00 2001 From: Oli Wenman Date: Mon, 29 Jun 2026 11:07:27 +0000 Subject: [PATCH 35/35] Suggested changes --- src/dodal/beamlines/i09_2_shared.py | 2 +- src/dodal/beamlines/i10_shared.py | 19 ++++--- src/dodal/beamlines/i21.py | 2 +- src/dodal/beamlines/k07.py | 13 +++-- src/dodal/testing/fixtures/config_client.py | 52 +++++++++++++------ .../beamlines/i09_1_shared/conftest.py | 13 ++--- .../i09_1_shared/test_hard_energy.py | 4 +- .../i09_1_shared/test_undulator_functions.py | 4 ++ 8 files changed, 67 insertions(+), 42 deletions(-) diff --git a/src/dodal/beamlines/i09_2_shared.py b/src/dodal/beamlines/i09_2_shared.py index 65927b4df01..1bf31496610 100644 --- a/src/dodal/beamlines/i09_2_shared.py +++ b/src/dodal/beamlines/i09_2_shared.py @@ -38,7 +38,7 @@ @devices.fixture def jconfig_client() -> ConfigClient: - return ConfigClient(url="https://daq-config.diamond.ac.uk") + return ConfigClient() @devices.factory() diff --git a/src/dodal/beamlines/i10_shared.py b/src/dodal/beamlines/i10_shared.py index 6367c135c95..4e479916ec5 100644 --- a/src/dodal/beamlines/i10_shared.py +++ b/src/dodal/beamlines/i10_shared.py @@ -54,6 +54,11 @@ devices = DeviceManager() +@devices.fixture +def config_client() -> ConfigClient: + return ConfigClient() + + @devices.factory() def synchrotron() -> Synchrotron: return Synchrotron() @@ -86,8 +91,6 @@ def switching_mirror() -> XYZPiezoCollimatingMirror: """ID""" -I10_CONF_CLIENT = ConfigClient(url="https://daq-config.diamond.ac.uk") - LOOK_UPTABLE_DIR = "/dls_sw/i10/software/gda/workspace_git/gda-diamond.git/configurations/i10-shared/lookupTables/" @@ -126,16 +129,16 @@ def idd( @devices.factory() -def idd_controller(idd: I10Apple2) -> I10Apple2Controller: +def idd_controller(idd: I10Apple2, config_client: ConfigClient) -> I10Apple2Controller: """I10 downstream insertion device controller.""" source = Source(column="Source", value="idd") idd_gap_energy_motor_lut = ConfigServerEnergyMotorLookup( - config_client=I10_CONF_CLIENT, + config_client=config_client, lut_config=LookupTableColumnConfig(source=source), path=Path(LOOK_UPTABLE_DIR, DEFAULT_GAP_FILE), ) idd_phase_energy_motor_lut = ConfigServerEnergyMotorLookup( - config_client=I10_CONF_CLIENT, + config_client=config_client, lut_config=LookupTableColumnConfig(source=source), path=Path(LOOK_UPTABLE_DIR, DEFAULT_PHASE_FILE), ) @@ -206,16 +209,16 @@ def idu( @devices.factory() -def idu_controller(idu: I10Apple2) -> I10Apple2Controller: +def idu_controller(idu: I10Apple2, config_client: ConfigClient) -> I10Apple2Controller: """I10 upstream insertion device controller.""" source = Source(column="Source", value="idu") idu_gap_energy_motor_lut = ConfigServerEnergyMotorLookup( - config_client=I10_CONF_CLIENT, + config_client=config_client, lut_config=LookupTableColumnConfig(source=source), path=Path(LOOK_UPTABLE_DIR, DEFAULT_GAP_FILE), ) idu_phase_energy_motor_lut = ConfigServerEnergyMotorLookup( - config_client=I10_CONF_CLIENT, + config_client=config_client, lut_config=LookupTableColumnConfig(source=source), path=Path(LOOK_UPTABLE_DIR, DEFAULT_PHASE_FILE), ) diff --git a/src/dodal/beamlines/i21.py b/src/dodal/beamlines/i21.py index 78396c2491f..dca6873ffc6 100644 --- a/src/dodal/beamlines/i21.py +++ b/src/dodal/beamlines/i21.py @@ -38,7 +38,7 @@ I21_PHASE_POLY_DEG_COLUMNS = ["b"] I21_GRATING_COLUMNS = "Grating" -I21_CONF_CLIENT = ConfigClient(url="https://daq-config.diamond.ac.uk") +I21_CONF_CLIENT = ConfigClient() LOOK_UPTABLE_DIR = "/dls_sw/i21/software/gda/workspace_git/gda-diamond.git/configurations/i21-config/lookupTables/" GAP_LOOKUP_FILE_NAME = "IDEnergy2GapCalibrations.csv" PHASE_LOOKUP_FILE_NAME = "IDEnergy2PhaseCalibrations.csv" diff --git a/src/dodal/beamlines/k07.py b/src/dodal/beamlines/k07.py index b7c27de6610..f7e184316eb 100644 --- a/src/dodal/beamlines/k07.py +++ b/src/dodal/beamlines/k07.py @@ -22,9 +22,6 @@ devices = DeviceManager() - -K07_CONF_CLIENT = ConfigClient(url="https://daq-config.diamond.ac.uk") - LOOK_UPTABLE_DIR = "/dls_sw/k07/software/gda/workspace_git/gda-diamond.git/configurations/k07/lookupTables/" GAP_LOOKUP_FILE_NAME = "JIDEnergy2GapCalibrations.csv" PHASE_LOOKUP_FILE_NAME = "JIDEnergy2PhaseCalibrations.csv" @@ -37,6 +34,11 @@ set_utils_beamline(BL) +@devices.fixture +def config_client() -> ConfigClient: + return ConfigClient() + + @devices.factory() def synchrotron() -> Synchrotron: return Synchrotron() @@ -92,20 +94,21 @@ def id( @devices.factory(skip=True) def id_controller( id: Apple2[UndulatorPhaseAxes], + config_client: ConfigClient, ) -> Apple2EnforceLHMoveController[UndulatorPhaseAxes]: """I21 insertion device controller.""" return Apple2EnforceLHMoveController[UndulatorPhaseAxes]( apple2=id, gap_energy_motor_lut=ConfigServerEnergyMotorLookup( lut_config=LookupTableColumnConfig(grating=K07_GRATING_COLUMNS), - config_client=K07_CONF_CLIENT, + config_client=config_client, path=Path(LOOK_UPTABLE_DIR, GAP_LOOKUP_FILE_NAME), ), phase_energy_motor_lut=ConfigServerEnergyMotorLookup( lut_config=LookupTableColumnConfig( grating=K07_GRATING_COLUMNS, poly_deg=K07_PHASE_POLY_DEG_COLUMNS ), - config_client=K07_CONF_CLIENT, + config_client=config_client, path=Path(LOOK_UPTABLE_DIR, GAP_LOOKUP_FILE_NAME), ), units="eV", diff --git a/src/dodal/testing/fixtures/config_client.py b/src/dodal/testing/fixtures/config_client.py index f80db8609e6..518d728b072 100644 --- a/src/dodal/testing/fixtures/config_client.py +++ b/src/dodal/testing/fixtures/config_client.py @@ -1,5 +1,6 @@ import json from collections.abc import Callable +from functools import partial from pathlib import Path from typing import Any, TypeVar from unittest.mock import MagicMock @@ -11,21 +12,19 @@ T = TypeVar("T", str, dict, ConfigModel) + TModel = TypeVar("TModel", bound=ConfigModel) -# Tests can register a specific file path with an assoicated file contents to model -# conversion function. This should be moved to daq-config-server. -# See https://github.com/DiamondLightSource/daq-config-server/issues/194 -MOCK_CONFIG_CLIENT_PATH_TO_MODEL_CONVERSION: dict[ - str, Callable[[str], ConfigModel] -] = {} + +ConverterDict = dict[str, Callable[[str], ConfigModel]] -def mock_config_server_get_file_contents( +def _mock_config_server_get_file_contents( file_path: str | Path, desired_return_type: type[T] = str, reset_cached_result: bool = True, force_parser: Callable[[str], Any] | None = None, + mock_data_converters: ConverterDict | None = None, ) -> T: """Mocks getting a file from the config server by reading it directly. @@ -37,6 +36,8 @@ def mock_config_server_get_file_contents( force_parser (Callable[[str], Any] | None, optional): Use a certain converter function. Only needed for the interim where the converter exists but the config server has not been redeployed. Defaults to None. + mock_data_converters (ConverterDict | None): If there is a converter in here matching + the file path then that will be used for conversion. Raises: ValueError: Raised if an invalid type is requested @@ -45,30 +46,51 @@ def mock_config_server_get_file_contents( T: The contents of the config file. """ requested_file = Path(file_path) + # Minimal logic required for unit tests with requested_file.open("r") as f: contents = f.read() + if force_parser: return TypeAdapter(desired_return_type).validate_python(force_parser(contents)) + if desired_return_type is str: return contents # type: ignore + if desired_return_type is dict: return json.loads(contents) + if issubclass(desired_return_type, ConfigModel): - if str(requested_file) in MOCK_CONFIG_CLIENT_PATH_TO_MODEL_CONVERSION: - callable = MOCK_CONFIG_CLIENT_PATH_TO_MODEL_CONVERSION[str(requested_file)] + if not mock_data_converters: + mock_data_converters = {} + + if str(requested_file) in mock_data_converters: + callable = mock_data_converters[str(requested_file)] return callable(contents) # type: ignore + return desired_return_type.model_validate(json.loads(contents)) - raise ValueError("Invalid return type requested") + raise ValueError(f"Invalid return type requested: {desired_return_type}") -@pytest.fixture -def mock_config_client() -> ConfigClient: + +def _mock_config_client_factory(mock_data_converters: ConverterDict) -> ConfigClient: # Don't actually talk to central service during unit tests, and reset caches between test mock_config_client = ConfigClient() mock_config_client.get_file_contents = MagicMock(spec=["get_file_contents"]) - - mock_config_client.get_file_contents.side_effect = ( - mock_config_server_get_file_contents + mock_config_client.get_file_contents.side_effect = partial( + _mock_config_server_get_file_contents, + mock_data_converters=mock_data_converters, ) return mock_config_client + + +@pytest.fixture +def mock_config_client() -> ConfigClient: + return _mock_config_client_factory({}) + + +@pytest.fixture +def mock_config_client_with_data( + mock_config_client_data: ConverterDict, +) -> ConfigClient: + return _mock_config_client_factory(mock_config_client_data) diff --git a/tests/devices/beamlines/i09_1_shared/conftest.py b/tests/devices/beamlines/i09_1_shared/conftest.py index 69e0e64d642..3ec19a71153 100644 --- a/tests/devices/beamlines/i09_1_shared/conftest.py +++ b/tests/devices/beamlines/i09_1_shared/conftest.py @@ -1,19 +1,12 @@ import pytest -from daq_config_server import ConfigClient from daq_config_server.models.lookup_tables.insertion_device import ( parse_i09_hu_undulator_energy_gap_lut, ) -from dodal.testing.fixtures.config_client import ( - MOCK_CONFIG_CLIENT_PATH_TO_MODEL_CONVERSION, -) +from dodal.testing.fixtures.config_client import ConverterDict from tests.devices.beamlines.i09_1_shared.test_data import TEST_HARD_UNDULATOR_LUT @pytest.fixture -def mock_config_client(mock_config_client: ConfigClient): - MOCK_CONFIG_CLIENT_PATH_TO_MODEL_CONVERSION[TEST_HARD_UNDULATOR_LUT] = ( - parse_i09_hu_undulator_energy_gap_lut - ) - yield mock_config_client - MOCK_CONFIG_CLIENT_PATH_TO_MODEL_CONVERSION.pop(TEST_HARD_UNDULATOR_LUT, None) +def mock_config_client_data() -> ConverterDict: + return {TEST_HARD_UNDULATOR_LUT: parse_i09_hu_undulator_energy_gap_lut} diff --git a/tests/devices/beamlines/i09_1_shared/test_hard_energy.py b/tests/devices/beamlines/i09_1_shared/test_hard_energy.py index 3c15089210e..6df8bb0d510 100644 --- a/tests/devices/beamlines/i09_1_shared/test_hard_energy.py +++ b/tests/devices/beamlines/i09_1_shared/test_hard_energy.py @@ -47,7 +47,7 @@ async def undulator_in_mm() -> UndulatorInMm: @pytest.fixture async def hu_id_energy( - mock_config_client: ConfigClient, + mock_config_client_with_data: ConfigClient, undulator_order: UndulatorOrder, undulator_in_mm: UndulatorInMm, ) -> HardInsertionDeviceEnergy: @@ -55,7 +55,7 @@ async def hu_id_energy( hu_id_energy = HardInsertionDeviceEnergy( undulator_order=undulator_order, undulator=undulator_in_mm, - config_server=mock_config_client, + config_server=mock_config_client_with_data, filepath=TEST_HARD_UNDULATOR_LUT, gap_to_energy_func=calculate_energy_i09_hu, energy_to_gap_func=calculate_gap_i09_hu, diff --git a/tests/devices/beamlines/i09_1_shared/test_undulator_functions.py b/tests/devices/beamlines/i09_1_shared/test_undulator_functions.py index d67fb674108..0bd400ab86b 100644 --- a/tests/devices/beamlines/i09_1_shared/test_undulator_functions.py +++ b/tests/devices/beamlines/i09_1_shared/test_undulator_functions.py @@ -4,6 +4,9 @@ import pytest from daq_config_server import ConfigClient from daq_config_server.models.lookup_tables import GenericLookupTable +from daq_config_server.models.lookup_tables.insertion_device import ( + parse_i09_hu_undulator_energy_gap_lut, +) from dodal.devices.beamlines.i09_1_shared import ( calculate_energy_i09_hu, @@ -20,6 +23,7 @@ def lut( file_path=TEST_HARD_UNDULATOR_LUT, desired_return_type=GenericLookupTable, reset_cached_result=True, + force_parser=parse_i09_hu_undulator_energy_gap_lut, )