ci(e2e): gate playground dev-mode smoke on PRs (EMB-446)#779
Conversation
|
🔍 QA Review — EMB-446
🧠 What this ticket doesAdds a new PR-gating CI workflow (
📋 Acceptance Criteria Check
AC-1 note: The "required" gating is a GitHub branch-protection setting (outside this PR's scope). The README correctly documents that both modes gate PRs; the post-merge configuration step is tracked on the ticket. The CI job name 🔁 Re-review — Previously Flagged Items
✅ Item 1 — [Medium] Report job silent failure: ResolvedThe original concern was that The developer replaced the
This is a materially stronger design than the suggested
✅ Item 2 — [Low]
|
| Layer | Score | Notes |
|---|---|---|
| Unit (Vitest) | N/A | Pure CI/config change — no implementation files |
| E2E (Playwright) | ✅ Good | e2e/tests/dev/smoke.spec.ts (pre-existing, 4 tests covering widget render, settings, live-API token route) wired into CI gating |
No test coverage gaps — the mandate is to wire existing tests into CI, which is done correctly.
Additional Checks
- SHA pinning (E9): All
uses:entries carry 40-character commit SHAs (checkout,cache,upload-artifact×2,download-artifact,marocchino/sticky-pull-request-comment). ✅ - Minimal permissions:
dev-smokejob:contents: readonly.reportjob:pull-requests: write+actions: read(for artifact download). ✅ - Concurrency: Group
e2e-playground-dev-${{ github.event.pull_request.number }}— distinct frome2e-playground-${{ ... }}. ✅ - No secrets:
pnpm devuses committed.env(public wallet-connect IDs);dev:dev/--mode dev(which would need a git-ignoredVITE_API_KEY) is deliberately avoided. ✅ reuseExistingServerpattern: Workflow starts the dev server before Playwright; the config'sreuseExistingServer: trueattaches to the running server. ✅- Live CI validation confirmed: Developer verified on this PR — reverting EMB-413 fix turned the check red (4 failed tests, ❌ comment posted); restoring it flipped the sticky comment back to ✅. ✅
🔗 Downstream Impact
- EMB-413 (node-polyfill shim resolution failure) — AC-3 correctness verified end-to-end by developer. ✅
- EMB-414 (source-consuming app gating) —
widget-embedded,nft-checkoutremain out of scope per ticket. No gap.
QA Agent — 2026-06-12
There was a problem hiding this comment.
Requesting changes on 3 items — each requires either a code fix or an explicit acceptance comment with justification before this review is considered complete.
| # | Severity | Type | Issue / File |
|---|---|---|---|
| 1 | 🟡 Medium | Code | Report job breaks silently when dev server fails before tests run — missing if-no-files-found: error on artifact upload |
| 2 | 🟢 Low | Documentation | fullyParallel removal undocumented in e2e/playwright.dev.config.ts |
| 3 | 🟢 Low | Documentation | retries reduction from 2 to 1 undocumented in e2e/playwright.dev.config.ts |
1. [Medium] Report job breaks silently when dev server fails before tests run
When the dev server fails to start (the Start dev server step exits 1), dev-smoke-report.json is never created. The Upload JSON report step uses if: ${{ !cancelled() }} and silently succeeds because actions/upload-artifact defaults to if-no-files-found: warn — it creates an empty artifact. The report job then downloads the empty file and passes it to daun/playwright-report-summary, which will error or post a malformed comment. This leaves the report job in an undefined failure state rather than surfacing the real root cause (dev server crashed).
Fix: add if-no-files-found: error to the Upload JSON report step in .github/workflows/e2e-playground-dev.yml. This causes the upload step (and therefore dev-smoke) to fail fast and clearly, and means Download JSON report in the report job will fail with artifact not found rather than silently consuming empty data.
2. [Low] fullyParallel removal undocumented
fullyParallel: true was present in the original playwright.dev.config.ts and was removed in this PR (Playwright default is false). With fullyParallel: false, the four tests in smoke.spec.ts now run sequentially. This is a minor CI time increase rather than a correctness issue, but the intent is not stated anywhere. Please either add a brief comment in the config explaining the reasoning, or confirm in a PR comment that the removal is intentional.
3. [Low] retries reduction from 2 to 1 undocumented
retries: isCI ? 2 : 0 was changed to isCI ? 1 : 0. Reasonable for a smoke suite but not mentioned in the PR description. Please add a one-liner to the description explaining why 1 retry is sufficient here.
💡 Once you've addressed the items above, re-apply the "Agent Review Request" label to trigger an automated re-review.
There was a problem hiding this comment.
Requesting changes on 3 items — each requires either a code fix or an explicit acceptance comment with justification before this review is considered complete.
| # | Severity | Type | Issue / File |
|---|---|---|---|
| 1 | 🟡 Medium | Code | Report job breaks silently when dev server fails before tests run (missing artifact guard) |
| 2 | 🟢 Low | Documentation | fullyParallel removal undocumented — implicit behaviour change in playwright.dev.config.ts |
| 3 | 🟢 Low | Documentation | retries reduction from 2 to 1 undocumented in PR description |
1. [Medium] Report job breaks silently when dev server fails before tests run
When the Start dev server step fails (dev server can't start within 60s), dev-smoke-report.json is never produced. The Upload JSON report step (if: !cancelled()) runs but finds no file — upload-artifact defaults to if-no-files-found: warn, so it emits a warning but creates an empty/absent artifact. The report job then tries to download that non-existent artifact and either errors or passes malformed data to daun/playwright-report-summary, producing no useful PR comment.
This is the exact failure mode the workflow is designed to detect (broken dev build = server fails), so the reporting path breaks precisely when it's most needed.
Fix: add if-no-files-found: error to .github/workflows/e2e-playground-dev.yml Upload JSON report step:
with:
name: dev-smoke-report-json
path: e2e/dev-smoke-report.json
if-no-files-found: error # ← add this line
retention-days: 1Or alternatively adopt the check-blobs / graceful "no data" comment pattern from e2e-playground.yml.
2. [Low] fullyParallel removal undocumented
fullyParallel: true was removed from e2e/playwright.dev.config.ts without mention in the PR description. Playwright's default is false (sequential within a file). For a 4-test suite the impact is minor, but the intent should be documented — a one-line comment in the config file (e.g. // intentionally serial — smoke suite is small and sequential aids stability) or a sentence in the PR description.
3. [Low] retries reduction from 2 to 1 undocumented
retries: isCI ? 2 : 0 was changed to retries: isCI ? 1 : 0 in e2e/playwright.dev.config.ts without mention in the PR description. This is a reasonable choice for a stable smoke suite but should be acknowledged as an intentional decision (e.g. "reduced to 1 — smoke suite should be stable and 2 retries masked real flakiness").
💡 Once you've addressed the items above, re-apply the "Agent Review Request" label to trigger an automated re-review.
E2E Examples — all passedAll examples passed in the latest run. |
✅ E2E Dev Smoke — passing
4 passed · 0 failed · 0 skipped · 21s |
E2E Playground resultsDetails
📥 Download full HTML report (open the run → Artifacts → |
…446) The report job posts/updates a single sticky comment every run (no conditional posting), branching on the dev-smoke STEP outcomes (exposed as job outputs) so the comment reflects what actually happened — a later step failure (e.g. artifact upload) can't masquerade as a test failure. Four states: smoke passed (stats), smoke failed (stats + failed tests), smoke didn't run (setup failure/cancellation), dev server never started (dev build broken). Folds in the QA review on #779: - dev-server-start failure is surfaced explicitly, not a generic 'report unavailable'; - retries kept at 1 (a smoke suite shouldn't be flaky; one retry rides out a hiccup), documented in playwright.dev.config.ts; fullyParallel kept on. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add e2e-playground-dev.yml: boots the playground in Vite dev mode (pnpm dev, port 3000) and runs the existing dev smoke suite against it, gating the source-resolution path the preview suite cannot catch (the EMB-413 class). No package prebuild — dev mode resolves @lifi/widget from source; no secrets — plain `pnpm dev` uses the committed .env. The dev server starts before the Playwright browser install so a broken dev build fails fast. Reporting: every run posts/updates one sticky PR comment, branching on the dev-smoke job's step outcomes (not the job result, so a later step failure can't masquerade as a test failure) — passing, failed (stats + failed tests), did-not-run, or dev-build-broken. Stats come from dev-smoke-report.json via jq and only add detail, never the verdict. playwright.dev.config.ts emits JSON + HTML reports in CI; retries kept at 1 (a smoke suite should not be flaky). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add the dev-mode smoke workflow to the suite/CI table and reporters section of e2e/README.md, and describe the single always-updated sticky PR comment. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
58fdc04 to
aeee868
Compare
|
Addressed all three. Note this review ran against an earlier commit (it references [Medium] Report job when the dev server fails before tests run — resolved. daun is gone (replaced by a
[Low] fullyParallel / retries — Verified live on this PR: reverting the EMB-413 fix (#762) turned the check red and rendered the ❌ failed comment (stats + 4 failed tests); restoring it flipped the same sticky comment back to ✅. Re-applying the Agent Review Request label for a re-review. |
Which Linear task is linked to this PR?
EMB-446 — Gate playground dev-mode smoke on PRs via a dev-server build-and-smoke job
Why was it implemented this way?
The dev-mode smoke suite already existed (
e2e/tests/dev/smoke.spec.ts, run viaplaywright.dev.config.ts/pnpm test:dev) but did not run on PRs. Only the preview suite gated (e2e-playground.yml, against the production build at:4173). So dev-only regressions could still ship — most recently the node-polyfill shim resolve break in EMB-413: the playground builds and previews fine, butpnpm devis broken for anyone consuming@lifi/widgetfrom source.This adds a new PR-gating workflow that boots the playground in Vite dev mode, confirms the dev server is live, then runs the existing smoke suite against it.
What's in the PR
.github/workflows/e2e-playground-dev.yml— adev-smokejob + areportjob.pnpm --filter widget-playground-vite dev(port 3000) with a curl readiness loop, then runspnpm test:dev.reuseExistingServerattaches the suite to the running server.@lifi/widgetfrom TS source. That source-resolution path is exactly what EMB-413 broke and what the preview suite (which buildsdist/first) cannot catch.e2e-playground.ymlconventions: same pinned action SHAs, concurrency group, minimal permissions,pnpm-installcomposite, Playwright browser cache, and the source-package path filter.e2e/playwright.dev.config.ts— adds CI reporters: JSON (dev-smoke-report.json) + HTML (playwright-report-dev-smoke/, uploaded as a failure artifact).e2e/README.md— documents the new dev-smoke gate in the suite/CI table and reporters section.e2e/.gitignore— ignore the generated reports.Single always-updated sticky PR comment — every run posts/updates one comment (one
marocchinoheader), so it always reflects current state and a stale signal can't survive a corrective commit. It branches on the dev-smoke job's step outcomes (exposed as job outputs), not the job result — so a later step failure (e.g. artifact upload) can't masquerade as a test failure:Stats/failed-test names are built from
dev-smoke-report.jsonwithjq— they only supply detail, never the verdict (which comes from the step outcomes), so a missing report degrades the detail, not the correctness. (daun was dropped — its always-create-or-update behaviour can't express these distinct states cleanly.)Other design decisions & alternatives
e2e-playground.yml→ chose a separate file for clean separation and an independent required-check name; the dev job has no dependency on the preview build pipeline.webServer→ chose the manual readiness loop to matche2e-playground.ymland to give an explicit "server failed to start" failure./is not a health signal (it's just the Vite index.html shell); the real "dev build healthy" assessment is the smoke suite rendering the widget. The readiness loop is only a coarse gate to start the tests.pnpm devruns indevelopmentmode and reads the committed.env(public wallet-connect IDs);dev:dev/--mode devwould need a git-ignoredVITE_API_KEY, so it's deliberately avoided.@lifi/widget-e2econfig change; nothing publishable.Follow-ups (not in this PR)
Visual showcase (Screenshots or Videos)
CI/test-infra change — no UI. Local verification:
pnpm test:dev→ 4 passed (sidebar, widget mount + Exchange heading, settings, live-API token route). CI-mode run confirmeddev-smoke-report.json+playwright-report-dev-smoke/are produced, and thejqstats/failed-title extraction was validated against real and synthetic reports.Checklist before requesting a review