Skip to content

Fail CI on a committed test.only, and keep a failed E2E's screenshots - #649

Merged
jehanazad merged 2 commits into
mainfrom
ci/playwright-forbid-only-failure-artifacts
Oct 1, 2026
Merged

jehanazad merged 2 commits into
mainfrom
ci/playwright-forbid-only-failure-artifacts

Conversation

@Babissimo

Copy link
Copy Markdown
Contributor

ClickUp: 2.8 Small CI and test hygiene fixes, its two Playwright items: forbidOnly, and evidence from a failed E2E.

Stacked on #648 (1.1), itself on #590 (1.4). This PR's own change is the tip commit.

Why

  • A stray test.only would go unnoticed. Playwright had no forbidOnly, so a test.only committed by mistake would quietly shrink the suite that decides whether production rolls back.
  • A failed E2E left nothing to look at. Neither E2E job kept any evidence beyond its log.

What changes

  • forbidOnly in CI. A committed test.only fails the run instead of narrowing it. Staging's E2E runs first, so that's where it fires.
  • Production collects its suite before running it. A new step, "Collect the E2E tests (production)", lists the suite with the same command and environment as the test step, capped at 3 minutes. A suite that can't load, from a test.only or from a spec that throws at module scope, then fails e2e-prod without failing the step the rollback reads. Only tests that ran and failed roll production back.
  • Both E2E jobs keep the failed tests' screenshots. The upload is capped at 2 minutes and allowed to fail, so collecting evidence can't cancel e2e-prod before it publishes the verdict the rollback reads. The artifact name carries the run attempt, because a name can't repeat within a run. e2e-prod's job cap rises from 30 to 35 minutes to cover the new steps.

Traces are excluded

The ticket asks for traces. This PR uploads only screenshots, and CI records no traces at all, until Alexander decides otherwise. The reasons:

  • Artifacts are public. This repository is public, so its artifacts are too, and GitHub masks secrets only in logs.
  • Both kinds of evidence leak keys. A Playwright trace records every request's headers. error-context.md holds a call log that lists them as well. Either would publish the node API key and the Cloudflare Access token.
  • The screenshots and the log are enough to start from. The error text is already in the job log, masked.

Local runs trace as they did before, on the first retry.

What the tests pin

In test_deploy_workflows.py:

  • Uploads: every upload from a job that runs Playwright, whatever its path, and every upload of test results in any workflow, is screenshots only, with hidden files off, and within the cap. A workspace-wide upload from e2e-prod now fails the test.
  • Ordering: production's collect step comes before its test step and matches it.
  • Job caps: each E2E job's cap sits above its steps' own caps, with 5 minutes for the pull and npm ci. A job timeout cancels the test step, and a cancelled step is no verdict, so a production wedged enough to hang the suite would never roll back. A test step raised to 30 minutes now fails the test.

Overlap with #625

#625 (web/strict-e2e-shared) edits e2e/playwright.config.ts too, on different lines, and git merges it with this stack cleanly in either order, so neither has to wait for the other.

It also carries this ticket's stale-reference item (e2e/specs/env.d.ts), so this PR leaves that file alone.

If #625 lands first, its pre-merge listing runs under CI=true, and forbidOnly then fails a PR's typecheck step before merge rather than staging's E2E after it. Until then, staging's E2E is where a committed test.only first fails.

Known limits

  • The upload delays the E2E rollback. On the failure path the rollback waits for the screenshot upload, by at most its 2-minute cap. That's the price of keeping any evidence at all.
  • Admin-surface screenshots show nobody signed in. They render as the Access service token sees the page.
  • nodes.spec.ts failures leave no screenshot. Their error is in the masked log.
  • Other non-test failures still roll production back. A failure after collection that isn't a test verdict, such as a missing browser or a worker crash, can still fail the step the rollback reads.

Verification

  • forbidOnly: listing production's suite under CI=true exits 0 with 98 tests, and exits 1 with forbidOnly's error once a trial test.only is added. This ran under the shared suite lock, and the trial file is gone.
  • Controls: a workspace-wide upload and an over-long test step each fail their test.
  • Workflow tests: the pins, permissions and deploy-workflow tests and their neighbours pass (339), and so does the backend suite with CI's flags. pre-commit is clean.
  • Not exercised yet: both E2E jobs run only after a merge to main, so the upload's first real run is the first failed E2E after this lands.

