Repository navigation
Adding a devcontainer for local testing - #198
Conversation
|
fixes #200 |
|
Reviewed the diff. The devcontainer is straightforward and the local E2E runner is careful work — One design concern worth your answer before this merges, and two smaller things. The concern:
|
0433d79 to
822b5f8
Compare
diffwireauto
left a comment
There was a problem hiding this comment.
Approving on Strycher's instruction ("Rebase and merge #198"), acting as diffwireauto.
Rebased onto 5206c709 (current master) — the rebase preserved content exactly, diff hash 41f6a50ce1c6989005014c23b48842e9640b039f before and after, and kept ITBlacksheep as commit author. New head 822b5f8f.
CI green on the rebased head: Playwright pass 18m54s, Go Build & Test pass 12m43s, Docker image, change-detection and author-email all pass. Also checked locally: bash -n scripts/e2e-local.sh clean, devcontainer.json parses as valid JSON.
The devcontainer itself is sound — Ubuntu 24.04, Node 22 and Go 1.26 pinned to match CI, plus sqlite3 and psmisc which the E2E scripts actually need. e2e-local.sh is careful: mktemp workdir with trap cleanup EXIT, argument validation, a sqlite3 presence check, it copies the fixture rather than mutating the tracked one, runs the goose baseline, polls readiness with a kill -0 liveness check, tails the log on failure, and accumulates a per-test exit code.
Merging with one known latent defect, deliberately
The YAML scraping in e2e-local.sh is unchanged:
WF=.github/workflows/deploy.yml
grep -E "sqlite3 test-fixtures/e2e-fixture.db \"UPDATE observers" "$WF" | sed ... | bash
awk '/sqlite3 test-fixtures\/e2e-fixture.db <<.SQL.$/{f=1;next} /^ *SQL$/{f=0} f' "$WF" | sqlite3 "$DB"This is a real bug, not a style note: it breaks silently when deploy.yml is reformatted, an empty grep is not caught by set -euo pipefail, and the result is an under-seeded fixture so local tests fail for reasons unrelated to the code.
It is not blocking because its blast radius is one developer's local run. Nothing in CI invokes e2e-local.sh, nothing on prod depends on it, and it cannot produce a bad release. Filing the extraction as a follow-up so it is not lost — the fix is the pattern used in #199: move the seeding to scripts/e2e-seed-fixture.sh and have both deploy.yml and e2e-local.sh call it.
Two non-issues for the record: remoteEnv: CI=true is a deliberate parity choice in your own dev environment, and npm ci --production=false is a deprecated spelling that still works.
Refs #200.
|
Merged as The seeding-extraction concern from my review is filed as #203 so it does not get lost. Not urgent, and the reasoning for merging without it is on that issue: the blast radius is a single developer's local run, nothing in CI calls The fix there is the pattern you already used in #199 — pull the shared logic into |
Just a chore. Doesn't need a new push to staging.
Refs #200