Improve golden master tests by MJGaughran · Pull Request #59 · DiamondLightSource/deploy-tools

MJGaughran · 2026-06-29T12:27:25Z

This now exercises multiple stages of the module lifecycle, such as deprecation, removal, restore etc.

This includes the refactor to use a single DeployToolsError, since that was used in the code for patching out the apptainer pull from the lifecycle test. Other code changes (other than applying consistent type hints) have been reserved for the PR that will include the CLI / command testing.

There are a lot of changed files since we have all the different configuration files and generated output. I have tried to name the modules to indicate their purpose, and they have been stripped down from the originals.

You'll probably want to minimise the tests/configs & tests/samples to start rather than read all "Files changed" top-to-bottom.

No longer uses the demo_configuration folder for test configuration.

This includes sorting output to ensure it is deterministic. The 'file' utility has been installed to allow tests to run on the devcontainer.

These are caught in the main() function and printed without traceback to reduce clutter in failed runs.

This removes the need to depend on an image pull in CI.

These were stripped down for the golden master tests, but those are now using separate configuration.

codecov · 2026-06-29T12:28:19Z

Codecov Report

❌ Patch coverage is 73.33333% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.59%. Comparing base (093ffa8) to head (22028aa).

Files with missing lines	Patch %	Lines
src/deploy_tools/external_tools.py	46.15%	7 Missing ⚠️
src/deploy_tools/validate.py	50.00%	3 Missing ⚠️
src/deploy_tools/__main__.py	71.42%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #59      +/-   ##
==========================================
+ Coverage   75.51%   83.59%   +8.08%     
==========================================
  Files          25       27       +2     
  Lines         931      957      +26     
==========================================
+ Hits          703      800      +97     
+ Misses        228      157      -71

☔ 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.

ptsOSL · 2026-06-29T19:37:15Z

+    except FileNotFoundError as exc:
+        raise ExternalToolError(
+            f"Required external tool not found: '{executable}'.\n"
+            f"Ensure it is installed and available on PATH."
+        ) from exc


Does this exception only get raised when the tool itself cant be found, rather than if the tool fails to find a file in cases where the tool handles files/directories? I guess if the tool failed to find a file, it would return non zero which would raise a CalledProcessError

As far as I can tell it only raises FileNotFoundError when the command itself cannot be run.

ptsOSL · 2026-06-29T19:40:31Z

