Skip to content

Commit 8067b1f

Browse files
ericallamTrigger.dev RepoOps
authored andcommitted
perf(ci): balance internal tests across two runners
Balance internal test files across packages and split the workload across two runners to reduce CI waits. Preserve package test configuration, refresh duration estimates, and combine all shard reports for timing updates. Mono-RevId: f43f740e7a8db6a812a157a8a02b2a15a7ac66fa
1 parent 230b5b0 commit 8067b1f

14 files changed

Lines changed: 800 additions & 156 deletions

.github/workflows/pr_checks.yml

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ name: 🤖 PR Checks
22

33
on:
44
pull_request:
5-
types: [opened, synchronize, reopened]
5+
types: [opened, synchronize, reopened, ready_for_review]
66
merge_group:
77

88
concurrency:
@@ -91,6 +91,10 @@ jobs:
9191
- 'pnpm-lock.yaml'
9292
- 'pnpm-workspace.yaml'
9393
- 'turbo.json'
94+
- 'vitest.internal.config.ts'
95+
- 'vitest.internal-reports.config.mjs'
96+
- 'internal-test-projects.mts'
97+
- 'test-timings.json'
9498
# The whole webapp app tree, not just its routes, and that is the whole reason this
9599
# filter exists. Two tests in @internal/observability-map read it: integration.test.ts
96100
# scans the live route tree, and webappSymbols.test.ts walks all of apps/webapp/app and
@@ -110,8 +114,8 @@ jobs:
110114
# Deliberately NOT here: this package's own paths, and packages/plugins/src and
111115
# internal-packages/rbac/src, the other two trees webappSymbols.test.ts reads.
112116
# `internal` above already matches `internal-packages/**` and the shared packages, and
113-
# `unit-tests-internal.yml` runs `turbo run test --filter "@internal/*"`, which picks up
114-
# @internal/observability-map and runs the same vitest suite. Listing them here as well
117+
# `unit-tests-internal.yml` loads every @internal/* test project through
118+
# vitest.internal.config.ts, including the same observability-map suite. Listing them here as well
115119
# ran the suite twice on every PR touching them, which was this filter's own doing.
116120
#
117121
# Also deliberately NOT here: pr_checks.yml, package.json, pnpm-lock.yaml,

.github/workflows/unit-tests-internal.yml

Lines changed: 87 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -14,14 +14,18 @@ on:
1414
jobs:
1515
unitTests:
1616
name: "🧪 Unit Tests: Internal"
17-
# Single big machine instead of a 12-job matrix: the internal suites are serial
18-
# (fileParallelism: false) and container-wait-bound, so 12 in-machine shard processes
19-
# fit comfortably in 32 vCPUs while paying the setup cost (install, prisma generate,
20-
# image pulls) once instead of 12 times.
17+
# Two Docker daemons and disks, with twelve serial test processes per runner.
18+
# Global shard indices ensure every file runs exactly once across both machines.
19+
strategy:
20+
fail-fast: false
21+
matrix:
22+
machine: [1, 2]
2123
runs-on: warp-ubuntu-latest-x64-32x
2224
env:
2325
DOCKERHUB_USERNAME: ${{ secrets.DOCKERHUB_USERNAME }}
24-
SHARD_TOTAL: 12
26+
MACHINE_INDEX: ${{ matrix.machine }}
27+
SHARDS_PER_MACHINE: 12
28+
SHARD_TOTAL: 24
2529
steps:
2630
- name: 🔧 Disable IPv6
2731
run: |
@@ -104,24 +108,32 @@ jobs:
104108
run: pnpm run generate
105109

106110
- name: 🏗️ Build test dependencies
107-
# Build once up-front so the parallel shard runs below (turbo --only) never race
108-
# to build or cache-restore the same outputs concurrently.
111+
# Build once up-front so shard processes never race to build or
112+
# cache-restore the same outputs concurrently.
109113
run: pnpm exec turbo run build --filter "@internal/*..."
110114

