test(e2e): stabilize playground smoke - #2834
Conversation
|
@emdash-cms/admin
@emdash-cms/auth
@emdash-cms/auth-atproto
@emdash-cms/blocks
@emdash-cms/cloudflare
@emdash-cms/contentful-to-portable-text
emdash
create-emdash
@emdash-cms/gutenberg-to-portable-text
@emdash-cms/plugin-cli
@emdash-cms/plugin-types
@emdash-cms/registry-client
@emdash-cms/registry-lexicons
@emdash-cms/registry-moderation
@emdash-cms/registry-verification
@emdash-cms/sandbox-workerd
@emdash-cms/x402
@emdash-cms/plugin-ai-moderation
@emdash-cms/plugin-atproto
@emdash-cms/plugin-audit-log
@emdash-cms/plugin-color
@emdash-cms/plugin-embeds
@emdash-cms/plugin-field-kit
@emdash-cms/plugin-forms
@emdash-cms/plugin-webhook-notifier
commit: |
There was a problem hiding this comment.
This PR stabilizes the workerd-backed Playground E2E smoke by moving it to a dedicated CI job, extending the Playwright test timeout from 120 s to 240 s while keeping the webServer startup budget at 120 s, and switching tracing from on-first-retry (ineffective because the Playground config has no retries) to retain-on-failure. It also updates the E2E Tests rollup to gate on Node, Cloudflare, and Playground lanes. The approach is focused and matches the stated problem.
I checked the diff, the full workflow file, the Playground Playwright config, the package scripts, and related workflow files. Everything is consistent except the artifact upload paths in the new test-e2e-playground job: Playwright resolves outputDir relative to the config file's directory, so traces and screenshots land under e2e/playground/test-results/, not the repo-root test-results/ the upload step currently references. Since retaining traces on the first failure is one of the PR's stated goals, this path should be fixed before merge.
🚀 Deploying Preview to Cloudflare 🚀Preview URL: https://codex-stabilize-playground-e2e.try.emdashcms.com, https://codex-stabilize-playground-e2e-emdash-playground.emdash-cms.workers.dev (commit dcbb6de)This URL reflects your latest Preview deploymentPreview Deployments by commit
|
There was a problem hiding this comment.
Re-review summary
This is a re-review of the Playground E2E stabilization PR. The approach is still focused and sensible: split the Playground smoke into its own CI job so it is no longer gated behind the Cloudflare shard 1 runtime, raise the in-test navigation budget to 240 s, and keep a 360 s per-test ceiling. Switching tracing to retain-on-failure while adding one CI retry means failed runs should now leave useful artifacts.
I re-checked the diff, the full workflow file, and the Playground Playwright config. The only previously raised issue is still unresolved. The new test-e2e-playground job advertises retain-on-failure traces, but its artifact upload step points at the repo-root test-results/ directory. Because the Playground config lives in e2e/playground/playwright.config.ts and Playwright resolves the default outputDir relative to the config file, traces/screenshots actually land in e2e/playground/test-results/. The upload step will therefore collect nothing (and the actions/upload-artifact step will warn or fail because the path does not exist), which directly undermines one of the PR’s stated goals.
No other blocking concerns. The timeout and retry changes are proportionate to the failure evidence, and the rollup job correctly gates on Node, Cloudflare, and Playground lanes.
Still open from the previous review: fix the Playground artifact upload path.
There was a problem hiding this comment.
This is a re-review of the Playground E2E stabilization PR. The previously raised artifact-path mismatch is now resolved: e2e/playground/playwright.config.ts explicitly sets outputDir to the repo-root test-results/ directory, so the test-e2e-playground job’s upload-artifact step will actually collect traces and screenshots.
The overall approach is still sensible and proportionate to the failure evidence:
- Pulling the Playground smoke out of the Cloudflare shard 1 job removes it from the long workerd runtime lane.
- Raising
waitForURLto 240 s and the per-test ceiling to 360 s gives the workerd-backed admin init enough headroom. retain-on-failureplus one CI retry aligns trace retention with the goal of diagnosing flaky failures.- The rollup job now correctly gates on Node, Cloudflare, and Playground lane results.
I re-read the full ci.yml, the Playground Playwright config, and the spec. No code-level bugs or AGENTS.md convention violations jumped out.
One non-blocking thing to double-check before merge: the new CI build step uses pnpm run --filter "@emdash-cms/playground^..." build (.github/workflows/ci.yml, line 297). That ^... selector pattern does not appear anywhere else in the repo — every other dependency-expansion build step uses <pkg>... without the caret (e.g. --filter emdash..., --filter @emdash-cms/admin..., --filter "emdash-e2e-fixture-cloudflare..."). I cannot execute pnpm here to confirm whether @emdash-cms/playground^... is valid syntax in pnpm 11. If it is a typo, the job will fail with “No projects matched the filters.” If it’s intentional and supported, it’s fine; otherwise drop the caret to match the existing convention.
What does this PR do?
Stabilizes the workerd-backed Playground smoke after repeated CI runs exhausted its 120-second initialization deadline. The retained trace from this PR confirmed that
POST /_playground/initwas still pending when the navigation timeout fired.test-results/directory consumed by the artifact upload.E2E Testsroll-up.Failure evidence: run 33536074249 and run 33511649004.
Related issue: none.
Type of change
Checklist
pnpm typecheckpassespnpm lintpassesAI-generated code disclosure
Screenshots / test output
No screenshots: this changes test orchestration only.
pnpm buildpnpm run --filter "@emdash-cms/playground^..." buildtest-results/pnpm typecheckpnpm lintgit diff --check