Standardise config client and fix tests dependent on order they are ran by oliwenmandiamond · Pull Request #2099 · DiamondLightSource/dodal

oliwenmandiamond · 2026-06-22T13:15:33Z

Fixes #2098

Instructions to reviewer on how to test:

Run a few dodal/tests/common/beamlines/test_device_instantiation.py::test_device_creation[iXX] in main branch and this branch. Confirm they now pass and doesn't depend on order.
Confirm tests pass
Confirm added documentation makes sense

Checks for reviewer

Would the PR title make sense to a scientist on a set of release notes
If a new device has been added does it follow the standards
If changing the API for a pre-existing device, ensure that any beamlines using this device have updated their Bluesky plans accordingly
Have the connection tests for the relevant beamline(s) been run via dodal connect ${BEAMLINE}

…ne energy

… parse it

codecov · 2026-06-23T13:57:31Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.15%. Comparing base (c101e17) to head (bafdbf8).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2099   +/-   ##
=======================================
  Coverage   99.15%   99.15%           
=======================================
  Files         346      346           
  Lines       13536    13541    +5     
=======================================
+ Hits        13422    13427    +5     
  Misses        114      114

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

…gClient

…est_device_creation pass

DominicOram

Thanks, I couldn't reproduce the flaky tests issue but I think this cleans things up a bit anyway. Couple of spelling mistakes, I would suggest installing https://marketplace.visualstudio.com/items?itemName=streetsidesoftware.code-spell-checker

DominicOram · 2026-06-25T17:39:25Z


+@devices.fixture
+def iconfig_client() -> ConfigClient:
+    return ConfigClient()


Could: In other beamlines this fixture then calls set_config_client. I know it's not strictly required as you're not relying on get_config_client() but I would consider doing it here (and the other beamlines you've changed) anyway as you may later expect it there

DominicOram · 2026-06-25T17:40:15Z


+@devices.fixture
+def jconfig_client() -> ConfigClient:
+    return ConfigClient(url="https://daq-config.diamond.ac.uk")


Could: This is the default URL, why are we specifying it?

DominicOram · 2026-06-25T17:45:56Z

 @pytest.fixture
-async def oav() -> OAV:
-    oav_config = OAVConfig(TEST_OAV_ZOOM_LEVELS, ConfigClient(""))
+async def oav(mock_config_client: ConfigClient) -> OAV:


Should: This fixture isn't actually used anywhere, just remove it.

DominicOram · 2026-06-25T17:48:38Z

 # 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):


This test is failing on main anyway - I've made #2108.

DominicOram · 2026-06-25T17:52:02Z


-def fake_config_server_get_file_contents(
-    filepath: str | Path,
+# Tests can register a specific file path with an assoicated file contents to model


Suggested change

# Tests can register a specific file path with an assoicated file contents to model

# Tests can register a specific file path with an associated file contents to model

DominicOram · 2026-06-25T18:26:34Z

+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),
+    )
+```
+


Should: I like this bit of documentation but it doesn't belong in the "how to write tests" document. It's more about how to use the config server

DominicOram · 2026-06-25T18:27:56Z

+
+### 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`.


Should: If you accept my suggestion this documentation needs changing a bit

DominicOram · 2026-06-25T18:32:14Z

    set(all_beamline_modules()),
    indirect=True,
 )
+# Increase timeout for this specific test as running inividually can take over a second


Suggested change

# 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

DominicOram · 2026-06-25T18:32:21Z

    set(all_beamline_modules()),
    indirect=True,
 )
+# Increase timeout for this specific test as running inividually can take over a second


Suggested change

# 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

DominicOram · 2026-06-25T18:37:56Z

