Feature/ccm 14778 fix e2 e tests by stevebux · Pull Request #534 · NHSDigital/nhs-notify-supplier-api

stevebux · 2026-04-14T12:47:50Z

Description

Context

Type of changes

Refactoring (non-breaking change)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would change existing functionality)
Bug fix (non-breaking change which fixes an issue)

Checklist

I am familiar with the contributing guidelines
I have followed the code style of the project
I have added tests to cover my changes
I have updated the documentation accordingly
This PR is a result of pair or mob programming

DT3-Specific Checklist

If I have added a new resource (SQS, Lambda, Gateway, DDB table, etc), I have created the appropriate alarms

Sensitive Information Declaration

To ensure the utmost confidentiality and protect your and others privacy, we kindly ask you to NOT including PII (Personal Identifiable Information) / PID (Personal Identifiable Data) or any other sensitive data in this PR (Pull Request) and the codebase changes. We will remove any PR that do contain any sensitive information. We really appreciate your cooperation in this matter.

I confirm that neither PII/PID nor sensitive data are included in this PR and the codebase changes.

Copilot

Pull request overview

This PR updates the repository’s end-to-end testing and CI wiring to make the e2e suite more reliable by (a) seeding letters via the shared letter-test-data utility and (b) supporting API-key auth for PR proxies and the /_status endpoint.

Changes:

Add Python e2e helpers to seed PENDING letters via the shared Node CLI and to poll until letters are visible.
Update e2e auth/header generation to support both bearer tokens and API-key auth (status + PR proxies).
Restructure GitHub Actions acceptance testing to include an e2e test type and add support for passing extra secrets to internal workflow dispatches.

Reviewed changes

Copilot reviewed 26 out of 26 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
tests/e2e-tests/lib/secret.py	Extend `Secret` to carry an `auth_type` for header generation.
tests/e2e-tests/lib/letters.py	New helper to create letter test data via CLI and fetch pending IDs with retry.
tests/e2e-tests/lib/generators.py	Update header generation to support API-key vs bearer auth.
tests/e2e-tests/lib/fixtures.py	Replace bearer-token fixture with auth “secret” fixtures for `/letters` and `/_status`.
tests/e2e-tests/lib/authentication.py	Support API-key auth for `/_status` and PR proxies; path-based auth selection.
tests/e2e-tests/api/test_endpoint.py	Update endpoint smoke tests to use new fixtures/header generation.
tests/e2e-tests/api/letters/test_update_letter_status.py	Seed/poll for PENDING letters before updating status.
tests/e2e-tests/api/letters/test_multiple_letter_status.py	Seed/poll for multiple letters before batch status updates.
tests/e2e-tests/api/letters/test_get_list_of_letters.py	Switch to helper that ensures at least one letter is available.
tests/e2e-tests/api/letters/test_get_letter_status.py	Switch to helper to obtain a valid letter ID before GET-by-id.
tests/e2e-tests/api/letters/conftest.py	New autouse session fixture to seed letters for this test directory.
tests/e2e-tests/api/headers/test_x_request_id.py	Update fixtures used for auth in X-Request-ID tests.
tests/e2e-tests/api/data/test_get_letter_data.py	Switch to helper to obtain a valid letter ID before fetching letter data.
tests/e2e-tests/README.md	Update local e2e setup docs (proxy name + API keys).
tests/constants/api-constants.ts	Use `TARGET_ENVIRONMENT` instead of `PR_NUMBER` for environment naming.
Makefile	Pass `STATUS_ENDPOINT_API_KEY` into the pytest invocation environment.
.gitleaksignore	Add ignore entries for detected IP-like strings in built SAM artefacts.
.github/workflows/stage-4-acceptance.yaml	Pass extra secret names to internal workflow dispatch.
.github/workflows/stage-3-build.yaml	Split OAS spec artefact jobs (main vs PR) and adjust job dependencies/conditions.
.github/scripts/dispatch_internal_repo_workflow.sh	Add `--extraSecretNames` support for dispatch payload construction.
.github/actions/test-types.json	Add `e2e` to the allowed acceptance test types.
.github/actions/e2e-tests/action.yml	Remove legacy composite action for e2e tests.
.github/actions/acceptance-tests/action.yml	Route to component vs e2e acceptance sub-actions based on `testType`.
.github/actions/acceptance-tests-e2e/action.yml	New composite action to run the Python e2e suite in CI.
.github/actions/acceptance-tests-component/action.yml	New composite action encapsulating component acceptance test execution.
.env.template	Add `STATUS_ENDPOINT_API_KEY` to local env template.

sidnhs · 2026-04-20T10:48:20Z

            --infraRepoName "nhs-notify-supplier-api" \
            --releaseVersion "${GITHUB_HEAD_REF:-${GITHUB_REF#refs/heads/}}" \
            --overrideProjectName "nhs" \
+            --internalRef "feature/CCM-14778" \


would we be changing it?

Yep, I'm going to revert this once the internal repo change has merged

…et, rather than fetching from APIM

francisco-videira-nhs · 2026-04-23T11:52:09Z

+      run: |
+        echo "$DEV_E2E_KEYS_PRIVATE" > "${GITHUB_WORKSPACE}/internal-dev-test-1.pem"
+        chmod 600 "${GITHUB_WORKSPACE}/internal-dev-test-1.pem"
+        BASE_PROXY_NAME=nhs-notify-supplier--internal-dev--nhs-notify-supplier


