Draft: Add command behaviour tests by MJGaughran · Pull Request #60 · DiamondLightSource/deploy-tools

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

These test the parts of functionality that are not adequately covered by the golden master tests, such as errors.

Code changes are added for:

Consistency in error handling
Removal of dead code (to identify coverage issues, indicated with comments)
Changed from_scratch -> allow_all in a function where original naming was incorrect

codecov · 2026-06-29T12:39:34Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.48%. Comparing base (22028aa) to head (df673cc).

Additional details and impacted files

@@                       Coverage Diff                        @@
##           improve-golden-master-tests      #60       +/-   ##
================================================================
+ Coverage                        83.59%   99.48%   +15.89%     
================================================================
  Files                               27       27               
  Lines                              957      977       +20     
================================================================
+ Hits                               800      972      +172     
+ Misses                             157        5      -152

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

MJGaughran · 2026-06-29T12:50:34Z

    DEFAULT_VERSION_FILENAME = ".version"

    DEPLOYMENT_SNAPSHOT_FILENAME = "deployment.yaml"
-    PREVIOUS_DEPLOYMENT_SNAPSHOT_FILENAME = "previous-deployment.yaml"


Dead code (now uses git refs)

MJGaughran · 2026-06-29T12:52:00Z


    It should be the Module's name and version as /<config_folder>/<name>/<version>.yaml
    """
-    if version_path.is_dir() and version_path.suffix == YAML_FILE_SUFFIX:


Dead code as _load_release() already checks if path.is_dir()

This covers cases that aren't easily tested with golden master tests.

Tests relating to non-existent output dir use the compare function as otherwise the Typer configuration prevents the exception being raised.

This feature was replaced with the git ref option to the compare command.

ptsOSL · 2026-07-03T11:49:44Z

Is the target branch supposed to be improve-golden-master-tests?

ptsOSL

I've reviewed it as best I can. I will say there are over 1300 lines of code in this PR and I was losing concentration a bit towards the end. It might have been better to split up some of the tests into their own PRs.

ptsOSL · 2026-07-03T10:46:51Z

+                f"Deployment snapshot not found at git ref:\n{ref}"
+            ) from exc
+
+        with io.BytesIO(ref_snapshot.data_stream.read()) as snapshot_f:  # type: ignore


Why does this need # type: ignore ?

Although GitPython has type hints, it appears gitdb (a dependency) does not. I'll add a minor edit to remove the type ignore statement.

This is also just an indentation change.

ptsOSL · 2026-07-03T10:57:13Z

+    with repo:
+        logger.info("Creating snapshot")
+        create_snapshot(deployment, layout)


Why is this "with" needed now when it wasnt before? Does it actually do anything here?

It's used in Python for context managers, and allows us to automatically handle the closing of the object. I'll probably split off the code improvements as it seems there were a few too many in this PR.

I understand that its to handle context, I just havnt seen it used in a situation like this where the "repo" object has already been initialised earlier and already exists in the outer context.

Is the with section just opening a connection using the already created Repo instance and then after the "with" section, is the connection is closed? I would have expected something more like
with repo.open_connetion() as con

It is just guaranteeing that repo.close() is called whenever you leave that block. The reason it can't do it the way you're describing is that the initialisation has an if/else block

I've looked at the options, and it seems this is the more common approach. You can, of course, create a function like "init_or_load_existing_repo" but that obviously has its own issues.

ptsOSL · 2026-07-03T12:11:04Z

I think the name of this file is perhaps a bit confusing. This isn't the only test file which tests the cli, but its name suggests that its singular purpose is to test the cli. I suppose the difference is that in this file we call it with subprocess, while elsewhere we use the typer interface to call the cli. Maybe it could be renamed to clarify that? Also is there really any difference between using subprocess and the typer interface for the purpose of the tests?

The difference between subprocess and typer is that typer.testing.CLIRunner doesn't actually spawn a separate process, and uses the typer app directly. Since the error handling logic is in main(), we need to use subprocess to verify the output.

Although many other (but not all) tests consider the cli commands, this file includes the two that are generic across all commands. One checks the version info, and the other that the exceptions are being properly handled.

Do you have a recommendation for an alternative name?

Maybe something like test_cli_configuration or test_cli_setup. In my mind the version check is just to check that the cli is setup correctly and the exception test is to check that the cli is setup with exception handling. I would also be happy with test_cli_basic just to make it obvious its not the only cli test file.

I'll rename to test_cli_common.py

ptsOSL · 2026-07-03T12:25:11Z

+    )
+    deprecated_link.parent.mkdir(parents=True, exist_ok=True)
+    deprecated_link.touch()
+    with pytest.raises(ComparisonError, match="Duplicate modulefiles"):


Do these error messages report which modulefiles are duplicated, or does the user just have to figure this out?

I have changed the tests to check for more of the error message, including e.g. the names of the modulefiles. It makes them a bit more brittle, but that's reasonable.

ptsOSL · 2026-07-03T12:42:22Z

+    ("name-mismatch", "Module name .* does not match path"),
+    ("version-mismatch", "Module version .* does not match path"),


Is the .* in these messages to account for variables in string names? If so how comes you dont do that elsewhere? I have seen other error messages you check for where you only check for part of the error message. I assumed this was to avoid unresolved variables, but could you use .* in these cases as well? I may be misunderstanding something here.

This is now done more consistently. .* is now used for e.g. the tmp dirs instead

MJGaughran · 2026-07-04T10:57:24Z

I've swapped it back to draft, I'll try and split this up into at least a couple separate PRs. I'll cover all the comments you raise here first, though

MJGaughran commented Jun 29, 2026

View reviewed changes

MJGaughran mentioned this pull request Jun 30, 2026

Improve golden master tests #59

Merged

MJGaughran force-pushed the add-command-behaviour-tests branch from e346540 to 5730578 Compare June 30, 2026 14:10

MJGaughran added 25 commits June 30, 2026 16:54

Add test for CLI error handling

9e54a3d

Add behavioural tests for validate command

c169842

This covers cases that aren't easily tested with golden master tests.

Have compare command dump values in JSON mode to support special types

a08460b

Add separate tests for the compare command

04c3d6c

Tests relating to non-existent output dir use the compare function as otherwise the Typer configuration prevents the exception being raised.

Only search non-deprecated modules for default versions in compare

bd94f8b

Add test_sync.py to test double-deprecation and removal issues

06cbd0c

Remove previous snapshot dead code from Layout

8f8ca4b

This feature was replaced with the git ref option to the compare command.

Improve exception handling for compare command and snapshots

9642087

Fix typo in validate.py for allow_all, and provide relevant tests

5f243d7

Use pathlib symlink_to instead of os function

0fc31b4

Use Layout variables to calculate .gitignore paths

6f85eff

Add test for external tool calls

662c578

Add consistent type hinting to all CLI tests

3662b01

Ensure GitPython repos are closed properly

4516b07

Have dedicated test for ensuring apptainer pulls are successful

0a2bf29

Add test for BinaryApp using a local generated file

6db1cb6

Add error handling when loading deployment configuration with tests

2c4d493

Wrap load logic with standard deploy-tools error handling

b7f52a9

Add dedicated unit tests for default version logic

c5ce9ad

Add tests for Module options not previously covered

3b3e043

Reorganise CLI test configuration for consistency

a5b1673

Improve names of configuration folders

db9aa4b

Reduce irrelevant parameters in configuration

5f5fcaa

Expand test_module_options to cover template branches

76a29a6

Add test for deploying from scratch into dir with snapshot

3352347

MJGaughran added 3 commits June 30, 2026 16:54

Add tests for validation edge cases

5081ea8

Add unit test for defensive check in apply_default_versions()

d7c76e8

Add explicit default configuration to test_sync.py

df673cc

MJGaughran force-pushed the add-command-behaviour-tests branch from 5730578 to df673cc Compare June 30, 2026 15:56

MJGaughran changed the title ~~Draft: Add command behaviour tests~~ Add command behaviour tests Jun 30, 2026

MJGaughran requested review from MichaelStubbings and ptsOSL July 1, 2026 18:17

ptsOSL reviewed Jul 3, 2026

View reviewed changes

MJGaughran changed the title ~~Add command behaviour tests~~ Draft: Add command behaviour tests Jul 4, 2026

MJGaughran removed the request for review from MichaelStubbings July 4, 2026 10:59

Base automatically changed from improve-golden-master-tests to main July 7, 2026 09:33

		("name-mismatch", "Module name .* does not match path"),
		("version-mismatch", "Module version .* does not match path"),

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!

ptsOSL commented Jul 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ptsOSL left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

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

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!

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!

Choose a reason for hiding this comment

Uh oh!

MJGaughran commented Jul 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

MJGaughran commented Jun 29, 2026 •

edited

Loading

codecov Bot commented Jun 29, 2026 •

edited

Loading

ptsOSL commented Jul 3, 2026 •

edited

Loading

MJGaughran Jul 6, 2026 •

edited

Loading

MJGaughran Jul 8, 2026 •

edited

Loading