From d6b3f9764f4d92e911ce25b8e115b765d7b4822d Mon Sep 17 00:00:00 2001 From: Niek Otten Date: Fri, 4 Sep 2026 10:53:35 +0000 Subject: [PATCH] fix(e2e): bound the Playwright run and make a failed login say why (closes #161) (#165) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## What & why #161 is really two defects, and the second one is why the first was undiagnosable. **A wedged suite consumed the job, and took the post-mortem with it.** Nothing bounded the Playwright run, so CI stopped the job mid-suite — and `if: always()` does not survive that. Run 739's job metadata shows every step after the e2e as a **0-second failure** stamped at the kill: ``` 14 failure 09:48:17 -> 10:14:54 Self-service e2e (Playwright …) 15 failure 10:14:54 -> 10:14:54 verify-stack check summary ← if: always() 16 failure 10:14:54 -> 10:14:54 e2e spec summary ← if: always() 17 failure 10:14:54 -> 10:14:54 Dump container logs on failure ← if: failure() 18 failure 10:14:54 -> 10:14:54 Tear down ← if: always() ``` So the per-spec summary, the container-log dump and the teardown never ran, and the log lost whatever the killed process had buffered — leaving the single `✘` line the issue was filed from. `globalTimeout` now makes Playwright stop and *report*: the JSON report is written and those steps still get their turn. (A `timeout-minutes` on the job would have reproduced the same failure, so there isn't one.) The "~24-minute gap" is that kill, not necessarily a hang — note run 739 shows `run_attempt: 2`, and `concurrency.cancel-in-progress` kills an in-flight run on any re-run or push. **A login that never got its form ate the 90-second test timeout.** Playwright actions auto-wait until the *test* timeout, not `expect.timeout` — so a portal that serves its page but never bootstraps (its `config.json` fetch or the OIDC discovery behind `authorize()` failed; `main.ts` only `console.error`s) spent 90s to report `locator.fill: Test timeout of 90000ms exceeded`: the symptom, not the cause. That is catalogus.spec's 1.8 minutes. Both Keycloak forms are now asserted visible first, with a 20s budget and a message naming the step that never happened. Verified against a real blank-bootstrap portal — the beheer image served with a `config.json` that is not JSON — which fails in **20.2s** with *"the Keycloak login form never appeared — the portal did not reach Keycloak (check its config.json fetch and the OIDC discovery …)"*. **And the summary now says why.** The per-spec table (#136) rendered a verdict icon and nothing else, so even a surviving summary cost a log dive. Failing specs now carry their first error, flattened for a table cell (ANSI stripped, newlines collapsed, `|` escaped, clipped) — shape verified against a real @playwright/test 1.61 failing report, with a stdlib assert self-check on `make unit`. Closes #161 ## Definition of Done - [x] Linked Gitea issue (above). - [x] Failing test committed before the implementation. - [x] Implementation makes the test pass; refactor commit follows (login helper dedup). - [x] Conventional Commits referencing the issue (`refs #161`). - [ ] CI green — all Gitea Actions jobs. - [x] `docker compose up` from a fresh clone reaches green health checks within 3 minutes (untouched). - [x] Docs updated — `docs/runbooks/gitea-actions-gotchas.md` §9. - [x] ADR — not needed: no boundary, dependency or coupling rule touched (test/CI infra only). - [x] Demo note — not applicable: nothing user-visible. ## Notes for reviewers **What this does not do: identify why the beheerder login failed that once.** The evidence to do that was destroyed by defect 2, which is what this PR fixes. The suite ran green here five times today (catalogus.spec 1.1–5.3s each) — but a local box is not the loaded CI runner, so that is weak evidence and I am not claiming the flake is gone. What changes is that the next occurrence is bounded and self-describing: it fails in 20s naming the failing step, the JSON report survives, and the summary prints the error. Please keep #161 in mind rather than treating this as proof. **Two follow-ups I did not pull into this PR:** - *All four portals show a permanently blank page if their startup fetch fails* — `main.ts` does `fetch('config.json').then(bootstrap).catch(console.error)`, one shot, no UI and no recovery. That is a real product gap (the deliberately-broken portal above is exactly what a user would see) and wants its own slice, not a test-infra PR. - `retries: 1` is untouched. CLAUDE.md §15 says flaky tests are fixed rather than retried, but removing retries while a real flake is unexplained would trade a rare red for a frequent one. Worth revisiting once #161 recurs (or doesn't) with the new diagnostics. The login-helper rename (`medewerker-login.ts` → `keycloak-login.ts`, citizen logins routed through `loginBurger`) is its own no-behaviour-change commit: the three citizen specs each duplicated the same three-line login, so guarding the login path once meant routing them through it first.Reviewed-on: https://git.labs.respellion.tech/eho/register-referentie/pulls/165 --- Makefile | 3 + docs/runbooks/gitea-actions-gotchas.md | 44 +++++++ infra/host-browser.yml | 20 ++++ infra/playwright-summary.py | 42 ++++++- infra/test_playwright_summary.py | 108 ++++++++++++++++++ tests/e2e/catalogus.spec.ts | 2 +- tests/e2e/default-fill.spec.ts | 2 +- ...r-login.spec.ts => keycloak-login.spec.ts} | 2 +- tests/e2e/keycloak-login.ts | 99 ++++++++++++++++ tests/e2e/medewerker-login.ts | 57 --------- tests/e2e/playwright.config.ts | 6 + tests/e2e/registration.spec.ts | 6 +- tests/e2e/resume.spec.ts | 5 +- tests/e2e/withdrawal.spec.ts | 5 +- 14 files changed, 326 insertions(+), 75 deletions(-) create mode 100644 infra/host-browser.yml create mode 100644 infra/test_playwright_summary.py rename tests/e2e/{medewerker-login.spec.ts => keycloak-login.spec.ts} (92%) create mode 100644 tests/e2e/keycloak-login.ts delete mode 100644 tests/e2e/medewerker-login.ts diff --git a/Makefile b/Makefile index 75d7163..d9efbc2 100644 --- a/Makefile +++ b/Makefile @@ -71,8 +71,11 @@ build: ## unit: run unit tests (excludes the container-backed Integration lane) # TRX per test project (→ TestResults/) feeds the CI per-service summary (#136); harmless locally. +# The CI reporting scripts are stdlib Python with their own assert-based self-checks (#161) — they +# ride this lane so a broken job summary is caught by CI rather than by the next red pipeline. unit: dotnet test $(SLN) -c Release --filter "Category!=Integration" --logger trx --results-directory TestResults + python3 infra/test_playwright_summary.py ## mutation: run the Stryker.NET ratchet on each service with branching logic (fails below baseline) # Stryker is pinned as a local dotnet tool (.config/dotnet-tools.json); `tool restore` diff --git a/docs/runbooks/gitea-actions-gotchas.md b/docs/runbooks/gitea-actions-gotchas.md index 8ecdfc0..e41c5cd 100644 --- a/docs/runbooks/gitea-actions-gotchas.md +++ b/docs/runbooks/gitea-actions-gotchas.md @@ -245,3 +245,47 @@ the verify-stack check table, and per-spec e2e results (`infra/playwright-summar - Getting a report out of the e2e container: Playwright writes `playwright-report.json` inside the container; `infra/run-e2e-check.sh` `docker cp`s it back to the host (capturing the test exit code first) so the summary step can read it. + +--- + +## 9. `if: always()` does not survive the job being killed — bound the work itself + +`if: always()` makes a step run when an *earlier step failed*. It does **not** help when +the job as a whole is stopped: the run's remaining steps are simply never dispatched. + +That is how #161 lost its diagnosis. `verify-stack` entered `make verify-e2e` at 09:48:17 +and the job ended at 10:14:54 — 26½ minutes later, mid-suite. Every step after the e2e +shows a **0-second `failure`** stamped at that same instant: + +``` +14 failure 09:48:17 -> 10:14:54 Self-service e2e (Playwright, login → submit → success) +15 failure 10:14:54 -> 10:14:54 verify-stack check summary ← if: always() +16 failure 10:14:54 -> 10:14:54 e2e spec summary ← if: always() +17 failure 10:14:54 -> 10:14:54 Dump container logs on failure ← if: failure() +18 failure 10:14:54 -> 10:14:54 Tear down ← if: always() +``` + +So the per-spec summary, the container-log dump and the teardown never ran, and the job +log — which also loses whatever the killed process had buffered — ended at a single `✘` +line. A job that dies takes its own post-mortem with it. + +**Read the step timings, not just the log.** `GET /api/v1/repos/{owner}/{repo}/actions/jobs/{id}` +returns every step with `started_at`/`completed_at`; a row of identical zero-length +steps at the end means *killed*, not *silent*. (Job ids come from +`…/actions/runs/{run}/jobs`, and that route returns only the **latest attempt** — a +re-run hides the failed one, so keep the failing job id from the original report. Logs: +`…/actions/jobs/{id}/logs`, see also `gitea-ci-logs`.) + +**Conventions that follow:** + +- **Bound long-running work inside the tool**, where it can still report. Playwright's + `globalTimeout` (`tests/e2e/playwright.config.ts`) ends the run, writes the JSON + report and exits, so the summary and log-dump steps still get their turn. A + `timeout-minutes` on the job would reproduce the very failure above. +- **Never let an auto-waiting action be the timeout.** Playwright actions (`fill`, + `click`) inherit the *test* timeout, not `expect.timeout`, so a missing element costs + the full 90 s and reports `locator.fill: Test timeout …` — the symptom. Assert the + element visible first with its own budget and a message (`tests/e2e/keycloak-login.ts`). +- Remember `concurrency.cancel-in-progress: true` in `ci.yaml`: a new push to the same + ref, or a re-run, kills the in-flight run the same way. Check `run_attempt` before + concluding a job hung. diff --git a/infra/host-browser.yml b/infra/host-browser.yml new file mode 100644 index 0000000..bb82990 --- /dev/null +++ b/infra/host-browser.yml @@ -0,0 +1,20 @@ +# Overlay: make the CI compose stack usable from a HOST browser. +# Same two mechanisms infra/docker-compose.local.yml already uses — pin Keycloak's issuer to the +# host-published address, and point each portal's runtime config.json at it. The BFF needs no +# change: it discovers metadata over keycloak:8080 and the discovered issuer is the pinned +# localhost:8180, which is what browser tokens carry. +services: + keycloak: + environment: + KC_HOSTNAME: http://localhost:8180 + KC_HOSTNAME_BACKCHANNEL_DYNAMIC: "true" + self-service: + volumes: + - ./local-config/self-service.config.json:/usr/share/nginx/html/config.json:ro,z + behandel: + volumes: + - ./local-config/behandel.config.json:/usr/share/nginx/html/config.json:ro,z + # beheer is the same medewerker realm as behandel, so it reuses behandel's config verbatim. + beheer: + volumes: + - ./local-config/behandel.config.json:/usr/share/nginx/html/config.json:ro,z diff --git a/infra/playwright-summary.py b/infra/playwright-summary.py index 25e1490..c2ed880 100644 --- a/infra/playwright-summary.py +++ b/infra/playwright-summary.py @@ -7,10 +7,34 @@ redirects it into $GITHUB_STEP_SUMMARY. Stdlib only. """ import json import os +import re import sys STATUS_ICON = {"expected": "✅", "unexpected": "❌", "skipped": "⏭️", "flaky": "⚠️"} +# A verdict alone still costs a log dive, and a killed or truncated job leaves no log to dive into +# (#161) — so a failing spec carries its first error into the table. Playwright errors are multi-line +# with a "Call log:", which a markdown table cell cannot hold, so they are flattened and clipped. +ERROR_CLIP = 300 + + +def first_error(spec): + """The first error message across a spec's test results, flattened for one table cell.""" + for test in spec.get("tests", []): + for result in test.get("results", []): + for error in result.get("errors", []): + message = (error.get("message") or "").strip() + if not message: + continue + # Strip ANSI colour, collapse to one line, and keep it inside the cell. + message = re.sub(r"\x1b\[[0-9;]*m", "", message) + message = " ".join(message.split()) + if len(message) > ERROR_CLIP: + message = message[:ERROR_CLIP - 1].rstrip() + "…" + # `|` would end the cell early. + return message.replace("|", "\\|") + return "" + def walk(suite, out): for spec in suite.get("specs", []): @@ -22,7 +46,8 @@ def walk(suite, out): else "expected" if spec.get("ok", False) else "unexpected") out.append({"file": spec.get("file") or suite.get("file") or suite.get("title", ""), - "title": spec.get("title", ""), "status": status}) + "title": spec.get("title", ""), "status": status, + "error": first_error(spec) if status in ("unexpected", "flaky") else ""}) for child in suite.get("suites", []): walk(child, out) @@ -46,10 +71,17 @@ def main(path): if not specs: print("_No specs ran._") return 0 - print("| Spec | Result |") - print("| ---- | :----: |") - for s in specs: - print(f"| {s['file']} › {s['title']} | {STATUS_ICON.get(s['status'], '❔')} |") + # The failure column only earns its width when something failed. + if any(s["error"] for s in specs): + print("| Spec | Result | Why |") + print("| ---- | :----: | --- |") + for s in specs: + print(f"| {s['file']} › {s['title']} | {STATUS_ICON.get(s['status'], '❔')} | {s['error']} |") + else: + print("| Spec | Result |") + print("| ---- | :----: |") + for s in specs: + print(f"| {s['file']} › {s['title']} | {STATUS_ICON.get(s['status'], '❔')} |") return 0 diff --git a/infra/test_playwright_summary.py b/infra/test_playwright_summary.py new file mode 100644 index 0000000..9255ca0 --- /dev/null +++ b/infra/test_playwright_summary.py @@ -0,0 +1,108 @@ +#!/usr/bin/env python3 +"""Self-check for infra/playwright-summary.py — stdlib asserts, no framework. + +Run: python3 infra/test_playwright_summary.py (also runs in `make unit`). + +A red e2e is only useful if the job summary says WHY it failed: #161 lost a 36-minute +verify-stack job whose only surviving output was one ✘ line with no assertion detail. +""" +import importlib.util +import io +import json +import os +import tempfile +from contextlib import redirect_stdout + +# The script's filename is not a valid module name, so load it by path. +spec = importlib.util.spec_from_file_location( + "playwright_summary", + os.path.join(os.path.dirname(os.path.abspath(__file__)), "playwright-summary.py"), +) +summary = importlib.util.module_from_spec(spec) +spec.loader.exec_module(summary) + + +def render(report): + """Run the renderer over a report dict and return its markdown.""" + with tempfile.NamedTemporaryFile("w", suffix=".json", delete=False) as fh: + json.dump(report, fh) + path = fh.name + try: + out = io.StringIO() + with redirect_stdout(out): + summary.main(path) + return out.getvalue() + finally: + os.unlink(path) + + +def spec_entry(title, status, errors=()): + return { + "title": title, + "file": "catalogus.spec.ts", + "ok": status == "expected", + "tests": [{"status": status, "results": [{"errors": [{"message": m} for m in errors]}]}], + } + + +def test_failing_spec_reports_why(): + md = render({ + "stats": {"expected": 4, "unexpected": 1, "flaky": 0, "skipped": 0, "duration": 108_000}, + "suites": [{"file": "catalogus.spec.ts", "specs": [ + spec_entry("a beheerder sees the published zaaktypen in the catalogus", "unexpected", + ["locator.fill: Test timeout of 90000ms exceeded.\n" + "Call log:\n - waiting for locator('#username')\n"]), + ]}], + }) + assert "❌" in md, md + # The point of the slice: the summary names the cause, not just the verdict. + assert "Test timeout of 90000ms exceeded" in md, md + assert "waiting for locator('#username')" in md, md + # A multi-line Playwright error must not break out of its table row. + assert not any(line.startswith("Call log:") for line in md.splitlines()), md + + +def test_real_playwright_error_is_flattened(): + # A real report's message is multi-line and ANSI-coloured, and embeds the source snippet with + # `|` gutters — all three would break the table cell. Shape verified against an actual + # @playwright/test 1.61 JSON report. + md = render({ + "stats": {"expected": 0, "unexpected": 1, "flaky": 0, "skipped": 0, "duration": 1_000}, + "suites": [{"file": "catalogus.spec.ts", "specs": [ + spec_entry("a beheerder sees the catalogus", "unexpected", + ["Error: expect(locator).toBeVisible() failed\n\n" + "\x1b[2mLocator: \x1b[22mgetByRole('heading')\n" + " 12 | await login(page);\n> 13 | await expect(heading).toBeVisible();\n"]), + ]}], + }) + row = [line for line in md.splitlines() if line.startswith("| catalogus.spec.ts")][0] + assert "\x1b" not in row, row + assert "Locator: getByRole('heading')" in row, row + # Every literal `|` from the snippet gutters is escaped, so the row keeps exactly 3 cells. + assert row.count("|") - row.count("\\|") == 4, row + + +def test_passing_run_stays_quiet(): + md = render({ + "stats": {"expected": 1, "unexpected": 0, "flaky": 0, "skipped": 0, "duration": 5_000}, + "suites": [{"file": "catalogus.spec.ts", + "specs": [spec_entry("a beheerder sees the catalogus", "expected")]}], + }) + assert "✅" in md, md + assert "timeout" not in md.lower(), md + + +def test_missing_report_is_not_a_crash(): + out = io.StringIO() + with redirect_stdout(out): + rc = summary.main("/nonexistent/playwright-report.json") + assert rc == 0 + assert "did not reach the e2e step" in out.getvalue() + + +if __name__ == "__main__": + for name, fn in sorted(globals().items()): + if name.startswith("test_") and callable(fn): + fn() + print(f" ok {name}") + print("playwright-summary self-check passed") diff --git a/tests/e2e/catalogus.spec.ts b/tests/e2e/catalogus.spec.ts index bb6e85f..5acd400 100644 --- a/tests/e2e/catalogus.spec.ts +++ b/tests/e2e/catalogus.spec.ts @@ -1,5 +1,5 @@ import { expect, test } from '@playwright/test'; -import { loginMedewerker } from './medewerker-login'; +import { loginMedewerker } from './keycloak-login'; // S-15a walking skeleton: a beheerder logs in to the beheer portal (medewerker realm) and sees the // read-only ZTC catalogus. The verify stack seeds and publishes the BIG-REGISTRATIE zaaktype (the diff --git a/tests/e2e/default-fill.spec.ts b/tests/e2e/default-fill.spec.ts index 637fe57..c9bf4e2 100644 --- a/tests/e2e/default-fill.spec.ts +++ b/tests/e2e/default-fill.spec.ts @@ -1,5 +1,5 @@ import { expect, test } from '@playwright/test'; -import { loginMedewerker } from './medewerker-login'; +import { loginMedewerker } from './keycloak-login'; // S-15b: a beheerder edits the ACL default-fill in the beheer portal and gets a saved confirmation. // Runs against the shared verify stack; it edits + saves (the ACL store is in-memory, ADR-0026) and diff --git a/tests/e2e/medewerker-login.spec.ts b/tests/e2e/keycloak-login.spec.ts similarity index 92% rename from tests/e2e/medewerker-login.spec.ts rename to tests/e2e/keycloak-login.spec.ts index b4e83bb..6580c31 100644 --- a/tests/e2e/medewerker-login.spec.ts +++ b/tests/e2e/keycloak-login.spec.ts @@ -1,5 +1,5 @@ import { expect, test } from '@playwright/test'; -import { OTP_PERIOD_MS, nextUnusedCounter } from './medewerker-login'; +import { OTP_PERIOD_MS, nextUnusedCounter } from './keycloak-login'; // Pure check of the TOTP counter guard in loginMedewerker — no browser, no stack. Keycloak refuses // a code it has already accepted (its otpPolicyCodeReusable defaults to false), so two logins as diff --git a/tests/e2e/keycloak-login.ts b/tests/e2e/keycloak-login.ts new file mode 100644 index 0000000..4d7aa87 --- /dev/null +++ b/tests/e2e/keycloak-login.ts @@ -0,0 +1,99 @@ +import { createHmac } from 'node:crypto'; +import { readFileSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { expect, type Page } from '@playwright/test'; + +// Every portal login in the suite goes through this module — citizen realms (mock DigiD) and the +// medewerker realm alike — so the shared Keycloak form handling lives in exactly one place. + +// The medewerker realm enforces MFA (S-15c), so a staff login is two steps: password, then a TOTP +// code. The realm export seeds every medewerker with this fixture secret — Keycloak HMACs the raw +// secret bytes — so the e2e can compute a valid code instead of enrolling an authenticator. +const OTP_SECRET = 'BIGMEDEWERKEROTPSEED'; + +export const OTP_PERIOD_MS = 30_000; + +/** + * How long a Keycloak form gets to appear. Generous enough for a cold first browser launch and a + * loaded stack, far short of the 90-second test timeout an auto-waiting action would otherwise eat. + */ +const FORM_TIMEOUT_MS = 20_000; +const FORM_NEVER_APPEARED = + 'the Keycloak login form never appeared — the portal did not reach Keycloak (check its ' + + 'config.json fetch and the OIDC discovery on the authority it was built with)'; +const OTP_NEVER_APPEARED = + 'the Keycloak OTP form never appeared — the password step did not complete (check the ' + + 'medewerker realm seeded this user with both a password and a TOTP credential)'; + +// RFC 6238 TOTP: HMAC-SHA1 over the 30-second counter, dynamically truncated to 6 digits. +export function totp(secret = OTP_SECRET, at = Date.now()): string { + const counter = Buffer.alloc(8); + counter.writeBigUInt64BE(BigInt(Math.floor(at / OTP_PERIOD_MS))); + const mac = createHmac('sha1', secret).update(counter).digest(); + const offset = mac[mac.length - 1] & 0x0f; + return String((mac.readUInt32BE(offset) & 0x7fffffff) % 1_000_000).padStart(6, '0'); +} + +// Keycloak refuses a TOTP code it has already accepted (its otpPolicyCodeReusable defaults to +// false), so two logins as the same medewerker inside one 30-second window would both submit the +// same code and the second is rejected. Spend the first counter this medewerker has left. +export function nextUnusedCounter(now: number, spent: number): number { + return Math.max(Math.floor(now / OTP_PERIOD_MS), spent + 1); +} + +// The spent counter lives on disk rather than in module state: Playwright starts a fresh worker +// process for a retry, which would otherwise forget it and resubmit the rejected code. +function spendCounter(username: string): number { + const file = join(tmpdir(), `otp-counter-${username}`); + let spent = -1; + try { + spent = Number(readFileSync(file, 'utf8')) || -1; + } catch { + // first login as this medewerker in this run + } + const counter = nextUnusedCounter(Date.now(), spent); + writeFileSync(file, String(counter)); + return counter; +} + +/** + * Fill Keycloak's login form. Every portal is guarded, so the first navigation redirects here; the + * form ids are stable across themes. + * + * The form is asserted visible *before* it is filled. A portal that never reaches Keycloak — its + * runtime `config.json` fetch or the OIDC discovery behind `authorize()` failed, so it never + * bootstrapped and shows a blank page (main.ts only logs to the console) — would otherwise leave + * `fill()` auto-waiting until the whole test times out: 90 seconds spent to report + * `locator.fill: Test timeout of 90000ms exceeded`, naming the symptom and not the cause. That is + * how #161's catalogus.spec burned 1.8 minutes. This fails in a quarter of the time and says which + * step never happened. + */ +async function submitPassword(page: Page, username: string): Promise { + await expect(page.locator('#username'), FORM_NEVER_APPEARED).toBeVisible({ timeout: FORM_TIMEOUT_MS }); + await page.locator('#username').fill(username); + await page.locator('#password').fill('test123'); + await page.locator('#kc-login').click(); +} + +/** A citizen login on a mock-DigiD realm — no second factor (ADR-0031). */ +export async function loginBurger(page: Page, username: string): Promise { + await submitPassword(page, username); +} + +/** A staff login on the medewerker realm: password, then the enforced TOTP second factor. */ +export async function loginMedewerker(page: Page, username: string): Promise { + await submitPassword(page, username); + + // Keycloak's conditional-OTP step. Same reasoning as the password form above: assert it arrived + // rather than letting `fill()` swallow the test timeout. + await expect(page.locator('#otp'), OTP_NEVER_APPEARED).toBeVisible({ timeout: FORM_TIMEOUT_MS }); + + // Wait out the rest of the window if the counter we may spend is still in the future; Keycloak's + // lookAheadWindow would accept the code a moment early, but only by one counter — waiting keeps a + // third login in the same window valid too. + const counter = spendCounter(username); + await page.waitForTimeout(Math.max(0, counter * OTP_PERIOD_MS - Date.now())); + await page.locator('#otp').fill(totp(OTP_SECRET, counter * OTP_PERIOD_MS)); + await page.locator('#kc-login').click(); +} diff --git a/tests/e2e/medewerker-login.ts b/tests/e2e/medewerker-login.ts deleted file mode 100644 index 53ba23a..0000000 --- a/tests/e2e/medewerker-login.ts +++ /dev/null @@ -1,57 +0,0 @@ -import { createHmac } from 'node:crypto'; -import { readFileSync, writeFileSync } from 'node:fs'; -import { tmpdir } from 'node:os'; -import { join } from 'node:path'; -import type { Page } from '@playwright/test'; - -// The medewerker realm enforces MFA (S-15c), so a staff login is two steps: password, then a TOTP -// code. The realm export seeds every medewerker with this fixture secret — Keycloak HMACs the raw -// secret bytes — so the e2e can compute a valid code instead of enrolling an authenticator. -const OTP_SECRET = 'BIGMEDEWERKEROTPSEED'; - -export const OTP_PERIOD_MS = 30_000; - -// RFC 6238 TOTP: HMAC-SHA1 over the 30-second counter, dynamically truncated to 6 digits. -export function totp(secret = OTP_SECRET, at = Date.now()): string { - const counter = Buffer.alloc(8); - counter.writeBigUInt64BE(BigInt(Math.floor(at / OTP_PERIOD_MS))); - const mac = createHmac('sha1', secret).update(counter).digest(); - const offset = mac[mac.length - 1] & 0x0f; - return String((mac.readUInt32BE(offset) & 0x7fffffff) % 1_000_000).padStart(6, '0'); -} - -// Keycloak refuses a TOTP code it has already accepted (its otpPolicyCodeReusable defaults to -// false), so two logins as the same medewerker inside one 30-second window would both submit the -// same code and the second is rejected. Spend the first counter this medewerker has left. -export function nextUnusedCounter(now: number, spent: number): number { - return Math.max(Math.floor(now / OTP_PERIOD_MS), spent + 1); -} - -// The spent counter lives on disk rather than in module state: Playwright starts a fresh worker -// process for a retry, which would otherwise forget it and resubmit the rejected code. -function spendCounter(username: string): number { - const file = join(tmpdir(), `otp-counter-${username}`); - let spent = -1; - try { - spent = Number(readFileSync(file, 'utf8')) || -1; - } catch { - // first login as this medewerker in this run - } - const counter = nextUnusedCounter(Date.now(), spent); - writeFileSync(file, String(counter)); - return counter; -} - -export async function loginMedewerker(page: Page, username: string): Promise { - await page.locator('#username').fill(username); - await page.locator('#password').fill('test123'); - await page.locator('#kc-login').click(); - - // Keycloak's conditional-OTP step. Wait out the rest of the window if the counter we may spend is - // still in the future; its lookAheadWindow would accept the code a moment early, but only by one - // counter — waiting keeps a third login in the same window valid too. - const counter = spendCounter(username); - await page.waitForTimeout(Math.max(0, counter * OTP_PERIOD_MS - Date.now())); - await page.locator('#otp').fill(totp(OTP_SECRET, counter * OTP_PERIOD_MS)); - await page.locator('#kc-login').click(); -} diff --git a/tests/e2e/playwright.config.ts b/tests/e2e/playwright.config.ts index f0106f6..4f23444 100644 --- a/tests/e2e/playwright.config.ts +++ b/tests/e2e/playwright.config.ts @@ -15,6 +15,12 @@ export default defineConfig({ timeout: 90_000, expect: { timeout: 15_000 }, retries: 1, + // Bound the whole run, not just each test (#161). A wedged suite used to run until CI killed the + // job — which also killed the `if: always()` steps that would have said why: the per-spec summary + // and the container-log dump never ran, leaving a 36-minute job whose entire surviving output was + // one ✘ line. On `globalTimeout` Playwright stops and *reports*, so the JSON report is written and + // those steps still run. Generous over the ~1-minute suite: this is a backstop, not a budget. + globalTimeout: 12 * 60_000, // Run the specs serially. Each spec drives a full `channel: 'chromium'` browser, and the e2e // shares an 8 GB runner with the entire compose stack (OpenZaak, NRC, Keycloak, Flowable, 4× // Postgres, every service + 3 portals). Two parallel browsers exhaust memory and the renderer is diff --git a/tests/e2e/registration.spec.ts b/tests/e2e/registration.spec.ts index e4182b7..5d7c8d3 100644 --- a/tests/e2e/registration.spec.ts +++ b/tests/e2e/registration.spec.ts @@ -1,5 +1,5 @@ import { expect, request, test } from '@playwright/test'; -import { loginMedewerker } from './medewerker-login'; +import { loginBurger, loginMedewerker } from './keycloak-login'; // Walking-skeleton happy path (S-08d + S-09 + S-09b + S-12 + S-10a + S-19b-2): a zorgprofessional // logs in via mock DigiD and submits through the self-service portal → BFF → domain; the entry @@ -23,9 +23,7 @@ test('DigiD submit → public INGEDIEND → documenten → behandelaar goedkeurt // checks submit as jan-burger (bsn 123456782) before the e2e runs on the shared stack, and // resume-on-load (S-26) would otherwise restore one of those on login — so each self-service spec // uses a dedicated citizen no other actor touches. - await page.locator('#username').fill('emma-burger'); - await page.locator('#password').fill('test123'); - await page.locator('#kc-login').click(); + await loginBurger(page, 'emma-burger'); // Back on the portal, authenticated. await expect(page.getByRole('heading', { name: /Zelfservice/i })).toBeVisible(); diff --git a/tests/e2e/resume.spec.ts b/tests/e2e/resume.spec.ts index cf749e0..fd1b14e 100644 --- a/tests/e2e/resume.spec.ts +++ b/tests/e2e/resume.spec.ts @@ -1,4 +1,5 @@ import { expect, test } from '@playwright/test'; +import { loginBurger } from './keycloak-login'; // S-26: a zorgprofessional submits, then reloads the self-service portal. On load the portal asks the // BFF for the caller's current open registration (owner-scoped by the DigiD token's bsn) and restores @@ -9,9 +10,7 @@ test('DigiD submit → reload → self-service restores the existing registratio // Its own DigiD user (like every self-service spec): on the shared verify stack, resume-on-load // (S-26) restores any open registration for the bsn, so each spec uses a dedicated citizen that no // other spec or verify-* check touches. This one in particular leaves an open registration. - await page.locator('#username').fill('sanne-burger'); - await page.locator('#password').fill('test123'); - await page.locator('#kc-login').click(); + await loginBurger(page, 'sanne-burger'); await expect(page.getByRole('heading', { name: /Zelfservice/i })).toBeVisible(); await page.getByRole('button', { name: /indienen/i }).click(); diff --git a/tests/e2e/withdrawal.spec.ts b/tests/e2e/withdrawal.spec.ts index a13d8f1..d9b2b33 100644 --- a/tests/e2e/withdrawal.spec.ts +++ b/tests/e2e/withdrawal.spec.ts @@ -1,4 +1,5 @@ import { expect, test } from '@playwright/test'; +import { loginBurger } from './keycloak-login'; // S-11 (Flow 3): a zorgprofessional logs in via mock DigiD, submits a registration, then withdraws // it ("trek aanvraag in") from the self-service portal. The withdrawal goes portal → BFF (owner- @@ -10,9 +11,7 @@ test('DigiD submit → trek aanvraag in → self-service confirms ingetrokken', // Its own DigiD user — isolated from the verify-* checks (jan-burger/123456782) so resume-on-load // (S-26) can't restore someone else's registration on the shared stack. - await page.locator('#username').fill('lars-burger'); - await page.locator('#password').fill('test123'); - await page.locator('#kc-login').click(); + await loginBurger(page, 'lars-burger'); await expect(page.getByRole('heading', { name: /Zelfservice/i })).toBeVisible();