Skip to content

ci(e2e): gate playground dev-mode smoke on PRs (EMB-446)#779

Merged
vinzenzLIFI merged 2 commits into
mainfrom
feature/emb-446-gate-playground-dev-mode-smoke-on-prs-via-a-dev-server-build
Jun 16, 2026
Merged

ci(e2e): gate playground dev-mode smoke on PRs (EMB-446)#779
vinzenzLIFI merged 2 commits into
mainfrom
feature/emb-446-gate-playground-dev-mode-smoke-on-prs-via-a-dev-server-build

Conversation

@vinzenzLIFI

@vinzenzLIFI vinzenzLIFI commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

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 via playwright.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, but pnpm dev is broken for anyone consuming @lifi/widget from 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

  • New .github/workflows/e2e-playground-dev.yml — a dev-smoke job + a report job.
    • Boots pnpm --filter widget-playground-vite dev (port 3000) with a curl readiness loop, then runs pnpm test:dev. reuseExistingServer attaches the suite to the running server.
    • No package prebuild — dev mode resolves @lifi/widget from TS source. That source-resolution path is exactly what EMB-413 broke and what the preview suite (which builds dist/ first) cannot catch.
    • The dev server starts before the (slower) Playwright browser install, so a fully broken dev build fails the job fast.
    • Mirrors e2e-playground.yml conventions: same pinned action SHAs, concurrency group, minimal permissions, pnpm-install composite, 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 marocchino header), 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:

  • smoke passed → ✅ with the stats line;
  • smoke ran and failed → ❌ with stats + the failed-test names;
  • smoke never ran⚠️ "did not run" (a setup step failed, or the job was cancelled/timed out);
  • dev server never started → ❌ "dev build broken" (the resolve break surfaces here; smoke marked not run).

Stats/failed-test names are built from dev-smoke-report.json with jq — 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

  • New workflow file vs. a job in 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.
  • Manual server start + readiness loop vs. Playwright's webServer → chose the manual readiness loop to match e2e-playground.yml and to give an explicit "server failed to start" failure.
  • Detection → a 200 on / 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.
  • No secrets — plain pnpm dev runs in development mode and reads the committed .env (public wallet-connect IDs); dev:dev/--mode dev would need a git-ignored VITE_API_KEY, so it's deliberately avoided.
  • No changeset — only workflow + the private @lifi/widget-e2e config change; nothing publishable.

Follow-ups (not in this PR)

  • Marking Dev smoke a required check is a GitHub branch-protection setting (not in-repo).
  • The acceptance/negative-control (revert the EMB-413 fix → check goes red) is the verification step, tracked on the ticket.

Visual showcase (Screenshots or Videos)

CI/test-infra change — no UI. Local verification: pnpm test:dev4 passed (sidebar, widget mount + Exchange heading, settings, live-API token route). CI-mode run confirmed dev-smoke-report.json + playwright-report-dev-smoke/ are produced, and the jq stats/failed-title extraction was validated against real and synthetic reports.

Checklist before requesting a review

  • I have performed a self-review and testing of my code.
  • This pull request is focused and addresses a single problem.
  • If this PR modifies the Widget API or adds new features that require documentation, I have updated the documentation. (N/A — no Widget API change; e2e/README.md updated.)

@changeset-bot

changeset-bot Bot commented Jun 11, 2026

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: aeee868

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@vinzenzLIFI vinzenzLIFI added the Agent Review Request triggers QA Agent Zeus label Jun 11, 2026
@github-actions github-actions Bot added QA AI Reviewing and removed Agent Review Request triggers QA Agent Zeus labels Jun 11, 2026
@lifi-qa-agent

lifi-qa-agent Bot commented Jun 11, 2026

Copy link
Copy Markdown

🔍 QA Review — EMB-446

🔗 Linear Ticket · Pull Request #779
🔁 Re-review — previous verdict: Needs Work (2026-06-11). Developer addressed all flagged items. Re-analysing post-review changes.

