Skip to content

WIP: test improvements#110

Draft
mikeeq wants to merge 45 commits intomainfrom
feature/hote-541/add-playwright-tests
Draft

WIP: test improvements#110
mikeeq wants to merge 45 commits intomainfrom
feature/hote-541/add-playwright-tests

Conversation

@mikeeq
Copy link
Copy Markdown
Collaborator

@mikeeq mikeeq commented Feb 18, 2026

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

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.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Feb 18, 2026

Lambdas Coverage Report

Lines Statements Branches Functions
Coverage: 91%
91.3% (21/23) 100% (0/0) 87.5% (7/8)

@github-actions
Copy link
Copy Markdown

github-actions bot commented Feb 18, 2026

UI Coverage Report

Lines Statements Branches Functions
Coverage: 93%
93.87% (4305/4586) 86.34% (512/593) 85.12% (166/195)

Copilot AI review requested due to automatic review settings March 3, 2026 14:04
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Improves the Playwright E2E test framework and supporting tooling (login flow resilience, configuration, CI workflows), plus a small UI change to silence a React Router hydration warning.

Changes:

  • Add HydrateFallback to the UI root route and update console-error ignores for CI stability.
  • Add/adjust Playwright UI tests and page objects (external link checks, NHS login consent handling, OTP wait reliability).
  • Expand test configuration and CI/dev tooling (new config loader, env vars, workflow and pre-commit updates).

Reviewed changes

Copilot reviewed 19 out of 19 changed files in this pull request and generated 9 comments.

Show a summary per file
File Description
ui/src/app.tsx Adds HydrateFallback to the guarded root route to avoid hydration warnings.
tests/tests/ui/HomeTestStartPage.spec.ts New UI spec validating external links from the start page.
tests/page-objects/NhsLoginHelper.ts Adds optional handling for an NHS login consent page during auth.
tests/page-objects/NHSLogin/NhsLoginConsentPage.ts New page object to accept/continue on the consent page.
tests/page-objects/NHSLogin/CodeSecurityPage.ts Switches OTP wait to a locator-based wait; deprecates network-based wait.
tests/page-objects/HomeTestStartPage.ts Adds a waitUntilPageLoad() helper for stability.
tests/global-setup.ts Adds SKIP_LOGIN support to reuse existing session files.
tests/fixtures/consoleErrorFixture.ts Extends ignored console/network patterns (incl. CSP font errors).
tests/configuration/configuration.ts Introduces a new config factory/wrapper (defaults + env + local + CLI overrides).
tests/configuration/.env.dev Adds external-link env vars for dev test runs.
scripts/config/gitleaks.toml Allowlists .pre-commit-config.yaml.
package.json Adjusts Playwright run script and adds a full clean script.
local-environment/scripts/localstack/get_supplier_id.sh Minor shell style change (unused loop var).
local-environment/scripts/localstack/deploy.sh Removes unused variables.
.pre-commit-config.yaml Adds/adjusts pre-commit hooks (yaml/json/toml checks, actionlint, shellcheck, etc.).
.github/workflows/playwright-e2e.yaml Adds workflow inputs and environment switching logic; refactors steps and summary output.
.github/workflows/cicd-1-pull-request.yaml Replaces Jira action with inline bash title/branch check; refactors outputs writing.
.github/workflows/cicd-2-publish.yaml Refactors outputs writing and quoting.
.github/workflows/cicd-3-deploy.yaml Removes unused tag input/outputs and refactors outputs writing.
Comments suppressed due to low confidence (1)

tests/global-setup.ts:5

  • ConfigFactory is imported but never used in this file. This will trigger the repo's @typescript-eslint/no-unused-vars rule; remove the unused import (or use it if it was intended for side effects).
import * as fs from 'fs';
import * as path from 'path';
import { ConfigFactory } from './configuration/configuration';
import { CredentialsHelper } from './utils/CredentialsHelper';
import { UserManagerFactory } from './utils/users/UserManagerFactory';

