Skip to content

fix: harden flaky e2e auth waits and capture CI server diagnostics#339

Merged
tnrdd merged 2 commits into
mainfrom
fix/e2e-ci-flake-hardening
Jul 3, 2026
Merged

fix: harden flaky e2e auth waits and capture CI server diagnostics#339
tnrdd merged 2 commits into
mainfrom
fix/e2e-ci-flake-hardening

Conversation

@tnrdd

@tnrdd tnrdd commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Why

test-main failed twice in a row on the #338 merge commit (runs 28615140933 original + rerun), both times on the same e2e test: submit-application. The code is not at fault: the identical tree passed the PR e2e run, and the full suite passes locally 10/10 against the same test DB. Two distinct first-attempt flake modes were observed in CI, plus one unexplained follow-on:

  1. Wallet auto-connect never fires (rerun attempt 1, and the June 30 flake): page hydrates, DB-backed fetches succeed, but the wagmi injected auto-reconnect never connects, so the account chip never appears.
  2. One-shot projects fetch never populates (original run attempt 1): page loads and authenticates fine, rounds API returns, but the single un-retried projects fetch leaves the list empty forever.
  3. On every retry after a first-attempt failure, the page navigation itself hung for the full test timeout with zero response recorded in the trace. Whether the server or the browser was stuck is currently unobservable: the backgrounded pnpm start & loses its output when the step ends.

What

  • waitForAutoConnect: reload once and re-wait if the account chip does not appear (mode 1).
  • submit-application.e2e.ts: reload once and re-assert if the seeded project does not appear (mode 2).
  • playwright.config.ts: test timeout 30s -> 60s so the reload fallbacks fit within a test.
  • Both workflows: capture server output to server.log, and on failure probe the server with curl from outside the browser (settles server-wedged vs browser-stuck for mode 3), print the log tail inline, and upload the log as an artifact.

Formatting-only churn in a few test files is from prettier, which does not normally cover tests/ and .github/.

@vercel

vercel Bot commented Jul 2, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
platform Ready Ready Preview, Comment Jul 2, 2026 8:03pm

Request Review

@gaston-review gaston-review Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Gaston Review

Verdict: Approved

Score: ███████░░░ 7/10

Pull Request Summary

This PR tackles flaky E2E tests in CI through three angles: (1) retry-with-reload fallbacks in waitForAutoConnect and the submit-application test to handle intermittent wagmi auto-reconnect and API fetch failures, (2) bumping the Playwright test timeout from 30s to 60s to accommodate those retries, and (3) capturing server stdout/stderr to a log file and adding a curl-based probe step plus artifact upload on failure for post-mortem diagnostics. The rest is formatter-driven whitespace cleanup.

Review Summary

🔵 Suggestion — Blanket catch in retry helpers swallows error context
Both waitForAutoConnect and the submit-application retry use bare catch {} blocks that discard the original error. If the first attempt fails for a reason other than a timeout (e.g., page crash, unexpected element count), the original error message is lost and the second attempt may produce a confusing, unrelated failure. Consider logging the caught error to stderr or Playwright's test info so the first-attempt failure is still visible in reports: catch (err) { console.warn('Auto-connect failed, retrying after reload:', err); ... }.

🔵 Suggestion — Timeout budget stacking under worst-case retry paths
enterAuthenticated calls waitForAutoConnect twice (navigate → wait → sign-in → navigate → wait). With the new retry, each call can burn up to 30s (15s + reload + 15s). Two calls = 60s worst case, which equals the new global timeout. Add in signInViaSiweApi and navigation time and a single test attempt could exceed the 60s timeout before the retry even surfaces the real error. This is mitigated by Playwright's 2 CI retries, but worth keeping an eye on — if flakes persist you may need to either lower the per-wait timeouts or raise the global one further.

Well-reasoned defensive fix for a real CI problem. The retry patterns are narrow and well-commented, the timeout increase is proportional, and the diagnostics additions (server log capture, curl probes, artifact uploads) will genuinely help debug the next flake. The blanket catch in the retry blocks is the only thing that gave me pause, but for e2e test helpers that fail loudly on the second attempt, it's acceptable. Clean approve.

📌 2 inline comments
🔵 Suggestion: 2


🔍 Reviewed by Gaston

Comment thread tests/e2e/helpers/signIn.ts Outdated

try {
await accountChip.waitFor({ state: "visible", timeout: 15_000 });
} catch {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 Suggestion: Bare catch {} discards the original error. If this fails for a non-timeout reason (page crash, multiple matching elements), you'll only see the second attempt's error with no indication of what the first one was. Consider catch (err) { console.warn('waitForAutoConnect: first attempt failed, reloading', err); } so first-attempt context shows up in the Playwright report.

Comment thread tests/e2e/submit-application.e2e.ts Outdated

try {
await expect(project).toBeVisible({ timeout: 15_000 });
} catch {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 Suggestion: Same bare catch concern as waitForAutoConnect — logging the original error before retrying would preserve diagnostic context in the Playwright report.

@gaston-review gaston-review Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Gaston Review

Verdict: Approved

Score: ███████░░░ 7/10

Pull Request Summary

This PR hardens flaky E2E tests in CI with retry-after-reload fallbacks in waitForAutoConnect and the submit-application test, bumps the Playwright timeout to 60s to accommodate worst-case retries, and adds server log capture plus curl-based probes on failure for post-mortem diagnostics. A large chunk of the diff is formatter-driven whitespace cleanup in the README and workflow files.

The author addressed the only concern from the first review: both retry blocks now log the first-attempt error via console.warn before reloading, so diagnostic context is preserved. No new issues introduced. The retry patterns are narrow, the diagnostics additions are genuinely useful, and the timeout increase is proportional. Clean approve.


🔍 Reviewed by Gaston

@tnrdd tnrdd merged commit c6cc829 into main Jul 3, 2026
4 checks passed
@tnrdd tnrdd deleted the fix/e2e-ci-flake-hardening branch July 3, 2026 04:47
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.

1 participant