diff --git a/docs/pregate.md b/docs/pregate.md index 354f2b32c..3176c6451 100644 --- a/docs/pregate.md +++ b/docs/pregate.md @@ -198,8 +198,8 @@ Two stages with separate clocks, both printed: A removed path (a deleted file, or a rename's old name) that no remaining 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 test renamed to another test path defers to its new - name (`old name of `). A deleted JS, TS, Python, Dart or bats test, or a + `N removed path(s)`). A JS, TS, Python, Dart or bats test renamed to another + test path defers to its new name (`old name of `). A deleted JS, TS, Python, Dart or bats 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 @@ -210,16 +210,19 @@ Two stages with separate clocks, both printed: remaining file imports it (directly or through a directory it reads), it is not a file a runner loads by name (`*.config.ts`, `.*rc.js`, `conftest.py`, `setup.py`, `noxfile.py`, `sitecustomize.py`, `__init__.py`, `__main__.py`), - and no remaining non-Markdown file names it by its repo path or by its path - relative to a directory that file sits under (`preload = ["./setup.ts"]`). + and no remaining non-Markdown file mentions its basename (a config naming it + relative to itself, `preload = ["./setup.ts"]`, or a computed path such as + `path.join(__dirname, 'lib', 'value.js')`), so a common basename such as + `index.ts` needs a `dependencies` entry. 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 or renamed manifest, lockfile, config, fixture, golden or workflow (renaming one into a waived path such as Markdown does not waive the old name), a Go, Ruby, shell, Rust or bin file (the graph does not resolve their importers file by file), - a Go or Ruby test (it may define helpers its package still uses), a file + a deleted or renamed Go or Ruby test (it may define helpers its package + still uses), a file remaining code still imports (`still imported by …`, even by a relative or - dotted import) or a config or script still names (`still named by …`), and a + dotted import) or a non-Markdown file still names (`still named by …`), 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, diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 7c6b731cf..abc6ceebf 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -3219,8 +3219,8 @@ removed: has no remaining test`). A removal owes what an edit owes, so these are still this error: a deleted or renamed manifest, lockfile, config, fixture, golden or workflow, a file a runner loads by name (`*.config.ts`, `conftest.py`, `sitecustomize.py`), a Go, Ruby, shell, Rust or bin file, a Go -or Ruby test, a file remaining code still imports or a config or script still -names (by repo path or relative to the config), a deleted test whose merge-base +or Ruby test (deleted or renamed), a file remaining code still imports or whose +basename a non-Markdown file still mentions, 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. `--explain` prints the reason as `: removed, but …`. diff --git a/lib/pregate/lane-selection.ts b/lib/pregate/lane-selection.ts index 5d724f560..83240304d 100644 --- a/lib/pregate/lane-selection.ts +++ b/lib/pregate/lane-selection.ts @@ -106,20 +106,13 @@ function readWorkingTree(repoRoot: string, rel: string): { text: string | null; function outputsOf(all: readonly string[], globs: readonly string[]): string[] { return all.filter(f => matchesAny(globs, f)); } /** - * Tracked files outside Markdown that name `file` where the import graph cannot see (a config, a script or a workflow): by its - * repo path anywhere, or by its path relative to an ancestor directory from a file under that directory (`preload = ["./setup.ts"]`). + * Tracked files outside Markdown that mention `file`'s basename, where the import graph cannot see: a config naming it relative to + * itself (`preload = ["./setup.ts"]`), a computed path (`path.join(__dirname, 'lib', 'value.js')`), a script or a workflow. */ function namedBy(ctx: PregateContext, file: string): string[] { - const parts = file.split('/'); - const hits = new Set(); - for (let i = 0; i < parts.length; i++) { - const scope = parts.slice(0, i).join('/') || '.'; - const r = spawnSync('git', ['grep', '--untracked', '-l', '-F', '-e', parts.slice(i).join('/'), '--', scope, ':(exclude)*.md', ':(exclude)*.mdx'], { cwd: ctx.repoRoot, encoding: 'utf8', timeout: 60_000, maxBuffer: 16 * 1024 * 1024, env: ctx.env }); - if (r.status === 1) continue; - if (r.status !== 0) return ['(git grep failed)']; - for (const f of (r.stdout ?? '').split('\n')) if (f) hits.add(f); - } - return [...hits].sort(); + const r = spawnSync('git', ['grep', '--untracked', '-l', '-F', '-e', path.posix.basename(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)']; } /** Languages whose importers the graph resolves file by file; Go packages, Ruby requires, shell `source`, Rust modules and bins are not. */ @@ -131,17 +124,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. Nothing is waived while a remaining file imports it, directly or through a directory reference. A - * test renamed to a test defers to its new name. A deleted JS, TS, Python, Dart or bats test, or one renamed to a non-test - * path, runs the tests that directly exercise the files it exercised and names those left with no test at all. Any other - * removed file, a rename's old name included, owes nothing only when it is JS, TS, Python or Dart outside fixture, golden and - * CI trees, not loaded by name, and no remaining non-Markdown file names it: deleting a manifest, a config, a workflow or a Go - * file owes what editing it owes. + * fail closed as no lane. Nothing is waived while a remaining file imports it, directly or through a directory reference, nor + * is a Go or Ruby test (it may define helpers its package still uses). Any other test renamed to a test defers to its new + * name. A deleted JS, TS, Python, Dart or bats test, or one renamed to a non-test path, runs the tests that directly exercise + * the files it exercised and names those left with no test at all. Any other removed file, a rename's old name included, owes + * nothing only when it is JS, TS, Python or Dart outside fixture, golden and CI trees, not loaded by name, and no remaining + * non-Markdown file mentions its basename: deleting a manifest, a config, a workflow or a Go file owes what editing it owes. */ function removedPath(ctx: PregateContext, graph: Graph, gone: ReadonlySet, file: string): { removed: Removed; suite: string[] } | { refused: string } { const edges = [...graph.reverse].filter(([k]) => k === file || (k.endsWith('/**') && file.startsWith(k.slice(0, -2)))).flatMap(([, list]) => list); const importers = [...new Set(edges.map(e => e.from))].filter(f => !gone.has(f) && path.posix.basename(f) !== 'Cargo.toml').sort(); if (importers.length) return { refused: `removed, but still imported by ${shown(importers)}` }; + if (isTestFile(file) && !RESOLVED_IMPORTS.test(file) && !file.endsWith('.bats')) return { refused: 'removed, but a Go or Ruby test may define helpers its package still uses, which the graph cannot see' }; 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)) { @@ -150,7 +144,6 @@ function removedPath(ctx: PregateContext, graph: Graph, gone: ReadonlySet { expect(explain.out + explain.err).toContain('lib/plugins/a.js: removed, but still imported by bin/load.js'); }); + test('a deleted file a tested loader reaches by a computed path fails closed', () => { + const loader = "const path = require('node:path');\nexports.value = () => require(path.join(__dirname, 'lib', 'value.js')).value;\n"; + const loaderTest = "const test = require('node:test');\nconst { value } = require('../loader.js');\ntest('loads', () => value());\n"; + const repo = fixture({ baseFiles: { 'lib/value.js': 'exports.value = 1;\n', 'loader.js': loader, 'test/loader.test.js': loaderTest } }); + commit(repo, { 'lib/value.js': null }); + const r = lanes(repo); + expect(r.check.code).toBe('PREGATE_NO_LANE'); + expect(r.removed).toEqual([]); + const explain = runBin(repo, 'gstack-pregate', ['--base', 'main', '--allow-repo-commands', '--stage', 'tests', '--explain']); + expect(explain.out + explain.err).toContain('lib/value.js: removed, but still named by loader.js'); + }); + + test('a Go test renamed into another directory is refused: its old package may still call its helpers', () => { + const repo = fixture({ baseFiles: { 'go.mod': 'module example.com/demo\n\ngo 1.22\n', 'pkg/a.go': 'package pkg\n', 'pkg/a_test.go': 'package pkg\n\nfunc helper() {}\n', 'pkg/b_test.go': 'package pkg\n\nfunc TestB(t *testing.T) { helper() }\n', 'isolated/keep.go': 'package isolated\n' } }); + move(repo, 'pkg/a_test.go', 'isolated/a_test.go'); + const r = lanes(repo); + expect(r.check.code).toBe('PREGATE_NO_LANE'); + expect(r.check.detail).toContain('pkg/a_test.go'); + expect(r.removed).toEqual([]); + }); + 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" });