From 6313baad63bc9981e8bc6309d718d9f2cd828de2 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Wed, 20 May 2026 20:37:07 -0400 Subject: [PATCH] =?UTF-8?q?chore(tests):=20lint=20rule=20=E2=80=94=20cap?= =?UTF-8?q?=20test=20files=20per=20production=20module=20at=202=20(#3738)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * chore(tests): lint rule — cap test files per production module at 2 Adds scripts/lint-test-file-count.cjs with a ratcheted allowlist (scripts/lint-test-file-count.allowlist.json) capturing today's 30 violating clusters as a ceiling. New entries blocked at PR time; reductions ratchet automatically. Wires into .github/workflows/test.yml as a new step in lint-tests. Refs #3737 Co-Authored-By: Claude Sonnet 4.6 * chore(tests): add docs-exempt to changeset fragment Internal CI lint rule — no user-facing docs impact. Co-Authored-By: Claude Sonnet 4.6 --------- Co-authored-by: Claude Sonnet 4.6 --- .changeset/3733-lint-test-file-count.md | 6 + .github/workflows/test.yml | 3 + package.json | 1 + scripts/lint-test-file-count.allowlist.json | 35 +++ scripts/lint-test-file-count.cjs | 190 ++++++++++++++++ tests/lint-test-file-count.test.cjs | 227 ++++++++++++++++++++ 6 files changed, 462 insertions(+) create mode 100644 .changeset/3733-lint-test-file-count.md create mode 100644 scripts/lint-test-file-count.allowlist.json create mode 100644 scripts/lint-test-file-count.cjs create mode 100644 tests/lint-test-file-count.test.cjs diff --git a/.changeset/3733-lint-test-file-count.md b/.changeset/3733-lint-test-file-count.md new file mode 100644 index 000000000..562a6ad56 --- /dev/null +++ b/.changeset/3733-lint-test-file-count.md @@ -0,0 +1,6 @@ +--- +type: Added +pr: 3733 +--- + +**New CI lint rule `lint-test-file-count` prevents per-feature test-file proliferation** — a recurring pattern where one production module (e.g. `phase.cjs`, `init.cjs`) accumulates 5–20 separate test files over time as PRs add issue-stamped files (`bug-NNNN-*.test.cjs`, `feat-NNNN-*.test.cjs`) next to an existing primary. The rule scans `sdk/src/query/`, `sdk/src/`, `get-shit-done/bin/lib/`, and `bin/` for production modules, then counts matching test files in `tests/` and `sdk/src/**/`. Each module is capped at 2 (primary + one integration). Existing over-limit clusters are frozen in `scripts/lint-test-file-count.allowlist.json` at their current count (30 modules; `phase` is the worst at 20 files). The allowlist ratchets downward automatically — reducing a cluster is always allowed; increasing it requires a PR-description justification. Added `"lint:test-file-count"` npm script and a `Lint — test file count per module` step in the `lint-tests` CI job. diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index f0ed6f1e2..97ff40f66 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -30,6 +30,9 @@ jobs: - name: Lint — no source-grep tests shell: bash run: node scripts/lint-no-source-grep.cjs + - name: Lint — test file count per module + shell: bash + run: node scripts/lint-test-file-count.cjs - name: Lint — command contract (ADR-0002) shell: bash run: node scripts/lint-command-contract.cjs diff --git a/package.json b/package.json index 1424e47c5..bf2b74652 100644 --- a/package.json +++ b/package.json @@ -76,6 +76,7 @@ "lint:descriptions": "node scripts/lint-descriptions.cjs", "lint:skill-deps": "node scripts/lint-skill-deps.cjs", "lint:tests": "node scripts/lint-no-source-grep.cjs", + "lint:test-file-count": "node scripts/lint-test-file-count.cjs", "lint:pr-checks": "node scripts/lint-pr-check-project-dir.cjs", "lint:changeset": "node scripts/changeset/lint.cjs", "lint:docs": "node scripts/lint-docs-required.cjs", diff --git a/scripts/lint-test-file-count.allowlist.json b/scripts/lint-test-file-count.allowlist.json new file mode 100644 index 000000000..d33af199c --- /dev/null +++ b/scripts/lint-test-file-count.allowlist.json @@ -0,0 +1,35 @@ +{ + "_doc": "Baseline of modules currently exceeding the 2-test-file limit. Each entry locks in TODAY's count as the ceiling. Reductions are ratcheted automatically — when a cluster drops to ≤ 2, remove its entry. New entries require justification in PR description.", + "modules": { + "phase": { "current": 20, "issue": "TBD" }, + "worktree": { "current": 13, "issue": "TBD" }, + "milestone": { "current": 10, "issue": "TBD" }, + "roadmap": { "current": 9, "issue": "TBD" }, + "verify": { "current": 9, "issue": "TBD" }, + "install": { "current": 9, "issue": "TBD" }, + "init": { "current": 8, "issue": "TBD" }, + "state": { "current": 9, "issue": "TBD" }, + "config": { "current": 8, "issue": "TBD" }, + "graphify": { "current": 7, "issue": "TBD" }, + "progress": { "current": 5, "issue": "TBD" }, + "cli": { "current": 5, "issue": "TBD" }, + "surface": { "current": 5, "issue": "TBD" }, + "commit": { "current": 4, "issue": "TBD" }, + "frontmatter": { "current": 4, "issue": "TBD" }, + "index": { "current": 4, "issue": "TBD" }, + "intel": { "current": 4, "issue": "TBD" }, + "mvp": { "current": 4, "issue": "TBD" }, + "install-profiles": { "current": 4, "issue": "TBD" }, + "audit-open": { "current": 3, "issue": "TBD" }, + "config-schema": { "current": 3, "issue": "TBD" }, + "profile": { "current": 3, "issue": "TBD" }, + "prompt-budget": { "current": 3, "issue": "TBD" }, + "uat": { "current": 3, "issue": "TBD" }, + "validate": { "current": 3, "issue": "TBD" }, + "workstream": { "current": 3, "issue": "TBD" }, + "gsd-tools": { "current": 3, "issue": "TBD" }, + "runtime-artifact-layout":{ "current": 3, "issue": "TBD" }, + "security": { "current": 3, "issue": "TBD" }, + "gsd-sdk": { "current": 3, "issue": "TBD" } + } +} diff --git a/scripts/lint-test-file-count.cjs b/scripts/lint-test-file-count.cjs new file mode 100644 index 000000000..c65f4f40f --- /dev/null +++ b/scripts/lint-test-file-count.cjs @@ -0,0 +1,190 @@ +#!/usr/bin/env node +/** + * lint-test-file-count.cjs — max 2 test files per production module. + * + * Scans sdk/src/query/, sdk/src/, get-shit-done/bin/lib/, bin/ for production + * modules, then counts matching test files in tests/ and sdk/src (recursive). Cap is 2 + * (primary + one integration). Over-limit clusters must be in the allowlist at + * their frozen count (ratchet: may only decrease). --json emits structured output. + * + * Verdicts: OK_UNDER_LIMIT | OK_IN_ALLOWLIST | FAIL_EXCEEDS_LIMIT | + * FAIL_EXCEEDS_ALLOWLIST | HINT_CAN_REMOVE_FROM_ALLOWLIST + */ +'use strict'; + +const fs = require('fs'); +const path = require('path'); + +const ROOT = path.join(__dirname, '..'); +const PROD_DIRS = [ + path.join(ROOT, 'sdk', 'src', 'query'), + path.join(ROOT, 'sdk', 'src'), + path.join(ROOT, 'get-shit-done', 'bin', 'lib'), + path.join(ROOT, 'bin'), +]; +const TEST_DIRS = [ + path.join(ROOT, 'tests'), + path.join(ROOT, 'sdk', 'src'), +]; +const ALLOWLIST_PATH = path.join(__dirname, 'lint-test-file-count.allowlist.json'); +const MAX_FILES = 2; + +const Verdict = Object.freeze({ + OK_UNDER_LIMIT: 'OK_UNDER_LIMIT', + OK_IN_ALLOWLIST: 'OK_IN_ALLOWLIST', + FAIL_EXCEEDS_LIMIT: 'FAIL_EXCEEDS_LIMIT', + FAIL_EXCEEDS_ALLOWLIST: 'FAIL_EXCEEDS_ALLOWLIST', + HINT_CAN_REMOVE_FROM_ALLOWLIST: 'HINT_CAN_REMOVE_FROM_ALLOWLIST', +}); + +function isTestFile(name) { + return name.endsWith('.test.ts') || name.endsWith('.test.cjs'); +} + +function listFiles(dir, pred) { + try { + return fs.readdirSync(dir, { withFileTypes: true }) + .filter(e => e.isFile() && pred(e.name)) + .map(e => path.join(dir, e.name)); + } catch (_) { return []; } +} + +function findTestFilesRecursive(dir) { + const out = []; + let entries; + try { entries = fs.readdirSync(dir, { withFileTypes: true }); } + catch (_) { return out; } + for (const e of entries) { + const full = path.join(dir, e.name); + if (e.isDirectory()) out.push(...findTestFilesRecursive(full)); + else if (isTestFile(e.name)) out.push(full); + } + return out; +} + +function prodPrefix(filename) { + return filename.replace(/\.(cjs|ts|js)$/, ''); +} + +// Strip .test.{cjs,ts} and .integration.test.ts, then strip issue stamps. +function testEffectivePrefix(testName) { + const bare = testName + .replace(/\.integration\.test\.(ts|cjs)$/, '') + .replace(/\.test\.(ts|cjs)$/, ''); + const m = bare.match(/^(?:feat|bug|enh|fix)-\d+(?:-\d+)*-(.+)$/); + return m ? m[1] : bare; +} + +function collectProdPrefixes() { + const map = new Map(); + for (const dir of PROD_DIRS) { + for (const f of listFiles(dir, n => + !isTestFile(n) && + !/\.(generated|md|json)(\.|$)/.test(n) && + /\.(ts|cjs|js)$/.test(n) + )) { + const prefix = prodPrefix(path.basename(f)); + if (!map.has(prefix)) map.set(prefix, f); + } + } + return map; +} + +function collectAllTestFiles() { + const seen = new Set(); + const all = []; + for (const dir of TEST_DIRS) { + for (const f of findTestFilesRecursive(dir)) { + if (!seen.has(f)) { seen.add(f); all.push(f); } + } + } + return all; +} + +function buildTestMap(prodPrefixes, allTestFiles) { + const map = new Map([...prodPrefixes.keys()].map(p => [p, []])); + for (const tf of allTestFiles) { + const ep = testEffectivePrefix(path.basename(tf)); + for (const prefix of prodPrefixes.keys()) { + if (ep === prefix || ep.startsWith(prefix + '-')) { + map.get(prefix).push(tf); + break; + } + } + } + return map; +} + +function loadAllowlist() { + try { return JSON.parse(fs.readFileSync(ALLOWLIST_PATH, 'utf-8')).modules || {}; } + catch (_) { return {}; } +} + +function evaluateLint({ prefix, testFiles, allowlist }) { + const count = testFiles.length; + const entry = allowlist[prefix]; + const ceiling = entry ? entry.current : null; + if (entry !== undefined) { + if (count <= MAX_FILES) return { verdict: Verdict.HINT_CAN_REMOVE_FROM_ALLOWLIST, prefix, count, ceiling, files: testFiles }; + if (count <= ceiling) return { verdict: Verdict.OK_IN_ALLOWLIST, prefix, count, ceiling, files: testFiles }; + return { verdict: Verdict.FAIL_EXCEEDS_ALLOWLIST, prefix, count, ceiling, files: testFiles }; + } + if (count <= MAX_FILES) return { verdict: Verdict.OK_UNDER_LIMIT, prefix, count, ceiling: null, files: testFiles }; + return { verdict: Verdict.FAIL_EXCEEDS_LIMIT, prefix, count, ceiling: null, files: testFiles }; +} + +function run() { + const jsonMode = process.argv.includes('--json'); + const prodPrefixes = collectProdPrefixes(); + const allTestFiles = collectAllTestFiles(); + const testMap = buildTestMap(prodPrefixes, allTestFiles); + const allowlist = loadAllowlist(); + + const results = []; + for (const [prefix, files] of testMap) { + if (files.length === 0) continue; + results.push(evaluateLint({ prefix, testFiles: files, allowlist })); + } + + const failures = results.filter(r => + r.verdict === Verdict.FAIL_EXCEEDS_LIMIT || r.verdict === Verdict.FAIL_EXCEEDS_ALLOWLIST); + const hints = results.filter(r => r.verdict === Verdict.HINT_CAN_REMOVE_FROM_ALLOWLIST); + + if (jsonMode) { + console.log(JSON.stringify({ ok: failures.length === 0, results, failures, hints }, null, 2)); + process.exit(failures.length > 0 ? 1 : 0); + } + + if (failures.length === 0) { + const inAllowlist = results.filter(r => r.verdict === Verdict.OK_IN_ALLOWLIST).length; + console.log(`ok lint-test-file-count: ${results.length} module(s) checked, 0 failures` + + (inAllowlist > 0 ? `, ${inAllowlist} allowlisted` : '') + + (hints.length > 0 ? `, ${hints.length} hint(s)` : '')); + for (const h of hints) { + console.log(` hint: "${h.prefix}" is allowlisted at ${h.ceiling} but now has ${h.count} — remove from allowlist`); + } + process.exit(0); + } + + process.stderr.write(`\nERROR lint-test-file-count: ${failures.length} module(s) exceed the test-file limit\n\n`); + for (const f of failures) { + const tag = f.verdict === Verdict.FAIL_EXCEEDS_LIMIT + ? `${f.count} files (limit ${MAX_FILES})` + : `${f.count} files (allowlist ceiling ${f.ceiling})`; + process.stderr.write(` ${f.prefix}: ${tag}\n`); + for (const tf of f.files) process.stderr.write(` ${path.relative(ROOT, tf)}\n`); + } + process.stderr.write('\nFix: consolidate test files per module (one primary + one integration).\n'); + process.stderr.write('Or add the module to scripts/lint-test-file-count.allowlist.json with PR justification.\n\n'); + process.exit(1); +} + +module.exports = { + Verdict, evaluateLint, testEffectivePrefix, prodPrefix, + _collectProdPrefixes: collectProdPrefixes, + _collectAllTestFiles: collectAllTestFiles, + _buildTestMap: buildTestMap, + _loadAllowlist: loadAllowlist, +}; + +if (require.main === module) run(); diff --git a/tests/lint-test-file-count.test.cjs b/tests/lint-test-file-count.test.cjs new file mode 100644 index 000000000..02671d473 --- /dev/null +++ b/tests/lint-test-file-count.test.cjs @@ -0,0 +1,227 @@ +'use strict'; + +/** + * Tests for scripts/lint-test-file-count.cjs + * + * Uses node --test + the exported evaluateLint() pure function. + * Also exercises the CLI via --json mode to verify end-to-end wiring. + */ + +const { describe, test } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('fs'); +const os = require('os'); +const path = require('path'); +const { spawnSync } = require('child_process'); + +const ROOT = path.join(__dirname, '..'); +const LINT_SCRIPT = path.join(ROOT, 'scripts', 'lint-test-file-count.cjs'); + +const { + Verdict, + evaluateLint, + testEffectivePrefix, +} = require(LINT_SCRIPT); + +// --------------------------------------------------------------------------- +// Helpers +// --------------------------------------------------------------------------- + +function makeFiles(prefix, names) { + return names.map(n => `/fake/tests/${n}`); +} + +function runCliJson(extraArgs = []) { + const result = spawnSync( + process.execPath, + [LINT_SCRIPT, '--json', ...extraArgs], + { encoding: 'utf8' } + ); + const parsed = JSON.parse(result.stdout); + return { status: result.status, data: parsed }; +} + +// --------------------------------------------------------------------------- +// evaluateLint — core verdict logic +// --------------------------------------------------------------------------- + +describe('evaluateLint — OK_UNDER_LIMIT', () => { + test('1-file module passes', () => { + const result = evaluateLint({ + prefix: 'my-module', + testFiles: makeFiles('my-module', ['my-module.test.cjs']), + allowlist: {}, + }); + assert.strictEqual(result.verdict, Verdict.OK_UNDER_LIMIT); + assert.strictEqual(result.count, 1); + assert.strictEqual(result.ceiling, null); + }); + + test('2-file module passes (primary + integration)', () => { + const result = evaluateLint({ + prefix: 'my-module', + testFiles: makeFiles('my-module', [ + 'my-module.test.cjs', + 'my-module.integration.test.ts', + ]), + allowlist: {}, + }); + assert.strictEqual(result.verdict, Verdict.OK_UNDER_LIMIT); + assert.strictEqual(result.count, 2); + }); +}); + +describe('evaluateLint — FAIL_EXCEEDS_LIMIT', () => { + test('3-file module fails when not in allowlist', () => { + const result = evaluateLint({ + prefix: 'my-module', + testFiles: makeFiles('my-module', [ + 'my-module.test.cjs', + 'my-module-edge-case.test.cjs', + 'my-module-regression.test.cjs', + ]), + allowlist: {}, + }); + assert.strictEqual(result.verdict, Verdict.FAIL_EXCEEDS_LIMIT); + assert.strictEqual(result.count, 3); + assert.strictEqual(result.ceiling, null); + }); +}); + +describe('evaluateLint — allowlist behaviour', () => { + test('3-file module allowlisted at 3 passes (OK_IN_ALLOWLIST)', () => { + const result = evaluateLint({ + prefix: 'phase', + testFiles: makeFiles('phase', [ + 'phase.test.cjs', + 'phase-edge.test.cjs', + 'phase-regression.test.cjs', + ]), + allowlist: { phase: { current: 3, issue: 'TBD' } }, + }); + assert.strictEqual(result.verdict, Verdict.OK_IN_ALLOWLIST); + assert.strictEqual(result.count, 3); + assert.strictEqual(result.ceiling, 3); + }); + + test('2-file module allowlisted at 3 emits HINT_CAN_REMOVE_FROM_ALLOWLIST', () => { + const result = evaluateLint({ + prefix: 'phase', + testFiles: makeFiles('phase', [ + 'phase.test.cjs', + 'phase-edge.test.cjs', + ]), + allowlist: { phase: { current: 3, issue: 'TBD' } }, + }); + assert.strictEqual(result.verdict, Verdict.HINT_CAN_REMOVE_FROM_ALLOWLIST); + assert.strictEqual(result.count, 2); + assert.strictEqual(result.ceiling, 3); + }); + + test('4-file module allowlisted at 3 fails (FAIL_EXCEEDS_ALLOWLIST)', () => { + const result = evaluateLint({ + prefix: 'phase', + testFiles: makeFiles('phase', [ + 'phase.test.cjs', + 'phase-a.test.cjs', + 'phase-b.test.cjs', + 'phase-c.test.cjs', + ]), + allowlist: { phase: { current: 3, issue: 'TBD' } }, + }); + assert.strictEqual(result.verdict, Verdict.FAIL_EXCEEDS_ALLOWLIST); + assert.strictEqual(result.count, 4); + assert.strictEqual(result.ceiling, 3); + }); + + test('ratchet: count equal to ceiling passes', () => { + const result = evaluateLint({ + prefix: 'init', + testFiles: makeFiles('init', [ + 'init.test.cjs', + 'init-manager.test.cjs', + 'init-manager-deps.test.cjs', + ]), + allowlist: { init: { current: 3, issue: 'TBD' } }, + }); + assert.strictEqual(result.verdict, Verdict.OK_IN_ALLOWLIST); + }); +}); + +// --------------------------------------------------------------------------- +// testEffectivePrefix — issue-stamp stripping +// --------------------------------------------------------------------------- + +describe('testEffectivePrefix', () => { + test('normal test file returns bare prefix', () => { + assert.strictEqual(testEffectivePrefix('query-dispatch.test.cjs'), 'query-dispatch'); + }); + + test('integration test file returns bare prefix', () => { + assert.strictEqual(testEffectivePrefix('init.integration.test.ts'), 'init'); + }); + + test('bug-stamped file strips stamp', () => { + assert.strictEqual(testEffectivePrefix('bug-1736-local-install-commands.test.cjs'), 'local-install-commands'); + }); + + test('feat-stamped file strips stamp', () => { + assert.strictEqual(testEffectivePrefix('feat-3347-graphify-auto-update-config.test.cjs'), 'graphify-auto-update-config'); + }); + + test('enh-stamped file strips stamp', () => { + assert.strictEqual(testEffectivePrefix('enh-100-phase-runner-edge.test.cjs'), 'phase-runner-edge'); + }); + + test('fix-stamped file strips stamp', () => { + assert.strictEqual(testEffectivePrefix('fix-200-config-merge.test.cjs'), 'config-merge'); + }); + + test('double-numbered stamp is stripped correctly', () => { + assert.strictEqual(testEffectivePrefix('bug-2550-2552-discuss-phase-context.test.cjs'), 'discuss-phase-context'); + }); +}); + +// --------------------------------------------------------------------------- +// CLI — JSON mode end-to-end +// --------------------------------------------------------------------------- + +describe('CLI --json', () => { + test('script parses without syntax errors', () => { + const result = spawnSync(process.execPath, ['--check', LINT_SCRIPT], { encoding: 'utf8' }); + assert.strictEqual(result.status, 0, result.stderr); + }); + + test('exits 0 against real repo (allowlist covers all current violations)', () => { + const { status, data } = runCliJson(); + assert.strictEqual(status, 0, `Expected clean run; failures: ${JSON.stringify(data.failures)}`); + assert.strictEqual(data.ok, true); + assert.strictEqual(data.failures.length, 0); + }); + + test('--json output has required fields', () => { + const { data } = runCliJson(); + assert.ok(Array.isArray(data.results), 'results must be array'); + assert.ok(Array.isArray(data.failures), 'failures must be array'); + assert.ok(Array.isArray(data.hints), 'hints must be array'); + assert.ok(typeof data.ok === 'boolean', 'ok must be boolean'); + }); + + test('each result has verdict, prefix, count, ceiling, files', () => { + const { data } = runCliJson(); + for (const r of data.results) { + assert.ok(typeof r.verdict === 'string', `verdict missing on ${r.prefix}`); + assert.ok(typeof r.prefix === 'string', 'prefix must be string'); + assert.ok(typeof r.count === 'number', 'count must be number'); + assert.ok(Array.isArray(r.files), 'files must be array'); + } + }); + + test('all verdicts are valid enum values', () => { + const valid = new Set(Object.values(Verdict)); + const { data } = runCliJson(); + for (const r of data.results) { + assert.ok(valid.has(r.verdict), `Unknown verdict "${r.verdict}" on prefix "${r.prefix}"`); + } + }); +});