111-
- name: 🧪 Run Internal Unit Tests (${{ env.SHARD_TOTAL }} in-machine shards)
115+
- name: 🧪 Run Internal Unit Tests (machine ${{ matrix.machine }}/2)
112116
run: |
113-
# Same shard partitioning as the old 12-job matrix (DurationShardingSequencer
114-
# keys off --shard=i/N), but as parallel local processes. --only skips the
115-
# ^build dependency handled by the step above.
117+
# Each process discovers all internal projects, then takes its globally
118+
# balanced share of files. Keep one test file active per process.
119+
first=$(( (MACHINE_INDEX - 1) * SHARDS_PER_MACHINE + 1 ))
120+
last=$(( MACHINE_INDEX * SHARDS_PER_MACHINE ))
116121
status=0
117122
declare -a pids
118-
for i in $(seq 1 "$SHARD_TOTAL"); do
119-
pnpm exec turbo run test --only --concurrency=1 --filter "@internal/*" -- \
120-
--run --reporter=default --reporter=blob --shard="$i/$SHARD_TOTAL" --passWithNoTests \
121-
> "/tmp/internal-shard-$i.log" 2>&1 &
123+
for i in $(seq "$first" "$last"); do
124+
(
125+
started=$SECONDS
126+
result=0
127+
pnpm exec vitest run --config vitest.internal.config.ts \
128+
--sequence.concurrent=false --reporter=default --reporter=blob \
129+
--shard="$i/$SHARD_TOTAL" --passWithNoTests \
130+
> "/tmp/internal-shard-$i.log" 2>&1 || result=$?
131+
echo "::notice::internal shard $i/$SHARD_TOTAL finished in $((SECONDS - started))s (exit $result)"
132+
exit "$result"
133+
) &
122134
pids[i]=$!
123135
done
124-
for i in $(seq 1 "$SHARD_TOTAL"); do
136+
for i in $(seq "$first" "$last"); do
125137
if ! wait "${pids[i]}"; then
126138
status=1
127139
echo "::error::internal unit test shard $i/$SHARD_TOTAL failed"
@@ -132,13 +144,67 @@ jobs:
132144
done
133145
exit "$status"
134146
135-
- name: Gather all reports
147+
- name: Upload blob reports
136148
if: ${{ !cancelled() }}
149+
uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1
150+
with:
151+
name: internal-blob-report-${{ matrix.machine }}
152+
path: .vitest-reports/*
153+
include-hidden-files: true
154+
if-no-files-found: error
155+
retention-days: 1
156+
157+
merge-reports:
158+
name: "📊 Merge Internal Reports"
159+
if: ${{ !cancelled() }}
160+
needs: [unitTests]
161+
runs-on: warp-ubuntu-latest-x64-2x
162+
steps:
163+
- name: ⬇️ Checkout repo
164+
uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
165+
with:
166+
fetch-depth: 1
167+
persist-credentials: false
168+
169+
- name: ⎔ Setup pnpm
170+
uses: pnpm/action-setup@ea17c68df8912ef543352723c149a84f56e3d413 # v6.1.0
171+
with:
172+
version: 10.33.2
173+
174+
- name: ⎔ Setup node
175+
uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0
176+
with:
177+
node-version: 24.18.0
178+
179+
- name: Download blob reports
180+
uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1
181+
with:
182+
path: .vitest-reports
183+
pattern: internal-blob-report-*
184+
merge-multiple: true
185+
186+
- name: Check all shard reports are present
137187
run: |
138-
mkdir -p .vitest-reports
139-
find . -type f -path '*/.vitest-reports/blob-*.json' \
140-
-exec bash -c 'src="$1"; basename=$(basename "$src"); pkg=$(dirname "$src" | sed "s|^\./||;s|/\.vitest-reports$||;s|/|_|g"); cp "$src" ".vitest-reports/${pkg}-${basename}"' _ {} \;
188+
for i in $(seq 1 24); do
189+
test -s ".vitest-reports/blob-$i-24.json"
190+
done
141191
142192
- name: 📊 Merge reports
193+
run: >-
194+
pnpm dlx vitest@4.1.7 run --config vitest.internal-reports.config.mjs --merge-reports
195+
--reporter=default --reporter=json --outputFile.json=internal-test-results.json
196+
197+
- name: Validate merged test timings
198+
run: |
199+
cp test-timings.json "$RUNNER_TEMP/checked-test-timings.json"
200+
node internal-packages/testcontainers/scripts/update-test-timings.mjs \
201+
internal-test-results.json "$RUNNER_TEMP/checked-test-timings.json"
202+
203+
- name: Upload internal test timings
143204
if: ${{ !cancelled() }}
144-
run: pnpm dlx vitest@4.1.7 run --merge-reports --pass-with-no-tests
205+
uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1
206+
with:
207+
name: internal-test-results
208+
path: internal-test-results.json
209+
if-no-files-found: error
210+
retention-days: 14

