diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index 8b74430b80..f157e12cdc 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -55,6 +55,9 @@ jobs: - name: Test no-git-push check run: node --test ./scripts/check-no-git-push.test.mjs + - name: Test general-server shard partition + run: node --test ./scripts/__tests__/run-vitest-stable-shard.test.mjs + - name: Validate release package manifest run: node ./scripts/release-package-map.mjs check @@ -135,8 +138,21 @@ jobs: fail-fast: false matrix: include: + # The server suite is pinned to maxWorkers=1 (server/vitest.config.ts), + # so it can only be parallelized across runners. Shard it to keep this + # lane off the PR critical path. - group: general-server - group_label: server + group_label: server (1/3) + shard_index: 0 + shard_count: 3 + - group: general-server + group_label: server (2/3) + shard_index: 1 + shard_count: 3 + - group: general-server + group_label: server (3/3) + shard_index: 2 + shard_count: 3 - group: general-workspaces-a group_label: workspaces-a - group: general-workspaces-b @@ -168,7 +184,13 @@ jobs: run: pnpm install --frozen-lockfile - name: Run grouped general test suites - run: pnpm test:run:general -- --group '${{ matrix.group }}' + run: | + if [ -n "${{ matrix.shard_count }}" ]; then + pnpm test:run:general -- --group '${{ matrix.group }}' \ + --shard-index ${{ matrix.shard_index }} --shard-count ${{ matrix.shard_count }} + else + pnpm test:run:general -- --group '${{ matrix.group }}' + fi verify: # Preserve the legacy required-check name while the underlying work runs in parallel. diff --git a/scripts/__tests__/run-vitest-stable-shard.test.mjs b/scripts/__tests__/run-vitest-stable-shard.test.mjs new file mode 100644 index 0000000000..889c5fb066 --- /dev/null +++ b/scripts/__tests__/run-vitest-stable-shard.test.mjs @@ -0,0 +1,63 @@ +import assert from "node:assert/strict"; +import { spawnSync } from "node:child_process"; +import path from "node:path"; +import { fileURLToPath } from "node:url"; +import test from "node:test"; + +const repoRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), "..", ".."); +const script = path.join(repoRoot, "scripts", "run-vitest-stable.mjs"); + +function dryRun(args) { + const result = spawnSync(process.execPath, [script, ...args, "--dry-run"], { + cwd: repoRoot, + encoding: "utf8", + }); + return result; +} + +function dryRunJson(args) { + const result = dryRun(args); + assert.equal(result.status, 0, `expected success for ${args.join(" ")}: ${result.stderr}`); + return JSON.parse(result.stdout); +} + +const SHARD_COUNT = 3; + +test("the general-server shards form a complete, non-overlapping partition", () => { + const shards = Array.from({ length: SHARD_COUNT }, (_, index) => + dryRunJson(["--mode", "general", "--group", "general-server", "--shard-index", String(index), "--shard-count", String(SHARD_COUNT)]), + ); + + const total = shards[0].generalServerSuiteCount; + assert.ok(total > 0, "expected a non-empty general-server suite set"); + + const seen = new Set(); + let selectedTotal = 0; + for (const shard of shards) { + assert.equal(shard.generalServerSuiteCount, total, "suite count must be stable across shards"); + for (const file of shard.selectedGeneralServerSuites) { + assert.ok(!seen.has(file), `suite assigned to more than one shard: ${file}`); + seen.add(file); + selectedTotal += 1; + } + } + + // Every suite runs exactly once: union covers the whole set with no overlap. + assert.equal(selectedTotal, total, "every suite must be selected exactly once"); + assert.equal(seen.size, total, "union of shards must cover the whole suite set"); +}); + +test("a route/authz suite never leaks into the general-server shards", () => { + const shard = dryRunJson(["--mode", "general", "--group", "general-server", "--shard-index", "0", "--shard-count", SHARD_COUNT.toString()]); + for (const file of shard.selectedGeneralServerSuites) { + assert.ok( + !/[^/]*(?:route|routes|authz)[^/]*\.test\.ts$/.test(file), + `route/authz suite must stay in the serialized lane, not general-server: ${file}`, + ); + } +}); + +test("shard flags are rejected for the parallel workspace groups", () => { + const result = dryRun(["--mode", "general", "--group", "general-workspaces-a", "--shard-index", "0", "--shard-count", "3"]); + assert.notEqual(result.status, 0, "workspace groups must not accept shard flags"); +}); diff --git a/scripts/run-vitest-stable.mjs b/scripts/run-vitest-stable.mjs index 117f5995f7..28a6888f14 100644 --- a/scripts/run-vitest-stable.mjs +++ b/scripts/run-vitest-stable.mjs @@ -6,6 +6,7 @@ import path from "node:path"; const repoRoot = process.cwd(); const serverRoot = path.join(repoRoot, "server"); +const serverSrcDir = path.join(repoRoot, "server", "src"); const serverTestsDir = path.join(repoRoot, "server", "src", "__tests__"); const nonServerProjects = [ "@paperclipai/shared", @@ -198,8 +199,13 @@ function parseCliOptions(argv) { fail("--shard-index and --shard-count must be provided together."); } - if (mode !== serializedModeName && shardIndex !== null) { - fail("--shard-index/--shard-count are only valid with --mode serialized."); + const shardAllowed = + mode === serializedModeName || + (mode === generalModeName && group === generalServerGroupName); + if (!shardAllowed && shardIndex !== null) { + fail( + "--shard-index/--shard-count are only valid with --mode serialized or --mode general --group general-server.", + ); } if (group !== null && mode !== generalModeName) { @@ -210,17 +216,17 @@ function parseCliOptions(argv) { fail(`Unknown group "${group}". Expected one of: ${generalGroupNames.join(", ")}.`); } - if (mode === serializedModeName) { - const resolvedShardCount = shardCount ?? 1; - const resolvedShardIndex = shardIndex ?? 0; - if (resolvedShardIndex >= resolvedShardCount) { - fail(`--shard-index must be less than --shard-count. Received ${resolvedShardIndex} of ${resolvedShardCount}.`); + if (shardIndex !== null) { + if (shardIndex >= shardCount) { + fail(`--shard-index must be less than --shard-count. Received ${shardIndex} of ${shardCount}.`); } + } + if (mode === serializedModeName) { return { mode, - shardIndex: resolvedShardIndex, - shardCount: resolvedShardCount, + shardIndex: shardIndex ?? 0, + shardCount: shardCount ?? 1, group: null, dryRun, }; @@ -228,8 +234,8 @@ function parseCliOptions(argv) { return { mode, - shardIndex: null, - shardCount: null, + shardIndex, + shardCount, group, dryRun, }; @@ -280,8 +286,31 @@ function runProjectGroup(projects, groupName) { } } -function runGeneralGroup(routeTests, groupName) { +function runGeneralGroup(routeTests, groupName, shardIndex = null, shardCount = null) { if (groupName === generalServerGroupName) { + if (shardCount !== null && shardCount > 1) { + const shardFiles = generalServerTestFiles.filter( + (_, index) => index % shardCount === shardIndex, + ); + console.log( + `\n[test:run] general-server shard ${shardIndex + 1}/${shardCount} running ${shardFiles.length} of ${generalServerTestFiles.length} suites`, + ); + if (shardFiles.length === 0) { + return; + } + + runVitest( + [ + "--project", + "@paperclipai/server", + ...serializedServerVitestArgs, + ...shardFiles, + ], + `${groupName} shard ${shardIndex + 1}/${shardCount}`, + ); + return; + } + const excludeRouteArgs = routeTests.flatMap((file) => ["--exclude", file.serverPath]); runVitest( [ @@ -336,6 +365,17 @@ const routeTests = walk(serverTestsDir) })) .sort((a, b) => a.repoPath.localeCompare(b.repoPath)); +// Every server test file that the general-server group is responsible for, +// i.e. the whole server project minus the route/authz suites that run in the +// dedicated serialized shards. Sharding this list across runners is what keeps +// the general-server lane from becoming the PR critical path: the server vitest +// config pins maxWorkers to 1, so the only way to parallelize is across jobs. +const generalServerTestFiles = walk(serverSrcDir) + .map((file) => toRepoPath(file)) + .filter((repoPath) => repoPath.endsWith(".test.ts")) + .filter((repoPath) => !isRouteOrAuthzTest(repoPath)) + .sort((a, b) => a.localeCompare(b)); + const options = parseCliOptions(process.argv.slice(2)); if (options.dryRun) { const serializedSuites = @@ -352,6 +392,15 @@ if (options.dryRun) { availableGeneralGroups: generalGroupNames, serializedSuiteCount: routeTests.length, selectedSerializedSuites: serializedSuites.map((routeTest) => routeTest.repoPath), + generalServerSuiteCount: generalServerTestFiles.length, + selectedGeneralServerSuites: + options.mode === generalModeName && + options.group === generalServerGroupName && + options.shardCount !== null + ? generalServerTestFiles.filter( + (_, index) => index % options.shardCount === options.shardIndex, + ) + : null, }, null, 2, @@ -362,7 +411,7 @@ if (options.dryRun) { if (options.mode === generalModeName || options.mode === allModeName) { if (options.group) { - runGeneralGroup(routeTests, options.group); + runGeneralGroup(routeTests, options.group, options.shardIndex, options.shardCount); } else { runGeneralSuites(routeTests); }