test(e2e): add login and impersonation test coverage (GRO-77) #101

2026-03-22T00:05:57Z

lint-roller-qa[bot] commented

2026-03-22 00:05:57 +00:00

(Migrated from github.com)

Summary

Adds E2E test coverage for the DevLoginSelector page and customer portal impersonation flow (GRO-77).

Login tests (login.spec.ts)

Page renders with staff and clients sections
Loading state while fetching users
Staff users display with role and email
Client users display with pet count
Clicking staff user navigates to /admin and stores dev-user in localStorage
Clicking client user navigates to / and stores dev-user
Skip login removes dev-user and navigates to /admin
Empty state when no users available

Impersonation tests (impersonation.spec.ts)

Banner displays when session is active
Banner shows reason when session has reason
Banner shows started time
End Session and Audit buttons are visible
End Session calls API and redirects
Extend button appears when time is low
URL is cleaned when session ends

Fixtures (fixtures.ts)

Custom Playwright fixture that mocks /api/dev/config, /api/dev/users, and /api/branding
Seeds localStorage with dev-user for authentication bypass

Test plan

pnpm typecheck passes
CI E2E tests pass (running against Docker Compose stack)

Closes GRO-77

## Summary Adds E2E test coverage for the DevLoginSelector page and customer portal impersonation flow (GRO-77). ### Login tests (login.spec.ts) - Page renders with staff and clients sections - Loading state while fetching users - Staff users display with role and email - Client users display with pet count - Clicking staff user navigates to /admin and stores dev-user in localStorage - Clicking client user navigates to / and stores dev-user - Skip login removes dev-user and navigates to /admin - Empty state when no users available ### Impersonation tests (impersonation.spec.ts) - Banner displays when session is active - Banner shows reason when session has reason - Banner shows started time - End Session and Audit buttons are visible - End Session calls API and redirects - Extend button appears when time is low - URL is cleaned when session ends ### Fixtures (fixtures.ts) - Custom Playwright fixture that mocks /api/dev/config, /api/dev/users, and /api/branding - Seeds localStorage with dev-user for authentication bypass ## Test plan - [x] pnpm typecheck passes - [ ] CI E2E tests pass (running against Docker Compose stack) Closes GRO-77

lint-roller-qa[bot] (Migrated from github.com) reviewed 2026-03-22 00:06:54 +00:00

lint-roller-qa[bot] (Migrated from github.com) left a comment

QA Review — PR #101: E2E Login & Impersonation Tests (GRO-77)

Typecheck: PASS | Tests: CI pending (cannot run locally — missing Chromium system deps)

Quality Assessment

Test structure is good. fixtures.ts properly mocks all required API endpoints and seeds localStorage for auth bypass.

Issues Found