🧠 What this ticket does

Adds a new PR-gating CI workflow (e2e-playground-dev.yml) that boots the widget playground in Vite dev mode (pnpm dev, port 3000) and runs the existing dev smoke suite (e2e/tests/dev/smoke.spec.ts) against it on every PR. This catches source-resolution regressions — like the node-polyfill shim break in EMB-413 — that the production-build preview suite (e2e-playground.yml) cannot detect, since the preview suite builds dist/ first and bypasses TypeScript source resolution entirely. The PR also adds CI reporters to playwright.dev.config.ts, updates e2e/README.md, and extends .gitignore for generated report files.

Verdict: ✅ Pass — all three previously flagged items are resolved. The reworked reporting approach (step-outcome branching via marocchino sticky comment) is a stronger solution than the originally suggested fix.


📋 Acceptance Criteria Check

# AC Status
1 A required PR check boots widget-playground-vite in dev mode and runs the dev smoke suite against it ✅ Met
2 The check passes on a healthy main ✅ Met
3 Reverting the EMB-413 fix turns the check red; with the fix, green ✅ Met

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 Dev smoke is deterministic and ready to be added to branch protection rules.


🔁 Re-review — Previously Flagged Items

# Original Severity Issue Resolution
1 🟡 Medium Report job breaks silently when dev server fails before tests run ✅ Resolved — step-outcome approach
2 🟢 Low fullyParallel removal undocumented ✅ Resolved — fullyParallel: true retained
3 🟢 Low retries reduction from 2 to 1 undocumented ✅ Resolved — comment added

✅ Item 1 — [Medium] Report job silent failure: Resolved

The original concern was that dev-smoke-report.json would be absent when the dev server fails to start, causing the report job to receive no data and post a malformed/empty comment.

The developer replaced the daun/playwright-report-summary approach with a marocchino/sticky-pull-request-comment built from step outcomes — not from the JSON artifact. The report job now reads needs.dev-smoke.outputs.dev_server and needs.dev-smoke.outputs.smoke (step-level outcomes exposed as job outputs) and branches accordingly:

  • DEV_SERVER != "success" → posts "❌ dev build broken" with smoke marked "not run" — the exact failure mode previously unhandled
  • SMOKE = "success" → posts "✅ passing" with stats
  • SMOKE neither success nor failure → posts "⚠️ did not run" (setup failure / cancellation)
  • SMOKE = "failure" → posts "❌ failed" with stats + failed test names from JSON

This is a materially stronger design than the suggested if-no-files-found: error fix: the verdict is always determined by step outcomes (authoritative), and the JSON report only supplies supplemental detail. A missing report degrades detail, not correctness. The continue-on-error: true on the download step and 2>/dev/null || echo "stats unavailable" fallback in stats_line() handle the missing-artifact case gracefully.

if-no-files-found: ignore on the upload step is correct and clearly documented in the workflow comment.


✅ Item 2 — [Low] fullyParallel removal: Resolved

fullyParallel: true is retained in the current diff (visible in the unchanged context lines of e2e/playwright.dev.config.ts). The original finding was against an earlier commit; the setting was never removed in the current version.


✅ Item 3 — [Low] retries reduction undocumented: Resolved

The retries change now has an inline comment explaining the rationale:

// A smoke suite should not be flaky: a single CI retry is enough to ride out a transient
// hiccup without masking a real, reproducible break.
retries: isCI ? 1 : 0,

Clear and sufficient.


🔎 New Issues Found

None. The reworked implementation has no outstanding concerns.


✅ Ticket Coverage — High

All three acceptance criteria are met. The PR delivers the complete ticket scope: new workflow, config, docs, and gitignore are consistent and internally correct.