It would be great if we could also run these tests in nonprod / int. If it's too complicated to add them now, maybe worth raising a ticket to it, it would make the release process a lot easier!

I'm reluctant to make this ticket any bigger than it is, but it's a good suggestion - the tests themselves have been written to support nonprod and int, so hopefully wouldn't need to change. I'll raise a ticket

francisco-videira-nhs · 2026-04-23T15:11:14Z

-      run: |
-        make test-${{ inputs.testType }}
+    - name: Run e2e tests
+      if: ${{ inputs.testType == 'e2e' && inputs.targetEnvironment == 'main' }}


Can we not run these against PR envs? The acceptance-tests-e2e action has some logic for PR envs

The blocker is that after running the build, we need to do a manual step to add the PR environment to the APIM application. I tried to automate that step but gave up and raised a follow up ticket to do it (CCM-17012). I left the PR logic in the tests so that they can be easily re-enabled once we complete that ticket.

francisco-videira-nhs · 2026-04-24T10:32:34Z

Can't figure out where these are set:
DEV_E2E_KEYS_APIM_PR
DEV_E2E_KEYS_APIM_MAIN
DEV_E2E_KEYS_PRIVATE

Ah yes, it would help to read this is conjunction with the internal repo PR (https://github.com/NHSDigital/nhs-notify-internal/pull/630/changes). Basically the idea is that we store these secrets in the SSM parameter store in the dev supplier API AWS instance. We pass the names of the parameters we want to retrieve to the internal workflow, then when the tests run it has fetched them for us and put them in the environment. This is still subject to approval by Aiden BTW

francisco-videira-nhs · 2026-04-24T10:40:08Z

-          else
-            echo "ENVIRONMENT=main" >> $GITHUB_ENV
-          fi
+          echo "ENVIRONMENT=main" >> $GITHUB_ENV


Would this need to support PR env as well?

This is left over from my testing - I need to remove it before merging

francisco-videira-nhs · 2026-04-24T11:39:54Z


-    get_message_response = requests.get(f"{url}/{LETTERS_ENDPOINT}?limit=1", headers=headers)
-
-    ErrorHandler.handle_retry(get_message_response)


Will we lose some kind of retry ability by taking this off?

I'm not sure this line adds anything - it blows the test up on certain HTTP error codes, but then the next line fails the test anyway if the status code is anything other than 200. The retry will happen regardless. The get_pending_letter_ids function I've added does a similar job by calling response.raise_for_status()

Ah wait, that's not true. In the Makefile, pytest is configured to only rerun on the errors thrown by ErrorHandler.handle_retry(get_message_response). I'll add it to get_pending_letter_ids to avoid changing behaviour unnecessarily.

stevebux requested a review from a team as a code owner April 14, 2026 12:47

stevebux added the deploy-proxy Set to 'true' to force a PR build to deploy the APIM proxy label Apr 14, 2026

stevebux requested a review from a team as a code owner April 14, 2026 12:47

stevebux force-pushed the feature/CCM-14778-Fix-E2E-Tests branch 4 times, most recently from 2487732 to 7d97b6c Compare April 17, 2026 12:32

stevebux requested a review from Copilot April 17, 2026 13:10

Copilot started reviewing on behalf of stevebux April 17, 2026 13:11 View session

Copilot AI reviewed Apr 17, 2026

View reviewed changes

stevebux force-pushed the feature/CCM-14778-Fix-E2E-Tests branch from 83c95f9 to 48052c5 Compare April 17, 2026 15:41

stevebux removed the deploy-proxy Set to 'true' to force a PR build to deploy the APIM proxy label Apr 20, 2026

stevebux force-pushed the feature/CCM-14778-Fix-E2E-Tests branch 2 times, most recently from 9980950 to adee573 Compare April 20, 2026 09:52

sidnhs reviewed Apr 20, 2026

View reviewed changes

sidnhs requested changes Apr 20, 2026

View reviewed changes

stevebux requested a review from a team as a code owner April 21, 2026 10:12

stevebux added the deploy-proxy Set to 'true' to force a PR build to deploy the APIM proxy label Apr 21, 2026

stevebux force-pushed the feature/CCM-14778-Fix-E2E-Tests branch from 284fe83 to adee573 Compare April 21, 2026 13:09

Re-enable end-to-end tests, sourcing status endpoint from github secr…

234d2c8

…et, rather than fetching from APIM

stevebux force-pushed the feature/CCM-14778-Fix-E2E-Tests branch from adee573 to d11890c Compare April 21, 2026 13:28

stevebux added 3 commits April 21, 2026 15:45

Point to my internal PR (DO NOT CHECK IN)

c40c3b6

Use SSM parameters

5d727d1

Fake being on main DO NOT CHECK IN

53a5d23

stevebux force-pushed the feature/CCM-14778-Fix-E2E-Tests branch from d11890c to 53a5d23 Compare April 23, 2026 09:18

Fix JSON

c6b3ac8

francisco-videira-nhs reviewed Apr 24, 2026

View reviewed changes


		get_message_response = requests.get(f"{url}/{LETTERS_ENDPOINT}?limit=1", headers=headers)

		ErrorHandler.handle_retry(get_message_response)

Conversation

stevebux commented Apr 14, 2026

Description

Context

Type of changes

Checklist

DT3-Specific Checklist

Sensitive Information Declaration

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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!

4 participants