🤖 Generated with Claude Code

@claude

This comment has been minimized.

@claude

This comment has been minimized.

@Babissimo

Copy link
Copy Markdown
Contributor Author

Rebased with the stack onto main at ad12bbd, over #644 (the E2E jobs run on the runner's Chrome, not in Playwright's container) and #651 (staging reads Chrome's version, production launches it). Head: e25c7b8.

This PR's premises hold under the runner's Chrome:

  • Screenshots and error-context.md still land in e2e/test-results/: the output directory is relative to the config, not to the browser or the container.
  • CI still records no traces, and the upload path is relative to the workspace as before, so the screenshots-only guard is unchanged in what it matches.

What changed:

  • e2e-prod's cap stays at 30 minutes. The description says it rises to 35 to cover the new steps. With no image to pull, collect (3), tests (20) and upload (2), with 5 minutes for checkout, npm ci and the Chrome launch, fit under 30. The cap test now counts those setup steps rather than "the pull and npm ci".
  • The collect step follows the Chrome launch. A Chrome that cannot start fails the launch before collection, which narrows the description's last known limit: "a missing browser" no longer reaches the step the rollback reads, though a worker crash still can.
  • _runs_playwright recognises a job by its test step alone. Its clause for a Playwright container matched nothing once Run the E2E suites in the runner's Chrome, not Playwright's container #644 landed.

Verification on this head: the backend suite with CI's flags passes (5451 passed, 3 skipped). Listing production's suite under CI=true exits 0 with 98 tests, and exits 1 with forbidOnly's error once a trial test.only is added; the trial file is gone. A workspace-wide upload from either E2E job, a 30-minute test step and a 4-minute collect step each fail their test. pre-commit is clean.

Playwright had no forbidOnly, so a test.only committed by mistake would quietly shrink the suite that decides whether production rolls back. In CI it now fails the run instead; staging's E2E, which runs first, is where it fires. Production's E2E job collects the suite in a step of its own before running it, so a suite that cannot load, from a test.only or a spec that throws at module scope, fails the job without failing the step the rollback reads: only tests that ran and failed roll production back.

A failed staging or production E2E left nothing to inspect. Both E2E jobs now upload the failed tests' screenshots. Only the screenshots, although the ticket asks for traces: this repository's artifacts are public and GitHub masks secrets only in logs. A trace records every request's headers, and error-context.md holds a call log that lists them too; both would publish the node API key and the Cloudflare Access token. The error text is already in the job log, masked. With nothing to publish them, CI records no traces. The upload is bounded and allowed to fail, so collecting evidence cannot cancel e2e-prod before it publishes the verdict the rollback reads. The artifact name carries the run attempt, because a name cannot repeat within a run. e2e-prod's cap rises from 30 to 35 minutes, so the new steps' caps do not come out of the ten minutes its setup steps had.

test_deploy_workflows.py holds every upload from a job that runs Playwright, and every upload of test-results in any workflow, to screenshots, with hidden files off, and to the bound. It pins production's collect step ahead of its test step, and each E2E job's cap above its steps' own caps with room for the setup steps, since a job timeout cancels the test step and a cancelled step is no verdict.

ClickUp 123zgec4mxy.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Babissimo
Babissimo force-pushed the ci/playwright-forbid-only-failure-artifacts branch from e25c7b8 to 3dbf964 Compare September 28, 2026 12:35
@claude

This comment has been minimized.

@Babissimo

Copy link
Copy Markdown
Contributor Author

On the review's cap finding: taken. e2e-prod's cap is now 35 minutes, as the description says, which supersedes "stays at 30" in my rebase note above.

With 30, the new collect (3) and upload (2) caps came out of the ten minutes main's job leaves its setup steps, down to five. A slow npm ci beside a test step that ran to its own cap could then see the job cancelled before or during the upload, which is the no-verdict case this PR guards against. 35 keeps the setup's ten minutes and costs nothing on a normal run. The cap test still asks for at least five, and now says so.

