Add smoke test for running gateway services by steventux · Pull Request #242 · NHSDigital/manage-breast-screening-gateway

steventux · 2026-09-24T14:44:56Z

Description

A smoke test we can run against existing gateway services via an Azure connected machine run command:

az connectedmachine run-command create \
  --machine-name gw-<site>-<ods>-<instance> \
  --resource-group rg-mbsgw-<env>-uks-arc-enabled-servers \
  --location uksouth \
  --name smoke-test \
  --script "uv run pytest tests/smoke_test.py"

Once #240 is merged the smoke test can also run in docker.

Jira link

https://nhsd-jira.digital.nhs.uk/browse/DTOSS-12894

Review notes

Review checklist

Check database queries are correctly scoped to current_provider

Copilot

Copilot review overview

🟡 Changes recommended

Multiple unresolved test correctness, reliability, collection, and Compose configuration issues remain.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 4 · 1

Open (5)

Invalid 0.0.0.0 host override for client connections · New
Relay response status is not validated · New
Use the valid storage lookup API and dictionary fields · New
Race with asynchronous upload before asserting status · New
Import ordering violates Ruff/isort configuration · New

What changed in this PR

Adds an end-to-end smoke test for Relay, MWL, PACS, and upload services, with supporting Docker Compose defaults.

Changes:

Adds Relay, C-FIND, C-STORE, and upload verification.
Adds configurable gateway service environment variables.

File	Review findings
`tests/smoke_test.py`	Moderate issues: invalid PACS storage API usage (4 votes), asynchronous upload race (2), unreliable host override (2), unresolved Docker storage path (1), unchecked Relay response (2), default pytest collection (1), and non-repeatable accession (1). Nit: import ordering (3).
`compose.yml`	Moderate issues: unsupported Linux `host.docker.internal` default (1 vote), unnecessary Relay secret exposure to PACS (1), and inconsistent configurable paths across services (1).

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copilot

Copilot review overview

🟡 Changes recommended

Address the smoke-test collection issue and propagate configurable service settings consistently.

Review effort: Lite
Findings: 1 · 2

Open (3)

Exclude the live smoke test from default pytest collection · New
Race with asynchronous upload before asserting status
Invalid 0.0.0.0 host override for client connections

Resolved since last review (3)

Use the valid storage lookup API and dictionary fields
Relay response status is not validated
Import ordering violates Ruff/isort configuration

Copilot

Copilot review overview

🟡 Changes recommended

Unresolved credential exposure and multiple smoke-test correctness and configuration issues remain.

Review effort: Lite
Findings: 1 · 3

Open (4)

Relay connection URL logs reusable SAS credentials · New
Repeated identifiers can validate stale gateway records · New
Relay check failures are logged but do not fail the smoke test · New
C-FIND test ignores final response status · New

Resolved since last review (3)

Exclude the live smoke test from default pytest collection
Race with asynchronous upload before asserting status
Invalid 0.0.0.0 host override for client connections

Copilot

Copilot review overview

🟡 Changes recommended

Critical environment-safety and multiple runtime configuration issues remain unresolved.

Review effort: Lite
Findings: 1 · 2

Open (3)

Fail closed when the environment is missing or invalid · New
Propagate configurable MWL settings to the MWL service · New
C-FIND test ignores final response status

Resolved since last review (3)

Relay connection URL logs reusable SAS credentials
Relay check failures are logged but do not fail the smoke test
Repeated identifiers can validate stale gateway records

Copilot

Copilot review overview

🟡 Changes recommended

The smoke test currently has a lint failure and multiple deployment or runtime issues that block reliable execution.

Review effort: Lite
Findings: 2 · 3

Open (5)

Unused Environment import fails Ruff lint · New
Fail closed when the environment is missing or invalid
Docker image omits pytest dependency · New
Missing ENVIRONMENT causes smoke module import failure · New
Smoke timeout expires before uploader reaches FAILED state · New

Resolved since last review (2)

Propagate configurable MWL settings to the MWL service
C-FIND test ignores final response status

+COPY tests/smoke_test.py ./tests/smoke_test.py
+COPY tests/integration/dicom_helpers.py ./tests/integration/dicom_helpers.py


+
+load_dotenv()
+
+if os.environ["ENVIRONMENT"] == "prod":


+        stored_image = storage.get_instance_by_accession(TEST_ACCESSION_NUMBER)
+        # We expect a FAILED upload as the smoke test action id won't match anything in Rubie
+        # or Rubie won't be available to receive the upload.
+        if stored_image["upload_status"] == "FAILED":
+            upload_attempted = True
+            break


The smoke test can be run on the deployed gateway VM, machine or docker container.

Copilot

Copilot review overview

🟡 Changes recommended

Unresolved environment, dependency, execution-path, safety, timeout, and cleanup issues prevent reliable and safe smoke-test execution.

Review effort: Lite
Findings: 1 · 5

Open (6)

Case-sensitive production guard permits PROD · New
Smoke test requires unset ENVIRONMENT variable · New
Default MWL host incorrectly uses wildcard address · New
Smoke timeout expires before uploader reaches FAILED state
Missing ENVIRONMENT causes smoke module import failure
Docker image omits pytest dependency

Resolved since last review (2)

Unused Environment import fails Ruff lint
Fail closed when the environment is missing or invalid

