diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5ee515119a4..66efb25a05c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -556,14 +556,19 @@ jobs: echo 'Items on this shard (a package name, or a package plus a k/n file-level slice):' cat "$RUNNER_TEMP/shard-packages.txt" - # ⛔ A FILE-LEVEL SLICE BUILDS ITS DEPENDENCY CLOSURE HERE, IN A RUN THAT - # CARRIES NO PASSTHROUGH, so that the sharded run in the next step can be - # `--only` (#16395). - # - # Turbo folds a run-level passthrough into the hash of EVERY task in the - # run, not only the task that receives it -- and `-- "--shard=k/n"` is the - # whole reason a slice gets its own invocation at all (the next step's - # comment says why it cannot ride the shared run). Measured on turbo + # ⛔ A FILE-LEVEL SLICE BUILDS ITS DEPENDENCY CLOSURE HERE, IN ITS OWN + # GUARDED STEP, so the slice leg in the next step REPLAYS it. Since #19278 + # that leg carries its slice in `OS_TEST_SHARD`, which only the `test` + # task declares, so its build tasks hash exactly as they do here (61 of 61 + # identical, measured) and it schedules its own closure: this step is no + # longer what makes the leg correct, only what keeps the closure build off + # the test step's stall-guard site (see the last paragraph below). + # + # Why the step first existed (#16395, when the slice was still a + # passthrough and the leg was `--only`): turbo folds a run-level + # passthrough into the hash of EVERY task in the run, not only the task + # that receives it -- and `-- "--shard=k/n"` was then the whole reason a + # slice got its own invocation at all. Measured on turbo # 2.10.10, `--filter=@objectstack/cli`, `turbo run test ... --dry=json` # (60 tasks: 59 `build` + 1 `test`): # @@ -677,12 +682,15 @@ jobs: # Split the shard's ITEMS into the whole packages, which share one # turbo run as they always have, and the file-level slices, which - # cannot: `--shard=k/n` is passed through to vitest by turbo as a - # RUN-level argument, so it would reach every package in the run — - # and on any package with fewer test files than n that is a hard - # vitest failure (or, with --passWithNoTests, silently no tests at - # all). A slice therefore gets its own invocation, filtered to the one - # package the partitioner sliced. + # cannot: a slice's `k/n` travels as `OS_TEST_SHARD` in the turbo + # run's environment, and a run has ONE environment — set on the + # shared run it would reach every package's `test` task, which + # turbo.json declares it on. (Before #19278 it travelled as a + # `--shard=k/n` passthrough, which reached every package's vitest: a + # hard failure on any package with fewer test files than n, or with + # --passWithNoTests silently no tests at all.) A slice therefore gets + # its own invocation, filtered to the one package the partitioner + # sliced. FILTERS="" SLICES="" while read -r PKG SLICE; do @@ -727,74 +735,73 @@ jobs: PKG="${LEG%%=*}" SLICE="${LEG#*=}" LOG="$RUNNER_TEMP/test-core-slice-$(printf '%s' "$PKG" | tr -c 'A-Za-z0-9' '-').log" - # `--only` (#16395): the step above already built this slice's - # dependency closure in a passthrough-free run, so this run must - # schedule the ONE task the passthrough is for. Without it turbo - # re-hashes the whole `^build` closure under `--shard=k/n` and - # rebuilds it -- that comment carries the measurement. ⚠ The build - # step is load-bearing for this flag: a sliced package whose build - # never ran fails LOUDLY here (its imports resolve to a missing - # dist), never as a silent green. + # THE SLICE RIDES IN `OS_TEST_SHARD` (#19278), a variable turbo.json + # declares on this package's `test` task and its vitest.config.ts + # reads into vitest's `shard` (vitest 4.1.11 reads no shard + # variable of its own). Not a `-- --shard=k/n` passthrough, and so + # neither `--only` nor `--force`. The partitioner's `--self-test` + # fails a package it can slice whose config or task does not. + # + # Why the two flags existed, and why they are gone: # - # ⛔ `--force` (#18671) IS NOT REDUNDANT BESIDE `--only` -- do not - # delete it as a no-op. Read the paragraph above backwards: if - # `--only` is what stops turbo re-hashing the `^build` closure, - # then under `--only` this task's hash NO LONGER CARRIES that - # closure, and the one path by which a change in a dependency - # reaches this task is gone with it. What is left is the package's - # own files plus the hand-declared `$TURBO_ROOT$` inputs in - # turbo.json. So a slice that IS in the affected set -- and this - # shard's package set is exactly the affected set, computed two - # steps up -- can match a main-seeded cache entry across the very - # change that put it in that set, and be replayed instead of run. + # `--only` (#16395). Turbo folds a run-level passthrough into the + # hash of EVERY task in the run (turbo 2.10.10, cli's plan, plain + # vs `-- --shard=1/2`: 0 of 62 hashes identical), so the leg + # re-hashed and rebuilt the `^build` closure the step above had + # just built. `--only` scheduled the test alone -- and dropped the + # closure out of the test's HASH with it (#18671), leaving the + # package's own files plus the hand-declared `$TURBO_ROOT$` + # inputs. A slice in the affected set (on a PR, queue or push run + # this shard's set IS the affected set) could then match a + # main-seeded entry across the very change that put it there. + # Run 34746808828 did: it replayed `@objectstack/cli:test` on + # this leg -- `Cached: 1 cached, 1 total`, `73ms >>> FULL + # TURBO` -- out of a log a main push run had produced ~15 + # minutes earlier, on a commit that did not contain the PR under + # test. All six shards reported success, + # `check-test-completeness` graded the replay OK, the shard + # attestation said "ran to completion", and the red reached + # `main`. Neither of those two can tell a run from a replay. # - # That is not a hypothetical. Run 34746808828 replayed - # `@objectstack/cli:test` on this leg -- `Cached: 1 cached, 1 - # total`, `73ms >>> FULL TURBO` -- out of a log a main push run - # had produced ~15 minutes earlier, on a commit that did not - # contain the PR under test. All six shards reported success, - # `check-test-completeness` graded the replay OK, and the shard - # attestation said "ran to completion". Neither of those two can - # tell a run from a replay; the red reached `main`. + # `--force` (#18671, PR #19271). Made the leg execute; a constant + # bypass cannot alarm, and the hash stayed blind to the closure. # - # Measured on this tree (turbo 2.10.10) with a package whose - # `test` task has the same shape (`dependsOn: ["^build"]`), after - # a source change in an upstream package PLUS the closure rebuild - # the step above performs -- i.e. exactly this job's sequence: + # A declared env reaches ONLY the task that declares it: same plan, + # plain vs `OS_TEST_SHARD=1/2` is 61 of 62 identical, the one that + # moves is `cli#test`, and `1/2` vs `2/2` moves that one alone. The + # closure keeps the hashes the step above gave it (61 of 61 equal + # to `turbo run build --filter=$PKG`) and replays, while the test's + # hash carries the closure again. `cli#test`, `--dry=json`: # - # --only (before) 1 task `1 cached, 1 total` 81ms >>> FULL TURBO <- NOT RUN - # --only --force (here) 1 task `0 cached, 1 total` 1.15s <- runs - # no --only (option) 6 tasks `0 cached, 6 total` 1m54.963s <- runs, and rebuilds the closure + # this leg, OS_TEST_SHARD=1/2 clean 1ee03ac2f26389a6 + # + packages/spec/src/ui/view.zod.ts 04eabd0b5a9db364 moves + # + packages/types/src/env.ts 4b15652b8c493471 moves + # + connector-slack/src/* (not in closure) 1ee03ac2f26389a6 still (control) + # old leg, --only --force -- --shard=1/2 + # clean / + view.zod.ts d5e7ec263710fe59 still (the defect) # - # The third row is why the repair is `--force` rather than - # "drop `--only`": dropping it re-executes the closure the step - # above just built, which is #16395's bill paid twice per job. - # That bill re-measured TODAY at cli's real scale, same tree, - # `--dry=json` against a cache the passthrough-free build had - # just filled: the `--only` leg plans 1 task; dropping `--only` - # plans 60 and every one of the 60 is a cache MISS, against 57 - # HIT / 3 MISS for the same plan without the passthrough (2 of - # those 3 are `#build` tasks for packages that declare no `build` - # script, so they never execute; the real miss is the test). Cost - # of re-executing that closure here, `--concurrency=2` on a - # SHARED container: 5m11.262s, against 60ms `>>> FULL TURBO` for - # the same command when its hashes are left alone. + # Executed (cli, slice 1/16 for time; `packages/client/src/index.ts` + # edited -- a cli dependency no `$TURBO_ROOT$` input names -- then + # the step above rebuilt the closure, 57 of 59 cached): # - # `--force` re-executes THIS ONE TASK and nothing else, which is - # why `--only` stays: the two are a pair. Precedent, for the same - # reason in one sentence -- a gate must not be satisfiable by a - # replayed artifact -- is the docs-build gate's `TURBO_FORCE` - # below. Two things fall out of it that are worth keeping: - # `--summarize` now records a REAL duration for the sliced - # package (measure-test-shard-timings.mjs refuses to read one - # from a replay), and the log says `cache bypass, force - # executing ` where it used to say `cache hit, replaying - # logs `. + # old leg, no --force `cache hit, replaying logs` 69ms FULL TURBO <- NOT RUN + # old leg, --force `cache bypass, force executing` (same hash) <- runs + # this leg `cache miss, executing`, 59 of 60 cached <- runs # - # ⚠ What `--force` does NOT do is make the hash honest. It stays - # blind to the closure, so nothing in this repo yet fails when a - # slice leg's hash stops moving for an upstream source change. - set -- pnpm turbo run test "--filter=$PKG" --only --force --concurrency=4 --summarize --log-order=stream -- "--shard=$SLICE" + # and on an unchanged tree this leg replays (`60 cached, 60 total`, + # 104ms): with the closure in the hash a HIT means these inputs were + # already tested, which is why no `--force`. Cost: 62 tasks planned, + # 59 HIT / 3 MISS against the cache the step above fills (the test, + # plus two `#build` tasks of packages with no `build` script, which + # never execute), where the passthrough without `--only` was 62 MISS. + # The step above is no longer load-bearing for correctness -- this + # run schedules its own closure -- but it keeps that build on its + # own stall-guard site, and here it replays. `--summarize` records + # the slice only as the sha256 of `OS_TEST_SHARD` (`cliArguments` + # is empty); measure-test-shard-timings.mjs resolves that digest + # against the k/n the partitioner can emit and refuses one it + # cannot, so a slice is never timed as the whole package. + set -- env "OS_TEST_SHARD=$SLICE" pnpm turbo run test "--filter=$PKG" --concurrency=4 --summarize --log-order=stream fi LOGS="$LOGS $LOG" node scripts/run-with-stall-guard.mjs --log "$LOG" --stall-minutes 10 \ diff --git a/packages/cli/vitest.config.ts b/packages/cli/vitest.config.ts index 3109b7cf595..36a510bd4df 100644 --- a/packages/cli/vitest.config.ts +++ b/packages/cli/vitest.config.ts @@ -643,7 +643,7 @@ // `node_modules` exclusion: an exact-path list matches nothing it does not name. import { defineConfig } from 'vitest/config'; import path from 'path'; -import { parseCLI } from 'vitest/node'; +import { parseCLI, type TestUserConfig } from 'vitest/node'; import { runFilterPreflight, runProjectCliOverridePreflight, @@ -710,6 +710,35 @@ runProjectCliOverridePreflight({ parse: parseCLI, }); +// #19278 — THE FILE-LEVEL SLICE ARRIVES AS AN ENV VAR, NOT AS A PASSTHROUGH. +// `scripts/partition-test-shards.mjs` slices this package (`FILE_SHARDED_PACKAGES`), +// and Test Core runs each slice as `OS_TEST_SHARD=k/n turbo run test`. The +// value reaches vitest HERE because vitest 4.1.11 reads no shard variable of its +// own (no `VITEST_SHARD`: the variables it reads are enumerable in its dist), +// and it reaches this process at all only because `turbo.json` declares +// `OS_TEST_SHARD` in this package's `test` task `env` — which is also what +// puts the slice in the task hash. Unset (every local run, the whole-package +// leg, the nightly) it is `undefined`, and the run is unsharded as before; a +// `--shard` on the command line still wins, because vitest merges the CLI +// options OVER this block. +// +// Why a passthrough (`-- --shard=k/n`) is no longer the carrier: turbo folds a +// run-level passthrough into the hash of every task in the run, so the slice +// leg had to be `--only`, and `--only` drops the `build` closure out of the +// test's hash — a slice could replay across the very upstream change that put +// it in the affected set. An env declared on the task reaches only the task. +// +// ⚠️ Typed against vitest's CLI-options type (`TestUserConfig`), spread rather +// than written as a literal key: vitest declares `shard` on its CLI options and +// NOT on `InlineConfig`, the type of this `test` block (a literal `shard:` is +// TS2769 here: "'shard' does not exist in type 'InlineConfig'"), yet it +// resolves the two as one object (`deepMerge(configDefaults, test, cliOptions)`) +// — measured on 4.1.11, it honours this key, projects included. The +// partitioner's `--self-test` fails when a sliced package's config stops +// reading the variable — it looks for the read in code position, so the read +// lives at its use site in the `test` block below, not in a helper binding +// that could outlive the spread. + export default defineConfig({ resolve: { // Array form with an ANCHORED pattern, per the trap the gate documents: @@ -784,6 +813,9 @@ export default defineConfig({ ], }, test: { + // The file-level slice, when Test Core runs one (#19278) — see the section + // above `export default` for why it is spread and typed this way. + ...({ shard: process.env.OS_TEST_SHARD } satisfies Pick), // A late console.* must not redden a green suite (#10374): vitest's worker // forwards console output over RPC and discards the promise, and a write // landing after teardown's rpcDone() snapshot is rejected into an unhandled diff --git a/scripts/measure-test-shard-timings.mjs b/scripts/measure-test-shard-timings.mjs index 4b71bce7275..453a072feed 100644 --- a/scripts/measure-test-shard-timings.mjs +++ b/scripts/measure-test-shard-timings.mjs @@ -81,12 +81,17 @@ // --run ... [--merge-into ] [--out ] // node scripts/measure-test-shard-timings.mjs --self-test +import { createHash } from 'node:crypto'; import { existsSync, readFileSync, readdirSync, writeFileSync } from 'node:fs'; import path from 'node:path'; import { fileURLToPath } from 'node:url'; import process from 'node:process'; -import { countTestFiles } from './partition-test-shards.mjs'; +// FILE_SHARDED_PACKAGES and SLICE_ENV are read only inside functions: this +// import is circular (the partitioner imports samplesFromSummary from here), so +// a top-level read would meet an uninitialised binding when the partitioner is +// the entry point. +import { countTestFiles, FILE_SHARDED_PACKAGES, SLICE_ENV } from './partition-test-shards.mjs'; import { isEntrypoint } from './invoked-as.mjs'; const REPO_ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..'); @@ -140,6 +145,79 @@ export function sliceOfCliArguments(args) { return null; } +// Which file-level slice a task ran as when the slice travelled in +// `OS_TEST_SHARD` rather than as a passthrough (#19278), or null. +// +// Test Core's slice leg now runs `OS_TEST_SHARD=k/n turbo run test` with NO +// passthrough -- a passthrough is folded into the hash of every task in the run, +// which forced `--only`, which dropped the build closure out of the test's hash +// -- so `cliArguments` is `[]` on that leg and sliceOfCliArguments() above sees +// a WHOLE package. turbo does record the variable, but only as a digest: the +// task's `environmentVariables.configured` carries `OS_TEST_SHARD=` followed by +// the sha256 hex of the value (measured on turbo 2.10.10, executed summaries +// and `--dry=json` alike: `1/16` -> `ece2d97a…`, `1/2` -> `d939926f…`), and +// `OS_TEST_SHARD=` with nothing after it when the value is empty. +// +// So the digest is matched against every slice the partitioner CAN emit for +// that package, and nothing else: `k/n` for 1 <= k <= n, where n is +// FILE_SHARDED_PACKAGES[package] from partition-test-shards.mjs. The bound is +// that map -- n candidates for a package it slices (2 for @objectstack/cli +// today), none for one it does not. +// +// ⛔ A digest that matches no candidate is REFUSED, never read as a whole +// package. That is the #16173 direction exactly: one slice's window recorded as +// the package's whole cost, a number that reads right and is n times too small. +// It happens when FILE_SHARDED_PACKAGES changed since the run that wrote the +// summary, or when the variable was set by hand to a slice the partitioner does +// not emit; either way the remedy is a summary from a run under the current map. +// +// An empty value is not a slice: the package's vitest config hands vitest an +// empty `shard`, which it ignores, and the package runs whole. A task with no +// `environmentVariables` record at all carries no evidence either way and is +// read as carrying no env slice -- turbo 2.10.10 writes one on every task, and +// the self-test fixtures predate it. +export function sliceOfEnvironment(environmentVariables, name, label, sliced = FILE_SHARDED_PACKAGES) { + const configured = environmentVariables?.configured; + if (configured === undefined || configured === null) return null; + if (!Array.isArray(configured)) { + throw new Error(`${label}: environmentVariables.configured is not an array -- did the summary format change?`); + } + const prefix = `${SLICE_ENV}=`; + const entry = configured.find((e) => typeof e === 'string' && e.startsWith(prefix)); + if (entry === undefined) return null; + const digest = entry.slice(prefix.length); + if (digest === '') return null; + const count = Object.hasOwn(sliced, name) ? sliced[name] : 1; + const candidates = []; + for (let index = 1; count > 1 && index <= count; index++) { + const spec = `${index}/${count}`; + if (createHash('sha256').update(spec).digest('hex') === digest) return { index, count }; + candidates.push(spec); + } + throw new Error( + `${label}: ${name} ran with ${SLICE_ENV} set (digest ${digest.slice(0, 16)}…), and that digest ` + + `matches no slice the partitioner can emit for it (${candidates.length ? candidates.join(', ') : 'none: FILE_SHARDED_PACKAGES does not slice it'}). ` + + 'Refusing to read the window as a whole-package sample: it is one slice of the suite, and ' + + 'recording it as the whole cost is the #16173 defect. If FILE_SHARDED_PACKAGES changed since ' + + 'this run, refresh from a run made under the current map.' + ); +} + +// The slice one measured leg ran as, from either carrier. Both present and +// disagreeing is not a reading to pick from: vitest would have run the CLI's, +// but no workflow here sets both, so it is refused rather than resolved. +function sliceOfLeg(leg, name, label, sliced) { + const fromArgs = sliceOfCliArguments(leg.cliArguments); + const fromEnv = sliceOfEnvironment(leg.environmentVariables, name, label, sliced); + if (fromArgs && fromEnv && (fromArgs.index !== fromEnv.index || fromArgs.count !== fromEnv.count)) { + throw new Error( + `${label}: ${name} carries two different slices -- --shard=${fromArgs.index}/${fromArgs.count} ` + + `in cliArguments and ${fromEnv.index}/${fromEnv.count} in ${SLICE_ENV}. Refusing to guess.` + ); + } + return fromArgs ?? fromEnv; +} + // The task names whose windows this file counts as a package's test cost. // #16550: since #16466, six packages (core, objectql, rest, runtime, spec, // types) split their suite into `test` and `test:repo` (the repo-scanning @@ -172,7 +250,7 @@ const SAMPLED_TASKS = ['test', 'test:repo']; // Recording one leg's seconds alone (because the other was cached or failed) // would write a partial suite's cost as the package's whole cost, a reading // worse than today's undercount by #16466's own defect this card fixes. -export function samplesFromSummary(parsed, label) { +export function samplesFromSummary(parsed, label, sliced = FILE_SHARDED_PACKAGES) { const tasks = parsed?.tasks; if (!Array.isArray(tasks)) { throw new Error( @@ -182,7 +260,7 @@ export function samplesFromSummary(parsed, label) { } // One entry per package, holding whichever of its sampled tasks this // summary carries (almost always just `test`; `test` + `test:repo` for a - // split package). Each leg is recorded as EITHER a seconds+cliArguments + // split package). Each leg is recorded as EITHER a seconds+slice-carrier // reading, OR a `cached`/`failed` flag -- never both -- so the fold below // can tell "this leg disqualifies the package" from "this leg is a real // measurement" without re-reading the raw task. @@ -211,7 +289,7 @@ export function samplesFromSummary(parsed, label) { } const seconds = (endTime - startTime) / 1000; if (!(seconds >= 0)) throw new Error(`${label}: ${name}#${taskName} measured ${seconds}s`); - legs.set(taskName, { seconds, cliArguments: task.cliArguments }); + legs.set(taskName, { seconds, cliArguments: task.cliArguments, environmentVariables: task.environmentVariables }); } const samples = new Map(); @@ -230,13 +308,14 @@ export function samplesFromSummary(parsed, label) { } if (readings.some((leg) => leg.failed)) continue; let seconds = 0; - let cliArguments; + let slice = null; for (const leg of readings) { seconds += leg.seconds; - cliArguments ??= leg.cliArguments; + // A passthrough slice (`cliArguments`, the nightly tiers) or an + // `OS_TEST_SHARD` one (Test Core, #19278) -- see sliceOfEnvironment(). + slice ??= sliceOfLeg(leg, name, label, sliced); } samples.set(name, seconds); - const slice = sliceOfCliArguments(cliArguments); if (slice) slices.set(name, slice); } return { samples, skippedCached, slices }; @@ -480,11 +559,12 @@ export function buildDataset({ perSummary, fileCounts, provenance, carryFrom = n // remedy is to find what stopped registering, never to lower the number. const SELF_TEST_BATTERIES = Object.freeze({ 'measure-test-shard-timings self-test': 56, + 'env-carried slices (#19278)': 10, }); // DELETING an entry silences that battery's floor exactly as effectively as // zeroing it, so the roster's own size is pinned too. -const SELF_TEST_BATTERY_FLOOR = 1; +const SELF_TEST_BATTERY_FLOOR = 2; // The key an assertion is filed under when no battery is open. It is not a // declared battery, so it reds by the same set difference rather than silently @@ -1066,6 +1146,128 @@ function selfTest() { if (flat === null || path.basename(flat) !== 'spec') throw new Error('workspace: a depth-1 package stopped resolving'); }); + // -- ENV-CARRIED SLICES (#19278) -------------------------------------------- + // + // Test Core's slice leg carries k/n in OS_TEST_SHARD, which a summary records + // only as a sha256 digest in `environmentVariables.configured`. With the + // digest path dropped, the digest cases below fail: a slice read as a whole + // package (the #16173 direction), or an unmatched or conflicting digest let + // through. Three are controls that hold either way -- the passthrough + // carrier, an empty value, an unrelated variable -- and pin what the digest + // path must NOT read. Each case was ablated against the code it pins. + // `sliced` stands in for FILE_SHARDED_PACKAGES so the fixtures keep the short + // names above, and one case reads the REAL map. + battery('env-carried slices (#19278)'); + const digestOf = (value) => createHash('sha256').update(value).digest('hex'); + const envTask = (pkg, start, end, value, extra = {}) => ({ + ...testTask(pkg, start, end), + cliArguments: [], + environmentVariables: { + specified: { env: ['OS_TEST_SHARD', 'OS_TEST_TIERS'], passThroughEnv: null }, + configured: [`OS_TEST_SHARD=${value === '' ? '' : digestOf(value)}`], + inferred: [], + passthrough: null, + }, + ...extra, + }); + const map3 = Object.freeze({ cli: 3 }); + + check(() => { + const r = samplesFromSummary(summary([envTask('cli', 0, 118_073, '2/3')]), 'f', map3); + const s = r.slices.get('cli'); + if (!s || s.index !== 2 || s.count !== 3) { + throw new Error(`env slice: an OS_TEST_SHARD digest was read as ${JSON.stringify(s ?? null)}, not 2/3`); + } + if (r.samples.get('cli') !== 118.073) throw new Error('env slice: the slice window was not kept as the sample'); + }); + check(() => { + // The REAL map, the real package name, a digest of the form turbo writes. + const n = FILE_SHARDED_PACKAGES['@objectstack/cli']; + const r = samplesFromSummary(summary([envTask('@objectstack/cli', 0, 1000, `${n}/${n}`)]), 'f'); + const s = r.slices.get('@objectstack/cli'); + if (!s || s.index !== n || s.count !== n) { + throw new Error(`env slice: the live FILE_SHARDED_PACKAGES did not resolve ${n}/${n} (got ${JSON.stringify(s ?? null)})`); + } + }); + check(() => { + // The passthrough carrier is untouched: the nightly tiers still use it. + const r = samplesFromSummary(summary([slicedTask('cli', 0, 400_000, 1, 3)]), 'f', map3); + const s = r.slices.get('cli'); + if (!s || s.index !== 1 || s.count !== 3) throw new Error('env slice: the cliArguments carrier stopped working'); + }); + check(() => { + // ⛔ The refusal: a digest no candidate matches must never become a sample. + let message = ''; + try { + samplesFromSummary(summary([envTask('cli', 0, 118_073, '1/16')]), 'f', map3); + } catch (e) { + message = e.message; + } + if (!message.includes('matches no slice the partitioner can emit') || !message.includes('1/3, 2/3, 3/3')) { + throw new Error(`env slice: an unmatched digest was not refused naming its candidates (got ${JSON.stringify(message)})`); + } + }); + check(() => { + // A package the partitioner never slices has NO candidates, so any slice + // digest on it is refused as well. + if (!threw(() => samplesFromSummary(summary([envTask('a', 0, 10_000, '1/2')]), 'f', map3))) { + throw new Error('env slice: a slice digest on a package the partitioner does not slice was accepted'); + } + }); + check(() => { + // A refusal is a refusal of the whole summary: buildDataset never sees it. + if (!threw(() => + buildDataset({ + perSummary: [samplesFromSummary(summary([envTask('cli', 0, 400_000, '9/9')]), 'f', map3)], + fileCounts: new Map([['cli', 300]]), + provenance: {}, + }) + )) { + throw new Error('env slice: an unmatched digest reached the dataset'); + } + }); + check(() => { + // An empty value is no slice: vitest ignores an empty `shard`. + const r = samplesFromSummary(summary([envTask('cli', 0, 10_000, '')]), 'f', map3); + if (r.slices.size !== 0 || r.samples.get('cli') !== 10) { + throw new Error('env slice: an empty OS_TEST_SHARD was read as a slice'); + } + }); + check(() => { + // Other declared variables are not the slice. + const task = { + ...testTask('cli', 0, 10_000), + environmentVariables: { configured: [`OS_TEST_TIERS=${digestOf('queue')}`] }, + }; + if (samplesFromSummary(summary([task]), 'f', map3).slices.size !== 0) { + throw new Error('env slice: an unrelated configured variable was read as a slice'); + } + }); + check(() => { + const agree = envTask('cli', 0, 10_000, '1/3', { cliArguments: ['--shard=1/3'] }); + const s = samplesFromSummary(summary([agree]), 'f', map3).slices.get('cli'); + if (!s || s.index !== 1) throw new Error('env slice: two agreeing carriers were not read as that slice'); + const disagree = envTask('cli', 0, 10_000, '2/3', { cliArguments: ['--shard=1/3'] }); + if (!threw(() => samplesFromSummary(summary([disagree]), 'f', map3))) { + throw new Error('env slice: two disagreeing carriers were resolved instead of refused'); + } + }); + check(() => { + // The load-bearing case, env-carried: three slices are ONE package. + const ds = buildDataset({ + perSummary: [ + samplesFromSummary(summary([envTask('cli', 0, 400_000, '1/3')]), 'f', map3), + samplesFromSummary(summary([envTask('cli', 0, 380_000, '2/3')]), 'g', map3), + samplesFromSummary(summary([envTask('cli', 0, 420_000, '3/3'), testTask('a', 0, 10_000)]), 'h', map3), + ], + fileCounts: new Map([['cli', 300], ['a', 5]]), + provenance: {}, + }); + if (ds.packages.cli !== 1200) { + throw new Error(`env slice: three env-carried slices summed to ${ds.packages.cli}, expected 1200`); + } + }); + // -- The floor: every declared battery RAN, and ran its cases (#13489) ---- // // Evaluated after every battery has had its chance and BEFORE the verdict, so diff --git a/scripts/partition-test-shards.mjs b/scripts/partition-test-shards.mjs index 395139c9ac2..86804a477d9 100644 --- a/scripts/partition-test-shards.mjs +++ b/scripts/partition-test-shards.mjs @@ -99,13 +99,15 @@ // lines, which the caller must treat as "nothing to run", NOT as "no filter": // a `turbo run test` with no --filter args runs the entire workspace. -import { readFileSync, readdirSync } from 'node:fs'; +import { existsSync, readFileSync, readdirSync } from 'node:fs'; import path from 'node:path'; import { fileURLToPath } from 'node:url'; import process from 'node:process'; import { isEntrypoint } from './invoked-as.mjs'; +import { maskCommentsAndLiterals } from './js-comment-mask.mjs'; import { samplesFromSummary } from './measure-test-shard-timings.mjs'; +import { workspacePackages } from './workspace-enumerator.mjs'; const REPO_ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..'); const TIMINGS_PATH = path.join(REPO_ROOT, 'scripts', 'test-shard-timings.json'); @@ -188,7 +190,9 @@ export const MAX_MEASURED_OVER_PREDICTED = 1.5; // suite below package granularity. // // THIS IS THAT SPLIT, and it is the shape the Dogfood job has run since #4859: -// vitest's own `--shard=k/n` passthrough applied to ONE named package. The +// vitest's own `--shard=k/n` applied to ONE named package (carried to it in +// `OS_TEST_SHARD` rather than as a passthrough since #19278 -- see SLICE_ENV +// below; the argument that follows is about vitest's shard, not the carrier). The // objection this file records against passthrough is specific and it does not // reach here -- `--shard` on a package with fewer test files than the shard // count hard-fails on vitest 4, and `--passWithNoTests` converts that into @@ -267,6 +271,109 @@ export function sliceCountFor(name, fileCount = null) { return n; } +// ── A SLICE MUST REACH THE SUITE IT SLICES (#19278) ──────────────────────── +// +// Test Core hands a slice to its package as `OS_TEST_SHARD=k/n` in the +// environment of that slice's own turbo run -- no longer as a `-- --shard=k/n` +// passthrough, which turbo folds into the hash of every task in the run and +// which therefore needed `--only`, which in turn dropped the build closure out +// of the test task's hash (#18671). Two halves must both hold for the value to +// arrive, and missing either one is SILENT: the slice's run goes green having +// run the WHOLE suite, on every shard that carries a slice, because vitest +// never hears of a shard. +// +// 1. turbo.json declares it in the `env` of the package's `test` task. turbo +// 2.10 runs in strict env mode and strips an undeclared variable before +// the task's shell sees it -- and the declaration is also what puts the +// slice into the task hash, so `1/2` and `2/2` never share a cache entry. +// A `#test` entry REPLACES the generic `test` task for that +// package, so the declaration is read from the one that applies. +// 2. the package's vitest config reads it into vitest's `shard`. vitest +// 4.1.11 reads no shard variable of its own. +// +// ⛔ A SPELLING check, the check-tier-file-adoption idiom: it proves both halves +// are wired, never that vitest honours them. That was measured on the change +// that introduced the variable (the same small file set under unset / 1/2 / +// 2/2: 6 files, then two disjoint 3s whose union is the 6). The read is looked +// for in code position only -- comments and literals are masked first -- so a +// config that merely MENTIONS the variable in prose does not satisfy it. +export const SLICE_ENV = 'OS_TEST_SHARD'; +const SLICE_READ = new RegExp(`\\bshard\\s*:\\s*process\\.env\\.${SLICE_ENV}\\b`); +const VITEST_CONFIG_NAMES = Object.freeze([ + 'vitest.config.ts', + 'vitest.config.mts', + 'vitest.config.cts', + 'vitest.config.js', + 'vitest.config.mjs', + 'vitest.config.cjs', +]); + +// The verdict for one sliced package, from what was read. Pure, so the +// self-test pins every direction without a tree to break. Returns the +// problems, empty when both halves are wired. +export function judgeSliceWiring(name, { configFile, configSource, turbo, packageTurboJson = false }) { + const problems = []; + if (packageTurboJson) { + problems.push( + `${name}: carries its own turbo.json, whose task merge this check does not model -- ` + + 'teach judgeSliceWiring() to read it before trusting a green here.' + ); + } + const tasks = turbo?.tasks ?? {}; + const own = `${name}#test`; + const where = Object.hasOwn(tasks, own) ? `turbo.json tasks["${own}"]` : 'turbo.json tasks.test'; + const def = Object.hasOwn(tasks, own) ? tasks[own] : tasks.test; + if (!def) { + problems.push(`${name}: turbo.json defines no \`test\` task that applies to it.`); + } else if (!Array.isArray(def.env) || !def.env.includes(SLICE_ENV)) { + problems.push( + `${name}: ${where}.env does not declare ${SLICE_ENV}. turbo's strict env mode strips it ` + + 'before vitest starts, so every slice of this package runs its WHOLE suite, green.' + ); + } + if (configSource == null) { + problems.push( + `${name}: no vitest config (${VITEST_CONFIG_NAMES.join(' / ')}) to read ${SLICE_ENV} -- ` + + 'vitest reads no shard variable itself, so every slice would run the WHOLE suite.' + ); + } else if (!SLICE_READ.test(maskCommentsAndLiterals(configSource))) { + problems.push( + `${name}: ${configFile} never reads ${SLICE_ENV} into vitest's \`shard\` ` + + `(\`shard: process.env.${SLICE_ENV}\`, in code rather than a comment) -- every slice ` + + 'of this package would run its WHOLE suite, green.' + ); + } + return problems; +} + +// Both halves, read from the tree, for every package FILE_SHARDED_PACKAGES +// names. `judged` is returned beside the problems so a caller can tell "every +// sliced package is wired" apart from "no package was looked at". +export function sliceWiringProblems(root = REPO_ROOT, sliced = FILE_SHARDED_PACKAGES) { + const turbo = JSON.parse(readFileSync(path.join(root, 'turbo.json'), 'utf8')); + const dirOf = new Map(workspacePackages(root).map(({ dir, manifest }) => [manifest?.name, dir])); + const problems = []; + let judged = 0; + for (const name of Object.keys(sliced)) { + const dir = dirOf.get(name); + if (dir === undefined) { + problems.push(`${name}: named in FILE_SHARDED_PACKAGES, but no workspace package carries that name.`); + continue; + } + const file = VITEST_CONFIG_NAMES.find((f) => existsSync(path.join(root, dir, f))); + problems.push( + ...judgeSliceWiring(name, { + configFile: file ? `${dir}/${file}` : null, + configSource: file ? readFileSync(path.join(root, dir, file), 'utf8') : null, + turbo, + packageTurboJson: existsSync(path.join(root, dir, 'turbo.json')), + }) + ); + judged++; + } + return { problems, judged }; +} + // Expand weighed packages into shard items, splitting a file-sharded package's // WHOLE weight evenly across its slices. Every downstream consumer -- partition, // balanceOf, the balancing pins -- sees one flat list of `{name, weight}` whose @@ -651,11 +758,12 @@ const SELF_TEST_BATTERIES = Object.freeze({ 'the balancing pins (#10472)': 21, 'predicted-vs-measured drift (#16173)': 9, 'file-level slice items (#16173)': 20, + 'file-level slices reach vitest through OS_TEST_SHARD (#19278)': 9, }); // DELETING an entry silences that battery's floor exactly as effectively as // zeroing it, so the roster's own size is pinned too. -const SELF_TEST_BATTERY_FLOOR = 9; +const SELF_TEST_BATTERY_FLOOR = 10; // The key an assertion is filed under when no battery is open. It is not a // declared battery, so it reds by the same set difference rather than silently @@ -1355,6 +1463,93 @@ function selfTest() { } }); + // -- A SLICE MUST REACH THE SUITE IT SLICES (#19278) -------------------- + // + // The live tree first: every package this file can slice declares + // OS_TEST_SHARD on the `test` task that applies to it AND reads it into + // vitest's `shard`. Then the judge on synthetic inputs, each half removed in + // turn, so a judge that stopped looking at a half cannot stay green. + battery('file-level slices reach vitest through OS_TEST_SHARD (#19278)'); + + check(() => { + const { problems, judged } = sliceWiringProblems(); + const expected = Object.keys(FILE_SHARDED_PACKAGES).length; + if (expected === 0 || judged !== expected) { + throw new Error( + `slice wiring: judged ${judged} of ${expected} sliced package(s) -- a check that looked at ` + + 'nothing cannot vouch that every slice reaches vitest.' + ); + } + if (problems.length > 0) { + throw new Error(`slice wiring, live tree:\n - ${problems.join('\n - ')}`); + } + }); + + const wiredConfig = 'const s = { shard: process.env.OS_TEST_SHARD };\nexport default { test: { ...s } };\n'; + const genericOnly = { tasks: { test: { env: ['OS_TEST_TIERS', 'OS_TEST_SHARD'] } } }; + const judge = (over) => + judgeSliceWiring('@x/sliced', { configFile: 'x/vitest.config.ts', configSource: wiredConfig, turbo: genericOnly, ...over }); + + check(() => { + const p = judge({}); + if (p.length !== 0) throw new Error(`slice wiring: a fully wired package was refused: ${p.join(' | ')}`); + }); + check(() => { + const p = judge({ configSource: 'export default { test: {} };\n' }); + if (!p.some((m) => m.includes('never reads OS_TEST_SHARD'))) { + throw new Error('slice wiring: a config that never reads the variable was accepted'); + } + }); + check(() => { + // Prose is not a read: the variable named only in a comment must not pass. + const p = judge({ configSource: '// shard: process.env.OS_TEST_SHARD\nexport default { test: {} };\n' }); + if (!p.some((m) => m.includes('never reads OS_TEST_SHARD'))) { + throw new Error('slice wiring: a config naming the variable only in a comment was accepted'); + } + }); + check(() => { + const p = judge({ configSource: null, configFile: null }); + if (!p.some((m) => m.includes('no vitest config'))) { + throw new Error('slice wiring: a package with no vitest config was accepted'); + } + }); + check(() => { + const p = judge({ turbo: { tasks: { test: { env: ['OS_TEST_TIERS'] } } } }); + if (!p.some((m) => m.includes('tasks.test.env does not declare OS_TEST_SHARD'))) { + throw new Error('slice wiring: a generic `test` task without the variable was accepted'); + } + }); + check(() => { + // A `#test` entry REPLACES the generic task: declaring the + // variable on the generic one does not reach a package that has its own. + const p = judge({ + turbo: { tasks: { ...genericOnly.tasks, '@x/sliced#test': { env: ['OS_TEST_TIERS'] } } }, + }); + if (!p.some((m) => m.includes('tasks["@x/sliced#test"].env does not declare OS_TEST_SHARD'))) { + throw new Error('slice wiring: a package-specific `test` task without the variable was accepted'); + } + }); + check(() => { + const p = judge({ + turbo: { tasks: { test: { env: [] }, '@x/sliced#test': { env: ['OS_TEST_SHARD'] } } }, + }); + if (p.length !== 0) { + throw new Error(`slice wiring: a package-specific task that declares it was refused: ${p.join(' | ')}`); + } + }); + check(() => { + const p = judge({ packageTurboJson: true }); + if (!p.some((m) => m.includes('carries its own turbo.json'))) { + throw new Error('slice wiring: a package-level turbo.json this check does not model was accepted'); + } + }); + check(() => { + const { problems, judged } = sliceWiringProblems(REPO_ROOT, { '@objectstack/no-such-package': 2 }); + if (judged !== 0 || !problems.some((m) => m.includes('no workspace package carries that name'))) { + throw new Error('slice wiring: a sliced name with no workspace package was accepted'); + } + }); + // -- The floor: every declared battery RAN, and ran its cases (#13489) ---- // // Evaluated after every battery has had its chance and BEFORE the verdict, so @@ -1446,7 +1641,8 @@ function checkDrift(argv) { const merged = new Map(); // What the summaries say each package was RUN as. A shard that carries a // file-level slice writes two summaries -- one per turbo invocation -- and - // only the slice leg's tasks carry `--shard=k/n`, so this is per package and + // only the slice leg's tasks carry the slice (an `OS_TEST_SHARD` digest, or + // `--shard=k/n` on a passthrough run), so this is per package and // comes from the run rather than from FILE_SHARDED_PACKAGES. A package absent // here ran whole; that is a reading, not a default. const observedSlices = new Map(); diff --git a/scripts/report-test-timings.mjs b/scripts/report-test-timings.mjs index f8616573d0a..aac2a63a9cb 100644 --- a/scripts/report-test-timings.mjs +++ b/scripts/report-test-timings.mjs @@ -113,8 +113,10 @@ * - `samplesFromSummary` reads the `test` task only, so a package that also * has a `test:repo` task contributes its `test` seconds here. That is the * like-for-like comparison the pinned weights were measured in. - * - A SLICED package appears on two shards with a `--shard=k/n` passthrough, - * and each shard sees a PART. Parts are summed, and a package is labelled + * - A SLICED package appears on two shards, each running one `k/n` PART -- + * carried in `OS_TEST_SHARD` on Test Core (#19278; the summary holds its + * sha256, resolved by `samplesFromSummary`), as a `--shard=k/n` passthrough + * on the nightly tiers. Parts are summed, and a package is labelled * `slice k/n` and marked incomplete until every part is present. ⛔ A part's * seconds are never printed as the package's total. * - The pinned weights in `scripts/test-shard-timings.json` are READ ONLY @@ -263,9 +265,11 @@ export function parseFileTimings(rawText) { /** * Read every `.turbo/runs/*.json` in `dir`. A shard runs its whole-package leg * and each file-level slice as SEPARATE turbo invocations, so there is more - * than one summary per shard and their task sets differ (the slice leg runs - * with `--only`). Unreadable files become a stated problem, never a throw: - * this tool must not be able to fail the job it reports on. + * than one summary per shard and their task sets differ (the slice leg carries + * its slice in `OS_TEST_SHARD` and no passthrough, so it plans its own build + * closure beside the one test). Unreadable files -- a slice digest + * `samplesFromSummary` cannot resolve included -- become a stated problem, + * never a throw: this tool must not be able to fail the job it reports on. */ export function readSummaries(dir) { const packages = new Map(); diff --git a/turbo.json b/turbo.json index fb66762f0c2..0ddd48454ba 100644 --- a/turbo.json +++ b/turbo.json @@ -11,7 +11,7 @@ "test": { "dependsOn": ["^build"], "outputs": [], - "env": ["OS_TEST_TIERS"], + "env": ["OS_TEST_TIERS", "OS_TEST_SHARD"], "inputs": ["$TURBO_DEFAULT$", "!dist/**", "!coverage/**", "!.turbo/**"] }, "test:repo": { @@ -103,7 +103,7 @@ "@objectstack/cli#test": { "dependsOn": ["build"], "outputs": [], - "env": ["OS_TEST_TIERS"], + "env": ["OS_TEST_TIERS", "OS_TEST_SHARD"], "inputs": [ "$TURBO_DEFAULT$", "!dist/**",