diff --git a/docs/pregate.md b/docs/pregate.md index d9fd2aeb5..354f2b32c 100644 --- a/docs/pregate.md +++ b/docs/pregate.md @@ -198,24 +198,33 @@ 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 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 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 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 - deleted Rust module selects its crate's tests. /review lists a deleted test - as a gate edit (RH-1) either way. + `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 + 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 JS, TS, + Python or Dart file, or the old name of a renamed one, outside fixture, + golden and CI trees reads `nothing in the tree references it` when no + 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"]`). + 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 + 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 + 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 df04d5cda..7c6b731cf 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -3211,15 +3211,19 @@ 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 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 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. +A path the branch removed is usually not this error either: a renamed test, a +deleted JS, TS, Python, Dart or bats test, and a deleted JS, TS, Python or Dart +file nothing in the tree still imports or 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 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 +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 …`. **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/checks/lanes.ts b/lib/pregate/checks/lanes.ts index 5813de41b..59c14567e 100644 --- a/lib/pregate/checks/lanes.ts +++ b/lib/pregate/checks/lanes.ts @@ -14,8 +14,8 @@ * 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 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 + * owes no lane when it is a renamed or deleted test, or deleted JS, TS, Python + * or Dart 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. diff --git a/lib/pregate/lane-selection.ts b/lib/pregate/lane-selection.ts index 7584d3d40..5d724f560 100644 --- a/lib/pregate/lane-selection.ts +++ b/lib/pregate/lane-selection.ts @@ -15,7 +15,7 @@ import { PREGATE_CONFIG_PATH } from './config'; import { isDocsEmptyLane, isFixtureOrCiTree, isInertNoTests, isProtectedTree } from './docs-lane'; import * as fs from 'node:fs'; import * as path from 'node:path'; -import { buildGraph, isTestFile, listRepoFiles, SOURCE_EXT, testsFor, type Graph } from './import-graph'; +import { buildGraph, isTestFile, listRepoFiles, testsFor, type Graph } from './import-graph'; import { stampFileKind, versionStampOnly } from './stamp-only'; import type { PregateContext, RemoteObligation } from './types'; import type { Lane } from './workflows'; @@ -105,35 +105,52 @@ 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). */ +/** + * 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"]`). + */ 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 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(); } +/** Languages whose importers the graph resolves file by file; Go packages, Ruby requires, shell `source`, Rust modules and bins are not. */ +const RESOLVED_IMPORTS = /\.(?:[cm]?[jt]sx?|py|dart)$/; +/** Files a runner or interpreter loads by name, which no import or path mention points at. */ +const LOADED_BY_NAME = /(?:^|\/)(?:[^/]*\.config\.[cm]?[jt]s|\.[^/]*rc\.[cm]?js|(?:conftest|sitecustomize|usercustomize|setup|noxfile|__init__|__main__)\.py)$/; + 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, 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), 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. + * 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. */ 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)}` }; 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 (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)}` }; + if (!RESOLVED_IMPORTS.test(file) || isFixtureOrCiTree(file) || LOADED_BY_NAME.test(file)) return { refused: 'removed, but not JS, TS, Python or Dart code whose importers the graph resolves (a manifest, config, fixture, workflow, file loaded by name, or Go, Ruby, shell, Rust or bin file 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: [] }; + return { removed: { file, note: renamedTo ? `old name of ${renamedTo}; nothing in the tree references it` : 'nothing in the tree references it', lostCoverage: [] }, suite: [] }; } + if (!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 subjects = graph.goneSubjects?.get(file); 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: [] }; diff --git a/test/pregate-removed-paths.test.ts b/test/pregate-removed-paths.test.ts index 758d35178..0368554f7 100644 --- a/test/pregate-removed-paths.test.ts +++ b/test/pregate-removed-paths.test.ts @@ -1,7 +1,8 @@ /** * 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 + * when only a renamed or deleted test, or JS, TS, Python or Dart code nothing + * imports or names, 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 @@ -82,7 +83,7 @@ describe('removed paths in lanes (#3529)', () => { move(repo, 'lib/util.js', 'lib/util2.js', ['test/util.test.js', t => t.replace('../lib/util.js', '../lib/util2.js')]); const r = lanes(repo); expect(r.check.status).toBe('pass'); - expect(r.removed).toEqual(['removed (no lane owed): lib/util.js — old name of lib/util2.js']); + expect(r.removed).toEqual(['removed (no lane owed): lib/util.js — old name of lib/util2.js; nothing in the tree references it']); expect(r.ran).toContain('test/util.test.js'); }); @@ -203,6 +204,54 @@ describe('removed paths in lanes (#3529)', () => { expect(`${why.out}${why.err}`).toContain(`${deleted}: removed, but still imported by ${importer}`); }); + const GO_MOD = 'module example.com/demo\n\ngo 1.22\n'; + test.each([ + ['a Go file its package still uses', { 'go.mod': GO_MOD, 'pkg/value.go': 'package pkg\n\nfunc value() int { return 1 }\n', 'pkg/main.go': 'package pkg\n\nfunc Main() int { return value() }\n' }, 'pkg/value.go', 'not JS, TS, Python or Dart'], + ['a Go test whose package may share its helpers', { 'go.mod': GO_MOD, 'pkg/a.go': 'package pkg\n', 'pkg/a_test.go': 'package pkg\n\nfunc helper() {}\n', 'pkg/b_test.go': 'package pkg\n' }, 'pkg/a_test.go', 'Go or Ruby test may define helpers'], + ['a Ruby file loaded by require_relative', { 'lib/value.rb': 'VALUE = 1\n', 'lib/main.rb': "require_relative 'value'\n" }, 'lib/value.rb', 'not JS, TS, Python or Dart'], + ['a shell helper a bin sources', { 'bin/helper.sh': 'greet() { echo hi; }\n', 'bin/tool': '#!/bin/sh\nDIR=$(dirname "$0")\n. "$DIR/helper.sh"\ngreet\n' }, 'bin/helper.sh', 'not JS, TS, Python or Dart'], + ['a config its runner loads by name', { 'vitest.config.ts': 'export default {};\n' }, 'vitest.config.ts', 'loaded by name'], + ['a Python startup file', { 'sitecustomize.py': 'import builtins\nbuiltins.FEATURE = True\n' }, 'sitecustomize.py', 'loaded by name'], + ['a setup file a nested config names relative to itself', { 'packages/app/setup.ts': 'export {};\n', 'packages/app/bunfig.toml': '[test]\npreload = ["./setup.ts"]\n' }, 'packages/app/setup.ts', 'still named by packages/app/bunfig.toml'], + ])('deleting %s fails closed', (_what, files, deleted, why) => { + const repo = fixture({ baseFiles: files }); + commit(repo, { [deleted]: 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(deleted + ': removed'); + expect(explain.out + explain.err).toContain(why); + }); + + test('a rename into a waived path does not waive the old name: package.json renamed to Markdown fails closed', () => { + const repo = fixture(); + move(repo, 'package.json', 'docs/package.md'); + const r = lanes(repo); + expect(r.check.code).toBe('PREGATE_NO_LANE'); + expect(r.check.detail).toContain('package.json'); + expect(r.removed).toEqual([]); + }); + + test('a renamed source remaining untested code still imports by its old name fails closed', () => { + const repo = fixture({ baseFiles: { 'lib/a.js': 'exports.a = 1;\n', 'lib/u.js': "exports.u = require('./a.js').a;\n", 'notes/plan.md': '# plan\n' } }); + move(repo, 'lib/a.js', 'notes/a.md'); + const r = lanes(repo); + expect(r.check.code).toBe('PREGATE_NO_LANE'); + expect(r.check.detail).toContain('lib/a.js'); + expect(r.removed).toEqual([]); + }); + + test('a deleted file a remaining bin reads through its directory fails closed', () => { + const repo = fixture({ baseFiles: { 'lib/plugins/a.js': 'exports.a = 1;\n', 'lib/plugins/b.js': 'exports.b = 2;\n', 'bin/load.js': "const fs = require('node:fs');\nfor (const f of fs.readdirSync('lib/plugins')) require('../lib/plugins/' + f);\n" } }); + commit(repo, { 'lib/plugins/a.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/plugins/a.js: removed, but still imported by bin/load.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" });