mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-07 06:16:06 -10:00
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - Contributors wait for PR CI before merge, so CI wall-clock time sets the pace of work > - PR CI runs Vitest shards with 4 workers on 4-vCPU runners, so timing-sensitive tests sometimes time out under load > - One such timeout fails the whole run, and a re-run costs another 15 minutes or more > - In one 7-hour window on 2026-11-06, about 30 failed jobs were test flakes on commits that passed on a later attempt > - This pull request retries a failed Vitest case once in PR CI only, and reports every pass that needed the retry > - The benefit is fewer whole-run re-runs, while each flaky test stays visible so that we can fix it ## Linked Issues or Issue Description **What existing behavior does this improve?** PR CI fails the whole run when one Vitest case fails once because of runner load. **Subsystem affected** CI: the PR Vitest lanes (general, serialized, workspace and Runner package lanes). **Current behavior** Vitest uses `retry: 0` everywhere. One timeout under load fails the run. A person must notice it and re-run the failed jobs. About 20% of recently merged PRs needed at least one re-run. **Proposed behavior** PR CI retries a failed Vitest case one time. A test that passes only on the retry gets a GitHub warning annotation and a row in the job summary. Local runs, release, cloud readiness and master keep `retry: 0`. **Reason and benefit** A re-run of one test case costs seconds. A re-run of the CI run costs 15 minutes or more. The report keeps flaky tests visible. Separate pull requests fix the known flaky tests. **Breaking changes** None. ## What Changed - Add `scripts/ci/vitest-retry.mjs`. It sets 1 retry when `GITHUB_ACTIONS=true` and `GITHUB_WORKFLOW=PR`, and 0 in other contexts. `PAPERCLIP_VITEST_RETRY` overrides the value. - Add `scripts/ci/retry-reporter.mjs`. It writes a `::warning` annotation and a step-summary table row for each test that passed only on a retry. - `scripts/run-vitest-stable.mjs` passes `--retry` to `vitest run`. The `vitest list` collection step does not retry. - The root `vitest.config.ts` and `packages/paperclip-runner/vitest.config.ts` add the retry reporter beside the existing reporters. The Runner config sets `retry` from the same gate. - `.github/workflows/pr-trusted.yml` reads the `CI_VITEST_RETRY` repository variable. Set it to `0` to turn retries off without a code change. - Add `scripts/ci/retry-reporter.test.mjs` to `test:release-registry`, and document the behavior in `doc/DEVELOPING.md`. ## Verification - `node --test scripts/ci/retry-reporter.test.mjs`: 11/11 pass. The tests cover the gate matrix, the reporter output, a real Vitest run with a test that fails once, the wrapper arguments, and the Runner config. - `node --test scripts/__tests__/run-vitest-stable-shard.test.mjs`: 39/39 pass. - `node --test .github/scripts/tests/*.test.mjs`: 744/744 pass. - Manual check: a temporary test that fails once ran through `run-vitest-stable.mjs` and through the Runner package config. With the PR environment it passed and wrote the annotation and the summary row. With `GITHUB_WORKFLOW=Release` it failed. The temporary test is removed. ## Risks - A retry can hide a flaky test. The annotation and the summary row reduce this risk. - A test that fails for a real reason now runs twice in PR CI, so a failed run takes slightly longer. - `pr.yml` loads `pr-trusted.yml` from master, so the `CI_VITEST_RETRY` switch works only after merge. The retry itself is in the scripts and starts with this pull request. - Low risk otherwise: release and master verification are not changed. ## Model Used - Claude Opus 5.5 (`claude-opus-5-5`) in Claude Code, with tool use and code execution. A Claude sub-agent wrote the change, and the parent session reviewed it. ## 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 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5.5 <[email protected]>
195 lines
11 KiB
JavaScript
195 lines
11 KiB
JavaScript
import assert from "node:assert/strict";
|
|
import { spawnSync } from "node:child_process";
|
|
import { chmodSync, mkdirSync, mkdtempSync, readFileSync, realpathSync, rmSync, symlinkSync, writeFileSync } from "node:fs";
|
|
import { tmpdir } from "node:os";
|
|
import path from "node:path";
|
|
import { fileURLToPath } from "node:url";
|
|
import test from "node:test";
|
|
import RetryReporter, { annotation, flakyRecord, summaryMarkdown } from "./retry-reporter.mjs";
|
|
import { retryReporterEnabled, vitestRetryArgs, vitestRetryCount } from "./vitest-retry.mjs";
|
|
|
|
const root = fileURLToPath(new URL("../../", import.meta.url));
|
|
const reporterPath = fileURLToPath(new URL("./retry-reporter.mjs", import.meta.url));
|
|
const pr = { GITHUB_ACTIONS: "true", GITHUB_WORKFLOW: "PR" };
|
|
|
|
function testCase({ state = "passed", retryCount = 0, file = path.join(root, "server/src/a.test.ts"), name = "suite > case", line } = {}) {
|
|
return {
|
|
fullName: name,
|
|
module: { moduleId: file },
|
|
project: { name: "@paperclipai/server" },
|
|
location: line === undefined ? undefined : { line, column: 3 },
|
|
result: () => ({ state }),
|
|
diagnostic: () => ({ retryCount }),
|
|
};
|
|
}
|
|
|
|
test("retry defaults to one only in the PR workflow on GitHub Actions", () => {
|
|
assert.equal(vitestRetryCount({}), 0);
|
|
assert.equal(vitestRetryCount(pr), 1);
|
|
for (const workflow of ["Release", "Release Verify", "Cloud readiness", "Release Smoke", "Trusted PR CI", "Incremental CI qualification"]) {
|
|
assert.equal(vitestRetryCount({ GITHUB_ACTIONS: "true", GITHUB_WORKFLOW: workflow }), 0, workflow);
|
|
}
|
|
assert.equal(vitestRetryCount({ GITHUB_WORKFLOW: "PR" }), 0, "a local shell that happens to set the name stays at 0");
|
|
assert.deepEqual(vitestRetryArgs({}), []);
|
|
assert.deepEqual(vitestRetryArgs(pr), ["--retry=1"]);
|
|
});
|
|
|
|
test("PAPERCLIP_VITEST_RETRY overrides the default and rejects invalid values", () => {
|
|
assert.equal(vitestRetryCount({ ...pr, PAPERCLIP_VITEST_RETRY: "0" }), 0);
|
|
assert.deepEqual(vitestRetryArgs({ ...pr, PAPERCLIP_VITEST_RETRY: "0" }), []);
|
|
assert.equal(vitestRetryCount({ ...pr, PAPERCLIP_VITEST_RETRY: "" }), 1, "an unset repository variable keeps the PR default");
|
|
assert.equal(vitestRetryCount({ ...pr, PAPERCLIP_VITEST_RETRY: " " }), 1);
|
|
assert.equal(vitestRetryCount({ PAPERCLIP_VITEST_RETRY: "2" }), 2);
|
|
for (const value of ["-1", "1.5", "yes"]) {
|
|
assert.throws(() => vitestRetryCount({ PAPERCLIP_VITEST_RETRY: value }), /non-negative integer/, value);
|
|
}
|
|
});
|
|
|
|
test("the reporter loads in GitHub Actions jobs and wherever retry is on", () => {
|
|
assert.equal(retryReporterEnabled({}), false);
|
|
assert.equal(retryReporterEnabled({ GITHUB_ACTIONS: "true", GITHUB_WORKFLOW: "Release" }), true);
|
|
assert.equal(retryReporterEnabled({ PAPERCLIP_VITEST_RETRY: "1" }), true);
|
|
});
|
|
|
|
test("only passes that needed a retry are flaky records", () => {
|
|
assert.equal(flakyRecord(testCase()), null);
|
|
assert.equal(flakyRecord(testCase({ state: "failed", retryCount: 1 })), null);
|
|
assert.equal(flakyRecord(testCase({ state: "skipped", retryCount: 1 })), null);
|
|
assert.deepEqual(flakyRecord(testCase({ retryCount: 1, line: 12 })), {
|
|
file: "server/src/a.test.ts", line: 12, name: "suite > case", project: "@paperclipai/server", retries: 1,
|
|
});
|
|
});
|
|
|
|
test("annotations escape workflow-command separators", () => {
|
|
const record = flakyRecord(testCase({ retryCount: 2, file: path.join(root, "ui/src/a,b:c.test.ts"), name: "50% > line\nbreak" }));
|
|
assert.equal(record.line, null);
|
|
assert.equal(annotation(record),
|
|
"::warning file=ui/src/a%2Cb%3Ac.test.ts,title=Flaky test passed on retry::50%25 > line%0Abreak (retried 2)");
|
|
assert.equal(annotation({ ...record, line: 7 }),
|
|
"::warning file=ui/src/a%2Cb%3Ac.test.ts,line=7,title=Flaky test passed on retry::50%25 > line%0Abreak (retried 2)");
|
|
});
|
|
|
|
test("the step summary is a Markdown table with the job name", () => {
|
|
assert.equal(summaryMarkdown([]), "");
|
|
const markdown = summaryMarkdown([{ file: "server/src/a.test.ts", name: "a | b\nc", retries: 1 }], "General tests (server 1/4)");
|
|
assert.equal(markdown, [
|
|
"",
|
|
"### Flaky tests passed on retry",
|
|
"",
|
|
"| File | Test | Retries | Job |",
|
|
"| --- | --- | --- | --- |",
|
|
"| `server/src/a.test.ts` | a \\| b c | 1 | General tests (server 1/4) |",
|
|
"", ""].join("\n"));
|
|
assert.match(summaryMarkdown([{ file: "f", name: "n", retries: 1 }]), /\| 1 \| - \|$/m);
|
|
});
|
|
|
|
test("the reporter writes nothing for a clean run and both outputs for a flaky one", () => {
|
|
const directory = mkdtempSync(path.join(tmpdir(), "retry-reporter-"));
|
|
try {
|
|
const summary = path.join(directory, "summary.md");
|
|
let output = "";
|
|
const reporter = new RetryReporter({ env: { GITHUB_STEP_SUMMARY: summary, GITHUB_JOB: "general" }, root, write: (text) => { output += text; } });
|
|
reporter.onTestRunStart();
|
|
reporter.onTestCaseResult(testCase());
|
|
reporter.onTestRunEnd();
|
|
assert.equal(output, "");
|
|
assert.throws(() => readFileSync(summary), { code: "ENOENT" });
|
|
|
|
reporter.onTestRunStart();
|
|
reporter.onTestCaseResult(testCase({ retryCount: 1 }));
|
|
reporter.onTestCaseResult(testCase({ state: "failed", retryCount: 1, name: "still failing" }));
|
|
reporter.onTestRunEnd();
|
|
assert.equal(output, "::warning file=server/src/a.test.ts,title=Flaky test passed on retry::suite > case (retried 1)\n");
|
|
assert.match(readFileSync(summary, "utf8"), /^\| `server\/src\/a\.test\.ts` \| suite > case \| 1 \| general \|$/m);
|
|
} finally { rmSync(directory, { recursive: true, force: true }); }
|
|
});
|
|
|
|
function runFlakyFixture(env) {
|
|
const directory = realpathSync(mkdtempSync(path.join(tmpdir(), "retry-reporter-vitest-")));
|
|
symlinkSync(path.join(root, "node_modules"), path.join(directory, "node_modules"), "dir");
|
|
// The counter lives on disk because a retry reruns the case in the same worker.
|
|
writeFileSync(path.join(directory, "flaky.test.mjs"), `import { existsSync, writeFileSync } from "node:fs";
|
|
import { test } from "vitest";
|
|
test("fails on its first attempt", () => {
|
|
const marker = new URL("./attempted", import.meta.url);
|
|
if (!existsSync(marker)) { writeFileSync(marker, ""); throw new Error("first attempt fails"); }
|
|
});
|
|
test("always passes", () => {});
|
|
`);
|
|
const configFile = path.join(directory, "vitest.config.mjs");
|
|
writeFileSync(configFile, `export default ${JSON.stringify({ test: { include: ["*.test.mjs"], reporters: ["default", reporterPath] } })};`);
|
|
const summary = path.join(directory, "summary.md");
|
|
const childEnv = { ...process.env, ...env, GITHUB_STEP_SUMMARY: summary, GITHUB_WORKSPACE: directory, PAPERCLIP_CI_PROFILE_JOB: "probe" };
|
|
delete childEnv.NODE_TEST_CONTEXT;
|
|
const result = spawnSync(process.execPath, [path.join(root, "node_modules/vitest/vitest.mjs"), "run",
|
|
"--config", configFile, "--root", directory, ...vitestRetryArgs(childEnv)], {
|
|
cwd: directory, encoding: "utf8", timeout: 60_000, env: childEnv,
|
|
});
|
|
let summaryText = "";
|
|
try { summaryText = readFileSync(summary, "utf8"); } catch {}
|
|
rmSync(directory, { recursive: true, force: true });
|
|
return { result, summaryText };
|
|
}
|
|
|
|
test("a real PR-gated Vitest run retries the flake once and reports it", () => {
|
|
const { result, summaryText } = runFlakyFixture({ ...pr, PAPERCLIP_VITEST_RETRY: "" });
|
|
assert.equal(result.status, 0, result.stdout + result.stderr);
|
|
assert.match(result.stdout, /^::warning file=flaky\.test\.mjs,title=Flaky test passed on retry::fails on its first attempt \(retried 1\)$/m);
|
|
assert.doesNotMatch(result.stdout, /always passes \(retried/);
|
|
assert.match(summaryText, /^\| `flaky\.test\.mjs` \| fails on its first attempt \| 1 \| probe \|$/m);
|
|
});
|
|
|
|
test("outside the PR workflow the same flake fails without a retry", () => {
|
|
const { result, summaryText } = runFlakyFixture({ GITHUB_ACTIONS: "true", GITHUB_WORKFLOW: "Release", PAPERCLIP_VITEST_RETRY: "" });
|
|
assert.equal(result.status, 1, result.stdout + result.stderr);
|
|
assert.doesNotMatch(result.stdout, /Flaky test passed on retry/);
|
|
assert.doesNotMatch(summaryText, /Flaky tests passed on retry/);
|
|
});
|
|
|
|
test("run-vitest-stable passes --retry to vitest run only in the PR workflow", () => {
|
|
const directory = mkdtempSync(path.join(tmpdir(), "retry-wrapper-"));
|
|
try {
|
|
const bin = path.join(directory, "bin");
|
|
mkdirSync(bin);
|
|
writeFileSync(path.join(bin, "pnpm"), '#!/bin/sh\nprintf "%s\\n" "$@" >> "$RETRY_ARGS_FILE"\n');
|
|
chmodSync(path.join(bin, "pnpm"), 0o755);
|
|
const invoke = (env) => {
|
|
const argsFile = path.join(directory, `args-${Math.random()}`);
|
|
const childEnv = { ...process.env, ...env, PATH: `${bin}${path.delimiter}${process.env.PATH}`, RETRY_ARGS_FILE: argsFile };
|
|
for (const key of Object.keys(childEnv)) {
|
|
if (key === "PAPERCLIP_CI_TEST_SELECTION" || key === "PAPERCLIP_CI_PROFILE" || /^PAPERCLIP_.*_TEST_DATABASE_URL$/.test(key)) delete childEnv[key];
|
|
}
|
|
const result = spawnSync(process.execPath, [path.join(root, "scripts/run-vitest-stable.mjs"),
|
|
"--mode", "general", "--group", "general-workspaces-c"], { cwd: root, encoding: "utf8", env: childEnv, timeout: 60_000 });
|
|
assert.equal(result.status, 0, result.stdout + result.stderr);
|
|
return readFileSync(argsFile, "utf8").trim().split("\n");
|
|
};
|
|
assert.deepEqual(invoke({ ...pr, PAPERCLIP_VITEST_RETRY: "" }),
|
|
["exec", "vitest", "run", "--exclude", "**/dist/**", "--retry=1", "--project", "@paperclipai/db"]);
|
|
assert.deepEqual(invoke({ ...pr, PAPERCLIP_VITEST_RETRY: "0" }),
|
|
["exec", "vitest", "run", "--exclude", "**/dist/**", "--project", "@paperclipai/db"]);
|
|
assert.deepEqual(invoke({ GITHUB_ACTIONS: "true", GITHUB_WORKFLOW: "Cloud readiness", PAPERCLIP_VITEST_RETRY: "" }),
|
|
["exec", "vitest", "run", "--exclude", "**/dist/**", "--project", "@paperclipai/db"]);
|
|
} finally { rmSync(directory, { recursive: true, force: true }); }
|
|
});
|
|
|
|
test("the Runner package config retries only in the PR workflow", () => {
|
|
const base = { ...process.env };
|
|
for (const key of ["GITHUB_ACTIONS", "GITHUB_WORKFLOW", "PAPERCLIP_VITEST_RETRY", "PAPERCLIP_CI_PROFILE"]) delete base[key];
|
|
const load = (env) => {
|
|
const childEnv = { ...base, ...env };
|
|
const result = spawnSync(process.execPath, ["--input-type=module", "-e",
|
|
"const { default: config } = await import('./packages/paperclip-runner/vitest.config.ts');"
|
|
+ "process.stdout.write(JSON.stringify({ retry: config.test.retry ?? 0, reporters: config.test.reporters ?? [] }));"],
|
|
{ cwd: root, encoding: "utf8", env: childEnv, timeout: 60_000 });
|
|
assert.equal(result.status, 0, result.stderr);
|
|
return JSON.parse(result.stdout);
|
|
};
|
|
const prConfig = load({ ...pr, PAPERCLIP_VITEST_RETRY: "" });
|
|
assert.equal(prConfig.retry, 1);
|
|
assert.ok(prConfig.reporters.includes(reporterPath), JSON.stringify(prConfig.reporters));
|
|
assert.equal(load({ GITHUB_ACTIONS: "true", GITHUB_WORKFLOW: "Release", PAPERCLIP_VITEST_RETRY: "" }).retry, 0);
|
|
assert.equal(load({ ...pr, PAPERCLIP_VITEST_RETRY: "0" }).retry, 0);
|
|
assert.deepEqual(load({}), { retry: 0, reporters: [] }, "local runs keep the unmodified config");
|
|
});
|