diff --git a/docs/pregate.md b/docs/pregate.md index 2f581f805..d9fd2aeb5 100644 --- a/docs/pregate.md +++ b/docs/pregate.md @@ -205,11 +205,12 @@ Two stages with separate clocks, both printed: 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 + trees that no remaining file imports and 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 + a deleted file remaining code still imports (`still imported by …`, even by + a relative or dotted import) or 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 diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index c986fa0d1..df04d5cda 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -3217,7 +3217,7 @@ print `removed (no lane owed): — ` instead (a deleted test runs it 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 +file remaining code still imports or 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. diff --git a/lib/pregate/checks/lanes.ts b/lib/pregate/checks/lanes.ts index c2ad6ec77..5813de41b 100644 --- a/lib/pregate/checks/lanes.ts +++ b/lib/pregate/checks/lanes.ts @@ -14,9 +14,11 @@ * values is waived as version-stamp-only (lib/pregate/stamp-only.ts) and named * on its own line; any other change to it still needs a lane. * A removed path (deleted, or a rename's old name) no remaining test selects - * owes no lane and is named on its own `removed (no lane owed)` line; a deleted - * test runs its owning suite or names the files that lost their last test - * (issue #3529). + * owes no lane when it is a rename's old name, a deleted test, or deleted code + * nothing remaining imports or names, and is named on its own + * `removed (no lane owed)` line; a deleted test runs its owning suite or names + * the files that lost their last test (issue #3529). Any other removal owes + * what an edit owes. * A test that reads the file still selects that test. * Incidents: p0-4-pipe-hidden-failure (`false | tail` read as green), * p0-4-windows-lane-unrun (a Windows-only change merged with its lane unrun). diff --git a/lib/pregate/lane-selection.ts b/lib/pregate/lane-selection.ts index c30724dd5..7584d3d40 100644 --- a/lib/pregate/lane-selection.ts +++ b/lib/pregate/lane-selection.ts @@ -26,7 +26,7 @@ export interface Removed { file: string; note: string; lostCoverage: string[] } export interface Selection { /** Touched files with no lane and no `[]` declaration. */ noLane: string[]; - /** Removed paths no remaining test selects: each owes no lane and is named on its own line (issue #3529). */ + /** Removed paths no remaining test selects that owe no lane, each named on its own line (issue #3529); other removals stay in noLane. */ removed: Removed[]; /** Markdown (and `empty_lanes` paths) with no import-graph test: the docs/no-tests empty lane. */ emptyLane: string[]; @@ -118,15 +118,18 @@ const shown = (files: readonly string[]) => `${files.slice(0, 8).join(' ')}${fil * 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. + * file owes nothing only when it is code the import graph reads (outside fixture, golden and CI trees), no remaining file + * imports it 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[] } | { refused: string } { +function removedPath(ctx: PregateContext, graph: Graph, gone: ReadonlySet, file: string): { removed: Removed; suite: string[] } | { refused: string } { const renamedTo = ctx.touched.find(t => t.renamedFrom === file)?.path; 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 importers = [...new Set((graph.reverse.get(file) ?? []).map(e => e.from))].filter(f => !gone.has(f)).sort(); + if (importers.length) return { refused: `removed, but still imported by ${shown(importers)}` }; 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: [] }; @@ -159,6 +162,7 @@ export function selectLanes(ctx: PregateContext): Selection { if (text !== null) goneSources.set(removedTest, text); } const graph: Graph = buildGraph(ctx.repoRoot, { declarations: ctx.config.dependencies, extraFiles: gone, goneSources }); + const goneSet = new Set(gone); const all = listRepoFiles(ctx.repoRoot); const provenance: string[] = []; const tests = new Set(); @@ -216,7 +220,7 @@ export function selectLanes(ctx: PregateContext): Selection { provenance.push(`${file}: not version-stamp-only (${verdict.reason})`); } if (gone.includes(file)) { - const r = removedPath(ctx, graph, file); + const r = removedPath(ctx, graph, goneSet, file); if ('removed' in r) { removed.push(r.removed); for (const t of r.suite) tests.add(t); diff --git a/test/pregate-removed-paths.test.ts b/test/pregate-removed-paths.test.ts index 2a1f4002e..758d35178 100644 --- a/test/pregate-removed-paths.test.ts +++ b/test/pregate-removed-paths.test.ts @@ -1,6 +1,7 @@ /** - * Paths a branch removes (deleted files and the old name of a rename) never fail - * `lanes` with PREGATE_NO_LANE on their own (issue #3529, gbrain#7829). Each one + * Paths a branch removes (deleted files and the old name of a rename) owe no lane + * when only a rename, a deleted test or unreferenced code goes (issue #3529, + * gbrain#7829); any other removal owes what an edit owes. Each waived one * prints a visible `removed (no lane owed): — ` line: a deleted test * runs its owning suite (the remaining tests of the files it exercised) or says * which subjects lost their last test; a modified file whose only test was @@ -188,6 +189,20 @@ describe('removed paths in lanes (#3529)', () => { expect(r.check.detail).toContain('scripts/setup.js'); }); + test.each([ + ['a relative extensionless TS import', { 'src/d.ts': 'export const d = 1;\n', 'src/u.ts': "import { d } from './d';\nexport const u = d + 1;\n" }, 'src/d.ts', 'src/u.ts'], + ['a Python dotted import', { 'pkg/__init__.py': '', 'pkg/m.py': 'X = 1\n', 'app.py': 'from pkg import m\nprint(m.X)\n' }, 'pkg/m.py', 'app.py'], + ])('a deleted file untested code still imports by %s fails closed', (_how, files, deleted, importer) => { + const repo = fixture({ baseFiles: files }); + commit(repo, { [deleted]: null }); + const r = lanes(repo); + expect(r.check.code).toBe('PREGATE_NO_LANE'); + expect(r.check.detail).toContain(deleted); + expect(r.removed).toEqual([]); + const why = runBin(repo, 'gstack-pregate', ['--base', 'main', '--allow-repo-commands', '--stage', 'tests', '--explain']); + expect(`${why.out}${why.err}`).toContain(`${deleted}: removed, but still imported by ${importer}`); + }); + 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" });