+
+load_dotenv()
+
+if os.environ["ENVIRONMENT"] == "prod":


+      - AZURE_RELAY_SHARED_ACCESS_KEY=${AZURE_RELAY_SHARED_ACCESS_KEY}
+      - CLOUD_API_ENDPOINT=${CLOUD_API_ENDPOINT:-http://host.docker.internal:8000/api/v1/dicom}
+      - CLOUD_API_TOKEN=${CLOUD_API_TOKEN:-testtoken}
      - LOG_LEVEL=INFO


+
+# 2. Connect to MWL and perform a C-FIND to retrieve the worklist item
+def test_c_find_worklist_item():
+    mwl_host = os.getenv("MWL_HOST", "0.0.0.0")


carlosmartinez · 2026-10-01T10:44:18Z

+    # We are connecting to a Relay Listener, so we need to use the "connect" action instead of "listen".
+    url = RelayURI().connection_url().replace("sb-hc-action=listen", "sb-hc-action=connect")
+    with connect(url, compression=None, open_timeout=30) as conn:
+        conn.send(json.dumps(payload))


i don't think this will work on a deployed environment. the machine's managed identity won't have send permissions (presumably working locally because of SAS fallback). could grant hybrid connection-specific send permissions without causing major security issues? otherwise the start point for the smoke test would have to be C-FIND.

carlosmartinez · 2026-10-01T10:46:46Z

+
+load_dotenv()
+
+if os.environ["ENVIRONMENT"] == "prod":


Suggested change

if os.environ["ENVIRONMENT"] == "prod":

if os.getenv("ENVIRONMENT", "").lower() == "prod":

agree w copilot!

carlosmartinez · 2026-10-01T10:49:01Z

+            "worklist_item": {
+                "participant": {
+                    "nhs_number": TEST_PATIENT_ID,
+                    "name": "SMITH^JANE",


shall we follow the "ZZTEST" pattern or at least something more obviously fake?

steventux requested a lite review from Copilot September 24, 2026 14:45

Copilot started reviewing on behalf of steventux September 24, 2026 14:46 View session

Copilot AI reviewed Sep 24, 2026

View reviewed changes

Comment thread tests/smoke_test.py Outdated

Comment thread tests/smoke_test.py Outdated

Comment thread tests/smoke_test.py Outdated

Comment thread tests/smoke_test.py Outdated

Comment thread tests/smoke_test.py Outdated

steventux force-pushed the deployed-gateway-smoke-test branch 2 times, most recently from d6906ec to e35d071 Compare September 28, 2026 14:29

steventux requested a lite review from Copilot September 28, 2026 14:36

Copilot started reviewing on behalf of steventux September 28, 2026 14:37 View session

Copilot AI reviewed Sep 28, 2026

View reviewed changes

Comment thread tests/smoke_test.py

steventux force-pushed the deployed-gateway-smoke-test branch from e35d071 to 02c122c Compare September 29, 2026 10:39

steventux requested a lite review from Copilot September 29, 2026 10:42

Copilot started reviewing on behalf of steventux September 29, 2026 10:42 View session

Copilot AI reviewed Sep 29, 2026

View reviewed changes

Comment thread tests/smoke_test.py Outdated

Comment thread tests/smoke_test.py Outdated

Comment thread tests/smoke_test.py Outdated

Comment thread tests/smoke_test.py

steventux force-pushed the deployed-gateway-smoke-test branch 2 times, most recently from a636243 to ec07761 Compare September 29, 2026 12:56

steventux requested a lite review from Copilot October 1, 2026 07:19

Copilot started reviewing on behalf of steventux October 1, 2026 07:19 View session

Copilot AI reviewed Oct 1, 2026

View reviewed changes

Comment thread tests/smoke_test.py Outdated

Comment thread compose.yml

steventux force-pushed the deployed-gateway-smoke-test branch from ec07761 to 036596e Compare October 1, 2026 08:03

steventux requested a lite review from Copilot October 1, 2026 08:11

Copilot started reviewing on behalf of steventux October 1, 2026 08:12 View session

steventux marked this pull request as ready for review October 1, 2026 08:13

Copilot AI reviewed Oct 1, 2026

View reviewed changes

Add smoke test for running gateway services

322a5c4

The smoke test can be run on the deployed gateway VM, machine or docker container.

steventux force-pushed the deployed-gateway-smoke-test branch from 036596e to 322a5c4 Compare October 1, 2026 08:55

steventux requested a lite review from Copilot October 1, 2026 08:55

Copilot started reviewing on behalf of steventux October 1, 2026 08:56 View session

Copilot AI reviewed Oct 1, 2026

View reviewed changes

carlosmartinez reviewed Oct 1, 2026

View reviewed changes

		COPY tests/smoke_test.py ./tests/smoke_test.py
		COPY tests/integration/dicom_helpers.py ./tests/integration/dicom_helpers.py

	if os.environ["ENVIRONMENT"] == "prod":
	if os.getenv("ENVIRONMENT", "").lower() == "prod":

Conversation

steventux commented Sep 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Jira link

Review notes

Review checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

carlosmartinez Oct 1, 2026

Choose a reason for hiding this comment

Uh oh!

carlosmartinez Oct 1, 2026

Choose a reason for hiding this comment

Uh oh!

carlosmartinez Oct 1, 2026

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

steventux commented Sep 24, 2026 •

edited

Loading