fix(#370): scope affected-tests runner to PR suites, exclude push-only install/slow (#395)

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) <noreply@anthropic.com>

.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.

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,

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 };

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', () => {