From 170fe5fa0c2d658cf75fd56f1037223b677a5228 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Tue, 27 Oct 2026 07:40:10 +1100 Subject: [PATCH] ci: harden flake-check selection and Rust targeting Address the review of the flake check port (99b21e4fb). An integration-only Playwright selection no longer fails flake-select: Playwright exits 1 with a lone "No tests found" error when the file filters match nothing in the smoke project, which listPlaywright now reads as an empty selection, as buzz-app's listBrowser does. Listed journeys are deduplicated, since a looped test() declaration is listed once per iteration, and a selection over 250 journeys skips the Playwright lane with a note: the smoke lane runs ~150 journey-runs per shard in ~11 minutes, so 250 journey-runs per shard across ten shards is what fits the 45-minute timeout after setup and the E2E build. Rust entries now carry the file and the test's in-file module path (`crates/.../auth.rs::tests::name`). repeatRust runs one cargo invocation per package with target flags derived from the path (src/** builds --lib --bins, tests/.rs builds --test ), so a buzz-relay change no longer compiles every test target of the crate. It lists candidates by in-file path, prefers the test whose full name the file's path predicts, and falls back to every suffix match only when nothing matches (a `#[path]` module), so a same-named test in another module is no longer repeated for a change it never saw. The rust-cache bust knob moves into the shared key (`unit-tests-sherpa-cache-v1`) in both the Unit Tests and flake-rust jobs, since rust-cache ignores `key` once shared-key is set; the contract script checks both and the flake restore's save-if, and the regression harness covers both. flake-select installs desktop dependencies only when a spec changed. Changed .rs files without test functions get a summary row, test.fail() declarations count as tests, a captured nextest 0.9.136 list/JUnit pair pins the classname contract, and CONTRIBUTING lists what the lanes deliberately do not repeat. Co-Authored-By: Claude Fable 5.1 Signed-off-by: Matt Toohey --- .github/workflows/_ci-flake.yml | 30 +- .github/workflows/_ci-rust.yml | 7 +- CONTRIBUTING.md | 14 +- scripts/flake-check.mjs | 279 +++++++++++++----- scripts/flake-check.test.mjs | 172 ++++++++++- .../test-rust-cache-contract-regressions.sh | 21 +- scripts/test-rust-cache-contract.sh | 25 +- 7 files changed, 436 insertions(+), 112 deletions(-) diff --git a/.github/workflows/_ci-flake.yml b/.github/workflows/_ci-flake.yml index 2cd77f16ed..37f6d228e1 100644 --- a/.github/workflows/_ci-flake.yml +++ b/.github/workflows/_ci-flake.yml @@ -35,18 +35,30 @@ jobs: # diff is exactly the pull request's change without a full clone. fetch-depth: 2 - uses: cashapp/activate-hermit@cea9af7913204a965fd488637a8d1811bba2e616 # v1 + # Only a changed spec needs desktop dependencies: Playwright lists its + # smoke journeys and the desktop app's TypeScript parses it. Pull + # requests without one skip the install. + - name: Check for changed Playwright specs + id: specs + run: | + if git diff --quiet --diff-filter=AM HEAD^1 HEAD -- 'desktop/tests/e2e/*.spec.ts'; then + echo "changed=false" >> "$GITHUB_OUTPUT" + else + echo "changed=true" >> "$GITHUB_OUTPUT" + fi - name: Get pnpm store directory + if: steps.specs.outputs.changed == 'true' id: pnpm-cache run: echo "STORE_PATH=$(pnpm store path --silent)" >> "$GITHUB_OUTPUT" - name: Restore pnpm store cache + if: steps.specs.outputs.changed == 'true' uses: actions/cache/restore@caa296126883cff596d87d8935842f9db880ef25 # v5 with: path: ${{ steps.pnpm-cache.outputs.STORE_PATH }} key: pnpm-${{ runner.os }}-${{ hashFiles('**/pnpm-lock.yaml') }} restore-keys: pnpm-${{ runner.os }}- - # Playwright lists the smoke journeys of a changed spec, and the desktop - # app's TypeScript parses it. - name: Install desktop dependencies + if: steps.specs.outputs.changed == 'true' run: just desktop-install-ci - name: Map changed hunks to tests id: select @@ -64,13 +76,17 @@ jobs: - uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6.0.3 - uses: cashapp/activate-hermit@cea9af7913204a965fd488637a8d1811bba2e616 # v1 - uses: rui314/setup-mold@7e4f20ad28a2e8ca6fd0892ccf72e2abb706b9c3 # v1 - # Restores the Unit Tests lane's cache read-only: this lane only runs on - # pull requests, so it never saves one of its own. + # Restores the Unit Tests lane's cache (_ci-rust.yml) read-only; this + # lane never saves one of its own. The restore only hits while the cache + # key matches: the same shared-key, and rust-cache's environment hash — + # the CARGO*/RUST*/CC* environment variables, the lockfiles and the rustc + # version — must stay identical to the Unit Tests job's, so any env this + # job sets must be set there too (scripts/test-rust-cache-contract.sh + # checks the key). - uses: Swatinem/rust-cache@e18b497796c12c097a38f9edb9d0641fb99eee32 with: - shared-key: unit-tests - key: sherpa-cache-v1 - save-if: ${{ github.event_name != 'pull_request' }} + shared-key: unit-tests-sherpa-cache-v1 + save-if: false - name: Install cargo-nextest uses: taiki-e/install-action@0fd46367812ee04360509b4169d9f659d6892bb2 # v2.79.15 with: diff --git a/.github/workflows/_ci-rust.yml b/.github/workflows/_ci-rust.yml index fa60042f67..3190d877ae 100644 --- a/.github/workflows/_ci-rust.yml +++ b/.github/workflows/_ci-rust.yml @@ -58,9 +58,10 @@ jobs: - uses: rui314/setup-mold@7e4f20ad28a2e8ca6fd0892ccf72e2abb706b9c3 # v1 - uses: Swatinem/rust-cache@e18b497796c12c097a38f9edb9d0641fb99eee32 with: - # Named so the advisory flake lane (_ci-flake.yml) can restore it. - shared-key: unit-tests - key: sherpa-cache-v1 + # Named so the advisory flake lane (_ci-flake.yml) can restore it; + # with shared-key set, rust-cache ignores `key`, so the cache-bust + # suffix lives here. Bump it to discard every saved cache. + shared-key: unit-tests-sherpa-cache-v1 save-if: ${{ github.event_name != 'pull_request' }} - name: Install cargo-nextest uses: taiki-e/install-action@0fd46367812ee04360509b4169d9f659d6892bb2 # v2.79.15 diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index a5f3aa61f5..6d020a7f9a 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -310,10 +310,16 @@ select` maps the diff against the base branch to the Rust tests, desktop changed: a hunk inside a test body selects that test; a change anywhere else in a test file (imports, helpers, hooks, the code under test beside Rust tests) or a new file selects the whole file. The `Flake check (…)` lanes then -run that selection ten times with retries off, so a repetition that fails is -a race in the test or the code under it, worth fixing before it lands. The -lanes skip when no test changed and are not required to merge; their step -summaries show the test × iteration table. +run that selection ten times with retries off; a repetition that fails is +usually a race in the test or the code under it, worth fixing before it +lands, though a test that needs infrastructure or the regular lane's +cross-file concurrency can fail every repetition instead. Some changed tests +are deliberately not repeated: `#[ignore]`d and Postgres-backed Rust tests, +Playwright specs outside the `smoke` project, `*.perf.ts` measurements, and a +selection too large for its lane (over 64 KiB of entries, or more Playwright +journeys than ten repetitions can finish within the lane's timeout). +The lanes skip when no test changed and are not required to merge; their +step summaries show the selection and the test × iteration table. --- diff --git a/scripts/flake-check.mjs b/scripts/flake-check.mjs index b1c24ed895..e49d5c88e7 100644 --- a/scripts/flake-check.mjs +++ b/scripts/flake-check.mjs @@ -6,6 +6,7 @@ import { spawnSync } from "node:child_process"; import { appendFileSync, copyFileSync, + existsSync, mkdirSync, readFileSync, rmSync, @@ -16,18 +17,26 @@ import { fileURLToPath } from "node:url"; const root = fileURLToPath(new URL("../", import.meta.url)); export const ITERATIONS = 10; // Repetitions spread across shards, so a whole-file selection stays inside -// the Playwright lane's job timeout. Playwright shards at file × repetition -// granularity (workers: 1, no fullyParallel), and the smoke lane runs ~1,500 -// journeys over ten ~11-minute shards, so five journeys × ten repetitions is a -// few minutes of test time per shard. The regular smoke lane never uses more -// than ten runners, so neither does this one. +// the Playwright lane's 45-minute job timeout. Playwright shards at file × +// repetition granularity (workers: 1, no fullyParallel). The regular smoke +// lane runs ~150 journeys per shard in ~11 minutes, about 4.5 s each, so five +// journeys × ten repetitions is a few minutes of test time per shard; it never +// uses more than ten runners, so neither does this one. Past ten shards each +// one grows instead, and the timeout minus dependency setup and the E2E build +// (~10 minutes, more when apt retries) leaves room for about 250 journey-runs +// (~19 minutes) per shard, so a selection beyond that many journeys skips the +// lane with a note rather than timing out. const JOURNEYS_PER_SHARD = 5; const MAX_SHARDS = 10; +const MAX_JOURNEY_RUNS_PER_SHARD = 250; export const playwrightShards = (journeys) => Math.min(MAX_SHARDS, Math.max(1, Math.ceil(journeys / JOURNEYS_PER_SHARD))); -// GitHub caps a job's outputs at 1 MiB in total. A selection this large is a -// sweeping refactor, not a test change worth ten repetitions, so the lane -// skips rather than overrunning its timeout. +export const MAX_PLAYWRIGHT_JOURNEYS = Math.floor( + (MAX_JOURNEY_RUNS_PER_SHARD * MAX_SHARDS) / ITERATIONS, +); +// GitHub caps a job's outputs at 1 MiB in total. A selection this large, like +// one past the journey cap, is a sweeping refactor rather than a test change +// worth ten repetitions, so the lane skips rather than overrunning its timeout. export const MAX_OUTPUT_BYTES = 64 * 1024; export const runners = ["rust", "node", "playwright", "dart"]; // Mirrors desktop/package.json "test": plain helpers run under the TS loader; @@ -55,7 +64,9 @@ const infraTests = // Mirrors desktop/package.json "test", desktop/playwright.config.ts testDir, // the root Cargo workspace's crates and mobile's test directory. Paths are -// later split on whitespace in a shell, so admit only plain names. +// later split on whitespace in a shell, so admit only plain names. The +// desktop `*.perf.ts` measurements are deliberately not specs here: they time +// the app rather than assert on it, so a slow repetition is not a flake. export function classify(file) { if (!/^[\w./@-]+$/.test(file)) return undefined; if (/^crates\/[^/]+\/(?:src|tests)\/(?!fixtures\/).*\.rs$/.test(file)) @@ -95,6 +106,7 @@ const declaring = [ "only", "skip", "fixme", + "fail", "fails", "todo", "concurrent", @@ -226,10 +238,18 @@ const matching = (text, open, close, from) => { // Every test function in a Rust file: a `fn` whose attribute block has an // attribute path ending in `test` (`#[test]`, `#[tokio::test]`, …), spanning -// the first attribute line to the body's closing brace. `ignored` marks -// `#[ignore]`, which nextest skips by default. +// the first attribute line to the body's closing brace. `name` is the path +// within the file through its inline modules (`tests::plain`), so a namesake +// in another module stays a different test. `ignored` marks `#[ignore]`, +// which nextest skips by default. export function rustTests(source) { const text = blankRust(source); + const modules = []; + const inline = /\bmod\s+(\w+)\s*\{/g; + for (let m = inline.exec(text); m; m = inline.exec(text)) { + const open = m.index + m[0].length - 1; + modules.push({ name: m[1], open, close: matching(text, "{", "}", open) }); + } const lineStarts = [0]; for (let i = 0; i < text.length; i += 1) if (text[i] === "\n") lineStarts.push(i + 1); @@ -263,8 +283,11 @@ export function rustTests(source) { if (!fn) continue; const open = text.indexOf("{", j + fn[0].length); if (open === -1) continue; + const path = modules + .filter((mod) => mod.open < match.index && match.index < mod.close) + .map((mod) => mod.name); tests.push({ - name: fn[1], + name: [...path, fn[1]].join("::"), start: lineAt(match.index), end: lineAt(matching(text, "{", "}", open)), ignored: paths.includes("ignore"), @@ -371,17 +394,50 @@ const summarize = (text) => { }; const cell = (value) => String(value).replaceAll("|", "\\|"); +const crateDir = (file) => `${root}crates/${file.split("/")[1]}`; + // The `[package]` name of the crate owning a file under crates/. export function cratePackage(file) { - const manifest = readFileSync( - `${root}crates/${file.split("/")[1]}/Cargo.toml`, - "utf8", - ); + const manifest = readFileSync(`${crateDir(file)}/Cargo.toml`, "utf8"); return /^\[package\][\s\S]*?^name\s*=\s*"([^"]+)"/m.exec(manifest)[1]; } -// Smoke journeys per spec file as Playwright lists them. Integration-only -// specs list nothing for the smoke project, so a selection of them stays empty. +// Cargo target flags building only the test binaries a file can be part of: +// src/** is the lib (when the crate has one) or a bin, tests/.rs and +// tests//*.rs (with a main.rs) the integration test of that name. A +// file placed by a custom `[[test]]` path builds every target instead. +export function rustTarget(file) { + const [, , kind, first, second] = file.split("/"); + if (kind === "src") { + const lib = + existsSync(`${crateDir(file)}/src/lib.rs`) || + /^\[lib\]/m.test(readFileSync(`${crateDir(file)}/Cargo.toml`, "utf8")); + return lib ? ["--lib", "--bins"] : ["--bins"]; + } + if (second === undefined) return ["--test", first.replace(/\.rs$/, "")]; + return existsSync(`${crateDir(file)}/tests/${first}/main.rs`) + ? ["--test", first] + : []; +} + +// The module path a file's items carry in their binary under cargo's layout +// conventions: src/a/b.rs is a::b, a mod.rs/main.rs/lib.rs names its directory +// or the root, src/bin/x.rs and tests/.rs are roots, tests//x.rs +// is x. A `#[path]` attribute can break this; repeatRust then falls back to the +// in-file path alone. +export function rustModulePath(file) { + const [, , kind, ...parts] = file.replace(/\.rs$/, "").split("/"); + if (kind === "tests") parts.shift(); + else if (parts[0] === "bin") parts.splice(0, 2); + if (["mod", "main"].includes(parts.at(-1)) || parts.join() === "lib") + parts.pop(); + return parts; +} + +// Smoke journeys per spec file as Playwright lists them, each line once (a +// looped declaration is listed once per iteration). Integration-only specs +// list nothing for the smoke project, which Playwright reports as its "No +// tests found" error, so a selection of them stays empty. export function listPlaywright(files) { const result = run(`${root}bin/pnpm`, [ "-C", @@ -396,21 +452,24 @@ export function listPlaywright(files) { ...files, ]); const report = JSON.parse(result.stdout); - if (result.status !== 0 || report.errors.length) + const empty = + report.errors.length === 1 && + /No tests found/.test(report.errors[0].message); + if ((result.status !== 0 || report.errors.length) && !empty) throw new Error( `playwright --list: ${report.errors.map((error) => error.message).join("\n") || result.stderr}`, ); - const specs = new Map(files.map((file) => [file, []])); + const specs = new Map(files.map((file) => [file, new Set()])); const collect = (suites) => { for (const suite of suites) { for (const spec of suite.specs ?? []) if (spec.tests?.length) - specs.get(`desktop/tests/e2e/${spec.file}`)?.push(spec.line); + specs.get(`desktop/tests/e2e/${spec.file}`)?.add(spec.line); collect(suite.suites ?? []); } }; - collect(report.suites); - return specs; + collect(empty ? [] : report.suites); + return new Map([...specs].map(([file, lines]) => [file, [...lines]])); } function select(base = "HEAD^1", head = "HEAD") { @@ -454,9 +513,11 @@ function select(base = "HEAD^1", head = "HEAD") { previous: previous(file), }, ); - if (!selection) continue; - const crate = cratePackage(file); - entries = selection.names.map((name) => `${crate}::${name}`); + if (!selection) { + rows.push(`| ${cell(file)} | ${runner} | no test functions |`); + continue; + } + entries = selection.names.map((name) => `${file}::${name}`); note = selection.names.length === 0 ? "excluded (ignored tests only)" @@ -486,9 +547,16 @@ function select(base = "HEAD^1", head = "HEAD") { outputs[runner].push(...entries); rows.push(`| ${cell(file)} | ${runner} | ${cell(note)} |`); } - if (Buffer.byteLength(outputs[runner].join(" ")) > MAX_OUTPUT_BYTES) { + const exceeded = + Buffer.byteLength(outputs[runner].join(" ")) > MAX_OUTPUT_BYTES + ? `selection exceeds ${MAX_OUTPUT_BYTES} bytes` + : runner === "playwright" && + outputs[runner].length > MAX_PLAYWRIGHT_JOURNEYS + ? `selection exceeds ${MAX_PLAYWRIGHT_JOURNEYS} journeys, which ${ITERATIONS} repetitions would not finish in time` + : undefined; + if (exceeded) { rows.push( - `| *(${outputs[runner].length} ${runner} entries)* | ${runner} | selection exceeds ${MAX_OUTPUT_BYTES} bytes; lane skipped |`, + `| *(${outputs[runner].length} ${runner} entries)* | ${runner} | ${exceeded}; lane skipped |`, ); outputs[runner] = []; } @@ -587,45 +655,81 @@ export function parseJunit(xml) { return statuses; } -// nextest has no repeat flag, so run the filterset once per iteration under -// the `flake` profile (.config/nextest.toml: no retries, JUnit per run). The -// selection is listed first: a filterset matching no runnable test is a -// selection of ignored or infrastructure-backed tests, not a failure. -function repeatRust(entries, iterations) { - const byPackage = new Map(); - for (const entry of entries) { - const [, crate, name] = /^(.+)::(\w+)$/.exec(entry) ?? []; - if (!crate) - throw new Error(`rust entry must be ::: ${entry}`); - byPackage.set(crate, (byPackage.get(crate) ?? new Set()).add(name)); +// The listed tests an entry stands for: a test the file's module path predicts +// exactly when nextest lists one, otherwise every test whose name ends in the +// in-file path (a `#[path]` module breaks the prediction; a namesake in another +// module is then repeated too rather than nothing). +export function chooseRust(tests, candidates) { + const chosen = new Map(); + for (const { path, exact } of tests) { + const suffix = candidates.filter( + ({ name }) => name === path || name.endsWith(`::${path}`), + ); + const predicted = suffix.filter(({ name }) => name === exact); + for (const test of predicted.length ? predicted : suffix) + chosen.set(`${test.binary} ${test.name}`, test); } - const filterset = `(${[...byPackage] - .map( - ([crate, names]) => - `(package(=${crate}) & (${[...names].map((name) => `test(/(^|::)${name}$/)`).join(" | ")}))`, - ) - .join(" | ")}) & !(${infraTests})`; - const packages = [...byPackage.keys()].flatMap((crate) => ["-p", crate]); - const listed = run("cargo", [ - "nextest", - "list", - ...packages, - "-E", - filterset, - "--message-format", - "json", - ]); - if (listed.status !== 0) - throw new Error(`cargo nextest list: ${listed.stderr}`); - const expected = Object.entries( - JSON.parse(listed.stdout)["rust-suites"], - ).flatMap(([binary, suite]) => - Object.entries(suite.testcases) - .filter( - ([, test]) => - !test.ignored && test["filter-match"].status === "matches", - ) - .map(([name]) => `${binary} ${name}`), + return [...chosen.values()]; +} + +// nextest has no repeat flag, so run the selection once per iteration under +// the `flake` profile (.config/nextest.toml: no retries, JUnit per run), one +// cargo invocation per package so each builds only the targets its files can +// be in. The selection is listed first, by in-file path, and narrowed with +// chooseRust to exact (binary, test) pairs; a selection listing no runnable +// test is one of ignored or infrastructure-backed tests, not a failure. +function repeatRust(entries, iterations) { + const groups = new Map(); + for (const entry of entries) { + const [, file, path] = + /^(crates\/[^:]+\.rs)::((?:\w+::)*\w+)$/.exec(entry) ?? []; + if (!file) + throw new Error(`rust entry must be ::: ${entry}`); + const crate = cratePackage(file); + const group = groups.get(crate) ?? { targets: new Set(), tests: [] }; + const targets = rustTarget(file); + if (targets.length === 0) group.every = true; + for (const target of targets) group.targets.add(target); + group.tests.push({ + path, + exact: [...rustModulePath(file), path].join("::"), + }); + groups.set(crate, group); + } + const runs = []; + for (const [crate, group] of groups) { + group.args = ["-p", crate, ...(group.every ? [] : group.targets)]; + const suffixes = [...new Set(group.tests.map(({ path }) => path))] + .map((path) => `test(/(^|::)${path}$/)`) + .join(" | "); + const listed = run("cargo", [ + "nextest", + "list", + ...group.args, + "-E", + `(${suffixes}) & !(${infraTests})`, + "--message-format", + "json", + ]); + if (listed.status !== 0) + throw new Error(`cargo nextest list: ${listed.stderr}`); + const candidates = Object.entries( + JSON.parse(listed.stdout)["rust-suites"], + ).flatMap(([binary, suite]) => + Object.entries(suite.testcases) + .filter( + ([, test]) => + !test.ignored && test["filter-match"].status === "matches", + ) + .map(([name]) => ({ binary, name })), + ); + group.chosen = chooseRust(group.tests, candidates); + group.filterset = group.chosen + .map(({ binary, name }) => `(binary_id(=${binary}) & test(=${name}))`) + .join(" | "); + } + const expected = [...groups.values()].flatMap((group) => + group.chosen.map(({ binary, name }) => `${binary} ${name}`), ); if (expected.length === 0) { summarize( @@ -637,24 +741,35 @@ function repeatRust(entries, iterations) { // CARGO_TARGET_DIR; the `flake` profile writes its JUnit report there. const junit = `${root}target/nextest/flake/junit.xml`; const dir = resultsDir("rust"); - const runs = []; for (let i = 1; i <= iterations; i += 1) { - rmSync(junit, { force: true }); - const result = run( - "cargo", - ["nextest", "run", "--profile", "flake", ...packages, "-E", filterset], - { stdio: "inherit" }, - ); - let statuses = new Map(); - try { - statuses = parseJunit(readFileSync(junit, "utf8")); - copyFileSync(junit, `${dir}/iter-${i}.xml`); - } catch { - statuses = new Map(); + const statuses = new Map(); + let ok = true; + for (const [crate, group] of groups) { + if (group.chosen.length === 0) continue; + rmSync(junit, { force: true }); + const result = run( + "cargo", + [ + "nextest", + "run", + "--profile", + "flake", + ...group.args, + "-E", + group.filterset, + ], + { stdio: "inherit" }, + ); + ok &&= result.status === 0; + try { + for (const [test, status] of parseJunit(readFileSync(junit, "utf8"))) + statuses.set(test, status); + copyFileSync(junit, `${dir}/iter-${i}-${crate}.xml`); + } catch { + ok = false; + } } - const ok = - result.status === 0 && - expected.every((test) => statuses.get(test) === "passed"); + ok &&= expected.every((test) => statuses.get(test) === "passed"); runs.push({ ok, statuses }); progress(i, iterations, ok); } diff --git a/scripts/flake-check.test.mjs b/scripts/flake-check.test.mjs index 876675b973..21b4006ca2 100644 --- a/scripts/flake-check.test.mjs +++ b/scripts/flake-check.test.mjs @@ -2,16 +2,21 @@ import assert from "node:assert/strict"; import { readFileSync } from "node:fs"; import test from "node:test"; import { + chooseRust, classify, cratePackage, dartDefine, hunkRanges, + ITERATIONS, listPlaywright, + MAX_PLAYWRIGHT_JOURNEYS, nodeTestArgs, parseDartReport, parseJunit, playwrightRepetitions, playwrightShards, + rustModulePath, + rustTarget, rustTests, selectRust, selectTests, @@ -108,12 +113,16 @@ test("Playwright test calls span the declaration; groups, hooks and configuratio " }", "});", 'test.describe.configure({ mode: "serial" });', + 'test.fail("known broken", async () => {', + " await Promise.resolve();", + "});", "", ].join("\n"); const calls = testCalls(spec, "x.spec.ts"); assert.deepEqual(calls, [ { start: 8, end: 11 }, { start: 13, end: 15 }, + { start: 19, end: 21 }, ]); // Playwright lists a looped declaration once per iteration, at one line. assert.deepEqual(testBodies(calls, [13, 13, 8]), [ @@ -170,22 +179,104 @@ const rustSource = [ "", ].join("\n"); -test("Rust test functions span their attribute block to the closing brace, through strings, chars and comments", () => { +test("Rust test functions span their attribute block to the closing brace, through strings, chars and comments, named by their inline module path", () => { assert.deepEqual(rustTests(rustSource), [ - { name: "plain", start: 19, end: 22, ignored: false }, - { name: "needs_infra", start: 24, end: 30, ignored: true }, - { name: "also_ignored", start: 32, end: 34, ignored: true }, - { name: "in_nested_module", start: 37, end: 40, ignored: false }, + { name: "tests::plain", start: 19, end: 22, ignored: false }, + { name: "tests::needs_infra", start: 24, end: 30, ignored: true }, + { name: "tests::also_ignored", start: 32, end: 34, ignored: true }, + { + name: "tests::postgres_tests::in_nested_module", + start: 37, + end: 40, + ignored: false, + }, ]); assert.deepEqual(rustTests("fn main() {}\n"), []); }); +test("same-named Rust tests in different modules are distinct, and a hunk selects only its own", () => { + const source = [ + "mod alpha {", + " #[test]", + " fn same() {}", + "}", + "pub(crate) mod beta {", + " mod tests {", + " #[test]", + " fn same() { let _ = 1; }", + " }", + "}", + "", + ].join("\n"); + assert.deepEqual( + rustTests(source).map((test) => test.name), + ["alpha::same", "beta::tests::same"], + ); + assert.deepEqual( + selectRust(diff([8, 1, 8, 1]), source, { previous: source }), + { + lines: [7], + names: ["beta::tests::same"], + }, + ); + // The file's path predicts the rest of the name; cargo's layout rules. + for (const [file, modules] of [ + ["crates/buzz-relay/src/lib.rs", []], + ["crates/buzz-relay/src/main.rs", []], + ["crates/buzz-relay/src/api/mod.rs", ["api"]], + ["crates/buzz-relay/src/handlers/auth.rs", ["handlers", "auth"]], + ["crates/buzz-test-client/src/bin/mention.rs", []], + ["crates/buzz-test-client/tests/e2e_relay.rs", []], + ["crates/x/tests/suite/main.rs", []], + ["crates/x/tests/suite/helpers.rs", ["helpers"]], + ]) + assert.deepEqual(rustModulePath(file), modules, file); + // Listed candidates: the predicted name wins; without it every suffix + // match is kept rather than nothing. + const candidates = [ + { binary: "buzz-relay", name: "api::relay_members::tests::same" }, + { binary: "buzz-relay", name: "handlers::auth::tests::same" }, + { binary: "buzz-relay", name: "handlers::auth::tests::other" }, + { + binary: "buzz-relay::bin/buzz-relay", + name: "handlers::auth::tests::same", + }, + ]; + assert.deepEqual( + chooseRust( + [{ path: "tests::same", exact: "handlers::auth::tests::same" }], + candidates, + ), + [candidates[1], candidates[3]], + ); + assert.deepEqual( + chooseRust( + [ + { path: "tests::same", exact: "elsewhere::tests::same" }, + { path: "tests::other", exact: "handlers::auth::tests::other" }, + ], + candidates, + ), + [candidates[0], candidates[1], candidates[3], candidates[2]], + ); +}); + +test("Rust files build only the targets they can be part of", () => { + for (const [file, flags] of [ + ["crates/buzz-relay/src/api/mod.rs", ["--lib", "--bins"]], + ["crates/buzz-admin/src/main.rs", ["--bins"]], + ["crates/buzz-test-client/tests/e2e_relay.rs", ["--test", "e2e_relay"]], + ["crates/buzz-agent/tests/common/mod.rs", []], + ]) + assert.deepEqual(rustTarget(file), flags, file); +}); + test("Rust selection: a hunk inside a test selects it, elsewhere selects every runnable test; ignored tests never run", () => { const select = (text, options) => selectRust(text, rustSource, { previous: rustSource, ...options }); assert.deepEqual(select(diff([21, 1, 21, 1])), { lines: [19], - names: ["plain"], + names: ["tests::plain"], }); // Inside an ignored test: owned, so not shared code, but nothing to run. assert.deepEqual(select(diff([28, 0, 29, 1])), { lines: [24], names: [] }); @@ -194,7 +285,7 @@ test("Rust selection: a hunk inside a test selects it, elsewhere selects every r const whole = (reason) => ({ whole: true, reason, - names: ["plain", "in_nested_module"], + names: ["tests::plain", "tests::postgres_tests::in_nested_module"], }); assert.deepEqual(select(diff([4, 0, 5, 1])), whole("shared code changed")); assert.deepEqual( @@ -281,25 +372,84 @@ test("hunks inside tests select those tests; shared code, new or unlisted files ); }); -test("Playwright lists smoke journeys per spec; integration-only specs list none", () => { +test("Playwright lists smoke journeys per spec once each; integration-only specs list none", () => { const smoke = "desktop/tests/e2e/channel-sort.spec.ts"; + const looped = "desktop/tests/e2e/claude-effort-picker.spec.ts"; const integration = "desktop/tests/e2e/agents.spec.ts"; - const listed = listPlaywright([smoke, integration]); + // Alone, an integration spec makes Playwright exit 1 with "No tests found". + assert.deepEqual(listPlaywright([integration]), new Map([[integration, []]])); + const listed = listPlaywright([smoke, looped, integration]); assert.deepEqual(listed.get(integration), []); const lines = listed.get(smoke); assert.ok(lines.length > 0, "a smoke spec must list its journeys"); assert.equal( testBodies(testCalls(read(`../${smoke}`), smoke), lines).length, - new Set(lines).size, + lines.length, "every listed journey must be owned by a test call", ); + // A looped declaration is listed once per iteration at the same line. + const loopedLines = listed.get(looped); + assert.deepEqual(loopedLines, [...new Set(loopedLines)]); + assert.ok( + /^\s*for \(const .+\) \{\n\s+test\(/m.test(read(`../${looped}`)), + "the looped spec must still declare a test inside a loop", + ); }); -test("Playwright repetitions get one shard per five journeys, at most ten", () => { +test("Playwright repetitions get one shard per five journeys, at most ten, and a selection past ~19 minutes per shard skips", () => { assert.deepEqual( [0, 1, 5, 6, 18, 40, 41, 50, 51, 200].map(playwrightShards), [1, 1, 1, 2, 4, 8, 9, 10, 10, 10], ); + // 250 journey-runs per shard at ~4.5 s each, over ten shards. + assert.equal(MAX_PLAYWRIGHT_JOURNEYS, 250); + assert.equal(ITERATIONS, 10); +}); + +test("nextest's JUnit classname is the binary id its list reports", () => { + // Captured from cargo-nextest 0.9.136 on one buzz-core test: `cargo nextest + // list -p buzz-core --lib -E 'test(=kind::tests::no_duplicate_kind_values)' + // --message-format json` (trimmed to the matching testcase) and the + // target/nextest/flake/junit.xml of `cargo nextest run --profile flake` with + // the same arguments. repeatRust keys expectations as ` ` + // from the list and reads statuses as ` ` from the report, + // so the two must agree. + const list = { + "rust-suites": { + "buzz-core": { + "package-name": "buzz-core", + "binary-id": "buzz-core", + "binary-name": "buzz_core", + kind: "lib", + status: "listed", + testcases: { + "kind::tests::no_duplicate_kind_values": { + kind: "test", + ignored: false, + "filter-match": { status: "matches" }, + }, + }, + }, + }, + }; + const junit = [ + '', + '', + ' ', + ' ', + " ", + " ", + "", + "", + ].join("\n"); + const expected = Object.entries(list["rust-suites"]).flatMap( + ([binary, suite]) => + Object.keys(suite.testcases).map((name) => `${binary} ${name}`), + ); + assert.deepEqual( + parseJunit(junit), + new Map(expected.map((test) => [test, "passed"])), + ); }); test("reports give per-test statuses per iteration", () => { diff --git a/scripts/test-rust-cache-contract-regressions.sh b/scripts/test-rust-cache-contract-regressions.sh index 0275b87341..a184c3d745 100755 --- a/scripts/test-rust-cache-contract-regressions.sh +++ b/scripts/test-rust-cache-contract-regressions.sh @@ -139,13 +139,28 @@ YAML run_contract >/dev/null rm -rf "$tmp/.github/actions" "$tmp/.github/workflows/new-cache-user.yaml" +cp "$root/.github/workflows/_ci-flake.yml" "$tmp/flake.orig" +python3 - "$tmp/.github/workflows/_ci-flake.yml" <<'PY' +import pathlib +import sys + +path = pathlib.Path(sys.argv[1]) +workflow = path.read_text() +needle = " shared-key: unit-tests-sherpa-cache-v1\n save-if: false\n" +if workflow.count(needle) != 1: + raise SystemExit("expected one flake-rust cache restore key") +path.write_text(workflow.replace(needle, " shared-key: unit-tests\n save-if: false\n")) +PY +expect_failure 'flake-rust rust-cache action must restore with.shared-key unit-tests-sherpa-cache-v1' +mv "$tmp/flake.orig" "$tmp/.github/workflows/_ci-flake.yml" + python3 - "$tmp/.github/workflows/_ci-rust.yml" <<'PY' import pathlib import sys path = pathlib.Path(sys.argv[1]) workflow = path.read_text() -needle = " key: sherpa-cache-v1\n save-if:" +needle = " shared-key: unit-tests-sherpa-cache-v1\n save-if:" replacement = " save-if:" if workflow.count(needle) != 1: raise SystemExit("expected one Unit Tests cache generation key") @@ -153,9 +168,9 @@ workflow = workflow.replace(needle, replacement) job = " unit-tests:\n" if workflow.count(job) != 1: raise SystemExit("expected one Unit Tests job") -workflow = workflow.replace(job, job + " env:\n key: sherpa-cache-v1\n", 1) +workflow = workflow.replace(job, job + " env:\n shared-key: unit-tests-sherpa-cache-v1\n", 1) path.write_text(workflow) PY -expect_failure 'rust-cache action must keep with.key set to sherpa-cache-v1' +expect_failure 'rust-cache action must keep with.shared-key set to unit-tests-sherpa-cache-v1' echo "rust cache contract regressions passed" diff --git a/scripts/test-rust-cache-contract.sh b/scripts/test-rust-cache-contract.sh index cf69c8aadc..18cc7c8c86 100755 --- a/scripts/test-rust-cache-contract.sh +++ b/scripts/test-rust-cache-contract.sh @@ -104,9 +104,30 @@ cache_steps = steps.select do |step| end abort "Unit Tests must contain exactly one rust-cache action step" unless cache_steps.length == 1 +# With shared-key set, rust-cache ignores `key`, so the cache-bust suffix +# lives in the shared key; the advisory flake lane restores the same cache +# and must name it identically. +shared_key = "unit-tests-sherpa-cache-v1" cache_with = cache_steps.first["with"] -unless cache_with.is_a?(Hash) && cache_with["key"] == "sherpa-cache-v1" - abort "Unit Tests rust-cache action must keep with.key set to sherpa-cache-v1" +unless cache_with.is_a?(Hash) && cache_with["shared-key"] == shared_key && !cache_with.key?("key") + abort "Unit Tests rust-cache action must keep with.shared-key set to #{shared_key} and no with.key" +end + +flake_ci_path = root / ".github" / "workflows" / "_ci-flake.yml" +flake_ci = workflows.fetch(flake_ci_path) { load_workflow.call(flake_ci_path) } +flake_jobs = flake_ci["jobs"] +flake_rust = flake_jobs.is_a?(Hash) ? flake_jobs["flake-rust"] : nil +abort "flake-rust job missing" unless flake_rust.is_a?(Hash) +flake_steps = flake_rust["steps"] +abort "flake-rust steps missing" unless flake_steps.is_a?(Array) +flake_cache_steps = flake_steps.select do |step| + step.is_a?(Hash) && step["uses"].is_a?(String) && + step["uses"].match?(/\ASwatinem\/rust-cache@/i) +end +abort "flake-rust must contain exactly one rust-cache action step" unless flake_cache_steps.length == 1 +flake_with = flake_cache_steps.first["with"] +unless flake_with.is_a?(Hash) && flake_with["shared-key"] == shared_key && flake_with["save-if"] == false + abort "flake-rust rust-cache action must restore with.shared-key #{shared_key} with save-if false" end renovate = JSON.parse(Pathname(renovate_name).binread.force_encoding("UTF-8"))