@@ -39,7 +42,9 @@ def _print_version_updates(
    module_names = old_defaults.keys() | new_defaults.keys()


It might clarify things to type hint this as a set, this would also be consistent with update_messages being type hinted.

ptsOSL · 2026-06-29T19:50:21Z

+    # Sort for stable output: module_names is a set, whose iteration order depends on
+    # PYTHONHASHSEED.
+    for name in sorted(module_names):


This is definitely very nitpicky and you don't need to change anything, but this comment seems a bit over the top. It is explaining how a sets order is defined which isn't really relevant. All it needs to say, is something like "sort unordered set to give reproducible order". I wonder if this is Claude being overly verbose?

Pretty much every single comment is trimmed down from perhaps triple the original output. The only way to avoid the spam is to use Claude on a lower effort level, which often results in basic logic errors.

I agree it's OTT in this instance, I'll type hint it as set[str] above, and skip this comment entirely.

ptsOSL · 2026-06-29T20:25:01Z

+PATH_TO_SCHEMAS = (
+    Path(__file__).parent.parent / "src" / "deploy_tools" / "models" / "schemas"
+)


Is this path being used?

This will be deleted

ptsOSL · 2026-06-30T10:07:03Z

+# yaml-language-server: $schema=/workspaces/deploy-tools/src/deploy_tools/models/schemas/deployment-settings.json
+
+default_versions: {}


Are there any tests of modifying the default versions as defined in this file?

MichaelStubbings · 2026-06-30T11:10:39Z

This file has been added in preparation for the next test, but given that test two was 'added' and this test is 'updated' it is a little confusing.

Could version 2.0 be deprecated instead?

I see that you are also testing the version order to make sure 2.0 takes precedence over 2.1rc1. Another thought is that you could add this file in the initial deployment and test that behaviour there. Or at least in test 2 where it can be justified as being 'added'.

updatable will be added from 01-initial, and both new versions will be added in 02-added

Instead, we clearly indicate that the type is a set (and that therefore sorting is necessary).

The 'updatable' module has now been added from the start, and both new versions are added in 02-added. The test description has been updated to reflect these changes.

MJGaughran added 16 commits June 26, 2026 15:50

Trim down golden master tests

f54bc5b

Split tests into separate files

f061f99

Cover module deprecation and removal in golden master

8f987c1

No longer uses the demo_configuration folder for test configuration.

Cover incremental module creation in golden master tests

1ea0ae0

Cover module update and restore in golden master tests

65071d6

Add golden-master tests for the validate command

971af9c

This includes sorting output to ensure it is deterministic. The 'file' utility has been installed to allow tests to run on the devcontainer.

Use google docstring format for tests

b20d8f8

Use single DeployToolsError as base class for actionable errors

e3b8131

These are caught in the main() function and printed without traceback to reduce clutter in failed runs.

Stub the external call to apptainer in tests

f175bff

This removes the need to depend on an image pull in CI.

Wrap external tool calls to improve error messages and error handling

62f9051

Add ANN to ruff for consistent type hinting

c3c3176

Use tmp_path for all test builds to make parallel-safe

11a3474

Reorganise golden master test configuration for consistency

9bd07d4

Extract common helper for golden master test stages

61614a1

Refactor golden master tests for improved structure and naming

9ab34d0

Revert demo configuration to original files

fb387c5

These were stripped down for the golden master tests, but those are now using separate configuration.

MJGaughran requested review from MichaelStubbings and ptsOSL June 29, 2026 12:29

ptsOSL reviewed Jun 30, 2026

View reviewed changes

Comment thread tests/configs/golden-master/01-initial/apps/0.1.yaml

MichaelStubbings reviewed Jun 30, 2026

View reviewed changes

MJGaughran added 3 commits June 30, 2026 13:38

Remove unused schema paths variable from tests

b751132

Remove comment in print_updates.py that refers to PYTHONHASHSEED

591326d

Instead, we clearly indicate that the type is a set (and that therefore sorting is necessary).

Rework golden master tests to ensure each stage name is accurate

22028aa

The 'updatable' module has now been added from the start, and both new versions are added in 02-added. The test description has been updated to reflect these changes.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Improve golden master tests#59

Improve golden master tests#59
MJGaughran wants to merge 19 commits into
mainfrom
improve-golden-master-tests

MJGaughran commented Jun 29, 2026 •

edited

Loading

Uh oh!

codecov Bot commented Jun 29, 2026 •

edited

Loading

Uh oh!

ptsOSL Jun 29, 2026

Uh oh!

MJGaughran Jun 30, 2026

Uh oh!

ptsOSL Jun 29, 2026

Uh oh!

ptsOSL Jun 29, 2026

Uh oh!

MJGaughran Jun 30, 2026

Uh oh!

Uh oh!

ptsOSL Jun 29, 2026

Uh oh!

MJGaughran Jun 30, 2026

Uh oh!

ptsOSL Jun 30, 2026

Uh oh!

Uh oh!

MichaelStubbings Jun 30, 2026

Uh oh!

MJGaughran Jun 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		@@ -39,7 +42,9 @@ def _print_version_updates(
		module_names = old_defaults.keys() \| new_defaults.keys()

		# yaml-language-server: $schema=/workspaces/deploy-tools/src/deploy_tools/models/schemas/deployment-settings.json

		default_versions: {}

Uh oh!

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

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!

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!

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

MJGaughran commented Jun 29, 2026 •

edited

Loading

codecov Bot commented Jun 29, 2026 •

edited

Loading