From 8c8887f00ed3bc6971c7b84494d94d2dce943144 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Wed, 27 May 2026 20:21:43 -0400 Subject: [PATCH] fix(#370): scope affected-tests runner to PR suites, exclude push-only install/slow (#395) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Root cause: the affected-tests runner called runAllSuites() on critical-path changes (running every suite including install/slow on all matrix cells including Windows), and pickAffectedTests injected DEFAULT_SMOKE_TESTS (an install test) as the empty-selection fallback — causing install suite tests to run on PR lanes where they are push-only per docs/TESTING-SUITES.md. Fix: PR_EXCLUDED_SUITES filter at the pickAffectedTests chokepoint strips install/slow from every selection path (direct-change, reverse-index, stem-match). Empty selection now returns [] and the caller runs the unit suite as smoke. Critical-path fallback replaces runAllSuites with PR_FULL_SUITES (unit, integration, security). suiteOf exported from run-tests.cjs (with require.main guard) so affected-tests-lib reuses canonical detection. Fixes #370 Co-authored-by: Claude Opus 4.7 (1M context) --- ...370-affected-tests-exclude-install-slow.md | 5 ++ scripts/affected-tests-lib.cjs | 48 +++++++----- scripts/run-tests.cjs | 6 +- tests/affected-tests-lib.test.cjs | 77 +++++++++++++++++-- 4 files changed, 108 insertions(+), 28 deletions(-) create mode 100644 .changeset/370-affected-tests-exclude-install-slow.md diff --git a/.changeset/370-affected-tests-exclude-install-slow.md b/.changeset/370-affected-tests-exclude-install-slow.md new file mode 100644 index 000000000..cc783f430 --- /dev/null +++ b/.changeset/370-affected-tests-exclude-install-slow.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 370 +--- +**Affected-tests PR runner no longer selects or runs `install`/`slow` suites (#370)** — `pickAffectedTests` now filters out any file whose suite is `install` or `slow` at the single chokepoint, covering direct-change, reverse-index, and stem-match selections. The `DEFAULT_SMOKE_TESTS` install-file fallback injection is removed; an empty selection now runs `unit` as the smoke fallback. The critical-path branch replaces `runAllSuites` (which ran every suite including `install`/`slow`) with `PR_FULL_SUITES` (`unit`, `integration`, `security`). `suiteOf` is exported from `run-tests.cjs` (guarded by `require.main`) so the affected-tests lib can reuse the canonical suite-detection logic without duplication. diff --git a/scripts/affected-tests-lib.cjs b/scripts/affected-tests-lib.cjs index 9fe5f32cf..b975625d1 100644 --- a/scripts/affected-tests-lib.cjs +++ b/scripts/affected-tests-lib.cjs @@ -4,6 +4,8 @@ const { execFileSync } = require('node:child_process'); const { readdirSync, readFileSync, existsSync } = require('node:fs'); const path = require('node:path'); +const { suiteOf } = require('./run-tests.cjs'); + const CRITICAL_PATHS = [ '.github/workflows/', 'package.json', @@ -13,9 +15,11 @@ const CRITICAL_PATHS = [ 'scripts/run-affected-tests.cjs', ]; -const DEFAULT_SMOKE_TESTS = [ - 'tests/release-tarball-smoke.install.test.cjs', -]; +// Suites that are push-only. PRs must never select or run these. +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']; function toPosixPath(input) { return input.split(path.sep).join('/'); @@ -87,7 +91,7 @@ function listTestFiles(repoRoot) { .sort(); } -function pickAffectedTests(changedFiles, allTests, reverseIndex, smokeTests) { +function pickAffectedTests(changedFiles, allTests, reverseIndex) { const selected = new Set(); for (const file of changedFiles) { @@ -108,12 +112,14 @@ function pickAffectedTests(changedFiles, allTests, reverseIndex, smokeTests) { } } - if (selected.size === 0) { - for (const smokeTest of smokeTests) { - if (allTests.includes(smokeTest)) selected.add(smokeTest); - } + // 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) { + const suite = suiteOf(path.basename(file)); + if (PR_EXCLUDED_SUITES.has(suite)) selected.delete(file); } + // When nothing maps, return an empty array. The caller decides the fallback. return [...selected].sort(); } @@ -182,14 +188,6 @@ function runSuite(repoRoot, suite) { }); } -function runAllSuites(repoRoot) { - execFileSync(process.execPath, ['scripts/run-tests.cjs'], { - cwd: repoRoot, - stdio: 'inherit', - env: { ...process.env }, - }); -} - function resolveBaseRef() { if (process.env.GSD_AFFECTED_BASE) return process.env.GSD_AFFECTED_BASE; if (process.env.GITHUB_BASE_REF) return `origin/${process.env.GITHUB_BASE_REF}`; @@ -198,7 +196,6 @@ function resolveBaseRef() { function runAffectedTests(options = {}) { const repoRoot = options.repoRoot || path.resolve(__dirname, '..'); - const smokeTests = options.smokeTests || DEFAULT_SMOKE_TESTS; const baseRef = options.baseRef || resolveBaseRef(); const changed = changedFilesSinceBase(repoRoot, baseRef); @@ -209,24 +206,33 @@ function runAffectedTests(options = {}) { } if (shouldRunFullSuite(changed)) { - console.error('affected-tests: critical CI/runtime files changed; running full suite'); - runAllSuites(repoRoot); + console.error('affected-tests: critical CI/runtime files changed; running PR suites (unit, integration, security)'); + for (const suite of PR_FULL_SUITES) { + runSuite(repoRoot, suite); + } return; } const allTests = listTestFiles(repoRoot); const reverseIndex = buildReverseIndex(repoRoot, allTests); - const selected = pickAffectedTests(changed, allTests, reverseIndex, smokeTests); + const selected = pickAffectedTests(changed, allTests, reverseIndex); 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'); + return; + } + runNodeTestFiles(repoRoot, selected); } module.exports = { CRITICAL_PATHS, - DEFAULT_SMOKE_TESTS, + PR_EXCLUDED_SUITES, + PR_FULL_SUITES, buildReverseIndex, parseRelativeSpecifiers, pickAffectedTests, diff --git a/scripts/run-tests.cjs b/scripts/run-tests.cjs index 43b31cb4c..c2820d06d 100644 --- a/scripts/run-tests.cjs +++ b/scripts/run-tests.cjs @@ -275,4 +275,8 @@ function main() { if (firstFailureExit !== 0) process.exit(firstFailureExit); } -main(); +if (require.main === module) { + main(); +} + +module.exports = { suiteOf }; diff --git a/tests/affected-tests-lib.test.cjs b/tests/affected-tests-lib.test.cjs index 47b4f0536..78bc24971 100644 --- a/tests/affected-tests-lib.test.cjs +++ b/tests/affected-tests-lib.test.cjs @@ -8,6 +8,8 @@ const { pickAffectedTests, shouldRunFullSuite, resolveBaseRef, + PR_EXCLUDED_SUITES, + PR_FULL_SUITES, } = require('../scripts/affected-tests-lib.cjs'); test('parseRelativeSpecifiers captures local require/import paths', () => { @@ -27,11 +29,11 @@ test('shouldRunFullSuite true when critical paths change', () => { assert.equal(shouldRunFullSuite(['tests/foo.test.cjs']), false); }); -test('pickAffectedTests includes direct test changes and reverse-index matches', () => { +test('pickAffectedTests includes direct test changes and reverse-index matches, excluding install suite', () => { const allTests = [ 'tests/alpha.test.cjs', 'tests/install.test.cjs', - 'tests/release-tarball-smoke.install.test.cjs', + 'tests/tarball.install.test.cjs', ]; const reverse = new Map([ ['bin/install.js', new Set(['tests/install.test.cjs'])], @@ -40,12 +42,12 @@ test('pickAffectedTests includes direct test changes and reverse-index matches', ['tests/alpha.test.cjs', 'bin/install.js'], allTests, reverse, - ['tests/release-tarball-smoke.install.test.cjs'], ); + // tests/install.test.cjs is a plain unit test (no install suite marker) so it is included. + // tests/tarball.install.test.cjs is install suite — excluded. assert.deepEqual(selected, [ 'tests/alpha.test.cjs', 'tests/install.test.cjs', - 'tests/release-tarball-smoke.install.test.cjs', ]); }); @@ -55,9 +57,72 @@ test('pickAffectedTests falls back to smoke test when no matches found', () => { ['docs/README.md'], allTests, new Map(), - ['tests/release-tarball-smoke.install.test.cjs'], ); - assert.deepEqual(selected, ['tests/release-tarball-smoke.install.test.cjs']); + // New contract: install files are excluded; empty selection returns empty array. + assert.deepEqual(selected, []); +}); + +test('pickAffectedTests excludes a directly-changed install test file', () => { + // Even if the changed file IS an install test, it must be excluded from PR selection. + const allTests = [ + 'tests/foo.install.test.cjs', + 'tests/bar.test.cjs', + ]; + const selected = pickAffectedTests( + ['tests/foo.install.test.cjs'], + allTests, + new Map(), + ); + assert.ok(!selected.includes('tests/foo.install.test.cjs'), 'install test must be excluded'); + assert.deepEqual(selected, []); +}); + +test('pickAffectedTests excludes install/slow pulled in by stem match', () => { + // A changed source file whose stem matches an install or slow test file. + const allTests = [ + 'tests/release-tarball.install.test.cjs', + 'tests/perf-check.slow.test.cjs', + 'tests/release-tarball.test.cjs', + ]; + const selected = pickAffectedTests( + ['src/release-tarball.cjs'], + allTests, + new Map(), + ); + assert.ok(!selected.includes('tests/release-tarball.install.test.cjs'), 'install suite must be excluded via stem match'); + assert.ok(!selected.includes('tests/perf-check.slow.test.cjs'), 'slow suite must be excluded'); + // The plain unit test matched by stem is still included. + assert.ok(selected.includes('tests/release-tarball.test.cjs'), 'unit test matched by stem should be included'); +}); + +test('pickAffectedTests returns empty (no install smoke) when nothing maps', () => { + const allTests = [ + 'tests/release-tarball-smoke.install.test.cjs', + 'tests/some-unit.test.cjs', + ]; + // Changed file has no match and no stem match with unit tests. + const selected = pickAffectedTests( + ['docs/CONTRIBUTING.md'], + allTests, + new Map(), + ); + assert.ok( + !selected.includes('tests/release-tarball-smoke.install.test.cjs'), + 'install smoke test must not be injected as fallback', + ); + assert.deepEqual(selected, []); +}); + +test('PR_EXCLUDED_SUITES contains install and slow; PR_FULL_SUITES excludes them', () => { + assert.ok(PR_EXCLUDED_SUITES instanceof Set, 'PR_EXCLUDED_SUITES must be a Set'); + assert.ok(PR_EXCLUDED_SUITES.has('install'), 'install must be in PR_EXCLUDED_SUITES'); + assert.ok(PR_EXCLUDED_SUITES.has('slow'), 'slow must be in PR_EXCLUDED_SUITES'); + assert.ok(Array.isArray(PR_FULL_SUITES), 'PR_FULL_SUITES must be an array'); + assert.ok(PR_FULL_SUITES.includes('unit'), 'unit must be in PR_FULL_SUITES'); + assert.ok(PR_FULL_SUITES.includes('integration'), 'integration must be in PR_FULL_SUITES'); + assert.ok(PR_FULL_SUITES.includes('security'), 'security must be in PR_FULL_SUITES'); + assert.ok(!PR_FULL_SUITES.includes('install'), 'install must NOT be in PR_FULL_SUITES'); + assert.ok(!PR_FULL_SUITES.includes('slow'), 'slow must NOT be in PR_FULL_SUITES'); }); test('resolveBaseRef prefers explicit env override', () => {