diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 95260d2ed5..b0c73556f4 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -309,9 +309,11 @@ select` maps the diff against the base branch to the Rust tests (in the root and desktop Tauri workspaces), desktop `node --test` files, Playwright smoke journeys and Flutter test files it 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, -and a change to a helper module under a crate's `tests/` selects every -integration test binary that includes it. The `Flake check (…)` lanes then +the code under test beside Rust tests) or a new file selects the whole file. +An integration test binary's tests include those of the file modules its +root declares (`mod x;`, `#[path = "…"] mod x;`), and a change to a helper +module under a crate's `tests/` selects every integration test binary that +includes it. The `Flake check (…)` lanes then 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 diff --git a/scripts/flake-check.mjs b/scripts/flake-check.mjs index 224296c4c0..c9ef8a52cb 100644 --- a/scripts/flake-check.mjs +++ b/scripts/flake-check.mjs @@ -402,23 +402,28 @@ export function selectTests( // Rust tests a diff selects: hunks inside a test body select that test, any // other hunk selects every test in the file (the module's code under test -// lives beside them), and `#[ignore]`d tests own their lines but never run. -// A file without test functions selects nothing of its own; `select` decides -// what that means by where the file lives. +// lives beside them) and, for an integration-test root, every test of the +// file modules it declares (`included`: they are compiled into the same +// binary, so a root with no test of its own still selects them). `#[ignore]`d +// tests own their lines but never run. A file with no tests at all selects +// nothing of its own; `select` decides what that means by where it lives. export function selectRust( diff, source, - { added = false, previous = "" } = {}, + { added = false, previous = "", included = [] } = {}, ) { const tests = rustTests(source); - if (tests.length === 0) return undefined; - const selection = selectTests( - hunkRanges(diff), - tests.map((test) => ({ ...test, line: test.start })), - { added, previous: added ? [] : rustTests(previous) }, - ); + if (tests.length === 0 && included.length === 0) return undefined; + const selection = + tests.length === 0 + ? { whole: true, reason: added ? "new file" : "shared code changed" } + : selectTests( + hunkRanges(diff), + tests.map((test) => ({ ...test, line: test.start })), + { added, previous: added ? [] : rustTests(previous) }, + ); const chosen = selection.whole - ? tests + ? [...tests, ...included] : tests.filter((test) => selection.lines.includes(test.start)); return { ...selection, @@ -545,6 +550,65 @@ export function rustImporters(helper) { }; } +// Whether a tests/** file is the root of an integration-test binary +// (tests/.rs or tests//main.rs) rather than a module of one. +export function rustTargetRoot(file) { + const place = rustFile(file); + if (place?.kind !== "tests") return false; + const { parts } = place; + return parts.length === 1 || (parts.length === 2 && parts[1] === "main"); +} + +// The file modules a Rust file declares, as cargo compiles them into its +// binary: `#[path = "…"] mod x;` names a file relative to the declaring +// file's directory; a plain `mod x;` is x.rs or x/mod.rs under the module's +// directory, the file's own for a root (lib.rs, main.rs, mod.rs, a +// tests/.rs) and // otherwise. Declarations are read with +// comments and strings blanked; `#[path]` strings raw, since blanking erases +// them. A module no file backs is left out. +export function rustModules(file, source) { + const dir = path.dirname(file); + const stem = path.basename(file, ".rs"); + const owned = + ["lib", "main", "mod"].includes(stem) || rustTargetRoot(file) + ? dir + : path.join(dir, stem); + const pathed = new Map(); + for (const [, named, name] of source.matchAll( + /#\[\s*path\s*=\s*"([^"]+)"\s*\]\s*(?:#\[[^\]]*\]\s*)*(?:pub(?:\([^)]*\))?\s+)?mod\s+(\w+)\s*;/g, + )) + pathed.set(name, path.normalize(path.join(dir, named))); + const modules = []; + for (const [, name] of blankRust(source).matchAll( + /\b(?:pub(?:\([^)]*\))?\s+)?mod\s+(\w+)\s*;/g, + )) { + const found = ( + pathed.has(name) + ? [pathed.get(name)] + : [`${owned}/${name}.rs`, `${owned}/${name}/mod.rs`] + ).find((candidate) => existsSync(`${root}${candidate}`)); + if (found) modules.push({ name, file: found }); + } + return modules; +} + +// The tests a file compiles in from the file modules it declares, +// transitively, named by their module path from it +// (`pool_lifecycle::tests::name` for `#[path = "../src/pool_lifecycle.rs"] +// mod pool_lifecycle;`), which is how nextest lists them for the binary. An +// integration-test root with no test function of its own (a wrapper around a +// source module, as buzz-acp's pool_lifecycle_state) has only these. +export function rustIncludedTests(file, source, seen = new Set([file])) { + return rustModules(file, source).flatMap(({ name, file: module }) => { + if (seen.has(module)) return []; + seen.add(module); + const text = readFileSync(`${root}${module}`, "utf8"); + return [...rustTests(text), ...rustIncludedTests(module, text, seen)].map( + (test) => ({ ...test, name: `${name}::${test.name}` }), + ); + }); +} + // 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 @@ -611,24 +675,28 @@ function select(base = "HEAD^1", head = "HEAD") { added.has(file) ? "" : git("show", `${base}:${file}`); const diff = (file) => git("diff", "-U0", "--no-color", base, head, "--", file); + // The tests an integration-test root compiles in from its file modules; + // any other file's tests are its own. + const includedRust = (file, source) => + rustTargetRoot(file) ? rustIncludedTests(file, source) : []; // Every runnable test of a Rust file, as entries. - const wholeRust = (file) => - rustTests(readFileSync(`${root}${file}`, "utf8")) + const wholeRust = (file) => { + const source = readFileSync(`${root}${file}`, "utf8"); + return [...rustTests(source), ...includedRust(file, source)] .filter((test) => !test.ignored) .map((test) => `${file}::${test.name}`); + }; for (const runner of runners) { for (const file of files[runner]) { let entries = [file]; let note = "whole file"; if (rustWorkspaces[runner]) { - const selection = selectRust( - diff(file), - readFileSync(`${root}${file}`, "utf8"), - { - added: added.has(file), - previous: previous(file), - }, - ); + const source = readFileSync(`${root}${file}`, "utf8"); + const selection = selectRust(diff(file), source, { + added: added.has(file), + previous: previous(file), + included: includedRust(file, source), + }); if (selection) { entries = selection.names.map((name) => `${file}::${name}`); note = @@ -637,12 +705,20 @@ function select(base = "HEAD^1", head = "HEAD") { : selection.whole ? `whole file (${selection.reason}): ${selection.names.length} test(s)` : `${selection.names.length} changed test(s)`; + } else if (rustTargetRoot(file)) { + // An integration-test root with no test of its own and none in + // the modules it declares: a binary with nothing to repeat. + rows.push( + `| ${cell(file)} | ${runner} | integration test with no tests |`, + ); + continue; } else if (rustFile(file).kind === "tests") { - // A tests/** file without test functions is a helper module that - // integration binaries compile in (`mod common;`), so a change to - // it can change every test of each binary that includes it: select - // those binaries whole. The ignored and infrastructure exclusions - // still apply downstream, as for any whole-file selection. + // A tests/** file that is not a binary's root and has no test + // functions is a helper module that integration binaries compile + // in (`mod common;`), so a change to it can change every test of + // each binary that includes it: select those binaries whole. The + // ignored and infrastructure exclusions still apply downstream, as + // for any whole-file selection. const { importers, scanned } = rustImporters(file); entries = importers.flatMap(wholeRust); const names = importers.map( diff --git a/scripts/flake-check.test.mjs b/scripts/flake-check.test.mjs index 439b76d066..4f5f226c32 100644 --- a/scripts/flake-check.test.mjs +++ b/scripts/flake-check.test.mjs @@ -23,9 +23,12 @@ import { rustFilterset, rustGroups, rustImporters, + rustIncludedTests, rustIncludes, rustModulePath, + rustModules, rustTarget, + rustTargetRoot, rustTests, rustWorkspaces, selectRust, @@ -44,8 +47,12 @@ const git = (args, env = {}) => }); // `select` over a synthetic head tree: HEAD's tree with a comment appended to // each given file, so only those files differ and no test body changes. The -// outputs are keyed per runner as the workflow reads them. -function selectAppending(...files) { +// outputs are keyed per runner as the workflow reads them. The selector +// writes its summary and outputs to $GITHUB_STEP_SUMMARY and $GITHUB_OUTPUT +// when they are set (as under Actions, where this suite also runs) and to +// stdout otherwise, so the child gets one contract or the other by `github`, +// never whichever the parent happens to have. +function selectAppending(files, { github = false } = {}) { const dir = mkdtempSync(join(tmpdir(), "flake-select-")); try { const env = { GIT_INDEX_FILE: join(dir, "index") }; @@ -59,19 +66,34 @@ function selectAppending(...files) { git(["update-index", "--cacheinfo", `100644,${blob},${file}`], env); } const tree = git(["write-tree"], env).trim(); + const { GITHUB_STEP_SUMMARY, GITHUB_OUTPUT, ...inherited } = process.env; + const routed = { + GITHUB_STEP_SUMMARY: join(dir, "summary.md"), + GITHUB_OUTPUT: join(dir, "output.txt"), + }; const result = spawnSync( process.execPath, [join(root, "scripts/flake-check.mjs"), "select", "HEAD", tree], - { cwd: root, encoding: "utf8" }, + { + cwd: root, + encoding: "utf8", + env: github ? { ...inherited, ...routed } : inherited, + }, ); assert.equal(result.status, 0, result.stderr); + const summary = github + ? readFileSync(routed.GITHUB_STEP_SUMMARY, "utf8") + : result.stdout; + const output = github + ? readFileSync(routed.GITHUB_OUTPUT, "utf8") + : result.stdout; const outputs = Object.fromEntries( - [...result.stdout.matchAll(/^(\w+)=(.*)$/gm)].map(([, key, value]) => [ + [...output.matchAll(/^(\w+)=(.*)$/gm)].map(([, key, value]) => [ key, value.split(" ").filter(Boolean), ]), ); - return { summary: result.stdout, outputs }; + return { summary, outputs, stdout: result.stdout }; } finally { rmSync(dir, { recursive: true, force: true }); } @@ -484,14 +506,55 @@ test("a helper under tests/ selects the integration binaries that include it", ( read("../crates/buzz-agent/tests/golden_transcripts.rs"), /^mod common;$/m, ); + // A binary's root is tests/.rs or tests//main.rs; anything else + // under tests/ is a module of one. + for (const file of ["crates/x/tests/a.rs", "crates/x/tests/suite/main.rs"]) + assert.ok(rustTargetRoot(file), file); + for (const file of [ + "crates/x/tests/common/mod.rs", + "crates/x/tests/suite/util.rs", + "crates/x/src/lib.rs", + "crates/x/src/main.rs", + ]) + assert.ok(!rustTargetRoot(file), file); + // File modules resolve by cargo's conventions against the real tree: a + // root's `mod common;` is tests/common/mod.rs, a non-root's `mod launch;` + // is /launch.rs, a `#[path]` names its file, and a declaration in a + // comment or one no file backs is left out. + assert.deepEqual( + rustModules( + "crates/buzz-agent/tests/fake_llm.rs", + "mod common;\n// mod missing;\nmod nowhere;\n", + ), + [{ name: "common", file: "crates/buzz-agent/tests/common/mod.rs" }], + ); + assert.deepEqual(rustModules("crates/buzz-acp/src/acp.rs", "mod launch;\n"), [ + { name: "launch", file: "crates/buzz-acp/src/acp/launch.rs" }, + ]); + const wrapper = "crates/buzz-acp/tests/pool_lifecycle_state.rs"; + assert.deepEqual(rustModules(wrapper, read(`../${wrapper}`)), [ + { name: "pool_lifecycle", file: "crates/buzz-acp/src/pool_lifecycle.rs" }, + ]); + // The wrapper's tests are the included module's, under its module name: + // the names nextest lists for the pool_lifecycle_state binary. + const included = rustTests(read("../crates/buzz-acp/src/pool_lifecycle.rs")); + assert.ok(included.length > 0); + assert.deepEqual( + rustIncludedTests(wrapper, read(`../${wrapper}`)).map((test) => test.name), + included.map((test) => `pool_lifecycle::${test.name}`), + ); + assert.deepEqual( + rustIncludedTests("crates/buzz-agent/tests/fake_llm.rs", "mod common;\n"), + [], + ); }); test("select, through its entry point, repeats a changed Tauri test and the binaries including a changed helper", () => { // Reviewer probes on #8613: before, both produced an empty selection and // every repeat lane skipped. - const tauri = selectAppending( + const tauri = selectAppending([ "desktop/src-tauri/tests/rodio_mixer_diagnostic.rs", - ); + ]); assert.match( tauri.summary, /\| desktop\/src-tauri\/tests\/rodio_mixer_diagnostic\.rs \| tauri \| whole file \(shared code changed\): 3 test\(s\) \|/, @@ -505,8 +568,21 @@ test("select, through its entry point, repeats a changed Tauri test and the bina ), ); assert.equal(tauri.outputs.tauri.length, 3); + // Under Actions, where the Desktop JS lane runs this suite, the selector + // routes the summary and outputs to $GITHUB_STEP_SUMMARY and + // $GITHUB_OUTPUT: the same selection arrives there, and nothing on stdout. + const routed = selectAppending( + ["desktop/src-tauri/tests/rodio_mixer_diagnostic.rs"], + { github: true }, + ); + assert.equal(routed.stdout, ""); + assert.match( + routed.summary, + /\| desktop\/src-tauri\/tests\/rodio_mixer_diagnostic\.rs \| tauri \| whole file \(shared code changed\): 3 test\(s\) \|/, + ); + assert.deepEqual(routed.outputs, tauri.outputs); - const helper = selectAppending("crates/buzz-agent/tests/common/mod.rs"); + const helper = selectAppending(["crates/buzz-agent/tests/common/mod.rs"]); assert.match( helper.summary, /\| crates\/buzz-agent\/tests\/common\/mod\.rs \| rust \| helper for: anthropic_continuation, fake_llm, permission_boundary, regressions: \d+ test\(s\) \|/, @@ -522,15 +598,45 @@ test("select, through its entry point, repeats a changed Tauri test and the bina ]); assert.ok(helper.outputs.rust.length > 0); // Changed together, the helper and an importer select each test once. - const both = selectAppending( + const both = selectAppending([ "crates/buzz-agent/tests/common/mod.rs", "crates/buzz-agent/tests/fake_llm.rs", - ); + ]); assert.deepEqual(both.outputs.rust, [...new Set(both.outputs.rust)]); assert.deepEqual(both.outputs.rust.length, helper.outputs.rust.length); + // A tests/.rs root with no test of its own is a binary, not a + // helper: buzz-acp's pool_lifecycle_state compiles src/pool_lifecycle.rs + // in by `#[path]`, and a change to it repeats that binary's tests, not its + // siblings' (before, the selection was git_bootstrap, run_task and + // stdio_contract, and pool_lifecycle_state was not repeated at all). + const wrapper = "crates/buzz-acp/tests/pool_lifecycle_state.rs"; + assert.deepEqual(rustTests(read(`../${wrapper}`)), []); + const included = rustTests(read("../crates/buzz-acp/src/pool_lifecycle.rs")); + const wrapped = selectAppending([wrapper]); + assert.match( + wrapped.summary, + new RegExp( + `\\| crates/buzz-acp/tests/pool_lifecycle_state\\.rs \\| rust \\| whole file \\(shared code changed\\): ${included.length} test\\(s\\) \\|`, + ), + ); + assert.deepEqual( + wrapped.outputs.rust, + included.map((test) => `${wrapper}::pool_lifecycle::${test.name}`), + ); + const { groups } = rustGroups(wrapped.outputs.rust); + assert.deepEqual(groups.get("buzz-acp").args, [ + "-p", + "buzz-acp", + "--test", + "pool_lifecycle_state", + ]); + assert.deepEqual( + groups.get("buzz-acp").tests.map(({ exact }) => exact), + included.map((test) => `pool_lifecycle::${test.name}`), + ); // A src/** file without test functions still selects nothing: the regular // lanes run what depends on it. - const source = selectAppending("crates/buzz-core/src/lib.rs"); + const source = selectAppending(["crates/buzz-core/src/lib.rs"]); assert.match( source.summary, /\| crates\/buzz-core\/src\/lib\.rs \| rust \| no test functions \|/, @@ -564,7 +670,31 @@ test("Rust selection: a hunk inside a test selects it, elsewhere selects every r select(diff([21, 1, 21, 1]), { added: true, previous: "" }), whole("new file"), ); - // A file without test functions is not a test file. + // Tests of the file modules an integration-test root declares ride along + // with a whole selection, not with a hunk inside one of the root's own + // tests; a root with no test of its own selects only them, and one with no + // tests at all is not a test file. + const included = [ + { name: "m::a", ignored: false }, + { name: "m::b", ignored: true }, + ]; + assert.deepEqual(select(diff([4, 0, 5, 1]), { included }), { + ...whole("shared code changed"), + names: [...whole().names, "m::a"], + }); + assert.deepEqual(select(diff([21, 1, 21, 1]), { included }), { + lines: [19], + names: ["tests::plain"], + }); + assert.deepEqual(selectRust(diff([1, 1, 1, 1]), "mod m;\n", { included }), { + whole: true, + reason: "shared code changed", + names: ["m::a"], + }); + assert.deepEqual( + selectRust(diff([0, 0, 1, 1]), "mod m;\n", { added: true, included }), + { whole: true, reason: "new file", names: ["m::a"] }, + ); assert.equal(selectRust(diff([1, 1, 1, 1]), "fn main() {}\n"), undefined); assert.equal(cratePackage("crates/buzz-core/src/filter.rs"), "buzz-core"); });