Open
Conversation
cgitim
approved these changes
Mar 25, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a reusable tests/test-support workspace for AWS/deployment helpers, adds a new tests/performance Jest workspace for running load tests against a live AWS deployment, and hardens logging by enabling Pino redaction of sensitive fields. It also bumps shared Terraform module versions and enables X-Ray tracing for the transform/filter Lambda (conditionally).
Changes:
- Add
tests/test-supporthelpers (AWS clients, deployment naming, SQS URL builders) and consume them from integration/performance tests. - Add a new
tests/performanceworkspace with Jest config, global setup/teardown (S3 seed/cleanup), and a Lambda throughput/latency test reading CloudWatch Logs Insights. - Enable Pino redaction in
@nhs-notify-client-callbacks/loggerand update Terraform shared module versions (3.0.6 → 3.0.7) including conditional X-Ray tracing.
Reviewed changes
Copilot reviewed 43 out of 46 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/client-subscriptions-management/client-subscriptions-management | Adds a bash wrapper to run the TypeScript CLI via tsx. |
| tests/test-support/tsconfig.json | Adds TS config for the new test-support workspace. |
| tests/test-support/package.json | Defines the test-support workspace package and exports. |
| tests/test-support/helpers/sqs.ts | Adds SQS queue URL builders based on deployment naming. |
| tests/test-support/helpers/index.ts | Re-exports test-support helpers. |
| tests/test-support/helpers/deployment.ts | Adds deployment detail loading and resource-name builders. |
| tests/test-support/helpers/clients.ts | Adds AWS SDK client factories (SQS/S3/CloudWatch Logs). |
| tests/performance/tsconfig.json | Adds TS config for the new performance test workspace (incl. helpers path). |
| tests/performance/package.json | Adds performance test workspace package and dependencies. |
| tests/performance/lambda-throughput.test.ts | Adds the main load/latency performance test. |
| tests/performance/jest.global-teardown.ts | Removes perf client subscription config from S3 after tests. |
| tests/performance/jest.global-setup.ts | Seeds perf client subscription config into S3 before tests. |
| tests/performance/jest.config.ts | Adds Jest config for performance workspace (serial, forceExit, helpers mapper). |
| tests/performance/helpers/sqs.ts | Adds SQS send utilities and load generation helper. |
| tests/performance/helpers/index.ts | Re-exports performance helper modules. |
| tests/performance/helpers/event-factories.ts | Adds perf test event generators for Message/Channel status events. |
| tests/performance/helpers/deployment.ts | Adds perf helper for transform/filter Lambda log group naming. |
| tests/performance/helpers/cloudwatch.ts | Adds CloudWatch log collection + Logs Insights percentile query utilities. |
| tests/performance/README.md | Documents prerequisites and how to run performance tests. |
| tests/integration/tsconfig.json | Removes paths mapping from integration TS config (keeps isolatedModules). |
| tests/integration/package.json | Adds dependency on the new test-support workspace. |
| tests/integration/metrics.test.ts | Switches shared helpers to test-support and local helpers to relative imports. |
| tests/integration/jest.global-teardown.ts | Switches shared helpers import to test-support package. |
| tests/integration/jest.global-setup.ts | Switches shared helpers import to test-support package. |
| tests/integration/jest.config.ts | Removes helpers moduleNameMapper now that tests use relative imports. |
| tests/integration/infrastructure-exists.test.ts | Switches shared helpers import to test-support package. |
| tests/integration/helpers/sqs.ts | Removes duplicated queue URL builder logic now provided by test-support. |
| tests/integration/helpers/index.ts | Deletes the old barrel export previously used by the helpers alias. |
| tests/integration/event-bus-to-webhook.test.ts | Switches shared helpers to test-support and local helpers to relative imports. |
| tests/integration/dlq-redrive.test.ts | Switches shared helpers to test-support and local helpers to relative imports. |
| src/logger/src/index.ts | Adds REDACT_PATHS and configures Pino redaction. |
| src/logger/src/tests/redaction.test.ts | Adds unit tests covering Pino redaction behavior. |
| src/logger/src/tests/index.test.ts | Extends logger config tests to assert redaction config. |
| scripts/config/vale/styles/config/vocabularies/words/accept.txt | Updates Vale accepted vocabulary entries. |
| scripts/config/sonar-scanner.properties | Updates sonar coverage exclusions to include test-support path. |
| package.json | Adds new workspaces (tests/performance, tests/test-support). |
| package-lock.json | Updates lockfile for new workspaces/dependencies and workspace links. |
| infrastructure/terraform/modules/client-destination/module_target_dlq.tf | Bumps shared SQS module version to 3.0.7. |
| infrastructure/terraform/modules/client-destination/README.md | Updates generated module source reference to 3.0.7. |
| infrastructure/terraform/components/callbacks/s3_bucket_client_config.tf | Bumps shared S3 bucket module version to 3.0.7. |
| infrastructure/terraform/components/callbacks/module_transform_filter_lambda.tf | Bumps shared Lambda module version and enables X-Ray tracing conditionally. |
| infrastructure/terraform/components/callbacks/module_sqs_inbound_event.tf | Bumps shared SQS module version to 3.0.7. |
| infrastructure/terraform/components/callbacks/module_mock_webhook_lambda.tf | Bumps shared Lambda module version to 3.0.7. |
| infrastructure/terraform/components/callbacks/module_kms.tf | Bumps shared KMS module version to 3.0.7. |
| infrastructure/terraform/components/callbacks/README.md | Updates generated module source references to 3.0.7. |
| eslint.config.mjs | Adds new tsconfigs to typed linting and updates rule overrides for new test workspaces. |
Comments suppressed due to low confidence (2)
tests/integration/package.json:23
@aws-sdk/client-sqsis listed in bothdependenciesanddevDependencies. This duplicates installs and can cause version drift; keep it in just one section (typicallydependenciesfor a test workspace that executes against AWS).
"@aws-sdk/client-cloudwatch-logs": "^3.991.0",
"@aws-sdk/client-s3": "^3.821.0",
"@aws-sdk/client-sqs": "^3.990.0",
"async-wait-until": "^2.0.12",
"@nhs-notify-client-callbacks/test-support": "*"
},
"devDependencies": {
"@aws-sdk/client-sqs": "^3.990.0",
"@tsconfig/node22": "^22.0.2",
tests/integration/package.json:26
async-wait-untilis listed in bothdependenciesanddevDependencies. Remove the duplicate entry so there’s a single source of truth for the installed version.
"async-wait-until": "^2.0.12",
"@nhs-notify-client-callbacks/test-support": "*"
},
"devDependencies": {
"@aws-sdk/client-sqs": "^3.990.0",
"@tsconfig/node22": "^22.0.2",
"@types/jest": "^29.5.14",
"async-wait-until": "^2.0.12",
"jest": "^29.7.0",
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+28
to
+44
| await waitUntil( | ||
| async () => { | ||
| const response = await client.send( | ||
| new FilterLogEventsCommand({ | ||
| logGroupName, | ||
| startTime, | ||
| filterPattern: '{ $.msg = "batch-processing-completed" }', | ||
| }), | ||
| ); | ||
|
|
||
| for (const event of response.events ?? []) { | ||
| if (event.message) { | ||
| try { | ||
| const entry = JSON.parse(event.message) as BatchCompletedLogEntry; | ||
| if (typeof entry.processingTimeMs === "number") { | ||
| collected.push(entry.processingTimeMs); | ||
| } |
There was a problem hiding this comment.
collectBatchProcessingTimes appends all matching events on every poll, which will double-count the same CloudWatch log events across iterations and can satisfy expectedCount prematurely. Track processed events (e.g. by eventId/timestamp) or advance the query window/nextToken so each log event is only counted once.
Comment on lines
+46
to
+59
| const batchesPerSecond = Math.ceil( | ||
| targetEventsPerSecond / SQS_MAX_BATCH_SIZE, | ||
| ); | ||
| const start = Date.now(); | ||
| let sent = 0; | ||
|
|
||
| for (let second = 0; second < durationSeconds; second++) { | ||
| const waveStart = Date.now(); | ||
|
|
||
| const results = await Promise.all( | ||
| Array.from({ length: batchesPerSecond }, () => { | ||
| const batch = Array.from({ length: SQS_MAX_BATCH_SIZE }, eventFactory); | ||
| return sendSqsBatch(client, queueUrl, batch).then(() => batch.length); | ||
| }), |
There was a problem hiding this comment.
generateSqsLoad rounds batchesPerSecond up and always sends SQS_MAX_BATCH_SIZE events per batch, so it can exceed targetEventsPerSecond when the target isn’t a multiple of 10. Consider sizing the final batch each second to only send the remaining events needed to meet the requested rate.
Comment on lines
+12
to
+16
| | Variable | Required | Default | Description | | ||
| | --- | --- | --- | --- | | ||
| | `ENVIRONMENT` | Yes | — | Target environment name (e.g. `dev`) | | ||
| | `AWS_ACCOUNT_ID` | Yes | — | AWS account ID for the target environment | | ||
| | `AWS_REGION` | No | `eu-west-2` | AWS region | |
There was a problem hiding this comment.
The markdown table under “Environment Variables” uses double leading pipes (||) on the header/separator rows, which prevents it from rendering as a standard table. Use single | delimiters for the header and separator rows.
Comment on lines
+1
to
+3
| #!/bin/bash | ||
| cd $(dirname "${BASH_SOURCE[0]}") | ||
| npx tsx ./src/entrypoint/cli/index.ts "$@" |
There was a problem hiding this comment.
This script doesn’t follow the repo’s bash-script hardening pattern (e.g. set -euo pipefail) and the cd line is unquoted, so it can fail if the path contains spaces. Align with existing scripts (e.g. scripts/tests/integration.sh:3) by enabling strict mode and quoting the command substitution for the directory change.
rhyscoxnhs
force-pushed
the
feature/CCM-14205
branch
from
d347706 to
0304422
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Context
Type of changes
Checklist
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.