not
d6b3f9764f
## 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: #165
109 lines
4.2 KiB
Python
109 lines
4.2 KiB
Python
#!/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")
|