Skip to content

Prevent storage E2E retries from canceling CI - #4249

Merged
brandonpayton merged 7 commits into
trunkfrom
emdash/e2e-fix-flaky-canceled-tests-l5yrd
Aug 6, 2026
Merged

Prevent storage E2E retries from canceling CI#4249
brandonpayton merged 7 commits into
trunkfrom
emdash/e2e-fix-flaky-canceled-tests-l5yrd

Conversation

@brandonpayton

@brandonpayton brandonpayton commented Aug 5, 2026

Copy link
Copy Markdown
Member

Why this change is needed

@adamziel reported that flaky end-to-end tests were causing more CI jobs to be
canceled.

The canceled jobs shared the same pattern: one storage test failed late in a
Playwright serial group, so Playwright retried the whole group from its first
test. In one case, a 33-test storage group restarted more than once and the job
reached the existing 30-minute limit before it could finish. The OPFS tests had
the same retry behavior.

Adding another ordinary shard would not help because Playwright keeps a serial
group together. Increasing the timeout or reducing the three retries would
also leave the whole-group replay in place.

Examples:

Why this approach

The storage tests must not run at the same time because they share browser
storage. A dedicated storage lane with one worker preserves that isolation.
Using Playwright's default test mode inside that lane means a failure retries
only the failed test instead of replaying every earlier test in its group.

This directly addresses the cancellation pattern while keeping the existing
30-minute job timeout and three retries.

What this changes

  • Changes the OPFS tests and default-storage tests from serial mode to default
    mode, allowing Playwright to retry one failed test.
  • Adds @storage to the top-level OPFS suite and the default-storage group.
    CI routes tests only by this tag, so it does not need file-path rules.
  • Keeps three ordinary shards per browser and excludes storage tests from them.
  • Adds one unsharded storage lane per browser with workers: 1.
  • Runs the Nx CI-style target as two sequential groups: ordinary tests use the
    normal three workers, then storage tests use one worker. Missing or unsupported
    group values produce a clear error.
  • Gives only the three storage matrix entries a storage test group. Entries
    without a group use the ordinary-test behavior, so the matrix does not repeat
    a regular label nine times.
  • Adds a -storage suffix only to storage artifacts. Ordinary artifact names
    remain unchanged.

The tradeoff is three additional install/build runners, one per browser. This
is the same job-count increase as changing from three to four ordinary shards,
but it removes the whole-group retry amplification that caused the cancellations.

Verification

  • Collected all Chromium tests with the tag-only selector and confirmed that
    each test is in exactly one lane: 182 ordinary tests and 59 storage tests.
    The ordinary shards contain 61, 61, and 60 tests.
  • Confirmed the top-level OPFS suite contributes all 26 tagged tests.
  • Confirmed the Nx target resolves to two sequential commands with explicit
    groups, and a missing or unsupported group exits with a clear error.
  • Ran all 59 Chromium storage tests locally with one worker; all passed.
  • The full CI run passed for Chromium, Firefox, and WebKit storage lanes.
  • Ran the Chromium storage lane two more times after it first passed. All three
    attempts passed. Two attempts had one flaky ZIP-import test, and Playwright
    retried only that test instead of restarting the storage suite. This confirms
    the new retry scope works as intended.
  • Confirmed the simplified matrix has 12 entries: 9 without a group and 3 with
    the storage group.
  • Ran the website typecheck, lint, formatting checks, YAML parsing, and workflow
    validation.

The repetitions also exposed a separate race in two ZIP-import tests. PR #4213
addresses that underlying race. It remains separate so this PR stays focused on
making retries safe and preventing CI cancellations.

@brandonpayton brandonpayton changed the title Run storage E2E tests in a one-worker CI lane Prevent storage E2E retries from canceling CI Aug 6, 2026
Comment thread packages/playground/website/playwright/playwright.ci.config.ts Outdated
Comment thread packages/playground/website/playwright/e2e/website-ui.spec.ts Outdated
@brandonpayton
brandonpayton marked this pull request as ready for review August 6, 2026 04:39
Copilot AI lite review requested due to automatic review settings August 6, 2026 04:39

Copilot AI 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.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adjusts Playwright CI sharding so storage-sensitive E2E tests run in a dedicated single-worker lane, allowing retries to re-run only failed tests (instead of replaying full serial groups) and reducing CI cancellations caused by late failures.

Changes:

  • Adds CI test-group routing (regular vs storage) via PLAYWRIGHT_TEST_GROUP, with workers: 1 for the storage lane.
  • Marks default-storage tests with @storage and switches storage suites from serial to Playwright’s default mode.
  • Updates the GitHub Actions matrix to add one storage lane per browser and to suffix storage artifacts.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
packages/playground/website/playwright/playwright.ci.config.ts Adds PLAYWRIGHT_TEST_GROUP-based grep/grepInvert and workers: 1 to separate storage vs regular runs.
packages/playground/website/playwright/e2e/website-ui.spec.ts Tags the default-storage suite as @storage and switches it off serial.
packages/playground/website/playwright/e2e/opfs.spec.ts Switches OPFS tests from serial to default mode and updates comments.
.github/workflows/ci.yml Adds per-browser storage matrix entries, passes PLAYWRIGHT_TEST_GROUP, and updates step/artifact naming.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread packages/playground/website/playwright/playwright.ci.config.ts Outdated
Comment thread packages/playground/website/playwright/playwright.ci.config.ts Outdated
Comment thread packages/playground/website/playwright/e2e/opfs.spec.ts Outdated
Comment thread packages/playground/website/playwright/e2e/website-ui.spec.ts Outdated
Comment thread packages/playground/website/playwright/playwright.ci.config.ts Outdated
@github-actions github-actions Bot added the [Type] Documentation Improvements or additions to documentation label Aug 6, 2026
Comment thread packages/playground/website/playwright/playwright.ci.config.ts
Comment thread packages/playground/website/playwright/e2e/opfs.spec.ts Outdated
Comment thread AGENTS.md Outdated
Comment thread packages/playground/website/playwright/e2e/opfs.spec.ts Outdated
// OPFS is browser-scoped, so `@storage` routes this suite to the one-worker CI lane.
test.describe('OPFS', { tag: '@storage' }, () => {
// Default mode retries only the failed test instead of replaying the suite.
test.describe.configure({ mode: 'default' });

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think all the changes in the file from here down are the result of whitespace-only changes (indentation) that led to formatting changes.

@brandonpayton

Copy link
Copy Markdown
Member Author

This looks good to go. Let's merge.

@brandonpayton
brandonpayton merged commit 9153879 into trunk Aug 6, 2026
57 checks passed
@brandonpayton
brandonpayton deleted the emdash/e2e-fix-flaky-canceled-tests-l5yrd branch August 6, 2026 23:34
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