fix(ci): preserve required e2e check for sharded runs (#9923)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - The pull request workflow is the main merge gate for changes to that app. > - The Playwright e2e lane is expensive because every spec shares one isolated server and runs serially. > - Splitting that lane across runners shortens the critical path, but the public required-check contract still needs a check named exactly `e2e`. > - This pull request shards the real e2e work while preserving a fast aggregate `e2e` job for branch protection. > - The benefit is a faster PR workflow without making otherwise-good PRs unmergeable because a legacy required check disappeared. ## Linked Issues or Issue Description No public GitHub issue exists for this CI follow-up. Related prior CI work: - Refs #8360 - Refs #9168 - Refs #9516 Bug report: ### What happened? Sharding the PR e2e lane directly at the workflow job level changes the emitted check names to shard-specific names, while existing branch protection expects a check named exactly `e2e`. ### Expected behavior The PR workflow should be able to run e2e specs across multiple runners while still emitting a stable aggregate check named `e2e`. ### Steps to reproduce 1. Open a PR against `master`. 2. Run the PR workflow with the e2e lane split only as a matrix job. 3. Observe that the shard checks complete, but a required check named exactly `e2e` never appears. ### Paperclip version or commit Current `master`. ### Deployment mode GitHub Actions pull request workflow. ## What Changed - Added `scripts/e2e-shard.mjs`, which partitions default Playwright e2e specs by recorded per-spec duration. - Added `scripts/e2e-shard-durations.json` with measured e2e spec durations so the slow smoke-lab spec does not dominate one runner. - Split the PR workflow e2e lane into two `e2e_shards` matrix jobs and added a fast aggregate job named exactly `e2e`. - Added `scripts/__tests__/e2e-shard.test.mjs` to lock the shard partition, ignored-spec sync, manifest coverage, and aggregate required-check contract. ## Verification - `node --test scripts/__tests__/e2e-shard.test.mjs` - `node --test scripts/__tests__/run-vitest-stable-shard.test.mjs` - `git diff --check upstream/master..HEAD` - Searched GitHub for duplicate or related e2e-shard / required-check PRs and issues before opening this PR; no direct duplicate was found. ## Risks Low risk. The main risk is that the duration manifest can drift as specs are added or runtimes change; missing specs fall back to the median known duration, and the focused shard test catches empty, overlapping, or badly imbalanced partitions. ## Model Used OpenAI GPT-5 via Codex CLI coding agent, with shell/tool execution and repository inspection. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge Co-authored-by: Paperclip <noreply@paperclip.ing>
This commit is contained in:
Devin Foley
GitHub
parent
b9f4073a13
commit
1944c86153
|
|
@ -58,6 +58,9 @@ jobs:
|
|||
- name: Test general-server shard partition
|
||||
run: node --test ./scripts/__tests__/run-vitest-stable-shard.test.mjs
|
||||
|
||||
- name: Test e2e shard partition
|
||||
run: node --test ./scripts/__tests__/e2e-shard.test.mjs
|
||||
|
||||
- name: Test release verify workflow wiring
|
||||
run: node --test ./scripts/__tests__/release-verify-workflow.test.mjs
|
||||
|
||||
|
|
@ -356,10 +359,25 @@ jobs:
|
|||
fi
|
||||
./scripts/release.sh canary --skip-verify --dry-run
|
||||
|
||||
e2e:
|
||||
e2e_shards:
|
||||
name: e2e shard (${{ matrix.shard_label }})
|
||||
needs: [policy]
|
||||
runs-on: ubuntu-latest
|
||||
timeout-minutes: 30
|
||||
strategy:
|
||||
fail-fast: false
|
||||
matrix:
|
||||
include:
|
||||
# The Playwright lane is pinned to workers=1 (tests/e2e/playwright.config.ts)
|
||||
# because every spec shares one throwaway server and some toggle
|
||||
# instance-level flags, so it can only be parallelized across runners.
|
||||
# Each shard boots its own server, which keeps that isolation intact.
|
||||
- shard_index: 0
|
||||
shard_count: 2
|
||||
shard_label: 1/2
|
||||
- shard_index: 1
|
||||
shard_count: 2
|
||||
shard_label: 2/2
|
||||
|
||||
steps:
|
||||
- name: Checkout repository
|
||||
|
|
@ -411,14 +429,36 @@ jobs:
|
|||
env:
|
||||
PAPERCLIP_E2E_SKIP_LLM: "true"
|
||||
PAPERCLIP_PLAYWRIGHT_CHANNEL: "chrome"
|
||||
run: pnpm run test:e2e
|
||||
run: |
|
||||
# Playwright's own --shard balances by test count, and one spec
|
||||
# (smoke-lab) is ~40% of the lane's wall clock. Partition by recorded
|
||||
# spec duration instead so both runners finish together.
|
||||
specs="$(node ./scripts/e2e-shard.mjs \
|
||||
--shard-index ${{ matrix.shard_index }} --shard-count ${{ matrix.shard_count }})"
|
||||
echo "shard ${{ matrix.shard_label }} specs: $specs"
|
||||
pnpm run test:e2e -- $specs
|
||||
|
||||
- name: Upload Playwright report
|
||||
uses: actions/upload-artifact@v7
|
||||
if: always()
|
||||
with:
|
||||
name: playwright-report
|
||||
name: playwright-report-${{ matrix.shard_index }}
|
||||
path: |
|
||||
tests/e2e/playwright-report/
|
||||
tests/e2e/test-results/
|
||||
retention-days: 14
|
||||
|
||||
e2e:
|
||||
# Preserve the legacy required-check name while the specs run sharded
|
||||
# across the matrix above (same pattern as the `verify` aggregate).
|
||||
name: e2e
|
||||
if: ${{ always() }}
|
||||
needs: [e2e_shards]
|
||||
runs-on: ubuntu-latest
|
||||
timeout-minutes: 5
|
||||
|
||||
steps:
|
||||
- name: Fail if any e2e shard failed
|
||||
env:
|
||||
E2E_SHARDS_RESULT: ${{ needs.e2e_shards.result }}
|
||||
run: test "$E2E_SHARDS_RESULT" = "success"
|
||||
|
|
|
|||
|
|
@ -0,0 +1,120 @@
|
|||
import assert from "node:assert/strict";
|
||||
import { spawnSync } from "node:child_process";
|
||||
import { readFileSync } from "node:fs";
|
||||
import path from "node:path";
|
||||
import { fileURLToPath } from "node:url";
|
||||
import test from "node:test";
|
||||
|
||||
import { loadShardDurations } from "../general-server-shard.mjs";
|
||||
import { IGNORED_SPECS, listE2eSpecs, selectE2eShard } from "../e2e-shard.mjs";
|
||||
|
||||
const repoRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), "..", "..");
|
||||
const script = path.join(repoRoot, "scripts", "e2e-shard.mjs");
|
||||
const durationsManifest = path.join(repoRoot, "scripts", "e2e-shard-durations.json");
|
||||
const playwrightConfig = path.join(repoRoot, "tests", "e2e", "playwright.config.ts");
|
||||
const prWorkflow = path.join(repoRoot, ".github", "workflows", "pr.yml");
|
||||
|
||||
const SHARD_COUNT = 2;
|
||||
|
||||
function runShard(args) {
|
||||
const result = spawnSync(process.execPath, [script, ...args], { cwd: repoRoot, encoding: "utf8" });
|
||||
assert.equal(result.status, 0, `expected success for ${args.join(" ")}: ${result.stderr}`);
|
||||
return result.stdout.trim().split(/\s+/).filter(Boolean);
|
||||
}
|
||||
|
||||
test("the e2e shards form a complete, non-overlapping partition", () => {
|
||||
const specs = listE2eSpecs();
|
||||
assert.ok(specs.length > 0, "expected a non-empty e2e spec set");
|
||||
|
||||
const shards = Array.from({ length: SHARD_COUNT }, (_, index) =>
|
||||
runShard(["--shard-index", String(index), "--shard-count", String(SHARD_COUNT)]),
|
||||
);
|
||||
|
||||
const combined = shards.flat();
|
||||
assert.equal(combined.length, specs.length, "every spec must land on exactly one shard");
|
||||
assert.deepEqual([...combined].sort(), [...specs].sort());
|
||||
for (const shard of shards) {
|
||||
assert.ok(shard.length > 0, "no shard may be empty — Playwright fails a run with no matching specs");
|
||||
}
|
||||
});
|
||||
|
||||
test("the ignored spec list matches playwright.config.ts testIgnore", () => {
|
||||
const config = readFileSync(playwrightConfig, "utf8");
|
||||
const match = config.match(/testIgnore:\s*\[([^\]]*)\]/);
|
||||
assert.ok(match, "expected a testIgnore array in playwright.config.ts");
|
||||
const configured = [...match[1].matchAll(/"([^"]+)"/g)].map((entry) => entry[1]);
|
||||
assert.deepEqual([...configured].sort(), [...IGNORED_SPECS].sort());
|
||||
});
|
||||
|
||||
test("the duration manifest only names specs that still exist", () => {
|
||||
const durations = loadShardDurations(durationsManifest);
|
||||
assert.ok(Object.keys(durations).length > 0, "expected a populated duration manifest");
|
||||
const specs = new Set(listE2eSpecs());
|
||||
for (const file of Object.keys(durations)) {
|
||||
assert.ok(specs.has(file), `duration manifest names a spec that no longer runs: ${file}`);
|
||||
}
|
||||
});
|
||||
|
||||
test("the weighted partition keeps the shards close to balanced", () => {
|
||||
const durations = loadShardDurations(durationsManifest);
|
||||
const specs = listE2eSpecs();
|
||||
const weights = Array.from({ length: SHARD_COUNT }, (_, index) =>
|
||||
selectE2eShard(specs, index, SHARD_COUNT, durations).reduce((sum, file) => sum + (durations[file] ?? 0), 0),
|
||||
);
|
||||
|
||||
const heaviest = Math.max(...weights);
|
||||
const total = weights.reduce((sum, weight) => sum + weight, 0);
|
||||
// Round-robin/count-based sharding would strand the ~168s smoke-lab spec on
|
||||
// one runner. Assert the weighted split stays within 15% of an even cut so a
|
||||
// future spec-time regression surfaces here instead of on the PR critical path.
|
||||
assert.ok(
|
||||
heaviest <= (total / SHARD_COUNT) * 1.15,
|
||||
`heaviest shard ${heaviest}ms exceeds 115% of the even cut (${total / SHARD_COUNT}ms)`,
|
||||
);
|
||||
});
|
||||
|
||||
test("shard arguments are validated", () => {
|
||||
for (const args of [
|
||||
["--shard-index", "2", "--shard-count", "2"],
|
||||
["--shard-index", "-1", "--shard-count", "2"],
|
||||
["--shard-index", "0", "--shard-count", "0"],
|
||||
]) {
|
||||
const result = spawnSync(process.execPath, [script, ...args], { cwd: repoRoot, encoding: "utf8" });
|
||||
assert.notEqual(result.status, 0, `expected failure for ${args.join(" ")}`);
|
||||
}
|
||||
});
|
||||
|
||||
test("pr.yml keeps a stable aggregate check named e2e over the shard matrix", () => {
|
||||
// Branch protection requires a check literally named `e2e`. The shards run
|
||||
// as `e2e shard (n/2)`, so the aggregate job below is what keeps the
|
||||
// required-check contract intact — same pattern as the `verify` aggregate.
|
||||
const workflow = readFileSync(prWorkflow, "utf8");
|
||||
const jobs = new Map();
|
||||
let current = null;
|
||||
for (const line of workflow.split("\n")) {
|
||||
const header = /^ {2}([A-Za-z0-9_-]+):\s*$/.exec(line);
|
||||
if (header) {
|
||||
current = header[1];
|
||||
jobs.set(current, []);
|
||||
continue;
|
||||
}
|
||||
if (current && /^\S/.test(line)) current = null;
|
||||
if (current) jobs.get(current).push(line);
|
||||
}
|
||||
for (const [id, lines] of jobs) jobs.set(id, lines.join("\n"));
|
||||
|
||||
const aggregate = jobs.get("e2e");
|
||||
assert.ok(aggregate, "pr.yml must define an `e2e` job to satisfy branch protection");
|
||||
assert.match(aggregate, /^ {4}name: e2e$/m, "the aggregate job must be named exactly `e2e`");
|
||||
assert.match(aggregate, /^ {4}if: \$\{\{ always\(\) \}\}$/m, "the aggregate must run even when a shard fails");
|
||||
assert.match(aggregate, /^ {4}needs: \[e2e_shards\]$/m, "the aggregate must depend on the shard matrix");
|
||||
assert.match(
|
||||
aggregate,
|
||||
/test "\$E2E_SHARDS_RESULT" = "success"/,
|
||||
"the aggregate must fail unless every shard succeeded",
|
||||
);
|
||||
|
||||
const shards = jobs.get("e2e_shards");
|
||||
assert.ok(shards, "pr.yml must define the `e2e_shards` matrix job");
|
||||
assert.match(shards, /shard_count: 2/, "the shard matrix must match SHARD_COUNT");
|
||||
});
|
||||
|
|
@ -0,0 +1,20 @@
|
|||
{
|
||||
"$comment": "Per-spec wall-clock durations (ms) for the Playwright e2e lane, used by scripts/e2e-shard.mjs to balance specs across the PR shard matrix. Averaged from two real PR runs of .github/workflows/pr.yml (actions runs 29775184389 and 29769465682, 2026-07-20). Specs missing here get the median weight, so the manifest only needs occasional refreshes: pull the 'Run e2e tests' logs from a recent PR run and sum the Playwright list-reporter durations per spec file.",
|
||||
"unit": "ms",
|
||||
"durations": {
|
||||
"tests/e2e/app-not-connected.spec.ts": 26750,
|
||||
"tests/e2e/application-delete-screenshot.spec.ts": 6750,
|
||||
"tests/e2e/applications-crud.spec.ts": 16100,
|
||||
"tests/e2e/apps-dark-mode-shots.spec.ts": 18000,
|
||||
"tests/e2e/apps-prosumer-mcp-flow.spec.ts": 16800,
|
||||
"tests/e2e/conference-room-typing-intro.spec.ts": 4900,
|
||||
"tests/e2e/mcp-user-stories.spec.ts": 39000,
|
||||
"tests/e2e/nux-phase4-screenshots.spec.ts": 19650,
|
||||
"tests/e2e/onboarding.spec.ts": 3200,
|
||||
"tests/e2e/pipelines-tutorial-flow.spec.ts": 30198,
|
||||
"tests/e2e/planning-mode-visual-verification.spec.ts": 23250,
|
||||
"tests/e2e/sidebar-takeover.spec.ts": 16650,
|
||||
"tests/e2e/signoff-policy.spec.ts": 9696,
|
||||
"tests/e2e/smoke-lab.spec.ts": 168000
|
||||
}
|
||||
}
|
||||
|
|
@ -0,0 +1,59 @@
|
|||
import { readdirSync } from "node:fs";
|
||||
import path from "node:path";
|
||||
import { fileURLToPath } from "node:url";
|
||||
|
||||
import { loadShardDurations, partitionGeneralServerSuites } from "./general-server-shard.mjs";
|
||||
|
||||
const HERE = path.dirname(fileURLToPath(import.meta.url));
|
||||
const REPO_ROOT = path.resolve(HERE, "..");
|
||||
const E2E_DIR = path.join(REPO_ROOT, "tests", "e2e");
|
||||
const DURATIONS_MANIFEST = path.join(HERE, "e2e-shard-durations.json");
|
||||
|
||||
// Specs the default local_trusted Playwright project deliberately skips. Keep
|
||||
// this in sync with `testIgnore` in tests/e2e/playwright.config.ts — the unit
|
||||
// test in scripts/__tests__/e2e-shard.test.mjs fails if the two ever drift.
|
||||
export const IGNORED_SPECS = ["multi-user.spec.ts", "multi-user-authenticated.spec.ts"];
|
||||
|
||||
// Enumerates the specs the default e2e lane actually runs, as repo-relative
|
||||
// paths so the output can be handed straight to `playwright test`.
|
||||
export function listE2eSpecs(e2eDir = E2E_DIR, repoRoot = REPO_ROOT) {
|
||||
return readdirSync(e2eDir)
|
||||
.filter((entry) => entry.endsWith(".spec.ts") && !IGNORED_SPECS.includes(entry))
|
||||
.map((entry) => path.relative(repoRoot, path.join(e2eDir, entry)).split(path.sep).join("/"))
|
||||
.sort((a, b) => a.localeCompare(b));
|
||||
}
|
||||
|
||||
// Playwright's own --shard balances by test count, which is useless here: one
|
||||
// spec (smoke-lab) is ~40% of the lane's wall clock. Reuse the deterministic
|
||||
// longest-processing-time partition already proven on the general-server lane
|
||||
// so every runner computes the identical, non-overlapping split.
|
||||
export function selectE2eShard(files, shardIndex, shardCount, durations = {}) {
|
||||
return partitionGeneralServerSuites(files, shardCount, durations)[shardIndex].files;
|
||||
}
|
||||
|
||||
function parseArgs(argv) {
|
||||
const args = { shardIndex: 0, shardCount: 1 };
|
||||
for (let index = 0; index < argv.length; index += 1) {
|
||||
if (argv[index] === "--shard-index") args.shardIndex = Number(argv[index + 1]);
|
||||
if (argv[index] === "--shard-count") args.shardCount = Number(argv[index + 1]);
|
||||
}
|
||||
return args;
|
||||
}
|
||||
|
||||
function main(argv) {
|
||||
const { shardIndex, shardCount } = parseArgs(argv);
|
||||
if (!Number.isInteger(shardCount) || shardCount < 1) {
|
||||
throw new Error(`--shard-count must be a positive integer, got ${shardCount}`);
|
||||
}
|
||||
if (!Number.isInteger(shardIndex) || shardIndex < 0 || shardIndex >= shardCount) {
|
||||
throw new Error(`--shard-index must be in [0, ${shardCount}), got ${shardIndex}`);
|
||||
}
|
||||
|
||||
const specs = listE2eSpecs();
|
||||
const durations = loadShardDurations(DURATIONS_MANIFEST);
|
||||
process.stdout.write(`${selectE2eShard(specs, shardIndex, shardCount, durations).join(" ")}\n`);
|
||||
}
|
||||
|
||||
if (process.argv[1] && path.resolve(process.argv[1]) === fileURLToPath(import.meta.url)) {
|
||||
main(process.argv.slice(2));
|
||||
}
|
||||
Loading…
Reference in New Issue