diff --git a/docs/pregate.md b/docs/pregate.md index dc84ed95c..2f581f805 100644 --- a/docs/pregate.md +++ b/docs/pregate.md @@ -196,18 +196,25 @@ Two stages with separate clocks, both printed: transitively through non-test files; deleted and renamed files select the tests that named them. A removed path (a deleted file, or a rename's old name) that no remaining - test selects owes no lane: it prints `removed (no lane owed): — ` - and the `lanes` detail adds `N removed path(s)`. A rename's old name defers to - the new path (`old name of `); a deleted test runs its owning suite, the - remaining tests of the files its merge-base text exercised (`owning suite - runs: …`; test helpers and fixtures are not subjects), and names each file - left with no test (`coverage removed: has no remaining test`, and - `M subject(s) lost their last test` in the detail); any other deleted file - has nothing left to test (`no remaining test references it`). A deleted test - whose merge-base text cannot be read (a symlink, invalid UTF-8, over 1 MiB) - fails closed with `PREGATE_NO_LANE`, and a modified file whose only test the - branch deleted still has no lane. /review lists the deletion as a gate edit - (RH-1) either way. + test selects owes no lane when one of these holds, and prints + `removed (no lane owed): — ` (the `lanes` detail adds + `N removed path(s)`): a rename's old name defers to the new path (`old name + of `); a deleted test, or a test renamed to a non-test path, runs its + owning suite, the remaining tests that directly exercise the files its + merge-base text exercised (`owning suite runs: …`; test helpers and fixtures + are not subjects), and names each file left with no test at all (`coverage + removed: has no remaining test`, and `M subject(s) lost their last + test` in the detail); a deleted code file outside fixture, golden and CI + trees that no remaining non-Markdown file names reads `nothing in the tree + references it`. Every other removal owes what an edit owes, so it is still + `PREGATE_NO_LANE` until a test or a `dependencies` entry covers it: a + deleted manifest, lockfile, config, conftest.py, fixture, golden or workflow, + a deleted file a config or script still names, and a deleted test whose + merge-base text cannot be read (a symlink, invalid UTF-8, over 1 MiB). A + modified file whose only test the branch deleted still has no lane, a + remaining test that reads a deleted file or directory is selected, and a + deleted Rust module selects its crate's tests. /review lists a deleted test + as a gate edit (RH-1) either way. An extensionless JS/TS import (relative, a tsconfig/jsconfig `paths` alias or `baseUrl`) tries the exact path, then `.ts`, `.tsx`, `.js`, `.jsx`, `.mjs`, `.cjs`, `.mts`, `.cts`, then `index.ts`, `index.tsx`, `index.js`, `index.jsx` diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 5d2ad46e7..c986fa0d1 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -3211,12 +3211,15 @@ versions, line endings or any non-version byte changed, one under a test, fixture, golden, `.gstack/` or workflow tree, or one added, deleted, renamed or re-moded, is still this error; `--explain` prints `not version-stamp-only ()` for it. -A path the branch removed is not this error either: a deleted file, or a -rename's old name, that no remaining test selects prints `removed (no lane -owed): — ` instead (a deleted test runs its owning suite or says -`coverage removed: has no remaining test`). A modified file whose only -test the branch deleted is still this error, and so is a deleted test whose -merge-base text cannot be read (a symlink, invalid UTF-8, over 1 MiB). +A path the branch removed is usually not this error either: a rename's old +name, a deleted test, and a deleted code file nothing in the tree still names +print `removed (no lane owed): — ` instead (a deleted test runs its +owning suite or says `coverage removed: has no remaining test`). A +removal owes what an edit owes, so these are still this error: a deleted +manifest, lockfile, config, conftest.py, fixture, golden or workflow, a deleted +file a config or script still names, a deleted test whose merge-base text +cannot be read (a symlink, invalid UTF-8, over 1 MiB), and a modified file whose +only test the branch deleted. **Fix.** Add a test that names the file, or declare its lane (an explicit `[]` for files with no behaviour) under `dependencies` in `.gstack/pregate.json` (or the user-owned file the message names). diff --git a/lib/pregate/docs-lane.ts b/lib/pregate/docs-lane.ts index 3affa1132..930b16b57 100644 --- a/lib/pregate/docs-lane.ts +++ b/lib/pregate/docs-lane.ts @@ -41,6 +41,9 @@ function isMarkdown(rel: string): boolean { return MARKDOWN.test(rel); } +/** Fixture, snapshot and golden trees, CI workflows and actions, and `.gstack/`: files a test or the CI may read by a path the import graph cannot see. */ +export function isFixtureOrCiTree(rel: string): boolean { return PROTECTED_TREE.test(rel.replace(/\\/g, '/').replace(/^\.\//, '')); } + function isTestTree(rel: string): boolean { return TEST_TREE.test(rel); } diff --git a/lib/pregate/import-graph.ts b/lib/pregate/import-graph.ts index 164ecf0c8..45b6214e5 100644 --- a/lib/pregate/import-graph.ts +++ b/lib/pregate/import-graph.ts @@ -478,7 +478,7 @@ export function buildGraph(repoRoot: string, opts: BuildOptions = {}): Graph { const all = listRepoFiles(repoRoot).filter(f => !gone.has(f)); const files = new Set([...all, ...gone]); const dirs = new Set(); - for (const f of all) { let d = path.posix.dirname(f); while (d && d !== '.') { dirs.add(d); d = path.posix.dirname(d); } } + for (const f of files) { let d = path.posix.dirname(f); while (d && d !== '.') { dirs.add(d); d = path.posix.dirname(d); } } const config = readPathAliases(repoRoot); const graph: Graph = { files, tests: new Set(all.filter(isTestFile)), reverse: new Map(), unresolved: new Map(), aliasNotes: config.notes }; const maxBytes = opts.maxBytes ?? 2 * 1024 * 1024; @@ -493,8 +493,8 @@ export function buildGraph(repoRoot: string, opts: BuildOptions = {}): Graph { for (const { target, via } of referencesIn(files, dirs, rel, source, config, graph.tests.has(rel) ? unresolved : undefined, packages)) addEdge(graph, target, rel, via); if (unresolved.length) graph.unresolved!.set(rel, unresolved); } - const rust = rustEdges(all, rel => readRepoFile(repoRoot, rel, maxBytes)); - for (const t of rust.tests) graph.tests.add(t); + const rust = rustEdges([...files], rel => readRepoFile(repoRoot, rel, maxBytes)); + for (const t of rust.tests) if (!gone.has(t)) graph.tests.add(t); for (const e of rust.edges) addEdge(graph, e.dependency, e.dependent, e.via); const django = djangoEdges({ files: [...files], tests: graph.tests, read: rel => pySources.get(rel) ?? readRepoFile(repoRoot, rel, maxBytes), code: pythonCode, resolveDotted: dotted => pythonPatchTarget(files, dotted) }); for (const e of django) addEdge(graph, e.dependency, e.dependent, 'framework'); diff --git a/lib/pregate/lane-selection.ts b/lib/pregate/lane-selection.ts index 6c0d68ca2..c30724dd5 100644 --- a/lib/pregate/lane-selection.ts +++ b/lib/pregate/lane-selection.ts @@ -12,10 +12,10 @@ import { GENERATED_PATH } from '../regen'; import { runBashSync } from '../script-invocation'; import { matchesAny } from '../ship-policy'; import { PREGATE_CONFIG_PATH } from './config'; -import { isDocsEmptyLane, isInertNoTests, isProtectedTree } from './docs-lane'; +import { isDocsEmptyLane, isFixtureOrCiTree, isInertNoTests, isProtectedTree } from './docs-lane'; import * as fs from 'node:fs'; import * as path from 'node:path'; -import { buildGraph, isTestFile, listRepoFiles, testsFor, type Graph } from './import-graph'; +import { buildGraph, isTestFile, listRepoFiles, SOURCE_EXT, testsFor, type Graph } from './import-graph'; import { stampFileKind, versionStampOnly } from './stamp-only'; import type { PregateContext, RemoteObligation } from './types'; import type { Lane } from './workflows'; @@ -105,30 +105,47 @@ function readWorkingTree(repoRoot: string, rel: string): { text: string | null; /** Tracked files matching a registry entry's output globs. */ function outputsOf(all: readonly string[], globs: readonly string[]): string[] { return all.filter(f => matchesAny(globs, f)); } +/** Tracked files outside Markdown that name `file` by its repo path (a config, a script or a workflow may load it where the import graph cannot see). */ +function namedBy(ctx: PregateContext, file: string): string[] { + const r = spawnSync('git', ['grep', '--untracked', '-l', '-F', '-e', file, '--', '.', ':(exclude)*.md', ':(exclude)*.mdx'], { cwd: ctx.repoRoot, encoding: 'utf8', timeout: 60_000, maxBuffer: 16 * 1024 * 1024, env: ctx.env }); + if (r.status === 1) return []; + return r.status === 0 ? (r.stdout ?? '').split('\n').filter(Boolean) : ['(git grep failed)']; +} + +const shown = (files: readonly string[]) => `${files.slice(0, 8).join(' ')}${files.length > 8 ? ` … +${files.length - 8} more` : ''}`; + /** - * Why a removed path no remaining test selects owes no lane, and the owning-suite tests it selects: a rename's old name defers - * to the new path; a deleted test selects the remaining tests of the files it exercised and names those that lost their last - * test; any other removed file has nothing left to test. Null when a deleted test's merge-base text could not be read: it fails closed. + * Why a removed path no remaining test selects owes no lane, and the owning-suite tests it selects, or `refused` when it must + * fail closed as no lane. A rename's old name defers to the new path, unless a test was renamed to a non-test path. A deleted + * test runs the tests that directly exercise the files it exercised and names those left with no test at all. Any other removed + * file owes nothing only when it is code the import graph reads (outside fixture, golden and CI trees) and no remaining + * non-Markdown file names it: deleting a manifest, a lockfile, a config or a workflow owes what editing it owes. */ -function removedPath(ctx: PregateContext, graph: Graph, file: string): { removed: Removed; suite: string[] } | null { +function removedPath(ctx: PregateContext, graph: Graph, file: string): { removed: Removed; suite: string[] } | { refused: string } { const renamedTo = ctx.touched.find(t => t.renamedFrom === file)?.path; - if (renamedTo) return { removed: { file, note: `old name of ${renamedTo}`, lostCoverage: [] }, suite: [] }; - if (!isTestFile(file)) return { removed: { file, note: 'no remaining test references it', lostCoverage: [] }, suite: [] }; + if (renamedTo && !(isTestFile(file) && !isTestFile(renamedTo))) return { removed: { file, note: `old name of ${renamedTo}`, lostCoverage: [] }, suite: [] }; + if (!isTestFile(file)) { + const code = (SOURCE_EXT.test(file) || file.startsWith('bin/')) && !isFixtureOrCiTree(file) && path.posix.basename(file) !== 'conftest.py'; + if (!code) return { refused: 'removed, but not code the import graph reads (a manifest, lockfile, config, fixture or workflow owes what an edit owes)' }; + const names = namedBy(ctx, file); + if (names.length) return { refused: `removed, but still named by ${shown(names)}` }; + return { removed: { file, note: 'nothing in the tree references it', lostCoverage: [] }, suite: [] }; + } const subjects = graph.goneSubjects?.get(file); - if (!subjects) return null; + if (!subjects) return { refused: 'removed test whose merge-base text could not be read (not a regular file there, invalid UTF-8 or over 1 MiB)' }; if (!subjects.length) return { removed: { file, note: 'it exercised no file still in the tree', lostCoverage: [] }, suite: [] }; const suite = new Set(); const lostCoverage: string[] = []; for (const subject of subjects) { const m = testsFor(graph, subject, ctx.config.dependencies); - for (const t of m.tests) suite.add(t); + for (const e of graph.reverse.get(subject) ?? []) if (graph.tests.has(e.from)) suite.add(e.from); if (!m.tests.length && !m.declaredNone) lostCoverage.push(subject); } const notes = [ - suite.size ? `owning suite runs: ${[...suite].sort().join(' ')}` : '', + suite.size ? `owning suite runs: ${shown([...suite].sort())}` : '', lostCoverage.length ? `coverage removed: ${lostCoverage.join(' ')} ${lostCoverage.length === 1 ? 'has' : 'have'} no remaining test` : '', ].filter(Boolean); - return { removed: { file, note: notes.join('; ') || 'it exercised only files declared no lane needed', lostCoverage }, suite: [...suite] }; + return { removed: { file, note: notes.join('; ') || 'its files keep indirect or declared coverage', lostCoverage }, suite: [...suite] }; } export function selectLanes(ctx: PregateContext): Selection { @@ -136,9 +153,10 @@ export function selectLanes(ctx: PregateContext): Selection { const gone = touched.flatMap(t => [t.status === 'D' ? t.path : null, t.renamedFrom ?? null]).filter((p): p is string => !!p); const goneSources = new Map(); for (const t of touched) { - if (t.status !== 'D' || !isTestFile(t.path)) continue; - const text = readAtMergeBase(ctx, t.path, ['100644', '100755']); - if (text !== null) goneSources.set(t.path, text); + const removedTest = t.status === 'D' && isTestFile(t.path) ? t.path : t.renamedFrom && isTestFile(t.renamedFrom) && !isTestFile(t.path) ? t.renamedFrom : null; + if (!removedTest) continue; + const text = readAtMergeBase(ctx, removedTest, ['100644', '100755']); + if (text !== null) goneSources.set(removedTest, text); } const graph: Graph = buildGraph(ctx.repoRoot, { declarations: ctx.config.dependencies, extraFiles: gone, goneSources }); const all = listRepoFiles(ctx.repoRoot); @@ -199,13 +217,13 @@ export function selectLanes(ctx: PregateContext): Selection { } if (gone.includes(file)) { const r = removedPath(ctx, graph, file); - if (r) { + if ('removed' in r) { removed.push(r.removed); for (const t of r.suite) tests.add(t); provenance.push(`${file}: removed (no lane owed) — ${r.removed.note}`); continue; } - provenance.push(`${file}: removed test whose merge-base text could not be read (not a regular file there, invalid UTF-8 or over 1 MiB); no lane until declared`); + provenance.push(`${file}: ${r.refused}; no lane until declared`); } noLane.push(file); } diff --git a/test/pregate-removed-paths.test.ts b/test/pregate-removed-paths.test.ts index 5d583c542..2a1f4002e 100644 --- a/test/pregate-removed-paths.test.ts +++ b/test/pregate-removed-paths.test.ts @@ -9,7 +9,8 @@ import { afterEach, describe, expect, test } from 'bun:test'; import * as fs from 'node:fs'; import * as path from 'node:path'; -import { BASE_PREGATE, cleanup, commit, makeFixtureRepo, runBin, write, type FixtureRepo } from './helpers/pregate-fixture'; +import { buildGraph, testsFor } from '../lib/pregate/import-graph'; +import { BASE_PREGATE, cleanup, commit, makeFixtureRepo, runBin, type FixtureRepo } from './helpers/pregate-fixture'; const repos: FixtureRepo[] = []; const fixture = (opts?: Parameters[0]) => { const r = makeFixtureRepo(opts); repos.push(r); return r; }; @@ -60,7 +61,7 @@ describe('removed paths in lanes (#3529)', () => { const r = lanes(repo); expect(r.check.status).toBe('pass'); expect(r.removed).toEqual([ - 'removed (no lane owed): lib/util.js — no remaining test references it', + 'removed (no lane owed): lib/util.js — nothing in the tree references it', 'removed (no lane owed): test/util.test.js — it exercised no file still in the tree', ]); expect(r.check.detail).not.toContain('lost their last test'); @@ -89,7 +90,7 @@ describe('removed paths in lanes (#3529)', () => { commit(repo, { 'lib/orphan.js': null }); const r = lanes(repo); expect(r.check.status).toBe('pass'); - expect(r.removed).toEqual(['removed (no lane owed): lib/orphan.js — no remaining test references it']); + expect(r.removed).toEqual(['removed (no lane owed): lib/orphan.js — nothing in the tree references it']); }); test('an unstaged deletion behaves like a committed one: the missing test is never selected', () => { @@ -122,7 +123,71 @@ describe('removed paths in lanes (#3529)', () => { expect(r.ran).not.toContain('helper-user'); }); + test('a deleted test named after a bin/ entrypoint reports that bin when it loses its last test', () => { + const repo = fixture({ baseFiles: { 'bin/gstack-foo': '#!/bin/sh\necho foo\n', 'test/gstack-foo.test.js': "const test = require('node:test');\ntest('foo', () => {});\n" } }); + commit(repo, { 'test/gstack-foo.test.js': null }); + expect(lanes(repo).removed).toEqual(['removed (no lane owed): test/gstack-foo.test.js — coverage removed: bin/gstack-foo has no remaining test']); + }); + + test('a file declared no lane needed never reads as lost coverage', () => { + const repo = fixture({ pregate: { ...BASE_PREGATE, dependencies: { ...BASE_PREGATE.dependencies, 'lib/util.js': [] } } }); + commit(repo, { 'test/util.test.js': null }); + const r = lanes(repo); + expect(r.check.status).toBe('pass'); + expect(r.removed).toEqual(['removed (no lane owed): test/util.test.js — its files keep indirect or declared coverage']); + expect(r.check.detail).not.toContain('lost their last test'); + }); + + test('the owning suite is the tests that exercise the same files directly, not every test that reaches them', () => { + const wrap = "exports.addTwice = (a) => require('./util.js').add(a, a);\n"; + const wrapTest = "const test = require('node:test');\nconst assert = require('node:assert');\nconst { addTwice } = require('../lib/wrap.js');\ntest('twice', () => assert.strictEqual(addTwice(2), 4));\n"; + const repo = fixture({ baseFiles: { 'lib/wrap.js': wrap, 'test/wrap.test.js': wrapTest, 'test/util-v2.test.js': UTIL_V2 } }); + commit(repo, { 'test/util.test.js': null }); + const r = lanes(repo); + expect(r.removed).toEqual(['removed (no lane owed): test/util.test.js — owning suite runs: test/util-v2.test.js']); + expect(r.ran).not.toContain('test/wrap.test.js'); + }); + + test('a test renamed to a non-test path is accounted for like a deleted test', () => { + const repo = fixture(); + move(repo, 'test/util.test.js', 'docs/util-notes.md'); + expect(lanes(repo).removed).toEqual(['removed (no lane owed): test/util.test.js — coverage removed: lib/util.js has no remaining test']); + }); + + test('a deleted Rust module selects its crate\'s tests', () => { + const repo = fixture({ baseFiles: { 'crates/demo/Cargo.toml': '[package]\nname = "demo"\nversion = "0.1.0"\n', 'crates/demo/src/lib.rs': 'pub mod gone;\n#[cfg(test)]\nmod tests {\n #[test]\n fn t() {}\n}\n', 'crates/demo/src/gone.rs': 'pub fn f() {}\n' } }); + commit(repo, { 'crates/demo/src/gone.rs': null }); + const g = buildGraph(repo.work, { extraFiles: ['crates/demo/src/gone.rs'] }); + expect(testsFor(g, 'crates/demo/src/gone.rs').tests).toEqual(['crates/demo/src/lib.rs']); + }); + describe('negative controls', () => { + test('deleting every file of a fixture directory a remaining test reads selects that test', () => { + const reader = "const test = require('node:test');\nconst fs = require('node:fs');\ntest('reads the fixture dir', () => { fs.readdirSync('test/fixtures/data'); });\n"; + const repo = fixture({ baseFiles: { 'test/fixtures/data/a.json': '{}\n', 'test/fixtures/data/b.json': '{}\n', 'test/fixture-dir.test.js': reader } }); + commit(repo, { 'test/fixtures/data/a.json': null, 'test/fixtures/data/b.json': null }); + const r = lanes(repo); + expect(r.removed).toEqual([]); + expect(r.ran).toContain('test/fixture-dir.test.js'); + }); + + test.each(['package.json', '.github/workflows/windows.yml', 'test/golden/digest.txt'])('deleting %s owes what editing it owes: PREGATE_NO_LANE', file => { + const repo = fixture(); + commit(repo, { [file]: null }); + const r = lanes(repo); + expect(r.check.code).toBe('PREGATE_NO_LANE'); + expect(r.check.detail).toContain(file); + expect(r.removed).toEqual([]); + }); + + test('a deleted source file a remaining config still names fails closed', () => { + const repo = fixture({ baseFiles: { 'scripts/setup.js': "console.log('setup');\n", 'bunfig.toml': '[test]\npreload = ["./scripts/setup.js"]\n' } }); + commit(repo, { 'scripts/setup.js': null }); + const r = lanes(repo); + expect(r.check.code).toBe('PREGATE_NO_LANE'); + expect(r.check.detail).toContain('scripts/setup.js'); + }); + test('a modified file whose only test was deleted still fails PREGATE_NO_LANE', () => { const repo = fixture(); commit(repo, { 'test/util.test.js': null, 'lib/util.js': "exports.add = (a, b) => b + a;\n" }); @@ -131,6 +196,7 @@ describe('removed paths in lanes (#3529)', () => { expect(r.check.code).toBe('PREGATE_NO_LANE'); expect(r.check.detail).toContain('lib/util.js'); expect(r.check.detail).not.toContain('test/util.test.js'); + expect(r.check.detail).toContain('1 removed path(s); 1 subject(s) lost their last test'); expect(r.removed).toEqual(['removed (no lane owed): test/util.test.js — coverage removed: lib/util.js has no remaining test']); });