From dd0e0ac59d0bfe5fa09ee3c888653eccbf773672 Mon Sep 17 00:00:00 2001 From: Devin Foley Date: Thu, 10 Sep 2026 19:41:24 -0700 Subject: [PATCH] fix(ci): split release chat verification into test shards Co-Authored-By: Paperclip --- .github/workflows/release-verify.yml | 47 ++++++++------- doc/RELEASE-AUTOMATION-SETUP.md | 25 ++++++++ .../release-verify-workflow.test.mjs | 24 ++------ .../run-vitest-stable-shard.test.mjs | 52 +++++++++++++++++ scripts/run-vitest-stable.mjs | 57 +++++++++++++++---- scripts/test-line-shard.mjs | 45 +++++++++++++++ 6 files changed, 200 insertions(+), 50 deletions(-) create mode 100644 scripts/test-line-shard.mjs diff --git a/.github/workflows/release-verify.yml b/.github/workflows/release-verify.yml index 438649d30e..670aac3896 100644 --- a/.github/workflows/release-verify.yml +++ b/.github/workflows/release-verify.yml @@ -58,30 +58,37 @@ jobs: fail-fast: false matrix: include: - # Five-way server split, matching pr-trusted.yml. Three shards sat - # at 17-19 minutes against this job's 20-minute cap after the - # server suite grew on 2026-09-10, and every canary that evening - # died on the timeout instead of reporting a verdict. - - group: general-server - group_label: server (1/5) + # Split the long chat file by collected test locations, and balance + # the remaining server files across four runners. Normal PR/local + # invocations retain their complete general-server group. + - group: general-server-without-chat + group_label: server (1/4) shard_index: 0 - shard_count: 5 - - group: general-server - group_label: server (2/5) + shard_count: 4 + - group: general-server-without-chat + group_label: server (2/4) shard_index: 1 - shard_count: 5 - - group: general-server - group_label: server (3/5) + shard_count: 4 + - group: general-server-without-chat + group_label: server (3/4) shard_index: 2 - shard_count: 5 - - group: general-server - group_label: server (4/5) + shard_count: 4 + - group: general-server-without-chat + group_label: server (4/4) shard_index: 3 - shard_count: 5 - - group: general-server - group_label: server (5/5) - shard_index: 4 - shard_count: 5 + shard_count: 4 + - group: general-chat + group_label: chat (1/3) + shard_index: 0 + shard_count: 3 + - group: general-chat + group_label: chat (2/3) + shard_index: 1 + shard_count: 3 + - group: general-chat + group_label: chat (3/3) + shard_index: 2 + shard_count: 3 # Keep parity with pr.yml: workspaces-a is split with Vitest's # native --shard because the ui project dominates the lane. - group: general-workspaces-a diff --git a/doc/RELEASE-AUTOMATION-SETUP.md b/doc/RELEASE-AUTOMATION-SETUP.md index 7896b6cf98..dcafc9bd67 100644 --- a/doc/RELEASE-AUTOMATION-SETUP.md +++ b/doc/RELEASE-AUTOMATION-SETUP.md @@ -359,3 +359,28 @@ default branch) are master here. A workflow with authority to execute arbitrary code on master can affect verification directly and is already trusted. The cache contains dependency build artifacts, not credentials or workspace output. See [GitHub cache access restrictions](https://docs.github.com/en/actions/reference/workflows-and-actions/dependency-caching#restrictions-for-accessing-a-cache). + +## Chat integration test shards + +Release verification runs the large chat integration file on three independent +runners. Four other server shards cover every remaining general server file. +The ordinary local test command and trusted PR workflow keep their complete +`general-server` group. No application test assertions or fixtures change. + +Each chat job collects active tests with Vitest, groups cases by source line, +and balances those groups by case count. Parameterized cases and loop-generated +cases on one line stay together. The job re-collects with the exact line filters +it will execute and fails if the selected case identities differ. Hooks and test +execution remain sequential inside each runner with its own temporary home. + +Run one shard locally with: + +```sh +pnpm test:run:general -- --group general-chat --shard-index 0 --shard-count 3 +``` + +Use indexes 0, 1, and 2 to run the complete chat suite. The CLI validates that +each shard has work and that collection includes usable source locations. A +Vitest collection or filtering change fails verification instead of dropping +tests. Splitting adds two release-verification jobs and repeats collection and +fixture setup; it does not make a single test faster. diff --git a/scripts/__tests__/release-verify-workflow.test.mjs b/scripts/__tests__/release-verify-workflow.test.mjs index cc725f15ab..a1e9207c04 100644 --- a/scripts/__tests__/release-verify-workflow.test.mjs +++ b/scripts/__tests__/release-verify-workflow.test.mjs @@ -206,28 +206,16 @@ test("release verify workflow covers the same split test surface as stable PR ve assert.match(buildJob, /persist-credentials: false/); assert.doesNotMatch(buildJob, /cache: pnpm/); - for (const group of [ - "general-server", - "general-workspaces-a", - "general-workspaces-b", - ]) { + for (const group of ["general-server-without-chat", "general-chat", "general-workspaces-a", "general-workspaces-b"]) { assert.match(verifyWorkflow, new RegExp(`group: ${group}`)); } - - for (const shardIndex of [0, 1, 2, 3, 4]) { - assert.match( - verifyWorkflow, - new RegExp( - `group: general-server[\\s\\S]*?shard_index: ${shardIndex}[\\s\\S]*?shard_count: 5`, - ), - ); + for (const [group, count] of [["general-server-without-chat", 4], ["general-chat", 3]]) { + const rows = [...verifyWorkflow.matchAll(new RegExp(`group: ${group}\\n\\s+group_label: [^\\n]+\\n\\s+shard_index: (\\d+)\\n\\s+shard_count: (\\d+)`, "g"))]; + assert.deepEqual(rows.map((row) => [Number(row[1]), Number(row[2])]), + Array.from({ length: count }, (_, index) => [index, count])); } - for (const shardIndex of [0, 1, 2, 3, 4]) { - assert.match( - verifyWorkflow, - new RegExp(`shard_index: ${shardIndex}[\\s\\S]*?shard_count: 5`), - ); + assert.match(verifyWorkflow, new RegExp(`shard_index: ${shardIndex}[\\s\\S]*?shard_count: 5`)); } // workspaces-a splits with Vitest native --shard in pr.yml; release diff --git a/scripts/__tests__/run-vitest-stable-shard.test.mjs b/scripts/__tests__/run-vitest-stable-shard.test.mjs index 278d2cba2e..51bc54420a 100644 --- a/scripts/__tests__/run-vitest-stable-shard.test.mjs +++ b/scripts/__tests__/run-vitest-stable-shard.test.mjs @@ -10,6 +10,8 @@ import { partitionGeneralServerSuites, } from "../general-server-shard.mjs"; +import { assertSelectedTests, partitionTestLines } from "../test-line-shard.mjs"; + const repoRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), "..", ".."); const script = path.join(repoRoot, "scripts", "run-vitest-stable.mjs"); const durationsManifest = path.join(repoRoot, "scripts", "general-server-shard-durations.json"); @@ -264,3 +266,53 @@ test("the real shard partition is duration-balanced", () => { `shard weight spread ${maxTotal - minTotal}ms exceeds heaviest suite ${heaviest}ms: ${totals.join(", ")}`, ); }); + + +test("release server shards plus the dedicated chat file cover the original server group exactly", () => { + const full = dryRunJson(["--mode", "general", "--group", "general-server", "--shard-index", "0", "--shard-count", "1"]); + const shards = Array.from({ length: 4 }, (_, index) => dryRunJson([ + "--mode", "general", "--group", "general-server-without-chat", + "--shard-index", String(index), "--shard-count", "4", + ])); + const files = shards.flatMap((shard) => shard.selectedGeneralServerSuites); + const chat = "server/src/__tests__/chat-channels.integration.test.ts"; + assert.ok(!files.includes(chat)); + assert.deepEqual([...files, chat].sort(), full.selectedGeneralServerSuites.sort()); + assert.equal(new Set(files).size, files.length); + const defaultRun = dryRunJson([]); + assert.ok(defaultRun.generalServerSuiteCount === full.generalServerSuiteCount); +}); + +const lineShardFile = path.join(repoRoot, "server/src/__tests__/chat-channels.integration.test.ts"); +const caseAt = (line, name) => ({ name, file: lineShardFile, projectName: "@paperclipai/server", location: { line, column: 3 } }); + +test("test-line shards cover nested and parameterized cases exactly once without splitting a source line", () => { + const cases = [caseAt(10, "suite > nested > first"), caseAt(10, "suite > nested > second"), + caseAt(20, "same name"), caseAt(30, "same name"), caseAt(40, "last"), caseAt(50, "new case")]; + const shards = partitionTestLines(cases, 3, lineShardFile); + assert.deepEqual(shards.map((shard) => shard.tests.length), [2, 2, 2]); + assert.equal(shards.filter((shard) => shard.lines.includes(10)).length, 1); + assert.equal(shards.find((shard) => shard.lines.includes(10)).tests.length, 2); + assert.equal(shards.flatMap((shard) => shard.lines).length, 5); + assert.deepEqual(shards.flatMap((shard) => shard.tests).sort((a, b) => a.location.line - b.location.line), cases); + assert.deepEqual(partitionTestLines([...cases].reverse(), 3, lineShardFile).map((shard) => shard.lines), shards.map((shard) => shard.lines)); +}); + +test("line-shard collection rejects empty, foreign, or unlocated tests and invalid shard counts", () => { + const good = caseAt(10, "valid"); + for (const input of [[], null, [{ ...good, file: "/another.test.ts" }], [{ ...good, projectName: "wrong" }], + [{ ...good, location: undefined }], [{ ...good, location: { line: 0 } }], [{ ...good, name: "" }]]) { + assert.throws(() => partitionTestLines(input, 1, lineShardFile)); + } + for (const count of [0, -1, 1.5, Infinity, 2]) assert.throws(() => partitionTestLines([good], count, lineShardFile)); +}); + +test("filtered collection must match the exact assigned case identities, including duplicates", () => { + const expected = [caseAt(10, "same"), caseAt(10, "same"), caseAt(20, "nested > case")]; + assertSelectedTests(expected, [...expected].reverse(), lineShardFile); + for (const actual of [expected.slice(1), [...expected, caseAt(30, "extra")], + [expected[0], expected[1], caseAt(20, "renamed")], + [expected[0], expected[1], caseAt(21, "nested > case")]]) { + assert.throws(() => assertSelectedTests(expected, actual, lineShardFile)); + } +}); diff --git a/scripts/run-vitest-stable.mjs b/scripts/run-vitest-stable.mjs index db92138b64..1e7a91df35 100644 --- a/scripts/run-vitest-stable.mjs +++ b/scripts/run-vitest-stable.mjs @@ -1,11 +1,13 @@ #!/usr/bin/env node import { spawnSync } from "node:child_process"; -import { mkdirSync, mkdtempSync, readdirSync, realpathSync, statSync } from "node:fs"; +import { mkdirSync, mkdtempSync, readdirSync, readFileSync, realpathSync, statSync } from "node:fs"; import os from "node:os"; import path from "node:path"; import { fileURLToPath } from "node:url"; import { loadShardDurations, selectGeneralServerShard } from "./general-server-shard.mjs"; +import { assertSelectedTests, partitionTestLines } from "./test-line-shard.mjs"; + const repoRoot = process.cwd(); const scriptsDir = path.dirname(fileURLToPath(import.meta.url)); const generalServerShardDurations = loadShardDurations( @@ -66,11 +68,15 @@ const serializedModeName = "serialized"; const generalModeName = "general"; const allModeName = "all"; const generalServerGroupName = "general-server"; +const generalServerWithoutChatGroupName = "general-server-without-chat"; +const generalChatGroupName = "general-chat"; +const chatSuite = "server/src/__tests__/chat-channels.integration.test.ts"; const generalWorkspacesAGroupName = "general-workspaces-a"; const generalWorkspacesBGroupName = "general-workspaces-b"; const generalWorkspacesAProjects = ["@paperclipai/ui", "paperclipai"]; const generalWorkspacesBProjects = nonServerProjects.filter((project) => !generalWorkspacesAProjects.includes(project)); const generalGroupNames = [generalServerGroupName, generalWorkspacesAGroupName, generalWorkspacesBGroupName]; +const allowedGeneralGroupNames = [...generalGroupNames, generalServerWithoutChatGroupName, generalChatGroupName]; const serializedServerVitestArgs = [ "--no-file-parallelism", "--maxWorkers=1", @@ -216,10 +222,10 @@ function parseCliOptions(argv) { const shardAllowed = mode === serializedModeName || (mode === generalModeName && - (group === generalServerGroupName || group === generalWorkspacesAGroupName)); + ([generalServerGroupName, generalServerWithoutChatGroupName, generalChatGroupName, generalWorkspacesAGroupName].includes(group))); if (!shardAllowed && shardIndex !== null) { fail( - "--shard-index/--shard-count are only valid with --mode serialized, --mode general --group general-server, or --mode general --group general-workspaces-a.", + "--shard-index/--shard-count are only valid with serialized mode or a shardable general server/chat/workspaces-a group.", ); } @@ -227,8 +233,8 @@ function parseCliOptions(argv) { fail("--group is only valid with --mode general."); } - if (group !== null && !generalGroupNames.includes(group)) { - fail(`Unknown group "${group}". Expected one of: ${generalGroupNames.join(", ")}.`); + if (group !== null && !allowedGeneralGroupNames.includes(group)) { + fail(`Unknown group "${group}". Expected one of: ${allowedGeneralGroupNames.join(", ")}.`); } if (shardIndex !== null) { @@ -271,7 +277,7 @@ function selectSerializedSuites(routeTests, shardIndex, shardCount) { return shardFiles.map((file) => byRepoPath.get(file)); } -function runVitest(args, label) { +function runVitest(args, label, testShard = null) { console.log(`\n[test:run] ${label}`); invocationIndex += 1; const tempRootParent = process.platform === "win32" ? os.tmpdir() : "/tmp"; @@ -291,6 +297,25 @@ function runVitest(args, label) { }; mkdirSync(env.PAPERCLIP_HOME, { recursive: true }); mkdirSync(env.TMPDIR, { recursive: true }); + if (testShard) { + const collect = (filters, name) => { + const output = path.join(testRoot, `${name}.json`); + const result = spawnSync("pnpm", ["exec", "vitest", "list", ...sourceOnlyVitestArgs, + ...filters, "--allowOnly=false", "--includeTaskLocation", `--json=${output}`], { + cwd: repoRoot, env, stdio: "inherit", + }); + if (result.error || result.status !== 0) fail(`Vitest collection failed: ${result.error?.message ?? result.status}`); + return JSON.parse(readFileSync(output, "utf8")); + }; + const collected = collect(args, "all"); + const file = path.resolve(repoRoot, chatSuite); + const selected = partitionTestLines(collected, testShard.count, file)[testShard.index]; + const filters = selected.lines.map((line) => `${chatSuite}:${line}`); + args = [...args.filter((arg) => arg !== chatSuite), ...filters]; + assertSelectedTests(selected.tests, collect(args, "selected"), file); + console.log(`[test:run] chat shard ${testShard.index + 1}/${testShard.count}: ${selected.tests.length}/${collected.length} tests, ${selected.lines.length} source lines; exact filter coverage verified`); + args.push("--allowOnly=false"); + } const result = spawnSync("pnpm", ["exec", "vitest", "run", ...sourceOnlyVitestArgs, ...args], { cwd: repoRoot, env, @@ -325,16 +350,23 @@ function runProjectGroup(projects, groupName, shardIndex = null, shardCount = nu } function runGeneralGroup(routeTests, groupName, shardIndex = null, shardCount = null) { - if (groupName === generalServerGroupName) { + if (groupName === generalChatGroupName) { + runVitest(["--project", "@paperclipai/server", ...serializedServerVitestArgs, chatSuite], + "chat integration test shard", { index: shardIndex ?? 0, count: shardCount ?? 1 }); + return; + } + if (groupName === generalServerGroupName || groupName === generalServerWithoutChatGroupName) { + const withoutChat = groupName === generalServerWithoutChatGroupName; + const files = withoutChat ? generalServerTestFiles.filter((file) => file !== chatSuite) : generalServerTestFiles; if (shardCount !== null && shardCount > 1) { const shardFiles = selectGeneralServerShard( - generalServerTestFiles, + files, shardIndex, shardCount, generalServerShardDurations, ); console.log( - `\n[test:run] general-server shard ${shardIndex + 1}/${shardCount} running ${shardFiles.length} of ${generalServerTestFiles.length} suites`, + `\n[test:run] general-server shard ${shardIndex + 1}/${shardCount} running ${shardFiles.length} of ${files.length} suites`, ); if (shardFiles.length === 0) { return; @@ -353,6 +385,7 @@ function runGeneralGroup(routeTests, groupName, shardIndex = null, shardCount = } const excludeRouteArgs = routeTests.flatMap((file) => ["--exclude", file.serverPath]); + if (withoutChat) excludeRouteArgs.push("--exclude", "src/__tests__/chat-channels.integration.test.ts"); runVitest( [ "--project", @@ -436,16 +469,16 @@ if (options.dryRun) { shardIndex: options.shardIndex, shardCount: options.shardCount, group: options.group, - availableGeneralGroups: generalGroupNames, + availableGeneralGroups: allowedGeneralGroupNames, serializedSuiteCount: routeTests.length, selectedSerializedSuites: serializedSuites.map((routeTest) => routeTest.repoPath), generalServerSuiteCount: generalServerTestFiles.length, selectedGeneralServerSuites: options.mode === generalModeName && - options.group === generalServerGroupName && + [generalServerGroupName, generalServerWithoutChatGroupName].includes(options.group) && options.shardCount !== null ? selectGeneralServerShard( - generalServerTestFiles, + options.group === generalServerWithoutChatGroupName ? generalServerTestFiles.filter((file) => file !== chatSuite) : generalServerTestFiles, options.shardIndex, options.shardCount, generalServerShardDurations, diff --git a/scripts/test-line-shard.mjs b/scripts/test-line-shard.mjs new file mode 100644 index 0000000000..0d954115fb --- /dev/null +++ b/scripts/test-line-shard.mjs @@ -0,0 +1,45 @@ +import assert from "node:assert/strict"; +import path from "node:path"; + +function validateTests(tests, file) { + assert.ok(Array.isArray(tests) && tests.length > 0, "Vitest must collect at least one test"); + for (const test of tests) { + assert.equal(test.projectName, "@paperclipai/server", "unexpected test project"); + assert.equal(path.resolve(test.file), path.resolve(file), "unexpected test file"); + assert.ok(typeof test.name === "string" && test.name.length > 0, "missing test name"); + assert.ok(Number.isSafeInteger(test.location?.line) && test.location.line > 0, "missing test source line"); + } +} + +// Keep all cases registered on one source line together, including it.each +// and loop-generated cases. Balance by collected case count, not line count. +export function partitionTestLines(tests, count, file) { + validateTests(tests, file); + assert.ok(Number.isSafeInteger(count) && count > 0, "invalid shard count"); + const byLine = new Map(); + for (const test of tests) { + const line = test.location.line; + if (!byLine.has(line)) byLine.set(line, []); + byLine.get(line).push(test); + } + assert.ok(byLine.size >= count, "each shard must contain a source line"); + const groups = [...byLine].sort((a, b) => b[1].length - a[1].length || a[0] - b[0]); + const shards = Array.from({ length: count }, () => ({ lines: [], tests: [] })); + for (const [line, cases] of groups) { + const shard = shards.reduce((best, next) => next.tests.length < best.tests.length ? next : best); + shard.lines.push(line); + shard.tests.push(...cases); + } + for (const shard of shards) shard.lines.sort((a, b) => a - b); + return shards; +} + +// Re-collect using the exact filters passed to the subsequent test run. A +// Vitest filtering change must fail here instead of silently dropping cases. +export function assertSelectedTests(expected, actual, file) { + validateTests(actual, file); + const identities = (tests) => tests.map((test) => JSON.stringify([ + test.projectName, path.resolve(test.file), test.location.line, test.name, + ])).sort(); + assert.deepEqual(identities(actual), identities(expected), "Vitest filters must select exactly the assigned tests"); +}