+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`


Could: I'm not sure why we need to document this? Why does someone writing new tests need to know about it?

noemifrisina and others added 15 commits June 1, 2026 16:26

Add first version of an access controlled device to change the beamli…

9c80369

…ne energy

Instantiate device on i19-1 and i19-2 and tidy up a bit

7348647

Add tests

4b9a6d7

Add energy ranges to comment

43ce6c7

Update docstring

76f3566

Make the exception a ValueError

d278017

Merge branch 'main' into 2075_access-controlled-energy-device

536790f

Merge branch 'main' into 2075_access-controlled-energy-device

32dd0fb

Read the energy range bounds from a file and use the config server to…

9200db4

… parse it

Add small bit to test

1f808fd

And fix failing one

1d368eb

Try to fix failure in device instantiation test

b91cbcf

Use correct config path for optics as daq-config-server released

c38e7c9

And update all other tests by consequence maybe

0e32204

Standardise Daq config client in tests

9dd1171

oliwenmandiamond changed the title ~~Standardise config server in tests~~ Standardise config client in tests Jun 22, 2026

oliwenmandiamond added 8 commits June 22, 2026 13:16

Merge branch 'main' into standardise_config_server_in_tests

f700f5c

Fix all tests

5a22ffd

Update config_client to be more reusable

eb6c003

Add doc comment

f25e972

Add mock_config_client to system tests

d965932

Update comment

a486444

Merge branch 'main' into standardise_config_server_in_tests

b7145a9

Remove duplicate code

ea4ef4d

oliwenmandiamond added 5 commits June 23, 2026 14:08

Add missing test

2a77061

Simplify is_banned

a4c556a

Remove duplicate function

752f779

Add a hook for tests to customise return value for daq config server

8b2f472

Update comments

7d2e3a3

oliwenmandiamond added 8 commits June 23, 2026 16:01

Update banned_paths to be more flexible

3ff4475

Set timeout back to 1, increase timout for device creation test only

b59d756

Update dodal banned paths to include Path.open. Now blocks mock Confi…

44dc01a

…gClient

Increase timeout for test_devices_are_identical

031a698

Add documentation for how to use mock_config_client and how to make t…

2883b21

…est_device_creation pass

Update doc

67372c5

formatting

322ea11

Add light weight example

bafdbf8

oliwenmandiamond changed the title ~~Standardise config client in tests~~ Standardise config client and fix tests dependent on order they are ran Jun 24, 2026

oliwenmandiamond marked this pull request as ready for review June 24, 2026 12:30

oliwenmandiamond requested a review from a team as a code owner June 24, 2026 12:30

tpoliaw requested a review from DominicOram June 25, 2026 13:37

DominicOram requested changes Jun 25, 2026

View reviewed changes

oliwenmandiamond mentioned this pull request Jun 26, 2026

Add mock mode for ConfigClient DiamondLightSource/daq-config-server#195

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Standardise config client and fix tests dependent on order they are ran#2099

Standardise config client and fix tests dependent on order they are ran#2099
oliwenmandiamond wants to merge 36 commits into
mainfrom
standardise_config_server_in_tests

oliwenmandiamond commented Jun 22, 2026 •

edited

Loading

Uh oh!

codecov Bot commented Jun 23, 2026 •

edited

Loading

Uh oh!

DominicOram left a comment

Uh oh!

DominicOram Jun 25, 2026

Uh oh!

DominicOram Jun 25, 2026

Uh oh!

DominicOram Jun 25, 2026

Uh oh!

DominicOram Jun 25, 2026

Uh oh!

DominicOram Jun 25, 2026

Uh oh!

DominicOram Jun 25, 2026

Uh oh!

DominicOram Jun 25, 2026

Uh oh!

DominicOram Jun 25, 2026

Uh oh!

DominicOram Jun 25, 2026

Uh oh!

DominicOram Jun 25, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	# Tests can register a specific file path with an assoicated file contents to model
	# Tests can register a specific file path with an associated file contents to model


		### 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`.

	# 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

Uh oh!

Conversation

oliwenmandiamond commented Jun 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Instructions to reviewer on how to test:

Checks for reviewer

Uh oh!

codecov Bot commented Jun 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

DominicOram left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

oliwenmandiamond commented Jun 22, 2026 •

edited

Loading

codecov Bot commented Jun 23, 2026 •

edited

Loading