internal-packages/observability-map/src/integration.test.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import {
99
} from "node:fs";
1010
import { tmpdir } from "node:os";
1111
import { join, resolve } from "node:path";
12+
import internalTestConfig from "../../../vitest.internal.config.js";
1213
import { isScannableFile, scanDirectory, scanFile } from "./scan.js";
1314
import { buildReport } from "./score.js";
1415
import { SCORED_CHECK_IDS } from "./checks/index.js";
@@ -306,7 +307,10 @@ describe("the package's tests are wired into the gate", () => {
306307
const internal = text.split(" internal:")[1]!.split(" obsmap:")[0]!;
307308
expect(internal).toContain(`'${PATH_PREFIX}internal-packages/**'`);
308309
expect(internal).not.toContain(`!${PATH_PREFIX}internal-packages/observability-map`);
309-
expect(read(resolve(WORKFLOWS, "unit-tests-internal.yml"))).toContain('--filter "@internal/*"');
310+
expect(read(resolve(WORKFLOWS, "unit-tests-internal.yml"))).toContain(
311+
"--config vitest.internal.config.ts"
312+
);
313+
expect(internalTestConfig.test?.projects).toContain(resolve(__dirname, ".."));
310314
});
311315

312316
// The test above only checks the package's own source path, a different overlap that was already
Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
// Node 24 strips the helper's types, so the weekly job needs no dependency install.
2+
import { readFileSync, writeFileSync } from "node:fs";
3+
import { updateTestTimings } from "../src/test-timings.ts";
4+
5+
const [reportPath, timingsPath] = process.argv.slice(2);
6+
if (!reportPath || !timingsPath) {
7+
throw new Error(
8+
"Usage: update-test-timings.mjs <internal-test-results.json> <test-timings.json>"
9+
);
10+
}
11+
const previous = JSON.parse(readFileSync(timingsPath, "utf8"));
12+
const report = JSON.parse(readFileSync(reportPath, "utf8"));
13+
const updated = updateTestTimings(previous, report);
14+
writeFileSync(timingsPath, `${JSON.stringify(updated, null, 2)}\n`);
15+
console.log(
16+
`Refreshed ${Object.keys(updated).filter((file) => updated[file] !== previous[file]).length} timings`
17+
);
Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
1+
import { execFileSync } from "node:child_process";
2+
import { mkdtempSync, mkdirSync, readFileSync, rmSync, writeFileSync } from "node:fs";
3+
import { tmpdir } from "node:os";
4+
import { resolve } from "node:path";
5+
import { pathToFileURL } from "node:url";
6+
import { expect, test } from "vitest";
7+
8+
const monorepoRoot = resolve(__dirname, "../../..");
9+
const vitestCli = resolve(monorepoRoot, "node_modules/vitest/vitest.mjs");
10+
const projectHelper = pathToFileURL(resolve(monorepoRoot, "internal-test-projects.mts")).href;
11+
12+
test("replays named project blobs into a nonempty JSON report without package dependencies", () => {
13+
const root = mkdtempSync(resolve(tmpdir(), "internal-report-merge-"));
14+
try {
15+
const projects = ["first", "second"].map((name) => {
16+
const directory = resolve(root, "internal-packages", name);
17+
mkdirSync(directory, { recursive: true });
18+
writeFileSync(
19+
resolve(directory, "package.json"),
20+
JSON.stringify({ name: `@internal/${name}`, scripts: { test: "vitest" } })
21+
);
22+
writeFileSync(
23+
resolve(directory, "vitest.config.mjs"),
24+
"export default { test: { globals: true } };"
25+
);
26+
writeFileSync(
27+
resolve(directory, "example.test.js"),
28+
"test('passes', () => expect(2 + 2).toBe(4));"
29+
);
30+
return directory;
31+
});
32+
writeFileSync(
33+
resolve(root, "run.config.mjs"),
34+
`export default { test: { projects: ${JSON.stringify(projects)} } };`
35+
);
36+
writeFileSync(
37+
resolve(root, "merge.config.mjs"),
38+
`import { getInternalTestReportConfig } from ${JSON.stringify(projectHelper)};
39+
export default getInternalTestReportConfig(${JSON.stringify(root)});`
40+
);
41+
const run = (args: string[]) =>
42+
execFileSync(process.execPath, [vitestCli, "run", ...args], {
43+
cwd: root,
44+
encoding: "utf8",
45+
timeout: 10_000,
46+
});
47+
run(["--config=run.config.mjs", "--maxWorkers=1", "--reporter=blob"]);
48+
run([
49+
"--config=merge.config.mjs",
50+
"--merge-reports",
51+
"--reporter=json",
52+
"--outputFile.json=results.json",
53+
]);
54+
const merged = JSON.parse(readFileSync(resolve(root, "results.json"), "utf8"));
55+
expect(merged.success).toBe(true);
56+
expect(merged.numPassedTests).toBe(2);
57+
expect(merged.testResults.map((file: { name: string }) => file.name).sort()).toEqual(
58+
projects.map((directory) => resolve(directory, "example.test.js")).sort()
59+
);
60+
} finally {
61+
rmSync(root, { recursive: true, force: true });
62+
}
63+
}, 30_000);

