fix: harden flaky e2e auth waits and capture CI server diagnostics#339
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
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
|
|
||
| try { | ||
| await accountChip.waitFor({ state: "visible", timeout: 15_000 }); | ||
| } catch { |
There was a problem hiding this comment.
🔵 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.
|
|
||
| try { | ||
| await expect(project).toBeVisible({ timeout: 15_000 }); | ||
| } catch { |
There was a problem hiding this comment.
🔵 Suggestion: Same bare catch concern as waitForAutoConnect — logging the original error before retrying would preserve diagnostic context in the Playwright report.
There was a problem hiding this comment.
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
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:
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.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/.