mirror of
https://github.com/garrytan/gstack.git
synced 2026-11-05 08:23:59 +01:00
fix(pregate): a removed file remaining code still imports fails closed
Pass-2 red-team: namedBy matched only full repo paths, so a deleted module imported by a relative, extensionless or dotted specifier from untested code was waived. removedPath now refuses when the import graph shows a remaining dependent. Stale headers updated to say other removals owe what an edit owes. Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
This commit is contained in:
1 parent
6e69dc3681
commit
812dae6243
5 files changed
+36
-14
No files matched your search
+4
-3
@@ -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: <file> 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
|
||||
|
||||
@@ -3217,7 +3217,7 @@ print `removed (no lane owed): <path> — <why>` instead (a deleted test runs it
|
||||
owning suite or says `coverage removed: <file> 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.
|
||||
|
||||
|
||||
@@ -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).
|
||||
|
||||
@@ -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<string>, 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<string>();
|
||||
@@ -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);
|
||||
|
||||
@@ -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): <path> — <why>` 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" });
|
||||
|
||||
Reference in new issue
Block a user