Head: 3dbf964. The workflow, pin, permission and deploy-workflow tests and their neighbours pass (346) under the suite lock, a 30-minute test step and a 9-minute collect step each fail the cap test, and pre-commit is clean. Only this workflow's value and two comments changed since e25c7b8, whose full backend suite passed locally.

Base automatically changed from ci/prod-deploy-verify-lock to main October 1, 2026 00:55
@claude

claude Bot commented Oct 1, 2026 •

Copy link
Copy Markdown

Claude finished @jehanazad's task in 2m 37s —— View job


Code review

  • Gather context (PR description, changed files, prior review + author follow-up)
  • Diff current head (37e9e9d) against origin/main, and against 3dbf964 (the commit the prior Claude review covered)
  • Re-verify the previously-flagged cap issue and trace all four files by hand
  • Post findings

I diffed origin/main...HEAD (128/-8 across the four files, matching the PR stats) and separately diffed 3dbf964..HEAD to see what changed since the last review pass. The only delta in that range touches e2e/playwright.config.ts, and it's main's independent hostOrSkip/ENV refactor arriving via the Merge branch 'main' commit — not a change to this PR's own lines. Both of this PR's settings survived that merge intact:

  • forbidOnly: !!process.env.CI (e2e/playwright.config.ts:144)
  • trace: process.env.CI ? "off" : "on-first-retry" (e2e/playwright.config.ts:155)

The other three files (production-deploy-verify.yml, staging-deploy-verify.yml, test_deploy_workflows.py) are byte-identical to 3dbf964, so nothing new to review there.

Previously flagged cap issue stays fixed. e2e-prod.timeout-minutes is 35 (production-deploy-verify.yml:757), matching collect(3) + tests(20) + upload(2) = 25, with 10 minutes of headroom for checkout/npm ci/Chrome-report, well above the capped + 5 <= timeout-minutes floor the test now checks for.

Traced by hand and found sound on this head:

  • e2e-prod.outputs.tests is still steps.e2e.outcome, so a failed "Collect" step (a committed test.only) leaves e2e skipped, the rollback condition false, and production un-rolled-back — only a verdict from tests that actually ran trips it.
  • The "Collect" step's command and env are derived from, and asserted equal to, the test step's (npm run test:e2e:prod -w e2e -- --list, same CI/RADAR_API_KEY), and sits after the Chrome launch so a dead browser still fails before collection, not during it.
  • Both E2E jobs' screenshot uploads are if: failure(), continue-on-error: true, capped at 2 minutes, path e2e/test-results/**/*.png, if-no-files-found: ignore, named with ${{ github.run_attempt }} — matching the new test_a_failed_e2e_publishes_its_screenshots_alone / test_collecting_them_cannot_cost_the_verdict assertions.
  • _runs_playwright's regex (\btest:e2e\b|\bplaywright test\b) correctly matches test:e2e:prod -w e2e -- --list style commands (word boundary lands between e2e and the following :), and _test_result_uploads() resolves to exactly the two new upload steps — I checked the repo's only other upload-artifact use (ci.yml's coverage upload) and confirmed it neither runs Playwright nor matches the path regex, so the exact-set assertion in test_both_e2e_jobs_keep_their_screenshots holds.
  • npm run test:e2e:prod resolves to E2E_ENV=prod playwright test in e2e/package.json, so -- --list is a valid passthrough to Playwright's own list mode.
  • No node API route, node_config.py, or backend/pyproject.toml changes in this diff, so the contract-regen and uv.lock relock gates in CLAUDE.md don't apply here.

I could not execute pre-commit or the backend pytest suite in this sandbox (no backend/.venv, and shell invocations of python3/pytest require interactive approval unavailable here) — the above is from tracing the YAML and test assertions by hand against the current head, same limitation the prior review pass noted.

No new issues found. This PR already went through one review round (cap fix applied, confirmed above) and is sound as of 37e9e9d.

Branch ·

@jehanazad
jehanazad merged commit 50bf4c8 into main Oct 1, 2026
32 of 33 checks passed
@jehanazad
jehanazad deleted the ci/playwright-forbid-only-failure-artifacts branch October 1, 2026 01:03
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.

2 participants