Refactor email subscription E2E tests to use journey-based approach #154
Conversation
…ith test isolation Reduces test count from 22 to 3 by merging related tests into comprehensive user journey flows. Each test now creates and cleans up its own isolated test location, preventing parallel test conflicts. Improves selectors by using semantic role-based queries instead of positional selectors. Integrates accessibility checks along the journey rather than as separate tests. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
WalkthroughAdds Prisma-backed per-test location creation and cleanup to email-subscriptions E2E tests, consolidates multiple tests into a single end-to-end "Subscription Journey" with Axe accessibility scans, and replaces per-page auth checks with a loop over protected URLs. (47 words) Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
e2e-tests/tests/email-subscriptions.spec.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: TypeScript variables must use camelCase (e.g.,userId,caseDetails,documentId). Booleans should useis/has/canprefix (e.g.,isActive,hasAccess,canEdit).
Classes and interfaces must use PascalCase (e.g.,UserService,CaseRepository). DO NOT useIprefix for interfaces.
Constants must use SCREAMING_SNAKE_CASE (e.g.,MAX_FILE_SIZE,DEFAULT_TIMEOUT).
Module ordering: constants at the top, exported functions next, other functions ordered by usage, interfaces and types at the bottom.
Always add.jsextension to relative imports (e.g.,import { foo } from "./bar.js"). This is required for ESM with Node.js "nodenext" module resolution, even when importing TypeScript files.
Use workspace aliases (@hmcts/*) for imports instead of relative paths across packages.
Database queries must be parameterized using Prisma (never raw SQL with string concatenation).
Never put sensitive data in logs.
Don't add comments unless meaningful - explain why something is done, not what is done.
Favour functional style - use simple functions. Don't use a class unless you have shared state.
Data should be immutable by default - use const and avoid mutations to ensure predictable state.
Functions should have no side effects - avoid modifying external state or relying on mutable data.
Files:
e2e-tests/tests/email-subscriptions.spec.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Files and directories must use kebab-case (e.g.,user-service.ts,case-management/).
Never use CommonJS (require(),module.exports). Use ES modules (import/export) exclusively.
Files:
e2e-tests/tests/email-subscriptions.spec.ts
**/*.{test,spec}.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Test files must be co-located with source files using the pattern
*.test.tsor*.spec.tsand use Vitest.
Files:
e2e-tests/tests/email-subscriptions.spec.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: E2E Tests
🔇 Additional comments (6)
e2e-tests/tests/email-subscriptions.spec.ts (6)
1-4: LGTM!The import statements follow the coding guidelines correctly with
.jsextensions for relative imports and workspace aliases for cross-package imports.
6-9: LGTM!The test-scoped variables follow camelCase naming conventions and appropriately use
letfor mutable test state.
52-69: LGTM!The cleanup function appropriately handles the deletion order (subscriptions before location) and includes defensive error handling for idempotent cleanup.
73-102: LGTM!The test lifecycle hooks provide proper test isolation by creating and cleaning up a unique test location for each test.
121-124: Excellent integration of accessibility checks throughout the journey.The accessibility scans at key points in the user flow ensure WCAG compliance is maintained across all subscription pages.
Also applies to: 140-143, 178-181, 196-199
269-288: LGTM!The array-based approach for testing authentication protection is clean and maintainable, ensuring all subscription pages are properly protected.
🎭 Playwright E2E Test Results395 tests 395 ✅ 30m 43s ⏱️ Results for commit cf10dd3. ♻️ This comment has been updated with latest results. |
Changed testLocationId generation from limited 9,000 value range (90000 + random * 9000) to much larger namespace using Date.now() multiplied by 1000 plus random 0-999, providing virtually collision-free IDs for parallel test runs. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Expands the update block in the location upsert to include all scalar fields (email, contactNo) and relationships (locationSubJurisdictions, locationRegions) to prevent stale data if the location already exists. Uses deleteMany + create pattern to fully refresh relationships in the update path. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
e2e-tests/tests/email-subscriptions.spec.ts (3)
111-111: Ambiguous.last()selector.Using
.last()to select the "email subscriptions" link is fragile if the page structure changes. Per previous review, consider using a more explicit selector.- const emailSubsTile = page.getByRole("link", { name: /email subscriptions/i }).last(); + const emailSubsTile = page.locator(".verified-tile").filter({ hasText: /email subscriptions/i });
46-49: Incomplete update in upsert may leave stale data.The update block only updates
nameandwelshName, leavingcontactNo, and relationships unchanged if the location already exists from a failed cleanup.update: { name: testLocationName, welshName: testLocationWelshName, + email: "[email protected]", + contactNo: "01234567890", },
208-268: Test will always skip - no subscriptions exist.The
beforeEachhook creates a fresh test location with no subscriptions. Sincecount > 0(line 215) will always be false, the entire unsubscribe flow is never executed.Add subscription setup at the start of this test:
test.describe("Unsubscribe Journey", () => { test("should complete unsubscribe flow with validation and accessibility checks", async ({ page }) => { + // First create a subscription to unsubscribe from + await page.goto("/account-home"); + await page.getByRole("link", { name: /email subscriptions/i }).last().click(); + await page.getByRole("button", { name: /add email subscription/i }).click(); + await page.waitForLoadState("networkidle"); + await page.locator(`#location-${testLocationId}`).check(); + await page.locator("form[method='post']").getByRole("button", { name: /continue/i }).click(); + await page.getByRole("button", { name: /confirm/i }).click(); + await expect(page).toHaveURL("/subscription-confirmed", { timeout: 10000 }); + await page.goto("/subscription-management");Alternatively, remove the
if (count > 0)guard entirely so that test failure is explicit when subscriptions are missing.
🧹 Nitpick comments (1)
e2e-tests/tests/email-subscriptions.spec.ts (1)
66-69: Avoid logging full error objects.Logging the entire
errorobject could potentially expose sensitive information from database responses. Log only the error message for safer debugging.} catch (error) { // Ignore if location doesn't exist - console.log("Test location cleanup:", error); + console.log("Test location cleanup:", error instanceof Error ? error.message : "Unknown error"); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
e2e-tests/tests/email-subscriptions.spec.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: TypeScript variables must use camelCase (e.g.,userId,caseDetails,documentId). Booleans should useis/has/canprefix (e.g.,isActive,hasAccess,canEdit).
Classes and interfaces must use PascalCase (e.g.,UserService,CaseRepository). DO NOT useIprefix for interfaces.
Constants must use SCREAMING_SNAKE_CASE (e.g.,MAX_FILE_SIZE,DEFAULT_TIMEOUT).
Module ordering: constants at the top, exported functions next, other functions ordered by usage, interfaces and types at the bottom.
Always add.jsextension to relative imports (e.g.,import { foo } from "./bar.js"). This is required for ESM with Node.js "nodenext" module resolution, even when importing TypeScript files.
Use workspace aliases (@hmcts/*) for imports instead of relative paths across packages.
Database queries must be parameterized using Prisma (never raw SQL with string concatenation).
Never put sensitive data in logs.
Don't add comments unless meaningful - explain why something is done, not what is done.
Favour functional style - use simple functions. Don't use a class unless you have shared state.
Data should be immutable by default - use const and avoid mutations to ensure predictable state.
Functions should have no side effects - avoid modifying external state or relying on mutable data.
Files:
e2e-tests/tests/email-subscriptions.spec.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Files and directories must use kebab-case (e.g.,user-service.ts,case-management/).
Never use CommonJS (require(),module.exports). Use ES modules (import/export) exclusively.
Files:
e2e-tests/tests/email-subscriptions.spec.ts
**/*.{test,spec}.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Test files must be co-located with source files using the pattern
*.test.tsor*.spec.tsand use Vitest.
Files:
e2e-tests/tests/email-subscriptions.spec.ts
🔇 Additional comments (5)
e2e-tests/tests/email-subscriptions.spec.ts (5)
1-4: LGTM!Imports correctly use the
.jsextension for local imports and workspace aliases for cross-package imports as per coding guidelines.
6-9: Test-scoped state is acceptable here.Mutable
letdeclarations are appropriate for test lifecycle variables that are assigned inbeforeEachand consumed across test steps.
74-103: LGTM - Test lifecycle follows best practices.Creating isolated test data in
beforeEachand cleaning up inafterEachensures test isolation and prevents parallel test conflicts.
105-206: Well-structured subscription journey test.The consolidated journey approach with accessibility checks at each step is good. Using the specific test location ID (line 159) ensures proper test isolation.
270-289: Good consolidation of authentication tests.Looping through protected pages with a single test is cleaner than individual tests. Clearing cookies properly simulates unauthenticated access.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
e2e-tests/tests/email-subscriptions.spec.ts (3)
14-14: This issue has already been flagged in previous reviews.The ID generation
Date.now() * 1000produces values exceeding PostgreSQL INTEGER (Int32) limits, causing database insertion failures. See the existing critical issue comment for the recommended fix using modulo to constrain the value.
125-125: This issue has already been flagged in previous reviews.The
.last()selector on a role-based query for "email subscriptions" is ambiguous. See the existing minor issue comment for the recommended fix using an explicit CSS class selector.
222-282: This issue has already been flagged in previous reviews.The test creates a fresh location with no subscriptions in
beforeEach, so theif (count > 0)check on line 229 will always be false, causing the entire test to skip. See the existing critical issue comment for the recommended fix (creating a subscription at the start of this test).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
e2e-tests/tests/email-subscriptions.spec.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: TypeScript variables must use camelCase (e.g.,userId,caseDetails,documentId). Booleans should useis/has/canprefix (e.g.,isActive,hasAccess,canEdit).
Classes and interfaces must use PascalCase (e.g.,UserService,CaseRepository). DO NOT useIprefix for interfaces.
Constants must use SCREAMING_SNAKE_CASE (e.g.,MAX_FILE_SIZE,DEFAULT_TIMEOUT).
Module ordering: constants at the top, exported functions next, other functions ordered by usage, interfaces and types at the bottom.
Always add.jsextension to relative imports (e.g.,import { foo } from "./bar.js"). This is required for ESM with Node.js "nodenext" module resolution, even when importing TypeScript files.
Use workspace aliases (@hmcts/*) for imports instead of relative paths across packages.
Database queries must be parameterized using Prisma (never raw SQL with string concatenation).
Never put sensitive data in logs.
Don't add comments unless meaningful - explain why something is done, not what is done.
Favour functional style - use simple functions. Don't use a class unless you have shared state.
Data should be immutable by default - use const and avoid mutations to ensure predictable state.
Functions should have no side effects - avoid modifying external state or relying on mutable data.
Files:
e2e-tests/tests/email-subscriptions.spec.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Files and directories must use kebab-case (e.g.,user-service.ts,case-management/).
Never use CommonJS (require(),module.exports). Use ES modules (import/export) exclusively.
Files:
e2e-tests/tests/email-subscriptions.spec.ts
**/*.{test,spec}.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Test files must be co-located with source files using the pattern
*.test.tsor*.spec.tsand use Vitest.
Files:
e2e-tests/tests/email-subscriptions.spec.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: E2E Tests
🔇 Additional comments (5)
e2e-tests/tests/email-subscriptions.spec.ts (5)
46-63: Well done - upsert update block is now complete.The update block correctly includes all fields (
contactNo) and properly handles relationships by deleting existing records before creating new ones. This addresses the previous concern about stale data from incomplete updates.
67-84: LGTM - cleanup logic is robust.The error-tolerant cleanup properly handles the deletion order (subscriptions before location) and gracefully handles missing data with an early return and catch block. This prevents test failures during teardown.
173-177: Excellent use of test-specific location ID.Using
page.locator(\#location-${testLocationId}`)` ensures the test interacts with its own isolated test data rather than relying on generic selectors. This is a good practice for test isolation.
285-302: Good refactoring - consolidates auth tests efficiently.The loop-based approach to test authentication protection across all subscription pages is cleaner than separate test cases. Clearing cookies ensures a truly unauthenticated state.
289-296: The route handler correctly usesrequireAuth()middleware that executes before the handler logic, so the authentication check runs first and will redirect unauthenticated requests to sign-in before any subscription validation occurs. The test is correct as-is; the hardcoded UUID never needs to exist since the middleware prevents unauthenticated access.
…scriptions tile
Changes the email subscriptions tile selector from `getByRole("link").last()` to `locator(".verified-tile").nth(2)` for clarity and resilience. Also fixes location ID generation that was changed during rebase to use values too large for INT4 - reverts to safe 90000-98999 range.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <[email protected]>
The beforeEach hook creates a fresh location with no subscriptions, causing the unsubscribe test to skip its entire body due to the if (count > 0) guard. Now creates a subscription at the start of the unsubscribe test, then tests the full deletion flow including validation, cancel option, and successful unsubscription with accessibility checks. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
e2e-tests/tests/email-subscriptions.spec.ts (3)
6-9: Global mutable test location state undermines isolation and parallel safety
testLocationId/testLocationName/testLocationWelshNameare module-scopedlets mutated bycreateTestLocationand read by tests/cleanup. UnderfullyParallelor multi-worker runs, different tests in this file can race on these values, leading to:
- One test deleting another test’s location/subscriptions in
deleteTestLocation.- Locators (e.g.
#location-${testLocationId}) pointing at the wrong location mid-test.This was already raised in a previous review; it’s still a risk with the current structure.
A more robust pattern is to keep the state per-test instead of global, e.g.:
- let testLocationId: number; - let testLocationName: string; - let testLocationWelshName: string; - -async function createTestLocation(): Promise<void> { - // ... - testLocationId = /* generated */; - testLocationName = /* generated */; - testLocationWelshName = /* generated */; +type TestLocation = { + id: number; + name: string; + welshName: string; +}; + +async function createTestLocation(): Promise<TestLocation> { + const id = /* generated */; + const name = /* generated */; + const welshName = /* generated */; // Prisma upsert using `id`, `name`, `welshName` - await prisma.location.upsert({ ... }); + await prisma.location.upsert({ ... }); + return { id, name, welshName }; } -async function deleteTestLocation(): Promise<void> { - if (!testLocationId) return; - await prisma.subscription.deleteMany({ where: { locationId: testLocationId } }); - await prisma.location.delete({ where: { locationId: testLocationId } }); +async function deleteTestLocation(id: number): Promise<void> { + await prisma.subscription.deleteMany({ where: { locationId: id } }); + await prisma.location.delete({ where: { locationId: id } }); }Then wire this through
beforeEach/afterEachusing either a dedicated Playwright fixture or a per-test variable scoped inside the hook (e.g. viatest.extend), so each test gets its own location id without sharing module state.Also applies to: 11-16, 66-83
11-15: Random testLocationId range is very small and can collide across runs
testLocationId = 90000 + Math.floor(Math.random() * 9000);gives only 9,000 possible IDs. With repeated CI runs or multiple workers, it’s quite realistic to:
- Hit the same
locationIdas a previous run and upsert over its data.- Have two workers share the same
locationIdat the same time, so one test’s cleanup deletes another’s data.This was highlighted earlier; the issue remains with the current generator.
Consider incorporating time and/or worker index while still staying within the DB’s integer bounds and any domain constraints you have for test IDs, for example:
const workerId = Number(process.env.PLAYWRIGHT_WORKER_INDEX ?? 0); const tsPart = Date.now() % 1_000_000; // 0..999,999 const randPart = Math.floor(Math.random() * 1000); // 0..999 testLocationId = 90_000 + ((tsPart * 10 + workerId + randPart) % 900_000); // keeps within a “test-only” high rangeThat gives a much larger effective namespace and significantly reduces collision risk while respecting integer limits.
221-281: Unsubscribe Journey can silently become a no-op and still passThis test only executes the unsubscribe flow if there is already at least one “remove” button:
const deleteLinks = page.getByRole("button", { name: /remove/i }); const count = await deleteLinks.count(); if (count > 0) { // entire test body }If the user has zero subscriptions (e.g. running this spec in isolation, or the data has been reset), the test does nothing and still passes. That means you don’t actually assert the unsubscribe journey in the common “no pre-existing data” case. This was already pointed out in a previous review.
To make the test self-contained and aligned with the isolation goal of this PR:
- Create a subscription within this test (or via a small helper) for the current
testLocationId.- Remove the
if (count > 0)guard and instead expect at least one delete button; let the test fail loudly if setup breaks.Conceptually:
- await page.goto("/subscription-management"); - const deleteLinks = page.getByRole("button", { name: /remove/i }); - const count = await deleteLinks.count(); - - if (count > 0) { + // Ensure there is a subscription to remove (reuse the subscription-creation steps) + await ensureTestSubscriptionExists(page, testLocationId); + + await page.goto("/subscription-management"); + const deleteLinks = page.getByRole("button", { name: /remove/i }); + await expect(deleteLinks).toHaveCountGreaterThan(0); // pseudo-assertion; implement via `toBeGreaterThan(0)` // ... rest of unsubscribe flow without the guard ... - }This guarantees the test always exercises the full unsubscribe path and doesn’t depend on ordering or leftovers from other tests.
🧹 Nitpick comments (2)
e2e-tests/tests/email-subscriptions.spec.ts (2)
66-83: Cleanup swallows all errors; consider narrowing what you ignore
deleteTestLocationwraps all Prisma calls in a broadtry/catchand logs the error, which means:
- Genuine DB issues during cleanup (e.g. connectivity problems) are silently downgraded to a log line.
- Tests may appear to pass while leaving data behind.
It’s reasonable to ignore “record not found” for idempotent cleanup, but other failures are worth surfacing.
Example refinement:
async function deleteTestLocation(id: number): Promise<void> { if (!id) return; try { await prisma.subscription.deleteMany({ where: { locationId: id } }); await prisma.location.delete({ where: { locationId: id } }); } catch (error: any) { // Ignore only known "not found" errors if (error?.code !== "P2025") { throw error; } } }This keeps the cleanup idempotent but still fails fast on unexpected DB problems.
118-219: Subscription Journey flow is solid; selector can be made more semantic and less positionalThe end-to-end subscription journey plus inline Axe checks is well structured and gives good coverage of the main flow.
The one brittle spot is the tile selector:
const emailSubsTile = page.locator(".verified-tile").nth(2);This depends on the tile order staying fixed. You can keep using the
.verified-tileclass but avoid positional coupling by filtering on text:const emailSubsTile = page.locator(".verified-tile", { hasText: /email subscriptions/i, });That still targets the main tile explicitly but is more resilient to reordering. Please double-check the exact Playwright
locatoroptions signature against the version you’re using.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
e2e-tests/tests/email-subscriptions.spec.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: TypeScript variables must use camelCase (e.g.,userId,caseDetails,documentId). Booleans should useis/has/canprefix (e.g.,isActive,hasAccess,canEdit).
Classes and interfaces must use PascalCase (e.g.,UserService,CaseRepository). DO NOT useIprefix for interfaces.
Constants must use SCREAMING_SNAKE_CASE (e.g.,MAX_FILE_SIZE,DEFAULT_TIMEOUT).
Module ordering: constants at the top, exported functions next, other functions ordered by usage, interfaces and types at the bottom.
Always add.jsextension to relative imports (e.g.,import { foo } from "./bar.js"). This is required for ESM with Node.js "nodenext" module resolution, even when importing TypeScript files.
Use workspace aliases (@hmcts/*) for imports instead of relative paths across packages.
Database queries must be parameterized using Prisma (never raw SQL with string concatenation).
Never put sensitive data in logs.
Don't add comments unless meaningful - explain why something is done, not what is done.
Favour functional style - use simple functions. Don't use a class unless you have shared state.
Data should be immutable by default - use const and avoid mutations to ensure predictable state.
Functions should have no side effects - avoid modifying external state or relying on mutable data.
Files:
e2e-tests/tests/email-subscriptions.spec.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Files and directories must use kebab-case (e.g.,user-service.ts,case-management/).
Never use CommonJS (require(),module.exports). Use ES modules (import/export) exclusively.
Files:
e2e-tests/tests/email-subscriptions.spec.ts
**/*.{test,spec}.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Test files must be co-located with source files using the pattern
*.test.tsor*.spec.tsand use Vitest.
Files:
e2e-tests/tests/email-subscriptions.spec.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: E2E Tests
🔇 Additional comments (2)
e2e-tests/tests/email-subscriptions.spec.ts (2)
4-4: Prisma import via workspace alias and ESM looks goodUsing
import { prisma } from "@hmcts/postgres";matches the workspace-alias and ES module guidelines.
283-301: Authentication protection loop is concise and effectiveClearing cookies and looping over
protectedPagesto assert redirect to/sign-inkeeps auth checks DRY and readable. This nicely replaces multiple near-duplicate tests.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
e2e-tests/tests/email-subscriptions.spec.ts (3)
14-15: Consider using testLocationId in names for better traceability.The location names use independent
Date.now()andMath.random()values rather than incorporatingtestLocationId. Including the ID in the names would improve traceability when debugging test failures.- testLocationName = `E2E Test Location ${Date.now()}-${Math.random()}`; - testLocationWelshName = `Lleoliad Prawf E2E ${Date.now()}-${Math.random()}`; + testLocationName = `E2E Test Location ${testLocationId}`; + testLocationWelshName = `Lleoliad Prawf E2E ${testLocationId}`;</review_comment_end>
66-83: LGTM! Error-tolerant cleanup logic.The deletion order (subscriptions first, then location) respects foreign key constraints, and the try-catch ensures cleanup failures don't break tests.
Optionally, consider using Playwright's test reporter instead of
console.logfor better test output integration:} catch (error) { - // Ignore if location doesn't exist - console.log("Test location cleanup:", error); + // Location may not exist if test failed early }Or use
test.info().attach()if you need to preserve error details.
</review_comment_end>
221-287: LGTM! Complete unsubscribe journey with proper setup.The test correctly creates a subscription first (addressing the previous critical issue), then thoroughly tests the unsubscribe flow including validation errors, cancel behavior, and success confirmation.
The subscription creation in lines 223-234 duplicates code from the Subscription Journey test. If this pattern is used more than twice, consider extracting to a helper function:
async function createSubscriptionForLocation(page, locationId) { await page.goto("/account-home"); const emailSubsTile = page.locator(".verified-tile").nth(2); await emailSubsTile.click(); await page.getByRole("button", { name: /add email subscription/i }).click(); await page.waitForLoadState("networkidle"); const testLocationCheckbox = page.locator(`#location-${locationId}`); await testLocationCheckbox.check(); await page.locator("form[method='post']").getByRole("button", { name: /continue/i }).click(); await page.getByRole("button", { name: /confirm/i }).click(); await expect(page).toHaveURL("/subscription-confirmed", { timeout: 10000 }); }</review_comment_end>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
e2e-tests/tests/email-subscriptions.spec.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: TypeScript variables must use camelCase (e.g.,userId,caseDetails,documentId). Booleans should useis/has/canprefix (e.g.,isActive,hasAccess,canEdit).
Classes and interfaces must use PascalCase (e.g.,UserService,CaseRepository). DO NOT useIprefix for interfaces.
Constants must use SCREAMING_SNAKE_CASE (e.g.,MAX_FILE_SIZE,DEFAULT_TIMEOUT).
Module ordering: constants at the top, exported functions next, other functions ordered by usage, interfaces and types at the bottom.
Always add.jsextension to relative imports (e.g.,import { foo } from "./bar.js"). This is required for ESM with Node.js "nodenext" module resolution, even when importing TypeScript files.
Use workspace aliases (@hmcts/*) for imports instead of relative paths across packages.
Database queries must be parameterized using Prisma (never raw SQL with string concatenation).
Never put sensitive data in logs.
Don't add comments unless meaningful - explain why something is done, not what is done.
Favour functional style - use simple functions. Don't use a class unless you have shared state.
Data should be immutable by default - use const and avoid mutations to ensure predictable state.
Functions should have no side effects - avoid modifying external state or relying on mutable data.
Files:
e2e-tests/tests/email-subscriptions.spec.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Files and directories must use kebab-case (e.g.,user-service.ts,case-management/).
Never use CommonJS (require(),module.exports). Use ES modules (import/export) exclusively.
Files:
e2e-tests/tests/email-subscriptions.spec.ts
**/*.{test,spec}.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Test files must be co-located with source files using the pattern
*.test.tsor*.spec.tsand use Vitest.
Files:
e2e-tests/tests/email-subscriptions.spec.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: E2E Tests
🔇 Additional comments (5)
e2e-tests/tests/email-subscriptions.spec.ts (5)
26-63: LGTM! Complete upsert implementation.The upsert properly handles both creation and updates with all fields and relationships. The update block correctly deletes and recreates relationships to ensure clean state.
</review_comment_end>
87-111: LGTM! Clean test setup with isolation.The
beforeEachhook properly creates an isolated test location and authenticates before each test, ensuring test independence.</review_comment_end>
114-116: LGTM! Proper test cleanup.The
afterEachhook ensures test data is cleaned up, preventing test pollution.</review_comment_end>
118-219: LGTM! Comprehensive subscription journey with accessibility checks.The test properly:
- Uses the specific test location via
testLocationIdfor isolation- Includes accessibility scans at each step
- Tests back navigation
- Verifies all key pages in the journey
The use of
.nth(2)for tile selection is appropriate and more explicit than role-based selectors with.last().</review_comment_end>
289-308: LGTM! Efficient authentication protection validation.The test consolidates multiple authentication checks into a single loop, verifying that all subscription-related pages properly redirect unauthenticated users to the sign-in page. The use of a dummy UUID in the query parameter is appropriate for testing authentication guards.
</review_comment_end>
Replaces module-scoped mutable variables (testLocationId, testLocationName, testLocationWelshName) with a Map keyed by test ID. This prevents race conditions when tests run in parallel with fullyParallel: true. Each test now gets its own isolated location data stored in testLocationMap, retrieved via testInfo.testId. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Increases ID collision resistance from 9K to 100M possible values by combining timestamp (8 digits, cycles every ~3 years) with random component (6 digits). IDs range from 90M-189M, safely within INT4 bounds. This virtually eliminates collision risk during parallel test execution. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
…esistance Increases namespace from 100M to 1B possible values (1000000000-1999999999) by combining timestamp and random components with 1B range each. References commit d00d399 which introduced the larger namespace approach. IDs stay safely under PostgreSQL INTEGER limit (2,147,483,647) while providing excellent collision resistance for parallel test execution. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
e2e-tests/tests/email-subscriptions.spec.ts (4)
16-81: ID generation and upsert look correct; minor comment clean-up only.The new
createTestLocationimplementation addresses the earlier issues well:
- ID space is large (1e9-range), stays within Postgres
INTlimits, and uses an offset to avoid clashing with seed data.- The upsert
updateblock now fully overwritesname,welshName,contactNo, and resets relation tables viadeleteMany+create, which should prevent stale test data.Two small nits you might consider (non-blocking):
- The “~2B namespace” comment is slightly misleading now that you mod back to 1e9 and then offset; you could simplify the explanation to “1B ID namespace in the 1,000,000,000–1,999,999,999 range under the INT limit”.
- Using
testLocationIdin the human-readable names (instead of separateDate.now()/Math.random()) can make DB debugging easier, e.g.E2E Test Location ${testLocationId}.
84-101: Narrow error handling indeleteTestLocationto avoid hiding real failures.
deleteTestLocationcurrently swallows all errors and just logs them. That’s OK for “not found” cases, but it could also hide genuine DB or schema issues that you’d probably want tests to fail on.Consider:
- Detecting and ignoring only the “record not found”/idempotent cleanup case.
- Re-throwing or
console.error-ing other unexpected errors so they surface in CI.For example (pseudocode-level):
try { // deleteMany + delete } catch (error) { // if known "not found" case, ignore; otherwise rethrow }This keeps cleanup idempotent while making real problems visible.
104-139: Per-test Map works; consider a fixture for cleaner state wiring.Using
testLocationMapkeyed bytestInfo.testIdplusbeforeEach/afterEachgives you proper per-test isolation and fixes the earlier module-scoped mutable state issue.If you want to tighten things up later, you could:
- Wrap
createTestLocation/deleteTestLocationinto a custom Playwright fixture (e.g.test.extend<{ locationData: TestLocationData }>(...)) so tests receivelocationDataas a typed fixture rather than reaching into a shared Map.- This would remove the need to manually
get/set/deletefromtestLocationMapand make the dependency explicit in the test signatures.Not required for correctness, but it would simplify the test harness.
141-246: Subscription journey flow is solid;.nth(2)selector may be brittle.The end-to-end subscription journey and embedded Axe scans are nicely structured and exercise the main path thoroughly.
One potential fragility point is:
- Line 151:
const emailSubsTile = page.locator(".verified-tile").nth(2);This assumes the “email subscriptions” tile will always be the third
.verified-tile. If tile ordering changes (e.g. new tile added above), this test will start driving the wrong journey.If the DOM allows it, consider a more semantic locator scoped to the main content, such as:
- Scoping the existing
getByRole("link", { name: /email subscriptions/i })to the main content/tiles container (e.g. amainor.verified-tilesregion), instead of depending on index.- Or locating a
.verified-tilethat contains a link with the “email subscriptions” accessible name, then clicking that link.That would keep the selector robust while still avoiding the header link ambiguity mentioned in earlier reviews.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
e2e-tests/tests/email-subscriptions.spec.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: TypeScript variables must use camelCase (e.g.,userId,caseDetails,documentId). Booleans should useis/has/canprefix (e.g.,isActive,hasAccess,canEdit).
Classes and interfaces must use PascalCase (e.g.,UserService,CaseRepository). DO NOT useIprefix for interfaces.
Constants must use SCREAMING_SNAKE_CASE (e.g.,MAX_FILE_SIZE,DEFAULT_TIMEOUT).
Module ordering: constants at the top, exported functions next, other functions ordered by usage, interfaces and types at the bottom.
Always add.jsextension to relative imports (e.g.,import { foo } from "./bar.js"). This is required for ESM with Node.js "nodenext" module resolution, even when importing TypeScript files.
Use workspace aliases (@hmcts/*) for imports instead of relative paths across packages.
Database queries must be parameterized using Prisma (never raw SQL with string concatenation).
Never put sensitive data in logs.
Don't add comments unless meaningful - explain why something is done, not what is done.
Favour functional style - use simple functions. Don't use a class unless you have shared state.
Data should be immutable by default - use const and avoid mutations to ensure predictable state.
Functions should have no side effects - avoid modifying external state or relying on mutable data.
Files:
e2e-tests/tests/email-subscriptions.spec.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Files and directories must use kebab-case (e.g.,user-service.ts,case-management/).
Never use CommonJS (require(),module.exports). Use ES modules (import/export) exclusively.
Files:
e2e-tests/tests/email-subscriptions.spec.ts
**/*.{test,spec}.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Test files must be co-located with source files using the pattern
*.test.tsor*.spec.tsand use Vitest.
Files:
e2e-tests/tests/email-subscriptions.spec.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: E2E Tests
🔇 Additional comments (1)
e2e-tests/tests/email-subscriptions.spec.ts (1)
321-337: Auth protection loop is concise and effective.The consolidated “Authentication Protection” test is a good simplification:
- Clearing cookies once then iterating
protectedPageskeeps coverage high with minimal code.- Asserting redirect to
/sign-infor each route makes the expectations explicit and easy to extend.No issues from my side here.
Changes from using deleteLinks.first() (which could click any subscription's Remove button) to targeting the specific test location subscription using its aria-label. Both the validation ("No") path and final unsubscribe ("Yes") path now correctly target only the subscription created by this test, preventing accidental deletion of pre-existing subscriptions.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <[email protected]>
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
e2e-tests/tests/email-subscriptions.spec.ts (1)
26-27: Optional: Consider incorporatingtestLocationIdin names for easier debugging.Using independent timestamps for names makes it harder to correlate logs/screenshots with specific location IDs when debugging test failures.
Consider this diff:
- const testLocationName = `E2E Test Location ${Date.now()}-${Math.random()}`; - const testLocationWelshName = `Lleoliad Prawf E2E ${Date.now()}-${Math.random()}`; + const testLocationName = `E2E Test Location ${testLocationId}`; + const testLocationWelshName = `Lleoliad Prawf E2E ${testLocationId}`;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
e2e-tests/tests/email-subscriptions.spec.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: TypeScript variables must use camelCase (e.g.,userId,caseDetails,documentId). Booleans should useis/has/canprefix (e.g.,isActive,hasAccess,canEdit).
Classes and interfaces must use PascalCase (e.g.,UserService,CaseRepository). DO NOT useIprefix for interfaces.
Constants must use SCREAMING_SNAKE_CASE (e.g.,MAX_FILE_SIZE,DEFAULT_TIMEOUT).
Module ordering: constants at the top, exported functions next, other functions ordered by usage, interfaces and types at the bottom.
Always add.jsextension to relative imports (e.g.,import { foo } from "./bar.js"). This is required for ESM with Node.js "nodenext" module resolution, even when importing TypeScript files.
Use workspace aliases (@hmcts/*) for imports instead of relative paths across packages.
Database queries must be parameterized using Prisma (never raw SQL with string concatenation).
Never put sensitive data in logs.
Don't add comments unless meaningful - explain why something is done, not what is done.
Favour functional style - use simple functions. Don't use a class unless you have shared state.
Data should be immutable by default - use const and avoid mutations to ensure predictable state.
Functions should have no side effects - avoid modifying external state or relying on mutable data.
Files:
e2e-tests/tests/email-subscriptions.spec.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Files and directories must use kebab-case (e.g.,user-service.ts,case-management/).
Never use CommonJS (require(),module.exports). Use ES modules (import/export) exclusively.
Files:
e2e-tests/tests/email-subscriptions.spec.ts
**/*.{test,spec}.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Test files must be co-located with source files using the pattern
*.test.tsor*.spec.tsand use Vitest.
Files:
e2e-tests/tests/email-subscriptions.spec.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: E2E Tests
🔇 Additional comments (7)
e2e-tests/tests/email-subscriptions.spec.ts (7)
6-14: Excellent refactoring to fix parallel test isolation!The Map-based approach using
testInfo.testIdas the key successfully addresses the race condition issue flagged in past reviews. This ensures each test has isolated location data even when running in parallel.
16-82: LGTM! ID generation and upsert logic are sound.The ID generation approach now:
- Stays safely within PostgreSQL INTEGER limits (max 1,999,999,999 < 2,147,483,647)
- Provides excellent collision resistance with ~1B unique values
- Successfully addresses past review concerns about both collision risk and integer overflow
The upsert update block properly includes all fields and recreates relationships, ensuring no stale data remains from failed cleanups.
84-101: LGTM! Cleanup logic is robust.The function correctly:
- Deletes subscriptions before the location to avoid foreign key violations
- Guards against missing location data
- Handles cleanup errors gracefully without failing the test
103-139: LGTM! Test lifecycle properly manages isolated test data.The beforeEach/afterEach hooks correctly:
- Create isolated test locations per test using testInfo.testId as the key
- Clean up test data after each test
- Remove entries from the map to prevent memory leaks
This approach ensures proper test isolation in parallel execution.
141-246: LGTM! Comprehensive journey test with excellent isolation.The test successfully:
- Retrieves test-specific location data from the map (line 144-145)
- Targets the specific test location by ID (line 199), ensuring proper test isolation
- Integrates accessibility checks at each major step
- Uses the improved
.locator(".verified-tile").nth(2)selector (line 151) as suggested in past reviews
248-323: Excellent! Past critical issues fully resolved.This test now correctly:
- Creates a subscription first (lines 254-265), fixing the past issue where the test would always skip
- Targets the specific test location subscription using aria-label (lines 271-274, 295-298) instead of using
.first(), ensuring it only manipulates the test's own data- Tests validation, "No" flow, and complete unsubscribe flow
- Integrates accessibility checks
The test is now properly isolated and won't accidentally delete other subscriptions.
325-344: LGTM! Clean consolidation of authentication checks.The loop-based approach efficiently verifies that all subscription pages require authentication, replacing multiple individual test blocks with a single maintainable test.
3 participants
Jira link
https://tools.hmcts.net/jira/browse/VIBE-293
Change description
Refactor email subscription E2E tests to use journey-based approach with test isolation
Reduces test count from 22 to 3 by merging related tests into comprehensive user
journey flows. Each test now creates and cleans up its own isolated test location,
preventing parallel test conflicts. Improves selectors by using semantic role-based
queries instead of positional selectors. Integrates accessibility checks along the
journey rather than as separate tests.
Testing done
yes
Security Vulnerability Assessment
CVE Suppression: Are there any CVEs present in the codebase (either newly introduced or pre-existing) that are being intentionally suppressed or ignored by this commit?
Checklist
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.