internal-packages/testcontainers/src/sequencer.cjs

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -56,8 +56,15 @@ function median(nums) {
5656
// It's a rotation of the bin->shard mapping, so coverage stays exact (each file runs once).
5757
function packageOffset(specs, count) {
5858
if (specs.length === 0) return 0;
59-
const rel = path.relative(REPO_ROOT, specs[0].moduleId);
60-
const key = rel.split(path.sep).slice(0, 2).join("/");
59+
const keys = new Set(
60+
specs.map((spec) =>
61+
path.relative(REPO_ROOT, spec.moduleId).split(path.sep).slice(0, 2).join("/")
62+
)
63+
);
64+
// A multi-project run is already balanced globally. Its mapping must not depend
65+
// on which package happened to finish discovering files first.
66+
if (keys.size !== 1) return 0;
67+
const [key] = keys;
6168
// FNV-1a - spreads similar sibling package names (e.g. internal-packages/*) far better than a
6269
// simple polynomial hash mod count, which collided run-engine + schedule-engine onto one shard.
6370
let h = 2166136261;
Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
import { readFileSync } from "node:fs";
2+
import { resolve } from "node:path";
3+
import { describe, expect, test } from "vitest";
4+
import type { TestSpecification, Vitest } from "vitest/node";
5+
import { DurationShardingSequencer } from "./sequencer.cjs";
6+
7+
const root = resolve(__dirname, "../../..");
8+
const timings: Record<string, number> = JSON.parse(
9+
readFileSync(resolve(root, "test-timings.json"), "utf8")
10+
);
11+
const paths = Object.keys(timings).filter((file) => file.startsWith("internal-packages/"));
12+
// Sequencing only consumes moduleId; these are inputs to the scheduling algorithm.
13+
const specs = [...paths, "internal-packages/new-package/src/new.test.ts"].map(
14+
(file) => ({ moduleId: resolve(root, file) }) as TestSpecification
15+
);
16+
17+
function sequencer(index: number, count: number) {
18+
return new DurationShardingSequencer({ config: { shard: { index, count } } } as Vitest);
19+
}
20+
21+
describe("multi-project duration sharding", () => {
22+
test.each([1, 2, 3, 12, 24])(
23+
"assigns every file exactly once across %i shards",
24+
async (count) => {
25+
const shards = await Promise.all(
26+
Array.from({ length: count }, (_, index) => sequencer(index + 1, count).shard(specs))
27+
);
28+
const assigned = shards.flat().map((spec) => spec.moduleId);
29+
expect(assigned).toHaveLength(specs.length);
30+
expect(new Set(assigned).size).toBe(specs.length);
31+
expect(assigned.sort()).toEqual(specs.map((spec) => spec.moduleId).sort());
32+
}
33+
);
34+
35+
test("keeps the same assignment when project discovery order changes", async () => {
36+
for (let index = 1; index <= 12; index++) {
37+
const original = await sequencer(index, 12).shard(specs);
38+
const reversed = await sequencer(index, 12).shard([...specs].reverse());
39+
expect(reversed.map((spec) => spec.moduleId)).toEqual(original.map((spec) => spec.moduleId));
40+
}
41+
});
42+
43+
test("retains all files without sharding and accepts an empty project", async () => {
44+
const unsharded = new DurationShardingSequencer({ config: {} } as Vitest);
45+
expect(await unsharded.shard(specs)).toBe(specs);
46+
expect(await sequencer(1, 12).shard([])).toEqual([]);
47+
});
48+
});

0 commit comments

Comments
 (0)