Skip to content

Fixing issue where if a e2e test fails and cause issues - #199

Merged
diffwireauto merged 1 commit into
masterfrom
ci/e2e-fresh-fixture
Oct 12, 2026
Merged

diffwireauto merged 1 commit into
masterfrom
ci/e2e-fresh-fixture

Conversation

@ITblacksheep

@ITblacksheep ITblacksheep commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

Since we have e2e testing retrying once because of flaky test if the retry was over 15 minutes packets would not be available so random test would fail. This fixes that by refeshing the timestamps on the data before running the test.

Refs #201

@ITblacksheep
ITblacksheep requested a review from a team October 12, 2026 03:57
@ITblacksheep ITblacksheep self-assigned this Oct 12, 2026
@ITblacksheep

Copy link
Copy Markdown
Contributor Author

Fixes #201

@diffwireauto

Copy link
Copy Markdown
Contributor

Reviewed the diff. The diagnosis behind this is right and it is the part that matters.

The comment in the new script is the actual finding:

The Packets page defaults to a 15-minute window, so fixture data goes stale ~15 min after freshen; every suite attempt needs its own fresh start.

That makes the old flake-guard retry close to useless — attempt 1 runs long, the fixture ages past the window, and attempt 2 re-runs the same suite against data the page can no longer show. A retry that was guaranteed to fail for timing reasons looks exactly like a flaky test. Re-freshening before the retry fixes the retry rather than papering over it.

Checked rather than assumed:

  • tools/freshen-fixture.sh exists and is executable [verified: ls -la]
  • the pristine snapshot is taken before the server ever opens the DB, and the seeding heredoc runs in the step above, so the snapshot captures a seeded-but-untouched fixture — ordering is correct
  • both the initial start (~L392) and the retry (~L665) are in the same e2e-test job on ubuntu-latest [repo: deploy.yml:258,261], so /tmp/e2e-fixture.pristine.db persists across the retry but is clean between runs. No stale-pristine risk.
  • .DS_Store is already in .gitignore:47, so deleting the tracked copy makes it stay gone

Extracting to a shared script also removes the copy-paste between the two call sites, and redirecting the server to $LOG with a tail -50 on timeout is a real improvement — the old inline version threw server output away.

One concrete thing worth changing

The wait loop can't tell "still starting" from "already dead", so a crashed server costs the full 180s.

for i in $(seq 1 180); do
  if curl -sf "http://localhost:$PORT/api/healthz" >/dev/null 2>&1; then ...
  sleep 1
done

If the server exits at second 2 — bad fixture, port still held, missing -public dir — this polls a dead process 178 more times. With the flake-guard that is two 3-minute waits on a job that was already doomed, and the window went 30s → 180s so the cost of this got 6× worse.

You already solved this in #198. scripts/e2e-local.sh does:

kill -0 "$SERVER_PID" 2>/dev/null || { echo "server exited:"; tail -50 "$WORK/server.log"; exit 1; }

Same check here would make a dead server fail in ~2s with its log, instead of three minutes later. Worth lifting from your own other PR for consistency.

Nits, take or leave

  • scripts/e2e-fresh-server.sh has no trailing newline.
  • sleep 1 between fuser -k and rebinding the port is probably fine since fuser -k is synchronous, but if you ever see a bind failure on the retry path, that's the first place to look.

Not blocking from me

Everything above is one lifted line plus a newline. The design is sound and the root-cause reasoning is better than the issue it fixes. CI was still running when I looked (✅ Go Build & Test pending) — worth confirming green before merge, particularly that the E2E suite itself passes through the new script on the first attempt, since that path changed too.

Reviewed as diffwireauto on Strycher's request. Not approving or requesting changes — these are findings for you to take or decline.

@diffwireauto diffwireauto left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving on Strycher's instruction, acting as diffwireauto.

CI is green on head 2475b571: Playwright E2E pass (16m19s), Go Build & Test pass, Docker image pass, change-detection and author-email pass. That E2E pass is the meaningful one — the initial server start now routes through scripts/e2e-fresh-server.sh, so the changed path was exercised and works.

Verified rather than assumed:

  • the pristine snapshot is taken at the Start Go server step, which runs after Freshen fixture timestamps, Migrate fixture DB to current schema (#1287), Seed observer IATA codes and Seed grouped-packet row for #1486. So the snapshot carries the goose stamp and the seeded rows, and the retry restores a correctly-migrated DB. A snapshot taken any earlier would have left the restored DB without goose_db_version, which deploy.yml notes the server refuses to start against — and it would only have failed on the retry path.
  • PRAGMA wal_checkpoint(TRUNCATE) before the copy is necessary and correct: the migrate step leaves a WAL, and without the flush the pristine copy would be incomplete.
  • both call sites are in the same e2e-test job on ubuntu-latest, so /tmp/e2e-fixture.pristine.db persists across the retry and is clean between runs.
  • tools/freshen-fixture.sh exists and is executable.
  • .DS_Store is in .gitignore:47, so deleting the tracked copy makes it stay gone.

The diagnosis is the part worth crediting: a retry that outlived the 15-minute Packets window was guaranteed to fail, which is indistinguishable from flake. Fixing the retry instead of masking it is the right call.

My two earlier findings stand but neither blocks: the wait loop has no kill -0 "$SERVER_PID" liveness check, so a server that dies at second 2 costs the full 180s (and twice, with the flake guard) — the pattern is already in your scripts/e2e-local.sh on #198; and the script has no trailing newline.

One thing noted for the future, not against this PR: the retry path itself is still unexercised, since the suite passed first time. It gets its first real test the next time something fails late.

@diffwireauto
diffwireauto merged commit 5206c70 into master Oct 12, 2026
9 checks passed
@diffwireauto
diffwireauto deleted the ci/e2e-fresh-fixture branch October 12, 2026 06:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants