From b9ea06fa8b6ae5521bd1f3c1cafcf0ececedea59 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Fri, 29 May 2026 17:12:58 -0400 Subject: [PATCH] ci(#483): resolve transitive dependencies in affected-test selection + zero-dependent widen backstop (#485) Co-authored-by: CI Rebase Check Co-authored-by: Claude Sonnet 4.6 --- scripts/affected-tests-lib.cjs | 350 +++++++++++++++++-- tests/affected-tests-lib.test.cjs | 540 ++++++++++++++++++++++++++++++ 2 files changed, 864 insertions(+), 26 deletions(-) diff --git a/scripts/affected-tests-lib.cjs b/scripts/affected-tests-lib.cjs index b975625d1..6db893287 100644 --- a/scripts/affected-tests-lib.cjs +++ b/scripts/affected-tests-lib.cjs @@ -21,6 +21,20 @@ const PR_EXCLUDED_SUITES = new Set(['install', 'slow']); // Suites run on every PR cell when the critical-path fallback fires. const PR_FULL_SUITES = ['unit', 'integration', 'security']; +// Source trees to walk when building the forward graph (in addition to tests/). +// Relative to repoRoot. We walk these to discover SUT-internal requires so that +// a change to a deep helper propagates through re-export chains to tests. +const SOURCE_TREES = [ + 'get-shit-done/bin/lib', + 'bin/lib', + 'bin', + 'scripts', + 'commands', + 'hooks', + 'agents', + 'eslint-rules', +]; + function toPosixPath(input) { return input.split(path.sep).join('/'); } @@ -41,6 +55,8 @@ function parseRelativeSpecifiers(source) { return specifiers.filter(specifier => specifier.startsWith('.')); } +// Extended candidate list now includes .ts/.cts/.mts/.json as well as the +// standard .js/.cjs/.mjs and index variants. function resolveRelativeDependency(repoRoot, fromAbs, specifier) { const base = path.resolve(path.dirname(fromAbs), specifier); const candidates = [ @@ -48,9 +64,14 @@ function resolveRelativeDependency(repoRoot, fromAbs, specifier) { `${base}.js`, `${base}.cjs`, `${base}.mjs`, + `${base}.ts`, + `${base}.cts`, + `${base}.mts`, + `${base}.json`, path.join(base, 'index.js'), path.join(base, 'index.cjs'), path.join(base, 'index.mjs'), + path.join(base, 'index.ts'), ]; for (const candidate of candidates) { @@ -62,20 +83,172 @@ function resolveRelativeDependency(repoRoot, fromAbs, specifier) { return null; } -function buildReverseIndex(repoRoot, testFiles) { - const reverse = new Map(); - for (const testFile of testFiles) { - const absTest = path.join(repoRoot, testFile); - const source = readFileSync(absTest, 'utf8'); - const specs = parseRelativeSpecifiers(source); - for (const specifier of specs) { - const dep = resolveRelativeDependency(repoRoot, absTest, specifier); - if (!dep) continue; - if (!reverse.has(dep)) reverse.set(dep, new Set()); - reverse.get(dep).add(testFile); +// --------------------------------------------------------------------------- +// Source-file walker +// --------------------------------------------------------------------------- + +/** + * Collect all .cjs / .mjs / .js / .ts / .cts / .mts / .json files under a + * directory tree, returned as repo-relative POSIX paths. Silently skips + * trees that don't exist. + */ +function walkTree(repoRoot, relDir) { + const absDir = path.join(repoRoot, relDir); + if (!existsSync(absDir)) return []; + + const results = []; + const queue = [absDir]; + + while (queue.length > 0) { + const cur = queue.shift(); + let entries; + try { + entries = readdirSync(cur, { withFileTypes: true }); + } catch { + continue; + } + for (const entry of entries) { + const abs = path.join(cur, entry.name); + if (entry.isDirectory()) { + // Skip node_modules + if (entry.name === 'node_modules') continue; + queue.push(abs); + } else if (entry.isFile()) { + const ext = path.extname(entry.name); + if (['.js', '.cjs', '.mjs', '.ts', '.cts', '.mts', '.json'].includes(ext)) { + results.push(toPosixPath(path.relative(repoRoot, abs))); + } + } } } - return reverse; + + return results; +} + +// --------------------------------------------------------------------------- +// Forward graph: Map> +// --------------------------------------------------------------------------- + +/** + * Build a forward dependency graph over test files PLUS source trees. + * For each file: read, parseRelativeSpecifiers, resolve each specifier. + * Returns Map>. + */ +function buildForwardGraph(repoRoot, testFiles) { + // Collect all files to index: test files + source files + const sourceFiles = []; + for (const tree of SOURCE_TREES) { + for (const f of walkTree(repoRoot, tree)) { + sourceFiles.push(f); + } + } + + const allFiles = [...new Set([...testFiles, ...sourceFiles])]; + const forward = new Map(); + + for (const fileRel of allFiles) { + const absFile = path.join(repoRoot, fileRel); + let source; + try { + source = readFileSync(absFile, 'utf8'); + } catch { + continue; + } + + const specs = parseRelativeSpecifiers(source); + const deps = new Set(); + + for (const specifier of specs) { + const dep = resolveRelativeDependency(repoRoot, absFile, specifier); + if (dep) deps.add(dep); + } + + forward.set(fileRel, deps); + } + + return forward; +} + +// --------------------------------------------------------------------------- +// Reverse-transitive index: Map> +// --------------------------------------------------------------------------- + +/** + * Build the TRANSITIVE reverse index: Map>. + * + * Algorithm: + * 1. Build forward graph over all test + source files. + * 2. Invert to direct reverse edges: Map>. + * 3. For each test file, BFS backwards through all direct reverse edges + * to find every ancestor. Map each ancestor → the test. + * + * Cycle safety: visited set per BFS — each node is enqueued at most once. + * + * @param {string} repoRoot + * @param {string[]} testFiles repo-relative posix paths (e.g. ['tests/foo.test.cjs']) + * @returns {Map>} + */ +function buildTransitiveReverseIndex(repoRoot, testFiles) { + const forward = buildForwardGraph(repoRoot, testFiles); + + // Build direct reverse edges: dep → Set of files that directly require dep + const directReverse = new Map(); + for (const [fileRel, deps] of forward) { + for (const dep of deps) { + if (!directReverse.has(dep)) directReverse.set(dep, new Set()); + directReverse.get(dep).add(fileRel); + } + } + + // For each test file, BFS through direct reverse edges to collect all + // ancestors, then invert: ancestor → test. + // We do this test-file-first (not dep-first) so we know which test reached + // each ancestor. + const transitiveReverse = new Map(); + + for (const testFile of testFiles) { + // BFS from testFile following reverse edges (files that point TO testFile, + // then files that point to THOSE files, etc.). + // We want: "if X changed, would that eventually pull in testFile?" + // So we walk the FORWARD graph starting from testFile to find all deps, + // then any of those deps maps back to testFile. + + // Actually simpler: for each test we do a forward BFS to find ALL files + // the test transitively depends on. Then we record testFile as a + // dependent of each of those files. + const visited = new Set(); + visited.add(testFile); + const queue = [testFile]; + + while (queue.length > 0) { + const current = queue.shift(); + const deps = forward.get(current); + if (!deps) continue; + for (const dep of deps) { + if (visited.has(dep)) continue; + visited.add(dep); + queue.push(dep); + } + } + + // Every file in `visited` (except testFile itself) is a transitive dep. + // Record testFile as a dependent of each. + for (const dep of visited) { + if (dep === testFile) continue; + if (!transitiveReverse.has(dep)) transitiveReverse.set(dep, new Set()); + transitiveReverse.get(dep).add(testFile); + } + } + + return transitiveReverse; +} + +// --------------------------------------------------------------------------- +// Legacy shim — kept so that runAffectedTests can call buildTransitiveReverseIndex +// and existing call sites that still call buildReverseIndex still work. +// --------------------------------------------------------------------------- +function buildReverseIndex(repoRoot, testFiles) { + return buildTransitiveReverseIndex(repoRoot, testFiles); } function shouldRunFullSuite(changedFiles) { @@ -91,19 +264,43 @@ function listTestFiles(repoRoot) { .sort(); } -function pickAffectedTests(changedFiles, allTests, reverseIndex) { +/** + * Select the affected tests given a set of changed files and a reverse index. + * + * Options: + * detectWiden {boolean} — when true, attach `._widenRequired = true` to the + * returned array when a changed source file has zero transitive test + * dependents. The caller (runAffectedTests) uses this to widen to unit/all. + * + * The returned array is sorted and may have `._widenRequired` attached. + */ +function pickAffectedTests(changedFiles, allTests, reverseIndex, options = {}) { + const { detectWiden = false } = options; const selected = new Set(); + let widenRequired = false; + // Build a fast lookup of currently-existing test files (from readdirSync — deleted files absent). + const allTestsSet = new Set(allTests); + + // (a) directly-changed test files + (b) transitive test dependents + // Deleted test files are filtered out — they no longer exist and cannot be run. + // A deleted test file also must NOT trigger widen (the test is simply gone). for (const file of changedFiles) { if (file.startsWith('tests/') && file.endsWith('.test.cjs')) { - selected.add(file); - } - const dependents = reverseIndex.get(file); - if (dependents) { - for (const testFile of dependents) selected.add(testFile); + // Only select if the test file still exists (i.e. is present in allTests from readdirSync). + if (allTestsSet.has(file)) { + selected.add(file); + } + // Deleted test file — do not add to selected; do not look up reverse index. + } else { + const dependents = reverseIndex.get(file); + if (dependents) { + for (const testFile of dependents) selected.add(testFile); + } } } + // (c) stem heuristic — kept as secondary mechanism for (const file of changedFiles) { const stem = path.basename(file).replace(/\.[^.]+$/, '').toLowerCase(); if (!stem) continue; @@ -112,6 +309,39 @@ function pickAffectedTests(changedFiles, allTests, reverseIndex) { } } + // Widen backstop: if a changed file is a non-test, non-CRITICAL_PATH source file + // (recognised extension) under a SOURCE_TREE, AND it is either deleted (no longer + // on disk — so never in the forward graph and has no static dependents) OR it + // exists with ZERO transitive test dependents — signal a widen. + // NOTE: we deliberately do NOT skip deleted files here; a deleted source file's + // absence from the forward graph means dependents===undefined, which is the same + // as zero static dependents, and is itself the widen trigger. + if (detectWiden) { + for (const file of changedFiles) { + // Only care about source files, not test files or docs + if (file.startsWith('tests/')) continue; + if (shouldRunFullSuite([file])) continue; // critical path already triggers full suite + // Check: is this a source file (has a recognised extension)? + const ext = path.extname(file); + const isSourceFile = ['.js', '.cjs', '.mjs', '.ts', '.cts', '.mts', '.json'].includes(ext); + if (!isSourceFile) continue; + // Check: is this file under a recognised source tree? + const isUnderSourceTree = SOURCE_TREES.some( + tree => file === tree || file.startsWith(tree + '/'), + ); + if (!isUnderSourceTree) continue; + // Does it have any test dependents? + // A deleted file will have undefined here (not in the graph) — that is + // treated as zero static dependents and triggers widen conservatively. + const dependents = reverseIndex.get(file); + const hasStaticDependents = dependents && dependents.size > 0; + if (!hasStaticDependents) { + widenRequired = true; + break; + } + } + } + // Drop any file whose suite is push-only. This is the single chokepoint — // it catches direct-change, reverse-index, AND stem-match selections. for (const file of selected) { @@ -120,13 +350,15 @@ function pickAffectedTests(changedFiles, allTests, reverseIndex) { } // When nothing maps, return an empty array. The caller decides the fallback. - return [...selected].sort(); + const result = [...selected].sort(); + if (widenRequired) result._widenRequired = true; + return result; } function changedFilesSinceBase(repoRoot, baseRef) { const out = execFileSync( 'git', - ['diff', '--name-only', '--diff-filter=ACMR', `${baseRef}...HEAD`], + ['diff', '--name-only', '--no-renames', '--diff-filter=ACMRD', `${baseRef}...HEAD`], { cwd: repoRoot, encoding: 'utf8' }, ).trim(); if (!out) return []; @@ -194,6 +426,38 @@ function resolveBaseRef() { return 'origin/main'; } +/** + * Pure function: given the outputs of the selection phase, return a run plan + * describing what should be executed. No I/O is performed here. + * + * Return shapes: + * { mode: 'suite', suite: 'unit' } — no changed files + * { mode: 'suites', suites: PR_FULL_SUITES } — critical path triggered + * { mode: 'suites', suites: PR_FULL_SUITES } — widen required (orphan src file) + * { mode: 'suite', suite: 'unit' } — selection empty after widen=false + * { mode: 'files', files: string[] } — concrete selection, no widen + * + * Invariant: when widenRequired is true the executed set is ALWAYS ⊇ selected, + * because PR_FULL_SUITES covers every PR-eligible suite (unit + integration + + * security), so every concrete match that pickAffectedTests put into `selected` + * belongs to one of those suites and will be exercised by running all three. + */ +function resolveRunPlan({ changedFiles, selected, widenRequired, criticalPath, noChanges }) { + if (noChanges) { + return { mode: 'suite', suite: 'unit' }; + } + if (criticalPath) { + return { mode: 'suites', suites: PR_FULL_SUITES }; + } + if (widenRequired) { + return { mode: 'suites', suites: PR_FULL_SUITES }; + } + if (selected.length === 0) { + return { mode: 'suite', suite: 'unit' }; + } + return { mode: 'files', files: selected }; +} + function runAffectedTests(options = {}) { const repoRoot = options.repoRoot || path.resolve(__dirname, '..'); const baseRef = options.baseRef || resolveBaseRef(); @@ -214,29 +478,63 @@ function runAffectedTests(options = {}) { } const allTests = listTestFiles(repoRoot); - const reverseIndex = buildReverseIndex(repoRoot, allTests); - const selected = pickAffectedTests(changed, allTests, reverseIndex); + const reverseIndex = buildTransitiveReverseIndex(repoRoot, allTests); + const selected = pickAffectedTests(changed, allTests, reverseIndex, { detectWiden: true }); console.error(`affected-tests: base=${baseRef} changed=${changed.length} selected=${selected.length}`); console.error(`affected-tests: ${selected.join(' ')}`); - if (selected.length === 0) { - console.error('affected-tests: no affected tests found; running unit suite as smoke'); - runSuite(repoRoot, 'unit'); + const plan = resolveRunPlan({ + changedFiles: changed, + selected, + widenRequired: selected._widenRequired === true, + criticalPath: false, + noChanges: false, + }); + + if (plan.mode === 'suites') { + // Widen backstop: a source file changed that has no static test dependents. + // Run all PR suites (unit + integration + security) — a strict superset of + // the concretely-selected tests — so no integration/security match is lost. + for (const file of changed) { + const ext = path.extname(file); + const isSourceFile = ['.js', '.cjs', '.mjs', '.ts', '.cts', '.mts', '.json'].includes(ext); + if (!isSourceFile || file.startsWith('tests/') || shouldRunFullSuite([file])) continue; + const dependents = reverseIndex.get(file); + if (!dependents || dependents.size === 0) { + console.error( + `affected-tests: ${file} has no static test dependents; widening to PR suites (unit+integration+security)`, + ); + } + } + for (const suite of plan.suites) { + runSuite(repoRoot, suite); + } return; } - runNodeTestFiles(repoRoot, selected); + if (plan.mode === 'suite') { + console.error('affected-tests: no affected tests found; running unit suite as smoke'); + runSuite(repoRoot, plan.suite); + return; + } + + // plan.mode === 'files' + runNodeTestFiles(repoRoot, plan.files); } module.exports = { CRITICAL_PATHS, PR_EXCLUDED_SUITES, PR_FULL_SUITES, + buildForwardGraph, buildReverseIndex, + buildTransitiveReverseIndex, parseRelativeSpecifiers, pickAffectedTests, resolveBaseRef, + resolveRelativeDependency, + resolveRunPlan, shouldRunFullSuite, toPosixPath, runAffectedTests, diff --git a/tests/affected-tests-lib.test.cjs b/tests/affected-tests-lib.test.cjs index 78bc24971..61987f504 100644 --- a/tests/affected-tests-lib.test.cjs +++ b/tests/affected-tests-lib.test.cjs @@ -2,16 +2,41 @@ const test = require('node:test'); const assert = require('node:assert/strict'); +const path = require('node:path'); +const fs = require('node:fs'); +const os = require('node:os'); const { parseRelativeSpecifiers, pickAffectedTests, + resolveRunPlan, shouldRunFullSuite, resolveBaseRef, PR_EXCLUDED_SUITES, PR_FULL_SUITES, + buildTransitiveReverseIndex, + resolveRelativeDependency, } = require('../scripts/affected-tests-lib.cjs'); +// --------------------------------------------------------------------------- +// Helpers +// --------------------------------------------------------------------------- + +/** Create a temp repo with given files (keys = relative paths, values = content). */ +function makeFixture(files) { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-test-')); + for (const [rel, content] of Object.entries(files)) { + const abs = path.join(dir, rel); + fs.mkdirSync(path.dirname(abs), { recursive: true }); + fs.writeFileSync(abs, content, 'utf8'); + } + return dir; +} + +// --------------------------------------------------------------------------- +// Original tests (kept green) +// --------------------------------------------------------------------------- + test('parseRelativeSpecifiers captures local require/import paths', () => { const source = ` const a = require('./alpha.cjs'); @@ -149,3 +174,518 @@ test('resolveBaseRef prefers explicit env override', () => { else process.env.GITHUB_BASE_REF = original.GITHUB_BASE_REF; } }); + +// --------------------------------------------------------------------------- +// NEW: Transitive test (RED against old code) +// Fixture: tests/t.test.cjs -> ../get-shit-done/bin/lib/depA.cjs -> ./depB.cjs +// Changed: get-shit-done/bin/lib/depB.cjs +// Expected: tests/t.test.cjs is selected +// --------------------------------------------------------------------------- + +test('transitive: changing a deep dependency selects the test that depends on it', (t) => { + const dir = makeFixture({ + 'get-shit-done/bin/lib/depB.cjs': `'use strict';\nmodule.exports = { b: 1 };\n`, + 'get-shit-done/bin/lib/depA.cjs': `'use strict';\nconst depB = require('./depB.cjs');\nmodule.exports = { a: depB };\n`, + 'tests/t.test.cjs': `'use strict';\nconst depA = require('../get-shit-done/bin/lib/depA.cjs');\n`, + }); + t.after(() => fs.rmSync(dir, { recursive: true, force: true })); + + const reverseIndex = buildTransitiveReverseIndex(dir, ['tests/t.test.cjs']); + const selected = pickAffectedTests( + ['get-shit-done/bin/lib/depB.cjs'], + ['tests/t.test.cjs'], + reverseIndex, + ); + + assert.ok( + selected.includes('tests/t.test.cjs'), + `Expected tests/t.test.cjs in selection, got: ${JSON.stringify(selected)}`, + ); +}); + +// --------------------------------------------------------------------------- +// Adversarial matrix +// --------------------------------------------------------------------------- + +test('adversarial(a): cycle depA<->depB — changing depA selects test, no hang', (t) => { + const dir = makeFixture({ + 'get-shit-done/bin/lib/depA.cjs': `'use strict';\nconst depB = require('./depB.cjs');\nmodule.exports = {};\n`, + 'get-shit-done/bin/lib/depB.cjs': `'use strict';\nconst depA = require('./depA.cjs');\nmodule.exports = {};\n`, + 'tests/cycle.test.cjs': `'use strict';\nconst depA = require('../get-shit-done/bin/lib/depA.cjs');\n`, + }); + t.after(() => fs.rmSync(dir, { recursive: true, force: true })); + + // Must complete without hanging + const reverseIndex = buildTransitiveReverseIndex(dir, ['tests/cycle.test.cjs']); + const selected = pickAffectedTests( + ['get-shit-done/bin/lib/depA.cjs'], + ['tests/cycle.test.cjs'], + reverseIndex, + ); + + assert.ok( + selected.includes('tests/cycle.test.cjs'), + `Expected cycle.test.cjs selected, got: ${JSON.stringify(selected)}`, + ); +}); + +test('adversarial(b): missing require (gone file) — null resolve, no crash', (t) => { + const dir = makeFixture({ + // Requires a file that does not exist + 'tests/missing.test.cjs': `'use strict';\nconst x = require('../get-shit-done/bin/lib/gone.cjs');\n`, + }); + t.after(() => fs.rmSync(dir, { recursive: true, force: true })); + + // Should not throw + let reverseIndex; + assert.doesNotThrow(() => { + reverseIndex = buildTransitiveReverseIndex(dir, ['tests/missing.test.cjs']); + }); + + // Changing the missing file produces no selection (it doesn't exist, so no dependents) + const selected = pickAffectedTests( + ['get-shit-done/bin/lib/gone.cjs'], + ['tests/missing.test.cjs'], + reverseIndex, + ); + // gone.cjs is not in the graph (null resolve), so no test selected via index + // stem match may or may not fire; either way, no crash is the key assertion + assert.ok(Array.isArray(selected), 'result must be an array'); +}); + +test('adversarial(c): .json dependency — changing data.json selects the test', (t) => { + const dir = makeFixture({ + 'get-shit-done/bin/lib/data.json': `{"key":"value"}`, + 'tests/json.test.cjs': `'use strict';\nconst data = require('../get-shit-done/bin/lib/data.json');\n`, + }); + t.after(() => fs.rmSync(dir, { recursive: true, force: true })); + + const reverseIndex = buildTransitiveReverseIndex(dir, ['tests/json.test.cjs']); + const selected = pickAffectedTests( + ['get-shit-done/bin/lib/data.json'], + ['tests/json.test.cjs'], + reverseIndex, + ); + + assert.ok( + selected.includes('tests/json.test.cjs'), + `Expected json.test.cjs selected for data.json change, got: ${JSON.stringify(selected)}`, + ); +}); + +test('adversarial(d): re-export chain — changing depB selects the test that requires the re-exporter', (t) => { + const dir = makeFixture({ + 'get-shit-done/bin/lib/depB.cjs': `'use strict';\nmodule.exports = { deep: true };\n`, + 'get-shit-done/bin/lib/reexporter.cjs': `'use strict';\nmodule.exports = require('./depB.cjs');\n`, + 'tests/reexport.test.cjs': `'use strict';\nconst x = require('../get-shit-done/bin/lib/reexporter.cjs');\n`, + }); + t.after(() => fs.rmSync(dir, { recursive: true, force: true })); + + const reverseIndex = buildTransitiveReverseIndex(dir, ['tests/reexport.test.cjs']); + const selected = pickAffectedTests( + ['get-shit-done/bin/lib/depB.cjs'], + ['tests/reexport.test.cjs'], + reverseIndex, + ); + + assert.ok( + selected.includes('tests/reexport.test.cjs'), + `Expected reexport.test.cjs selected when depB changes, got: ${JSON.stringify(selected)}`, + ); +}); + +test('adversarial(e): bare and node: specifiers are ignored', () => { + const source = ` + const a = require('node:fs'); + const b = require('external-package'); + import c from 'node:path'; + import d from 'lodash'; + const e = require('./local.cjs'); + `; + const out = parseRelativeSpecifiers(source); + // Only the relative specifier survives + assert.deepEqual(out, ['./local.cjs']); +}); + +test('adversarial(f): WIDEN — changing a src file with no test dependents widens to unit/all', (t) => { + // orphan.cjs is a source file no test file requires (statically) + const dir = makeFixture({ + 'get-shit-done/bin/lib/orphan.cjs': `'use strict';\nmodule.exports = {};\n`, + 'tests/unrelated.test.cjs': `'use strict';\n// requires nothing\n`, + }); + t.after(() => fs.rmSync(dir, { recursive: true, force: true })); + + const reverseIndex = buildTransitiveReverseIndex(dir, ['tests/unrelated.test.cjs']); + + // Verify orphan.cjs has no dependents in the index + const dependents = reverseIndex.get('get-shit-done/bin/lib/orphan.cjs'); + assert.ok( + !dependents || dependents.size === 0, + 'orphan.cjs must have no transitive test dependents', + ); + + // The widen backstop is tested via the WIDEN_SIGNAL attached to pickAffectedTests result + const selected = pickAffectedTests( + ['get-shit-done/bin/lib/orphan.cjs'], + ['tests/unrelated.test.cjs'], + reverseIndex, + { detectWiden: true }, + ); + + assert.ok( + selected._widenRequired === true, + `Expected _widenRequired=true for orphan src file with no test dependents, got: ${JSON.stringify(selected._widenRequired)}`, + ); +}); + +test('adversarial(g): dynamic require in changed file with no static dependents — widen backstop catches it', (t) => { + // dynamic.cjs uses a template literal require — not statically parseable + // No test requires dynamic.cjs statically + const dir = makeFixture({ + 'get-shit-done/bin/lib/dynamic.cjs': `'use strict';\nconst x = 'foo';\nconst m = require(\`./\${x}\`);\nmodule.exports = {};\n`, + 'tests/unrelated.test.cjs': `'use strict';\n// does not require dynamic.cjs\n`, + }); + t.after(() => fs.rmSync(dir, { recursive: true, force: true })); + + const reverseIndex = buildTransitiveReverseIndex(dir, ['tests/unrelated.test.cjs']); + + const selected = pickAffectedTests( + ['get-shit-done/bin/lib/dynamic.cjs'], + ['tests/unrelated.test.cjs'], + reverseIndex, + { detectWiden: true }, + ); + + assert.ok( + selected._widenRequired === true, + `Expected _widenRequired=true for dynamic-require src file with no static dependents, got: ${JSON.stringify(selected._widenRequired)}`, + ); +}); + +test('resolveRelativeDependency resolves .ts, .json extensions', (t) => { + const dir = makeFixture({ + 'src/helper.ts': `export const x = 1;\n`, + 'src/data.json': `{"k":1}`, + }); + t.after(() => fs.rmSync(dir, { recursive: true, force: true })); + + const fromAbs = path.join(dir, 'tests/consumer.cjs'); + + const tsResult = resolveRelativeDependency(dir, fromAbs, '../src/helper'); + assert.equal(tsResult, 'src/helper.ts', `Expected src/helper.ts, got: ${tsResult}`); + + const jsonResult = resolveRelativeDependency(dir, fromAbs, '../src/data.json'); + assert.equal(jsonResult, 'src/data.json', `Expected src/data.json, got: ${jsonResult}`); +}); + +// --------------------------------------------------------------------------- +// resolveRunPlan — pure unit tests +// --------------------------------------------------------------------------- + +test('resolveRunPlan: noChanges → mode:suite unit', () => { + // Arrange + const plan = resolveRunPlan({ changedFiles: [], selected: [], widenRequired: false, criticalPath: false, noChanges: true }); + // Assert + assert.deepEqual(plan, { mode: 'suite', suite: 'unit' }); +}); + +test('resolveRunPlan: criticalPath → mode:suites PR_FULL_SUITES', () => { + // Arrange + const plan = resolveRunPlan({ changedFiles: ['package.json'], selected: [], widenRequired: false, criticalPath: true, noChanges: false }); + // Assert + assert.deepEqual(plan, { mode: 'suites', suites: PR_FULL_SUITES }); +}); + +test('resolveRunPlan: widenRequired → mode:suites PR_FULL_SUITES (not unit-only)', () => { + // Arrange: orphan source file, no static dependents → widen signal + const selected = []; + selected._widenRequired = true; + const plan = resolveRunPlan({ changedFiles: ['bin/orphan.cjs'], selected, widenRequired: true, criticalPath: false, noChanges: false }); + // Assert — must be suites covering all three PR suites, NOT unit-only + assert.equal(plan.mode, 'suites', 'widen must produce mode:suites, not mode:suite'); + assert.deepEqual(plan.suites, PR_FULL_SUITES); + assert.ok(plan.suites.includes('integration'), 'integration must be in widen plan'); + assert.ok(plan.suites.includes('security'), 'security must be in widen plan'); + assert.ok(plan.suites.includes('unit'), 'unit must be in widen plan'); +}); + +test('resolveRunPlan: empty selection (no widen) → mode:suite unit smoke', () => { + // Arrange + const plan = resolveRunPlan({ changedFiles: ['docs/README.md'], selected: [], widenRequired: false, criticalPath: false, noChanges: false }); + // Assert + assert.deepEqual(plan, { mode: 'suite', suite: 'unit' }); +}); + +test('resolveRunPlan: concrete selection without widen → mode:files', () => { + // Arrange + const files = ['tests/foo.test.cjs', 'tests/bar.integration.test.cjs']; + const plan = resolveRunPlan({ changedFiles: ['bin/foo.cjs'], selected: files, widenRequired: false, criticalPath: false, noChanges: false }); + // Assert + assert.deepEqual(plan, { mode: 'files', files }); +}); + +// --------------------------------------------------------------------------- +// Mixed-diff regression: widen must be a SUPERSET, never drop concrete matches +// --------------------------------------------------------------------------- + +test('regression(mixed-diff): widen plan covers integration suite; concrete match not dropped', (t) => { + // Arrange: two changed files in one diff. + // - bin/lib/server.cjs → has a concrete dependent: tests/server.integration.test.cjs + // - bin/lib/orphan.cjs → has NO test dependents (triggers widen) + // Under the OLD (buggy) code, widenRequired caused an early-return to + // runSuite(root,'unit'), which excludes integration-marked tests, so the + // concrete integration match was silently dropped. + // Under the NEW code, widen must resolve to mode:suites with PR_FULL_SUITES, + // which covers unit + integration + security — a strict superset of selected. + + const dir = makeFixture({ + 'bin/lib/server.cjs': `'use strict';\nmodule.exports = { serve: true };\n`, + 'bin/lib/orphan.cjs': `'use strict';\nmodule.exports = {};\n`, + 'tests/server.integration.test.cjs': `'use strict';\nconst s = require('../bin/lib/server.cjs');\n`, + 'tests/unrelated.test.cjs': `'use strict';\n// requires nothing\n`, + }); + t.after(() => fs.rmSync(dir, { recursive: true, force: true })); + + // Act: build graph over both test files + const allTests = [ + 'tests/server.integration.test.cjs', + 'tests/unrelated.test.cjs', + ]; + const reverseIndex = buildTransitiveReverseIndex(dir, allTests); + + // Verify the fixture: server.cjs DOES have a concrete dependent + const serverDependents = reverseIndex.get('bin/lib/server.cjs'); + assert.ok( + serverDependents && serverDependents.has('tests/server.integration.test.cjs'), + 'fixture: bin/lib/server.cjs must map to tests/server.integration.test.cjs in the reverse index', + ); + + // Verify the fixture: orphan.cjs has NO dependents + const orphanDependents = reverseIndex.get('bin/lib/orphan.cjs'); + assert.ok( + !orphanDependents || orphanDependents.size === 0, + 'fixture: bin/lib/orphan.cjs must have zero test dependents', + ); + + // pickAffectedTests with detectWiden=true on the mixed diff + const changedFiles = ['bin/lib/server.cjs', 'bin/lib/orphan.cjs']; + const selected = pickAffectedTests(changedFiles, allTests, reverseIndex, { detectWiden: true }); + + // The concrete match must still be in selected (widen doesn't strip it) + assert.ok( + selected.includes('tests/server.integration.test.cjs'), + `Concrete integration match must survive pickAffectedTests even when widen fires; selected=${JSON.stringify(selected)}`, + ); + + // widenRequired must be set because orphan.cjs has no static dependents + assert.ok( + selected._widenRequired === true, + `_widenRequired must be true when any source file has no static dependents; got ${JSON.stringify(selected._widenRequired)}`, + ); + + // resolveRunPlan must return mode:suites covering integration (not unit-only) + const plan = resolveRunPlan({ + changedFiles, + selected, + widenRequired: selected._widenRequired === true, + criticalPath: false, + noChanges: false, + }); + + assert.equal( + plan.mode, + 'suites', + `widen plan must be mode:suites (not mode:suite/mode:files); got mode:${plan.mode} — OLD behaviour would have been mode:suite/unit, dropping the integration test`, + ); + assert.ok( + plan.suites.includes('integration'), + `widen plan must include integration suite to cover the concrete match; suites=${JSON.stringify(plan.suites)}`, + ); + assert.ok( + plan.suites.includes('security'), + `widen plan must include security suite; suites=${JSON.stringify(plan.suites)}`, + ); + assert.ok( + plan.suites.includes('unit'), + `widen plan must include unit suite; suites=${JSON.stringify(plan.suites)}`, + ); + + // Prove the old behaviour would have FAILED this test: + // Old code: widenRequired → runSuite(root,'unit') only. + // 'unit' suite selects only files with NO suite marker (suiteOf === null). + // tests/server.integration.test.cjs has marker 'integration', so it would be + // excluded from the unit suite run → the concrete match is silently dropped. + // The assertion above catches this because old plan would have been + // { mode: 'suite', suite: 'unit' } which does NOT include 'integration'. +}); + +// --------------------------------------------------------------------------- +// Regression: delete-only diffs +// --------------------------------------------------------------------------- + +test('regression(delete-only-source): deleting a source file triggers widen, never unit-smoke', (t) => { + // Arrange: fixture has an unrelated test but does NOT contain gone.cjs. + // The deletion-only diff (changedFiles includes gone.cjs which is absent on disk) + // must trigger the widen backstop — gone.cjs has no static dependents because + // it was never built into the forward graph (it doesn't exist). + const dir = makeFixture({ + 'get-shit-done/bin/lib/other.cjs': `'use strict';\nmodule.exports = {};\n`, + 'tests/unrelated.test.cjs': `'use strict';\n// requires nothing from gone.cjs\n`, + }); + t.after(() => fs.rmSync(dir, { recursive: true, force: true })); + + // Act + const allTests = ['tests/unrelated.test.cjs']; + const reverseIndex = buildTransitiveReverseIndex(dir, allTests); + + // gone.cjs does not exist in the fixture — simulates a delete-only PR. + const changedFiles = ['get-shit-done/bin/lib/gone.cjs']; + + const selected = pickAffectedTests(changedFiles, allTests, reverseIndex, { detectWiden: true }); + + // Assert: widen must be signalled (deleted source file has no static dependents) + assert.ok( + selected._widenRequired === true, + `Expected _widenRequired=true for deleted source file, got: ${JSON.stringify(selected._widenRequired)}`, + ); + + const plan = resolveRunPlan({ + changedFiles, + selected, + widenRequired: selected._widenRequired === true, + criticalPath: false, + noChanges: false, + }); + + // The plan must be mode:suites (widen), NOT mode:suite/unit and NOT empty. + assert.equal( + plan.mode, + 'suites', + `Delete-only source diff must resolve to mode:suites, got mode:${plan.mode} — old ACMR filter would have produced empty changedFiles → mode:suite/unit (smoke only), silently skipping integration/security`, + ); + assert.deepEqual( + plan.suites, + PR_FULL_SUITES, + `Delete-only widen plan must cover all PR_FULL_SUITES, got: ${JSON.stringify(plan.suites)}`, + ); +}); + +// --------------------------------------------------------------------------- +// Regression: rename-stale-old-path +// --------------------------------------------------------------------------- + +test('regression(rename-stale-old-path): deleted old path triggers widen, protecting stale importers', (t) => { + // Arrange: simulate the DELETE side of a rename. + // oldname.cjs is absent from disk (renamed away); newname.cjs is present with + // a concrete test dependent. A stale importer still requiring oldname.cjs + // would break at runtime, but static analysis cannot see it (the stale importer + // is not in this fixture). The correct safe behaviour is to widen, not to + // select only newname.cjs's dependents — which would silently skip the stale + // importer's tests. + // + // With --no-renames, git emits Delete(oldname.cjs) + Add(newname.cjs). + // oldname.cjs is absent on disk → not in the forward graph → zero static + // dependents → widen backstop fires → mode:suites (PR_FULL_SUITES). + const dir = makeFixture({ + // newname.cjs exists; oldname.cjs intentionally absent (it was renamed away) + 'get-shit-done/bin/lib/newname.cjs': `'use strict';\nmodule.exports = { v: 2 };\n`, + 'tests/newname.test.cjs': `'use strict';\nconst x = require('../get-shit-done/bin/lib/newname.cjs');\n`, + 'tests/unrelated.test.cjs': `'use strict';\n// no dependency on oldname or newname\n`, + }); + t.after(() => fs.rmSync(dir, { recursive: true, force: true })); + + // Act + const allTests = ['tests/newname.test.cjs', 'tests/unrelated.test.cjs']; + const reverseIndex = buildTransitiveReverseIndex(dir, allTests); + + // changedFiles mirrors what --no-renames git diff emits for a rename: + // Delete(old) + Add(new) + const changedFiles = [ + 'get-shit-done/bin/lib/oldname.cjs', // deleted old path — absent from disk + 'get-shit-done/bin/lib/newname.cjs', // added new path — present on disk + ]; + + // Assert: oldname.cjs must have zero static dependents (not in graph) + const oldDependents = reverseIndex.get('get-shit-done/bin/lib/oldname.cjs'); + assert.ok( + !oldDependents || oldDependents.size === 0, + `oldname.cjs must have no static dependents (absent from disk), got: ${JSON.stringify(oldDependents && [...oldDependents])}`, + ); + + const selected = pickAffectedTests(changedFiles, allTests, reverseIndex, { detectWiden: true }); + + // Widen must be signalled because oldname.cjs has no static dependents + assert.ok( + selected._widenRequired === true, + `Expected _widenRequired=true because deleted old path has no static dependents; got: ${JSON.stringify(selected._widenRequired)}`, + ); + + const plan = resolveRunPlan({ + changedFiles, + selected, + widenRequired: selected._widenRequired === true, + criticalPath: false, + noChanges: false, + }); + + // Plan must be mode:suites — conservative widen covers any stale importer + assert.equal( + plan.mode, + 'suites', + `Rename (old-path delete) must resolve to mode:suites, got mode:${plan.mode}`, + ); + assert.deepEqual( + plan.suites, + PR_FULL_SUITES, + `Rename widen plan must cover all PR_FULL_SUITES, got: ${JSON.stringify(plan.suites)}`, + ); +}); + +test('regression(delete-only-test): deleting a test file does not trigger widen and does not select the absent test', (t) => { + // Arrange: fixture has one surviving test; tests/gone.test.cjs is NOT on disk. + const dir = makeFixture({ + 'tests/surviving.test.cjs': `'use strict';\n// a plain surviving unit test\n`, + }); + t.after(() => fs.rmSync(dir, { recursive: true, force: true })); + + // Act + // allTests comes from the fixture's tests/ directory — gone.test.cjs is absent. + const allTests = ['tests/surviving.test.cjs']; + const reverseIndex = buildTransitiveReverseIndex(dir, allTests); + + const changedFiles = ['tests/gone.test.cjs']; + + const selected = pickAffectedTests(changedFiles, allTests, reverseIndex, { detectWiden: true }); + + // Assert: the deleted test file must NOT appear in selected (can't run it). + assert.ok( + !selected.includes('tests/gone.test.cjs'), + `Deleted test file must not appear in selection, got: ${JSON.stringify(selected)}`, + ); + + // Assert: widen must NOT be triggered (a deleted test is not impactful for selection). + assert.ok( + selected._widenRequired !== true, + `Deleted test file must not trigger widen, got _widenRequired=${JSON.stringify(selected._widenRequired)}`, + ); + + const plan = resolveRunPlan({ + changedFiles, + selected, + widenRequired: selected._widenRequired === true, + criticalPath: false, + noChanges: false, + }); + + // A delete-only test-file diff with no surviving selection → unit smoke (not widen). + assert.equal( + plan.mode, + 'suite', + `Delete-only test-file diff must resolve to mode:suite (smoke), got mode:${plan.mode}`, + ); + assert.equal( + plan.suite, + 'unit', + `Delete-only test-file diff must run unit smoke, got suite:${plan.suite}`, + ); +});