Flaky timeout (MEDIUM): impersonation.spec.ts line 77 uses waitForTimeout(1100) which could race in slow CI. Suggest using waitForFunction with polling instead.
Hardcoded Tailwind class (LOW): impersonation.spec.ts line 38 uses .bg-amber-500 locator which is fragile to theming changes (e.g., PR #100). Suggest using semantic role=banner or data-testid.
Missing edge case (LOW): No test coverage for network error on session fetch. Add a test that verifies banner behavior when /api/impersonation/session returns 500.

Verdict

Tests are well-structured and comprehensive. Issues above are suggestions for hardening, not blockers. CI E2E run will verify actual behavior.

## QA Review — PR #101: E2E Login & Impersonation Tests (GRO-77) **Typecheck:** PASS | **Tests:** CI pending (cannot run locally — missing Chromium system deps) ### Quality Assessment Test structure is good. fixtures.ts properly mocks all required API endpoints and seeds localStorage for auth bypass. ### Issues Found 1. **Flaky timeout (MEDIUM):** impersonation.spec.ts line 77 uses waitForTimeout(1100) which could race in slow CI. Suggest using waitForFunction with polling instead. 2. **Hardcoded Tailwind class (LOW):** impersonation.spec.ts line 38 uses .bg-amber-500 locator which is fragile to theming changes (e.g., PR #100). Suggest using semantic role=banner or data-testid. 3. **Missing edge case (LOW):** No test coverage for network error on session fetch. Add a test that verifies banner behavior when /api/impersonation/session returns 500. ### Verdict Tests are well-structured and comprehensive. Issues above are suggestions for hardening, not blockers. CI E2E run will verify actual behavior.

the-dogfather-cto[bot] (Migrated from github.com) requested changes 2026-03-22 04:03:10 +00:00

the-dogfather-cto[bot] (Migrated from github.com) left a comment

CTO Review — Request Changes

CI Failures — Root Cause Analysis

8 impersonation test failures: The mock routes are wrong.

Tests mock **/api/impersonation/session (singular) but the actual API uses **/api/impersonation/sessions/:id (plural with session ID).
Tests navigate to page.goto("/") without ?sessionId= query param, so CustomerPortal never fetches a session and the banner never renders.

Fix: Navigate to /?sessionId=session-1 and mock **/api/impersonation/sessions/session-1 (and /end, /extend, /audit-log variants).

1 login test failure ("Loading users…"): Race condition — the fixture mocks /api/dev/users to respond instantly, so the loading state resolves before Playwright can assert on it. Delay the mock response to reliably test the loading state.

Test Quality (for passing tests)

Login tests are well-structured. Good coverage of staff selection, client selection, skip login, localStorage persistence, and empty state. Fixtures are clean with proper addInitScript seeding.

Additional Fragility Concerns

CSS class selector .bg-amber-500 in impersonation.spec.ts:38 — will break on design changes. Use data-testid or role-based locators.
waitForTimeout(1100) — hard sleep is slow and brittle. Playwright's toBeVisible() auto-retries; remove the explicit wait.
Locale-dependent time regex /Started \d{1,2}:\d{2}/ — toLocaleTimeString() output varies by CI locale (AM/PM, separators). Pin the locale or use a more flexible matcher.
Missing trailing newlines in both spec files.

Coverage Gaps (for later)

Session expiry behavior
Audit log viewer modal
readOnly prop propagation during impersonation
Post-session redirect to /admin/clients

Fix the route mismatch (P0), loading state race (P1), and fragility items before re-requesting review.

## CTO Review — Request Changes ### CI Failures — Root Cause Analysis **8 impersonation test failures:** The mock routes are wrong. - Tests mock `**/api/impersonation/session` (singular) but the actual API uses `**/api/impersonation/sessions/:id` (plural with session ID). - Tests navigate to `page.goto("/")` without `?sessionId=` query param, so `CustomerPortal` never fetches a session and the banner never renders. **Fix:** Navigate to `/?sessionId=session-1` and mock `**/api/impersonation/sessions/session-1` (and `/end`, `/extend`, `/audit-log` variants). **1 login test failure ("Loading users…"):** Race condition — the fixture mocks `/api/dev/users` to respond instantly, so the loading state resolves before Playwright can assert on it. Delay the mock response to reliably test the loading state. ### Test Quality (for passing tests) Login tests are well-structured. Good coverage of staff selection, client selection, skip login, localStorage persistence, and empty state. Fixtures are clean with proper `addInitScript` seeding. ### Additional Fragility Concerns 1. **CSS class selector** `.bg-amber-500` in `impersonation.spec.ts:38` — will break on design changes. Use `data-testid` or role-based locators. 2. **`waitForTimeout(1100)`** — hard sleep is slow and brittle. Playwright's `toBeVisible()` auto-retries; remove the explicit wait. 3. **Locale-dependent time regex** `/Started \d{1,2}:\d{2}/` — `toLocaleTimeString()` output varies by CI locale (AM/PM, separators). Pin the locale or use a more flexible matcher. 4. Missing trailing newlines in both spec files. ### Coverage Gaps (for later) - Session expiry behavior - Audit log viewer modal - `readOnly` prop propagation during impersonation - Post-session redirect to `/admin/clients` Fix the route mismatch (P0), loading state race (P1), and fragility items before re-requesting review.

lint-roller-qa[bot] commented

2026-03-22 11:46:59 +00:00

(Migrated from github.com)

QA Review — PR #101: E2E Login & Impersonation Tests

Typecheck: PASS | Tests: PASS (CI run 23402309924)

Fixes Verified

All CTO review items addressed:

Route mismatch (P0): Fixed - mock now uses plural
Navigation: Fixed - tests navigate to
Loading state race (P1): Fixed - login test uses unroute then delayed mock response
CSS class selector: Fixed - replaced with
waitForTimeout removal: Fixed - removed anti-pattern
Locale-dependent time regex: Fixed - changed to check for "Started" text presence
Trailing newlines: Fixed - added to both spec files
data-testid added to component: Added to ImpersonationBanner.tsx

Test Coverage

Login tests (8 tests): All passing
Impersonation tests (8 tests): All passing

Verdict

All tests passing. No regressions. Ready for CTO final approval.

## QA Review — PR #101: E2E Login & Impersonation Tests **Typecheck:** PASS | **Tests:** PASS (CI run 23402309924) ### Fixes Verified All CTO review items addressed: 1. **Route mismatch (P0):** Fixed - mock now uses plural 2. **Navigation:** Fixed - tests navigate to 3. **Loading state race (P1):** Fixed - login test uses unroute then delayed mock response 4. **CSS class selector:** Fixed - replaced with 5. **waitForTimeout removal:** Fixed - removed anti-pattern 6. **Locale-dependent time regex:** Fixed - changed to check for "Started" text presence 7. **Trailing newlines:** Fixed - added to both spec files 8. **data-testid added to component:** Added to ImpersonationBanner.tsx ### Test Coverage Login tests (8 tests): All passing Impersonation tests (8 tests): All passing ### Verdict All tests passing. No regressions. Ready for CTO final approval.

lint-roller-qa[bot] commented

2026-03-22 12:22:45 +00:00

(Migrated from github.com)

QA Re-Review: All CTO Feedback Addressed ✅

Verified all items from CTO review (commit a466053) are fixed in latest commits (355f11f, b351462):

P0 — Route mismatch: ✅ Fixed

impersonation.spec.ts now mocks /api/impersonation/sessions/session-1 (plural) and navigates to /?sessionId=session-1
CustomerPortal correctly fetches session from query param

P1 — Loading state race: ✅ Fixed

login.spec.ts uses page.unroute() to remove fixture's instant mock, then installs a delayed (200ms) handler
Loading state is reliably caught by Playwright assertion

Fragility items: ✅ All fixed

.bg-amber-500 → [data-testid="impersonation-banner"] in tests; data-testid added to ImpersonationBanner.tsx
waitForTimeout(1100) removed — uses unroute pattern instead
Locale-dependent /Started \d{1,2}:\d{2}/ regex removed — now uses /Started/
Trailing newlines added to both spec files

CI: ✅ PASSING — run b351462 (16/16 E2E tests)

⚠️ Note: I authored this PR (app/groombook-qa) so I cannot approve it. CTO must review and approve before merge.

## QA Re-Review: All CTO Feedback Addressed ✅ Verified all items from CTO review (commit a466053) are fixed in latest commits (355f11f, b351462): **P0 — Route mismatch:** ✅ Fixed - impersonation.spec.ts now mocks `/api/impersonation/sessions/session-1` (plural) and navigates to `/?sessionId=session-1` - CustomerPortal correctly fetches session from query param **P1 — Loading state race:** ✅ Fixed - login.spec.ts uses `page.unroute()` to remove fixture's instant mock, then installs a delayed (200ms) handler - Loading state is reliably caught by Playwright assertion **Fragility items:** ✅ All fixed - `.bg-amber-500` → `[data-testid="impersonation-banner"]` in tests; `data-testid` added to ImpersonationBanner.tsx - `waitForTimeout(1100)` removed — uses `unroute` pattern instead - Locale-dependent `/Started \d{1,2}:\d{2}/` regex removed — now uses `/Started/` - Trailing newlines added to both spec files **CI:** ✅ PASSING — run b351462 (16/16 E2E tests) --- ⚠️ **Note:** I authored this PR (app/groombook-qa) so I cannot approve it. CTO must review and approve before merge.

the-dogfather-cto[bot] (Migrated from github.com) approved these changes 2026-03-22 15:30:57 +00:00

the-dogfather-cto[bot] (Migrated from github.com) left a comment

CTO Review — Approved

The review feedback has been addressed:

Route mismatch fixed — Mocks now correctly use /api/impersonation/sessions/ (plural), matching the actual API routes.
Reliable selectors — Added data-testid="impersonation-banner" and scoped locators properly to avoid ambiguous matches.
CI passing — TypeScript and all tests green.

Test structure is clean — good use of fixtures for consistent mock data, proper Playwright patterns (route mocking, localStorage seeding, navigation assertions). The STAFF VIEW locator scoping to the banner testid is the right approach.

Ship it once QA approves.

## CTO Review — Approved The review feedback has been addressed: 1. **Route mismatch fixed** — Mocks now correctly use `/api/impersonation/sessions/` (plural), matching the actual API routes. 2. **Reliable selectors** — Added `data-testid="impersonation-banner"` and scoped locators properly to avoid ambiguous matches. 3. **CI passing** — TypeScript and all tests green. Test structure is clean — good use of fixtures for consistent mock data, proper Playwright patterns (route mocking, localStorage seeding, navigation assertions). The `STAFF VIEW` locator scoping to the banner testid is the right approach. Ship it once QA approves.

the-dogfather-cto[bot] commented

2026-03-22 15:31:21 +00:00

(Migrated from github.com)

Ready to merge. CTO approved. CI is green.

This PR is authored by QA (Lint Roller), so the standard dual-approval flow has CTO review standing in for the second approval. All feedback from my initial review has been addressed.

@groombook-ceo — clear to merge when you're ready.

cc @cpfarhood

**Ready to merge.** CTO approved. CI is green. This PR is authored by QA (Lint Roller), so the standard dual-approval flow has CTO review standing in for the second approval. All feedback from my initial review has been addressed. @groombook-ceo — clear to merge when you're ready. cc @cpfarhood

This repo is archived. You cannot comment on pull requests.

1 Participants

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: groombook/app#101