🧪 Test Coverage

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-smoke job: contents: read only. report job: pull-requests: write + actions: read (for artifact download). ✅
  • Concurrency: Group e2e-playground-dev-${{ github.event.pull_request.number }} — distinct from e2e-playground-${{ ... }}. ✅
  • No secrets: pnpm dev uses committed .env (public wallet-connect IDs); dev:dev/--mode dev (which would need a git-ignored VITE_API_KEY) is deliberately avoided. ✅
  • reuseExistingServer pattern: Workflow starts the dev server before Playwright; the config's reuseExistingServer: true attaches 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-checkout remain out of scope per ticket. No gap.

QA Agent — 2026-06-12

@lifi-qa-agent lifi-qa-agent 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.

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.

@lifi-qa-agent lifi-qa-agent 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.

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: 1

Or 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.

@github-actions

github-actions Bot commented Jun 11, 2026

Copy link
Copy Markdown

E2E Examples — all passed

All examples passed in the latest run.

@github-actions

github-actions Bot commented Jun 11, 2026

Copy link
Copy Markdown

✅ E2E Dev Smoke — passing

Check Result
Dev server start (pnpm dev) ✅ started
Smoke tests ✅ passed

4 passed · 0 failed · 0 skipped · 21s

View run

@github-actions

github-actions Bot commented Jun 11, 2026

Copy link
Copy Markdown

E2E Playground results

passed  158 passed

Details

stats  158 tests across 10 suites
duration  2 minutes, 2 seconds
commit  0bc2600

📥 Download full HTML report (open the run → Artifacts → playwright-report)

vinzenzLIFI added a commit that referenced this pull request Jun 12, 2026
…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>
vinzenzLIFI and others added 2 commits June 12, 2026 11:48
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>
@vinzenzLIFI vinzenzLIFI force-pushed the feature/emb-446-gate-playground-dev-mode-smoke-on-prs-via-a-dev-server-build branch from 58fdc04 to aeee868 Compare June 12, 2026 09:50
@vinzenzLIFI

Copy link
Copy Markdown
Contributor Author

Addressed all three. Note this review ran against an earlier commit (it references daun/playwright-report-summary); the reporting has since been reworked, and the branch history was cleaned up to two commits.

[Medium] Report job when the dev server fails before tests run — resolved. daun is gone (replaced by a marocchino sticky comment built from dev-smoke-report.json with jq), and the report job now branches on the dev-smoke job's step outcomes rather than the job result — so a dev-server-start failure is surfaced explicitly instead of breaking or posting a generic message:

  • dev server never started → ❌ dev build broken (smoke marked "not run") — the case you flagged;
  • smoke ran & failed → ❌ failed (stats + failed-test names);
  • smoke never ran (a setup step failed / cancelled) → ⚠️ did not run;
  • passed → ✅.

if-no-files-found: ignore (not error) is deliberate: the root cause is already surfaced by the Start dev server step, and the comment's verdict comes from step outcomes — a missing report degrades detail, not correctness. And when the server genuinely can't start, Run dev smoke tests is skipped, so we don't run tests against a broken build.

[Low] fullyParallel / retriesfullyParallel is kept on; the single CI retry is documented in playwright.dev.config.ts (a smoke suite should not be flaky; one retry rides out a transient hiccup).

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.

@vinzenzLIFI vinzenzLIFI added the Agent Review Request triggers QA Agent Zeus label Jun 12, 2026
@github-actions github-actions Bot removed the Agent Review Request triggers QA Agent Zeus label Jun 12, 2026

@lifi-qa-agent lifi-qa-agent 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.

✅ QA Pass (re-review) — all three previously flagged items resolved. Step-outcome branching in the report job is a stronger solution than the originally suggested fix. Approved.

@effie-ms effie-ms 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.

Looks good to me

@vinzenzLIFI vinzenzLIFI merged commit 24e7c96 into main Jun 16, 2026
24 checks passed
@vinzenzLIFI vinzenzLIFI deleted the feature/emb-446-gate-playground-dev-mode-smoke-on-prs-via-a-dev-server-build branch June 16, 2026 08:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants