From f8f1b44795147d1d7f906b1f9bc32dc6c61df34c Mon Sep 17 00:00:00 2001 From: Jonathon Leight Date: Thu, 2 Jul 2026 12:50:15 -0400 Subject: [PATCH] Add e2e test for maintenance author fix --- .claude/CLAUDE.md | 38 ++++++++++-- .config/mise/config.toml | 9 +++ .woodpecker/e2e.yaml | 45 ++++++++++++++ .../core/templates/repeater_maintenance.html | 2 +- internal/e2e/maintenance_test.go | 58 +++++++++++++++++++ 5 files changed, 145 insertions(+), 7 deletions(-) create mode 100644 .woodpecker/e2e.yaml create mode 100644 internal/e2e/maintenance_test.go diff --git a/.claude/CLAUDE.md b/.claude/CLAUDE.md index e7ebd01..2de8843 100644 --- a/.claude/CLAUDE.md +++ b/.claude/CLAUDE.md @@ -74,19 +74,45 @@ be fully covered by Go tests. For UI or client-JS changes, run the app (on :8090 and check it in a browser **with the devtools console open** (CSP violations only show there). If you can't validate it, say so — don't claim it works. +**Browser tests (`mise run e2e`).** Regressions worth locking in get a `chromedp` +test. These live in their own black-box package `internal/e2e/` (one +`_test.go` per feature, sharing `harness_test.go`) behind a +`//go:build browser` tag, so `go test ./...` never needs a browser — only +`mise run e2e` runs them. That task starts a `chromedp/headless-shell` **Docker +container** (no local Chrome install) and runs the `browser`-tagged suite against +an in-process test server; the browser reaches the server via +`host.docker.internal`, and the harness fails the test on any CSP violation. If +the container isn't up, the tests **skip** (never fail). Reuse the `e2eServer` +harness (`newE2EServer`, `login`, `newRepeater`, `startBrowser`, +`setSessionCookie`) — it authenticates via the public `/session/callback` handoff +and depends only on exported app APIs, so keep it that way (no reaching into +package internals). Give elements a stable `data-testid`/class hook rather than +selecting on Bootstrap layout classes. CI runs these non-gating +(`.woodpecker/e2e.yaml`); `E2E_DEVTOOLS_URL`/`E2E_BROWSER_HOST` override the +addresses there. + ### 5. CI is the gate, not the first line Woodpecker (`.woodpecker/`): `test` + `lint` + `vuln` (govulncheck) must pass before `build`; `deploy` follows `build` on `main`. A reachable CVE blocks the build. Catch problems locally first; if CI catches something you didn't, close the local-testing gap. -### 6. Git hygiene — stage, don't commit +### 6. Git hygiene — stage one batch at a time, don't commit This is a single-developer project with no PR workflow. **Stage your changes but do -NOT commit or push** — the developer does the committing. Keep each staged set to -one logical change (so it becomes one clean commit): stage exactly those files with -explicit `git add …` (never `-A`/`.`), and review `git diff --cached` before -handing off. If a task spans two logical changes, finish and hand off the first -(staged) before starting the second. +NOT commit or push** — the developer commits from a git GUI. Stage exactly the +files for one logical change with explicit `git add …` (never `-A`/`.`). + +**One batch at a time.** When a task spans multiple logical commits, do NOT stage +everything at once — that co-mingles the batches in the single index and makes them +impossible to commit separately. Instead: +1. Stage exactly one batch, give a one-line description the developer can base a + commit message on, and **stop** — hand off and wait. +2. The developer reviews `git diff --cached` and commits that batch in their GUI. +3. Continue only when the developer says so; then stage the next batch. + +Order batches so each is independently green (builds/tests pass) once committed. +Provide file-list `git add` commands only as reference — do not run the next +batch's staging until told to. ## Performance The product is server-rendered over Postgres; perf work lives mostly in SQL and diff --git a/.config/mise/config.toml b/.config/mise/config.toml index 352f092..eb9703b 100644 --- a/.config/mise/config.toml +++ b/.config/mise/config.toml @@ -20,3 +20,12 @@ run = "go run ./cmd/meshtender --seed" [tasks.reset] run = "go run ./cmd/meshtender --reset" + +[tasks.e2e] +run = """ +docker run -d --rm --name mt-headless -p 9222:9222 \ + --add-host=host.docker.internal:host-gateway chromedp/headless-shell:latest >/dev/null +trap 'docker stop mt-headless >/dev/null' EXIT +until curl -sf http://127.0.0.1:9222/json/version >/dev/null; do sleep 0.5; done +go test -tags browser ./internal/e2e/ +""" diff --git a/.woodpecker/e2e.yaml b/.woodpecker/e2e.yaml new file mode 100644 index 0000000..2029f45 --- /dev/null +++ b/.woodpecker/e2e.yaml @@ -0,0 +1,45 @@ +# Browser (end-to-end) tests: drive a real headless Chrome against the app to +# catch rendered-DOM and CSP regressions the Go tests can't. Runs the +# `browser`-tagged suite in internal/e2e. +# +# This is its OWN workflow and deliberately NOT a `build` dependency: a flaky +# browser must never block the release gate. Promote it to gating (add it to +# build.yaml's depends_on) once it has proven stable. +# +# NOTE: the container<->step networking here is unverified against this +# Woodpecker instance/backend. Two addresses must line up: +# - E2E_DEVTOOLS_URL: this step reaching the browser service (by service name; +# mirrors how the test step reaches `postgres`, which is known to work). +# - E2E_BROWSER_HOST: the browser service reaching back to this step's +# in-process test server. On the docker backend a step is reachable by its +# name (`e2e`); on other backends this may need adjusting. The tests skip +# cleanly if the DevTools endpoint can't be reached, so a misconfig here +# yields skips, not false failures. +when: + - event: [push, pull_request] + +services: + postgres: + image: postgres:17 + environment: + POSTGRES_USER: meshtender + POSTGRES_PASSWORD: meshtender + POSTGRES_DB: meshtender_test + headless: + image: chromedp/headless-shell:latest + +steps: + e2e: + image: golang:1.26.4 + environment: + MESHTENDER_TEST_DATABASE_URL: postgres://meshtender:meshtender@postgres:5432/meshtender_test?sslmode=disable + # This step -> the browser's DevTools endpoint (service by name). + E2E_DEVTOOLS_URL: http://headless:9222 + # The browser service -> this step's in-process test server (step by name). + E2E_BROWSER_HOST: e2e + commands: + - until bash -c 'echo > /dev/tcp/postgres/5432' 2>/dev/null; do sleep 1; done + - until bash -c 'echo > /dev/tcp/headless/9222' 2>/dev/null; do sleep 1; done + - go test -tags browser ./internal/e2e/ + +depends_on: [] diff --git a/internal/core/templates/repeater_maintenance.html b/internal/core/templates/repeater_maintenance.html index f5cb67f..e2e844d 100644 --- a/internal/core/templates/repeater_maintenance.html +++ b/internal/core/templates/repeater_maintenance.html @@ -39,7 +39,7 @@
{{.PerformedAt.Format "Jan 2, 2006"}}
{{.Note}}
- {{if .AuthorName}}{{.AuthorName}}{{else}}(unknown){{end}} + {{if .AuthorName}}{{.AuthorName}}{{else}}(unknown){{end}} {{if $.IsOwner}}
diff --git a/internal/e2e/maintenance_test.go b/internal/e2e/maintenance_test.go new file mode 100644 index 0000000..fe67267 --- /dev/null +++ b/internal/e2e/maintenance_test.go @@ -0,0 +1,58 @@ +//go:build browser + +package e2e + +import ( + "strings" + "testing" + "time" + + cdplog "github.com/chromedp/cdproto/log" + "github.com/chromedp/cdproto/network" + "github.com/chromedp/chromedp" +) + +// TestE2EMaintenanceShowsCurrentAuthorName is the browser-level guarantee for +// the stale-author-name bug: the maintenance history must render the author's +// *current* display name, not the name captured when the entry was logged. It +// renames the author after logging the entry, then asserts the rendered name +// reflects the rename — which only holds with the ListMaintenance fix in place. +// Also asserts the page runs clean under the strict CSP. +func TestE2EMaintenanceShowsCurrentAuthorName(t *testing.T) { + srv := newE2EServer(t) + user, cookie := srv.login(t, "e2euser") + rep := srv.newRepeater(t, user.ID, "Field Repeater") + + // Log an entry snapshotting the name as it was (the username, since the + // seeded user has no display name yet), exactly as the handler does. + if err := srv.store.AddMaintenanceEntry(srv.ctx, rep.ID, user.ID, "e2euser", "replaced battery", time.Now()); err != nil { + t.Fatalf("add maintenance entry: %v", err) + } + // Now rename the author. A correct read path shows the new name on the old + // entry; the pre-fix snapshot read would still show "e2euser". + const renamed = "Renamed In Browser" + if err := srv.store.SetDisplayName(srv.ctx, user.ID, renamed); err != nil { + t.Fatalf("set display name: %v", err) + } + + bctx, cancel, watch := startBrowser(t) + defer cancel() + + url := srv.browserURL + "/repeaters/" + rep.PublicID + "/maintenance" + var authorText string + if err := chromedp.Run(bctx, + network.Enable(), + cdplog.Enable(), + setSessionCookie(cookie), + chromedp.Navigate(url), + chromedp.WaitVisible(`.maint-author`, chromedp.ByQuery), + chromedp.Text(`.maint-author`, &authorText, chromedp.ByQuery), + ); err != nil { + t.Fatalf("browser run against %s: %v", url, err) + } + + if got := strings.TrimSpace(authorText); got != renamed { + t.Fatalf("maintenance author rendered %q, want current display name %q (stale snapshot?)", got, renamed) + } + watch.assertClean(t) +}