@@ -0,0 +1,42 @@
import { test } from '../../fixtures';
import { expect } from '@playwright/test';
import { config, EnvironmentVariables } from '../../configuration';
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The imports here won't resolve with the current tests folder structure: ../../fixtures and ../../configuration are directories without an index.ts, so TypeScript/Node module resolution will fail. Import from the concrete module files instead (e.g., the appropriate fixture module and the existing configuration module), or add barrel index.ts files that re-export test/config/EnvironmentVariables.

Suggested change
import { config, EnvironmentVariables } from '../../configuration';
import { config, EnvironmentVariables } from '../../configuration/configuration';

Copilot uses AI. Check for mistakes.
import * as dotenv from 'dotenv';
import * as path from 'path';
import * as fs from 'fs';
import { EnvironmentVariables, availableEnvironments, Environment } from './environment-variables';
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This import points to ./environment-variables, but there is no such file in tests/configuration/ (the existing module is ./EnvironmentVariables). On case-sensitive filesystems (CI/Linux) this will fail at compile time; update the import to the correct module path/casing or add the missing file.

Suggested change
import { EnvironmentVariables, availableEnvironments, Environment } from './environment-variables';
import { EnvironmentVariables, availableEnvironments, Environment } from './EnvironmentVariables';

Copilot uses AI. Check for mistakes.
Comment on lines +3 to +4
export class NhsLoginConsentPage {
readonly page: Page;
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The class/file name uses mixed acronym casing (NhsLoginConsentPage), but nearby NHS login page objects consistently use NHS... (e.g. NHSEmailAndPasswordPage). Renaming to NHSLoginConsentPage (and updating imports) would keep naming consistent and improve discoverability.

Copilot uses AI. Check for mistakes.
Comment on lines +92 to +98
if [ "$TARGET_ENV" == "dev" ]; then
echo "ui_url=https://dev.hometest.service.nhs.uk" >> "$GITHUB_OUTPUT"
echo "api_url=https://dev.hometest.service.nhs.uk/" >> "$GITHUB_OUTPUT"
else
echo "ui_url=${{ steps.terraform.outputs.ui_url }}" >> "$GITHUB_OUTPUT"
echo "api_url=${{ steps.terraform.outputs.api_base_url }}" >> "$GITHUB_OUTPUT"
fi
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For TARGET_ENV=dev, api_url is set to https://dev.hometest.service.nhs.uk/, but the test framework default apiBaseUrl includes the /api prefix (e.g. http://localhost:4000/api) and API clients build URLs relative to that. Unless the dev API is actually hosted at the root, this will produce incorrect request URLs; align the dev api_url with the expected API base path (and keep it consistent with local/terraform outputs).

Copilot uses AI. Check for mistakes.
- "main"
pull_request:
types: [opened, reopened, edited, synchronize]
types: [opened, reopened, edited, synchronize]
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pull_request has types: defined twice. YAML will keep only the last key, and the duplicate is confusing/noisy; remove the redundant types: line.

Suggested change
types: [opened, reopened, edited, synchronize]

Copilot uses AI. Check for mistakes.
await homeTestStartPage.waitUntilPageLoad();

// Test "your nearest sexual health clinic" link
await homeTestStartPage.clickNearestSexualHealthClinicLink(sexualHealthClinicUrl);
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

homeTestStartPage.clickNearestSexualHealthClinicLink(...) doesn't exist on HomeTestStartPage (the page object currently exposes clickSexualHealthServicesLink(...)). This will fail to compile/run; update the test to call the existing method or add the missing method on the page object.

Suggested change
await homeTestStartPage.clickNearestSexualHealthClinicLink(sexualHealthClinicUrl);
await homeTestStartPage.clickSexualHealthServicesLink(sexualHealthClinicUrl);

Copilot uses AI. Check for mistakes.
Comment on lines +129 to +145
// Map environment variables to config interface
return {
uiBaseUrl: process.env[EnvironmentVariables.UI_BASE_URL],
apiBaseUrl: process.env[EnvironmentVariables.API_BASE_URL],
headless: process.env[EnvironmentVariables.HEADLESS] === 'true',
timeout: process.env[EnvironmentVariables.TIMEOUT]
? parseInt(process.env[EnvironmentVariables.TIMEOUT], 10)
: undefined,
slowMo: process.env[EnvironmentVariables.SLOW_MO]
? parseInt(process.env[EnvironmentVariables.SLOW_MO], 10)
: undefined,
accessibilityStandards: process.env[EnvironmentVariables.ACCESSIBILITY_STANDARDS],
reportingOutputDirectory: process.env[EnvironmentVariables.REPORTING_OUTPUT_DIRECTORY],
externalLinkSexualHealthClinic: process.env[EnvironmentVariables.EXTERNAL_LINK_SEXUAL_HEALTH_CLINIC],
externalLinkNearestAE: process.env[EnvironmentVariables.EXTERNAL_LINK_NEAREST_AE],
externalLinkHivAidsInfo: process.env[EnvironmentVariables.EXTERNAL_LINK_HIV_AIDS_INFO],
};
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

readConfigurationFromEnvFile() returns an object with properties that can be undefined (or empty strings from dotenv), and then loadConfiguration() spreads that over defaultConfig. This can overwrite required defaults (e.g., uiBaseUrl, apiBaseUrl, external link URLs) with undefined/"", causing runtime failures later. Build the partial config by only including keys that are actually set (and non-empty) before spreading, similar to readConfigurationFromProcessEnv().

Suggested change
// Map environment variables to config interface
return {
uiBaseUrl: process.env[EnvironmentVariables.UI_BASE_URL],
apiBaseUrl: process.env[EnvironmentVariables.API_BASE_URL],
headless: process.env[EnvironmentVariables.HEADLESS] === 'true',
timeout: process.env[EnvironmentVariables.TIMEOUT]
? parseInt(process.env[EnvironmentVariables.TIMEOUT], 10)
: undefined,
slowMo: process.env[EnvironmentVariables.SLOW_MO]
? parseInt(process.env[EnvironmentVariables.SLOW_MO], 10)
: undefined,
accessibilityStandards: process.env[EnvironmentVariables.ACCESSIBILITY_STANDARDS],
reportingOutputDirectory: process.env[EnvironmentVariables.REPORTING_OUTPUT_DIRECTORY],
externalLinkSexualHealthClinic: process.env[EnvironmentVariables.EXTERNAL_LINK_SEXUAL_HEALTH_CLINIC],
externalLinkNearestAE: process.env[EnvironmentVariables.EXTERNAL_LINK_NEAREST_AE],
externalLinkHivAidsInfo: process.env[EnvironmentVariables.EXTERNAL_LINK_HIV_AIDS_INFO],
};
const envConfig: Partial<ConfigInterface> = {};
const getEnvValue = (key: EnvironmentVariables): string | undefined => {
const value = process.env[key];
if (value === undefined) {
return undefined;
}
const trimmed = value.trim();
return trimmed === '' ? undefined : trimmed;
};
const uiBaseUrl = getEnvValue(EnvironmentVariables.UI_BASE_URL);
if (uiBaseUrl !== undefined) {
envConfig.uiBaseUrl = uiBaseUrl;
}
const apiBaseUrl = getEnvValue(EnvironmentVariables.API_BASE_URL);
if (apiBaseUrl !== undefined) {
envConfig.apiBaseUrl = apiBaseUrl;
}
const headlessValue = getEnvValue(EnvironmentVariables.HEADLESS);
if (headlessValue !== undefined) {
envConfig.headless = headlessValue === 'true';
}
const timeoutValue = getEnvValue(EnvironmentVariables.TIMEOUT);
if (timeoutValue !== undefined) {
envConfig.timeout = parseInt(timeoutValue, 10);
}
const slowMoValue = getEnvValue(EnvironmentVariables.SLOW_MO);
if (slowMoValue !== undefined) {
envConfig.slowMo = parseInt(slowMoValue, 10);
}
const accessibilityStandards = getEnvValue(EnvironmentVariables.ACCESSIBILITY_STANDARDS);
if (accessibilityStandards !== undefined) {
envConfig.accessibilityStandards = accessibilityStandards;
}
const reportingOutputDirectory = getEnvValue(EnvironmentVariables.REPORTING_OUTPUT_DIRECTORY);
if (reportingOutputDirectory !== undefined) {
envConfig.reportingOutputDirectory = reportingOutputDirectory;
}
const externalLinkSexualHealthClinic = getEnvValue(
EnvironmentVariables.EXTERNAL_LINK_SEXUAL_HEALTH_CLINIC
);
if (externalLinkSexualHealthClinic !== undefined) {
envConfig.externalLinkSexualHealthClinic = externalLinkSexualHealthClinic;
}
const externalLinkNearestAE = getEnvValue(EnvironmentVariables.EXTERNAL_LINK_NEAREST_AE);
if (externalLinkNearestAE !== undefined) {
envConfig.externalLinkNearestAE = externalLinkNearestAE;
}
const externalLinkHivAidsInfo = getEnvValue(EnvironmentVariables.EXTERNAL_LINK_HIV_AIDS_INFO);
if (externalLinkHivAidsInfo !== undefined) {
envConfig.externalLinkHivAidsInfo = externalLinkHivAidsInfo;
}
return envConfig;

Copilot uses AI. Check for mistakes.
@przemyslawbiesek przemyslawbiesek force-pushed the feature/hote-541/add-playwright-tests branch from 17dacc7 to 7b368d6 Compare March 5, 2026 08:49
Copilot AI review requested due to automatic review settings March 11, 2026 10:10
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated no new comments.

Copilot AI review requested due to automatic review settings March 12, 2026 08:32
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated 5 comments.

runs-on: ubuntu-latest
timeout-minutes: 30
env:
TARGET_ENV: ${{ inputs.environment || 'dev' }}
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly, TARGET_ENV is derived from ${{ inputs.environment }} at job scope, which can break scheduled runs where inputs is not available. Prefer ${{ github.event.inputs.environment || 'dev' }} (or set TARGET_ENV in a step that handles both schedule and workflow_dispatch).

Suggested change
TARGET_ENV: ${{ inputs.environment || 'dev' }}
TARGET_ENV: ${{ github.event.inputs.environment || 'dev' }}

Copilot uses AI. Check for mistakes.
Comment on lines +200 to +205
case EnvironmentVariables.EXTERNAL_LINK_SEXUAL_HEALTH_CLINIC:
return config.externalLinkSexualHealthClinic;
case EnvironmentVariables.EXTERNAL_LINK_NEAREST_AE:
return config.externalLinkNearestAE;
case EnvironmentVariables.EXTERNAL_LINK_HIV_AIDS_INFO:
return config.externalLinkHivAidsInfo;
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EnvironmentVariables.EXTERNAL_LINK_* cases are referenced here, but the existing tests/configuration/EnvironmentVariables.ts enum does not define these members. As written, this won’t compile. Either add these enum values (and include them in the config sources) or remove these cases and use a different mechanism for external-link config.

Suggested change
case EnvironmentVariables.EXTERNAL_LINK_SEXUAL_HEALTH_CLINIC:
return config.externalLinkSexualHealthClinic;
case EnvironmentVariables.EXTERNAL_LINK_NEAREST_AE:
return config.externalLinkNearestAE;
case EnvironmentVariables.EXTERNAL_LINK_HIV_AIDS_INFO:
return config.externalLinkHivAidsInfo;

Copilot uses AI. Check for mistakes.
@@ -1,12 +1,26 @@
import * as fs from 'fs';
import * as path from 'path';
import { ConfigFactory } from './configuration/configuration';
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ConfigFactory is imported from ./configuration/configuration but never used. Besides being dead code, it also pulls in the newly added tests/configuration/configuration.ts (which currently has compile issues). Remove the unused import, or switch to the existing configuration/EnvironmentConfiguration if you intended to force config initialization here.

Suggested change
import { ConfigFactory } from './configuration/configuration';

Copilot uses AI. Check for mistakes.
Comment on lines +51 to +53
// React Router v7 warning: root route has a loader but no HydrateFallback defined
// This is a known issue to be fixed in the UI app (add HydrateFallback to root route)
/No `HydrateFallback` element provided to render during initial hydration/,,
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ignorePatterns array has a syntax error: the regex line ends with a double comma (,,). This will prevent TypeScript from compiling and will break the test run. Remove the extra comma (and consider updating the adjacent comment since HydrateFallback is now set in the UI route).

Suggested change
// React Router v7 warning: root route has a loader but no HydrateFallback defined
// This is a known issue to be fixed in the UI app (add HydrateFallback to root route)
/No `HydrateFallback` element provided to render during initial hydration/,,
// React Router v7 warning about HydrateFallback during initial hydration
// HydrateFallback is now set in the UI route; ignore this warning if it still appears
/No `HydrateFallback` element provided to render during initial hydration/,

Copilot uses AI. Check for mistakes.
Comment on lines +1 to +6
import * as dotenv from 'dotenv';
import * as path from 'path';
import * as fs from 'fs';
import { EnvironmentVariables, availableEnvironments, Environment } from './environment-variables';

export { EnvironmentVariables };
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new config module will not compile on a case-sensitive filesystem: it imports ./environment-variables, but the existing file is EnvironmentVariables.ts. Additionally, the switch below references EnvironmentVariables.EXTERNAL_LINK_*, which are not defined in the current EnvironmentVariables enum. Align the import path and either extend the enum or remove those switch cases.

Copilot uses AI. Check for mistakes.
Copilot AI review requested due to automatic review settings March 12, 2026 11:49
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated 3 comments.

Comment on lines +34 to +46
// Handle NHS Login consent page if it appears (first login or expired consent)
// Race between consent page and direct app redirect — whichever arrives first
const consentAppeared = await Promise.race([
page.waitForURL(/nhs-login-consent/, { timeout: 15000 }).then(() => true),
page.waitForURL('**/get-self-test-kit-for-HIV', { timeout: 15000 }).then(() => false)
]).catch(() => false);

if (consentAppeared) {
console.log('Consent page detected, agreeing to share information...');
await consentPage.agreeAndContinue();
}

await page.waitForURL('**/get-self-test-kit-for-HIV', { timeout: 60000 });
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The consent-page detection only waits 15s for either the consent URL or the app redirect. If the consent screen appears after that window (slow auth / network), consentAppeared becomes false and the final wait for the app URL can time out because the consent page is still blocking. Consider waiting for either URL for the full redirect timeout (or looping until one of them occurs) to avoid flakiness.

Copilot uses AI. Check for mistakes.
Comment on lines 67 to 83
- name: "Start the application"
if: env.TARGET_ENV == 'local' || env.TARGET_ENV == 'dev'
run: |
npm run start

- name: "Show application status"
if: env.TARGET_ENV == 'local' || env.TARGET_ENV == 'dev'
run: |
docker compose -f local-environment/docker-compose.yml ps
docker logs ui

- name: "Get terraform outputs"
if: env.TARGET_ENV == 'local' || env.TARGET_ENV == 'dev'
id: terraform
run: |
UI_URL=$(terraform -chdir=local-environment/infra output -raw ui_url)
API_URL=$(terraform -chdir=local-environment/infra output -raw api_base_url)
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For TARGET_ENV=dev this job still starts the local application and fetches terraform outputs, but later overrides URLs to point at the remote dev environment. This adds unnecessary runtime and risk of failure. Consider making these steps conditional on TARGET_ENV == 'local' only (and skip terraform/docker when running against remote dev).

Copilot uses AI. Check for mistakes.
Comment on lines +129 to +145
// Map environment variables to config interface
return {
uiBaseUrl: process.env[EnvironmentVariables.UI_BASE_URL],
apiBaseUrl: process.env[EnvironmentVariables.API_BASE_URL],
headless: process.env[EnvironmentVariables.HEADLESS] === 'true',
timeout: process.env[EnvironmentVariables.TIMEOUT]
? parseInt(process.env[EnvironmentVariables.TIMEOUT], 10)
: undefined,
slowMo: process.env[EnvironmentVariables.SLOW_MO]
? parseInt(process.env[EnvironmentVariables.SLOW_MO], 10)
: undefined,
accessibilityStandards: process.env[EnvironmentVariables.ACCESSIBILITY_STANDARDS],
reportingOutputDirectory: process.env[EnvironmentVariables.REPORTING_OUTPUT_DIRECTORY],
externalLinkSexualHealthClinic: process.env[EnvironmentVariables.EXTERNAL_LINK_SEXUAL_HEALTH_CLINIC],
externalLinkNearestAE: process.env[EnvironmentVariables.EXTERNAL_LINK_NEAREST_AE],
externalLinkHivAidsInfo: process.env[EnvironmentVariables.EXTERNAL_LINK_HIV_AIDS_INFO],
};
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

readConfigurationFromEnvFile() returns an object with keys set directly from process.env[...] even when those env vars are undefined. When spread-merged, this can overwrite defaults with undefined (e.g., uiBaseUrl/apiBaseUrl), causing runtime failures. Filter out undefined values before merging (similar to what readConfigurationFromProcessEnv() already does).

Copilot uses AI. Check for mistakes.
Copilot AI review requested due to automatic review settings March 12, 2026 14:49
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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


await this.captureFailureArtifacts(page, context, user, error as Error, networkErrors);
await browser.close();
console.log(`✅ Successfully logged in worker user ${user.nhsNumber} and saved session state. CATCH`);
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The catch block logs a success message ("✅ Successfully logged in...") immediately before re-throwing the error. This message is misleading and will make failures harder to diagnose; it should be removed or replaced with a failure-specific log.

Suggested change
console.log(`✅ Successfully logged in worker user ${user.nhsNumber} and saved session state. CATCH`);
console.log(`ℹ️ Cleaned up browser after failed login for worker user ${user.nhsNumber}.`);

Copilot uses AI. Check for mistakes.
Comment on lines +26 to +79
// Log file for debugging
const logFile = 'testResults/nhs-login-debug.log';
fs.mkdirSync('testResults', { recursive: true });
const log = (message: string) => {
const timestamp = new Date().toISOString();
const logMessage = `[${timestamp}] ${message}\n`;
console.log(message); // Also log to console
fs.appendFileSync(logFile, logMessage);
};

log('=== NHS Login Flow Started ===');

// Capture all browser console messages
page.on('console', (msg) => {
const type = msg.type();
const text = msg.text();
const location = msg.location();

log(`[Browser Console - ${type.toUpperCase()}] ${text}`);
if (location.url) {
log(` ↳ ${location.url}:${location.lineNumber}:${location.columnNumber}`);
}
});

// Capture all network requests and responses
page.on('request', (request) => {
log(`[Network Request] ${request.method()} ${request.url()}`);
const postData = request.postData();
if (postData) {
log(` ↳ Body: ${postData.substring(0, 200)}${postData.length > 200 ? '...' : ''}`);
}
});

page.on('response', async (response) => {
const status = response.status();
const statusText = response.statusText();
const url = response.url();

log(`[Network Response] ${status} ${statusText} - ${response.request().method()} ${url}`);

// Log response body for non-2xx responses or specific content types
if (status >= 400 || url.includes('/api/')) {
try {
const contentType = response.headers()['content-type'] || '';
if (contentType.includes('application/json')) {
const body = await response.text();
log(` ↳ Response Body: ${body.substring(0, 500)}${body.length > 500 ? '...' : ''}`);
}
} catch (error) {
log(` ↳ Could not read response body: ${error}`);
}
}
});

Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This debug logging captures and persists full request bodies (request.postData()) and selected response bodies to testResults/nhs-login-debug.log. During NHS login this can include credentials/OTP and other sensitive data, which is a security risk (especially if artifacts are uploaded). Please gate this behind an explicit debug flag, redact sensitive fields, and avoid logging auth payloads by default.

Suggested change
// Log file for debugging
const logFile = 'testResults/nhs-login-debug.log';
fs.mkdirSync('testResults', { recursive: true });
const log = (message: string) => {
const timestamp = new Date().toISOString();
const logMessage = `[${timestamp}] ${message}\n`;
console.log(message); // Also log to console
fs.appendFileSync(logFile, logMessage);
};
log('=== NHS Login Flow Started ===');
// Capture all browser console messages
page.on('console', (msg) => {
const type = msg.type();
const text = msg.text();
const location = msg.location();
log(`[Browser Console - ${type.toUpperCase()}] ${text}`);
if (location.url) {
log(` ↳ ${location.url}:${location.lineNumber}:${location.columnNumber}`);
}
});
// Capture all network requests and responses
page.on('request', (request) => {
log(`[Network Request] ${request.method()} ${request.url()}`);
const postData = request.postData();
if (postData) {
log(` ↳ Body: ${postData.substring(0, 200)}${postData.length > 200 ? '...' : ''}`);
}
});
page.on('response', async (response) => {
const status = response.status();
const statusText = response.statusText();
const url = response.url();
log(`[Network Response] ${status} ${statusText} - ${response.request().method()} ${url}`);
// Log response body for non-2xx responses or specific content types
if (status >= 400 || url.includes('/api/')) {
try {
const contentType = response.headers()['content-type'] || '';
if (contentType.includes('application/json')) {
const body = await response.text();
log(` ↳ Response Body: ${body.substring(0, 500)}${body.length > 500 ? '...' : ''}`);
}
} catch (error) {
log(` ↳ Could not read response body: ${error}`);
}
}
});
// Debug logging for NHS login flow, gated behind explicit flag
const isDebugEnabled = process.env.NHS_LOGIN_DEBUG === 'true';
const log = (message: string): void => {
if (!isDebugEnabled) {
return;
}
const logFile = 'testResults/nhs-login-debug.log';
fs.mkdirSync('testResults', { recursive: true });
const timestamp = new Date().toISOString();
const logMessage = `[${timestamp}] ${message}\n`;
console.log(message); // Also log to console
fs.appendFileSync(logFile, logMessage);
};
if (isDebugEnabled) {
log('=== NHS Login Flow Started ===');
// Capture browser console messages (no sensitive data expected)
page.on('console', (msg) => {
const type = msg.type();
const text = msg.text();
const location = msg.location();
log(`[Browser Console - ${type.toUpperCase()}] ${text}`);
if (location.url) {
log(` ↳ ${location.url}:${location.lineNumber}:${location.columnNumber}`);
}
});
// Capture high-level network request metadata (no bodies)
page.on('request', (request) => {
log(`[Network Request] ${request.method()} ${request.url()}`);
});
// Capture high-level network response metadata (no bodies)
page.on('response', (response) => {
const status = response.status();
const statusText = response.statusText();
const url = response.url();
log(
`[Network Response] ${status} ${statusText} - ${response.request().method()} ${url}`
);
});
}

Copilot uses AI. Check for mistakes.
# Development Environment Configuration
UI_BASE_URL=http://localhost:3000
API_BASE_URL=http://localhost:4566/_aws/execute-api/y18amry36z/local
API_BASE_URL=http://localhost:4566/_aws/execute-api/gppofwpzq9/local
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

API_BASE_URL in .env.local is set to a specific LocalStack API Gateway ID (gppofwpzq9). These IDs are typically ephemeral and will change across environments/recreates, so committing a concrete ID can break other developers/CI. Consider leaving this unset (and relying on terraform outputs / scripts) or documenting a stable way to refresh it locally.

Suggested change
API_BASE_URL=http://localhost:4566/_aws/execute-api/gppofwpzq9/local
# API base URL for local testing.
# Do not commit a concrete LocalStack API Gateway ID here, as it is environment-specific.
# Example (for local overrides only): http://localhost:4566/_aws/execute-api/<gateway-id>/local
API_BASE_URL=

Copilot uses AI. Check for mistakes.
Comment on lines 85 to 92
networkErrors.push({
url: response.url(),
status,
statusText: response.statusText(),
method: response.request().method(),
timestamp: new Date().toISOString(),
responseText: response.text().catch(() => "Unable to retrieve response body"),
});
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NetworkError.responseText is stored as a Promise<string> (response.text()), so later JSON.stringify(networkErrors) will serialize it as {} and the saved *-network-errors.json won’t contain the response body. Capture/await the text in the response handler and store it as a string (or omit it) rather than storing a Promise.

Copilot uses AI. Check for mistakes.
Comment on lines +38 to +78
// Capture all browser console messages
page.on('console', (msg) => {
const type = msg.type();
const text = msg.text();
const location = msg.location();

log(`[Browser Console - ${type.toUpperCase()}] ${text}`);
if (location.url) {
log(` ↳ ${location.url}:${location.lineNumber}:${location.columnNumber}`);
}
});

// Capture all network requests and responses
page.on('request', (request) => {
log(`[Network Request] ${request.method()} ${request.url()}`);
const postData = request.postData();
if (postData) {
log(` ↳ Body: ${postData.substring(0, 200)}${postData.length > 200 ? '...' : ''}`);
}
});

page.on('response', async (response) => {
const status = response.status();
const statusText = response.statusText();
const url = response.url();

log(`[Network Response] ${status} ${statusText} - ${response.request().method()} ${url}`);

// Log response body for non-2xx responses or specific content types
if (status >= 400 || url.includes('/api/')) {
try {
const contentType = response.headers()['content-type'] || '';
if (contentType.includes('application/json')) {
const body = await response.text();
log(` ↳ Response Body: ${body.substring(0, 500)}${body.length > 500 ? '...' : ''}`);
}
} catch (error) {
log(` ↳ Could not read response body: ${error}`);
}
}
});
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fillNhsLoginFormsAndWaitForStartPage() attaches page.on('console'|'request'|'response') listeners every time it is called, but never removes them. If this helper is invoked multiple times on the same page, logs will be duplicated and memory usage can grow. Consider registering these listeners once (e.g., in a fixture) or using page.once / removing listeners after the login flow completes.

Copilot uses AI. Check for mistakes.
Comment on lines +31 to 33
'''.idea/*''',
'''.pre-commit-config.yaml'''
]
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding .pre-commit-config.yaml to the global gitleaks allowlist excludes it from secret scanning entirely. Since hook configs can contain tokens/URLs/credentials, this weakens detection. Prefer narrowing the allowlist to the specific false-positive pattern(s) (via rule-level allowlists) rather than excluding the whole file.

Suggested change
'''.idea/*''',
'''.pre-commit-config.yaml'''
]
'''.idea/*'''
]

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants