Skip to content

Commit f0f6914

Browse files
BIMvoicelouistrue
andauthored
fix(ci): classify images/fonts/binaries as inert instead of production in revert-oracle (#4140)
An image, font, or archive has no observable behaviour: no runner in this repo compiles or executes one, so a test can neither accompany nor observe a change to it. classifyPath put such files in `production` by default, so the oracle ABORTed with "changes production code and adds/changes NO test file" on branches whose only changes were of this kind — e.g. five deleted favicon PNGs, or an archive of retained evidence files. That is a false positive about the classifier, not a finding about the branch. Add a narrow `inert` classification (images, fonts, a few other binary container formats) alongside the existing `production`/`test`/`ignored` kinds, extracted to scripts/lib/revert-oracle-inert.mjs to stay under the module-size budget. `.json` and other formats a runner's behaviour can depend on are deliberately excluded, so a source file with no test still ABORTs exactly as before. Closes #4137 Co-authored-by: Louis Trümpler <78563314+louistrue@users.noreply.github.com>
1 parent bea417a commit f0f6914

4 files changed

Lines changed: 110 additions & 14 deletions

File tree

scripts/check-test-revert-oracle.mjs

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -263,7 +263,7 @@ if (opts.ci && isDependabotDependencyOnly(process.env.PR_AUTHOR_LOGIN, entries))
263263
process.exit(0);
264264
}
265265

266-
const { production, test: testEntries, ignored, warnings } = classifyDiff(entries);
266+
const { production, test: testEntries, ignored, inert, warnings } = classifyDiff(entries);
267267
for (const w of warnings) console.log(` WARNING: ${w}`);
268268

269269
let prodPaths = production.map((e) => e.path);
@@ -280,7 +280,9 @@ if (opts.tests.length > 0) {
280280
if (testPaths.length === 0) die(EXIT_NOTHING_CHECKED, '--test matched none of the branch\'s changed test files.');
281281
}
282282

283-
console.log(` files: ${production.length} production, ${testEntries.length} test, ${ignored.length} ignored`);
283+
console.log(
284+
` files: ${production.length} production, ${testEntries.length} test, ${ignored.length} ignored, ${inert.length} inert`,
285+
);
284286

285287
if (prodPaths.length === 0) {
286288
const message = 'this branch changes no production files; there is nothing whose absence a test could notice.';
Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
/* This Source Code Form is subject to the terms of the Mozilla Public
2+
* License, v. 2.0. If a copy of the MPL was not distributed with this
3+
* file, You can obtain one at https://mozilla.org/MPL/2.0/. */
4+
5+
/**
6+
* File kinds no runner in this repo (vitest, node --test, cargo, pytest) ever
7+
* compiles or executes: an image's bytes, a font's glyphs, an archive's
8+
* contents. Changing one — including deleting it — leaves nothing for a test
9+
* to observe, so it is neither test nor production (#4137).
10+
*
11+
* Observed on two real branches before this existed: #4114 (five deleted
12+
* PNGs) and #4117 (an 87-file archive of JSON/patch/PNG evidence) both
13+
* tripped `check-test-revert-oracle.mjs`'s "changes production code and
14+
* adds/changes NO test file" ABORT — a false positive about the classifier,
15+
* not a finding about either branch, since no test can observe a deleted
16+
* PNG's absence.
17+
*
18+
* Deliberately narrow: this must NOT swallow anything a runner builds or
19+
* runs. `.json` stays production (e.g. `package.json` gates behaviour via
20+
* scripts/deps) — only formats with no runner-observable content at all are
21+
* listed here.
22+
*/
23+
const INERT_SUFFIXES = [
24+
// images
25+
'.png', '.jpg', '.jpeg', '.gif', '.svg', '.ico', '.webp', '.avif', '.bmp', '.tiff', '.tif',
26+
// fonts
27+
'.woff', '.woff2', '.ttf', '.otf', '.eot',
28+
// other binaries with no observable behaviour of their own
29+
'.zip', '.gz', '.tar', '.pdf',
30+
];
31+
32+
/** @param {string} path */
33+
export function isInertPath(path) {
34+
const lower = path.toLowerCase();
35+
return INERT_SUFFIXES.some((s) => lower.endsWith(s));
36+
}

scripts/lib/revert-oracle.mjs

Lines changed: 7 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -35,19 +35,14 @@
3535
*/
3636

3737
import { parsePython, PYTEST_MISSING_PATTERN } from './revert-oracle-python.mjs';
38+
import { isInertPath } from './revert-oracle-inert.mjs';
3839
// ---------------------------------------------------------------------------
3940
// Diff classification
4041
// ---------------------------------------------------------------------------
4142

4243
/** Paths whose change can neither be reverted usefully nor observed by a test. */
4344
const IGNORED_PREFIXES = ['.changeset/', '.github/', 'docs/', '.vscode/'];
44-
const IGNORED_EXACT = new Set([
45-
'pnpm-lock.yaml',
46-
'package-lock.json',
47-
'yarn.lock',
48-
'Cargo.lock',
49-
'CHANGELOG.md',
50-
]);
45+
const IGNORED_EXACT = new Set(['pnpm-lock.yaml', 'package-lock.json', 'yarn.lock', 'Cargo.lock', 'CHANGELOG.md']);
5146
const IGNORED_SUFFIXES = ['.md', '.mdx', '.txt', '.snap.orig'];
5247

5348
/**
@@ -83,7 +78,7 @@ export function classifyPath(path) {
8378
if (TEST_FILE_RE.test(path) || /(^|\/)(?:[^/]+_tests|tests)\.rs$/.test(path)) return 'test';
8479
if (TEST_DIR_RE.test(path)) return 'test';
8580
if (TEST_SEGMENT_RE.test(path)) return 'test';
86-
return 'production';
81+
return isInertPath(path) ? 'inert' : 'production';
8782
}
8883

8984
/**
@@ -93,12 +88,12 @@ export function classifyDiff(entries) {
9388
const production = [];
9489
const test = [];
9590
const ignored = [];
91+
const inert = [];
9692
const warnings = [];
9793
for (const { status, path } of entries) {
9894
const kind = classifyPath(path);
99-
if (kind === 'production') production.push({ status, path });
100-
else if (kind === 'test') test.push({ status, path });
101-
else ignored.push({ status, path });
95+
const bucket = kind === 'production' ? production : kind === 'test' ? test : kind === 'inert' ? inert : ignored;
96+
bucket.push({ status, path });
10297
}
10398
if (production.some((e) => isRustFile(e.path))) {
10499
warnings.push(
@@ -107,7 +102,7 @@ export function classifyDiff(entries) {
107102
'as the code — expect INCONCLUSIVE and use --mutation for a surgical revert.',
108103
);
109104
}
110-
return { production, test, ignored, warnings };
105+
return { production, test, ignored, inert, warnings };
111106
}
112107

113108
/** Parse `git diff --name-status -z`-free plain output. Renames carry two paths. */

scripts/lib/revert-oracle.test.mjs

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -498,6 +498,69 @@ test('classifyPath: the deploy-config rule does not swallow neighbouring code',
498498
assert.equal(classifyPath('vercel.json.ts'), 'production');
499499
});
500500

501+
// WHY "inert" IS ITS OWN KIND, NOT `ignored` (#4137).
502+
//
503+
// A changed file that no runner in this repo claims as a test AND no runner
504+
// compiles or executes as source has no observable behaviour at all: an
505+
// image's bytes, a font's glyphs, an archive's contents. Classifying such a
506+
// file as `production` made the oracle ABORT with "changes production code
507+
// and adds/changes NO test file" on branches like #4114 (five deleted PNGs)
508+
// and #4117 (an 87-file archive) — there is no test that could possibly
509+
// accompany a deleted PNG, so the ABORT was a false positive about the
510+
// classifier, not a finding about the branch.
511+
test('classifyPath: images, fonts, and other binaries are inert, not production', () => {
512+
assert.equal(classifyPath('apps/viewer/public/favicon-192x192.png'), 'inert');
513+
assert.equal(classifyPath('apps/viewer/public/favicon.ico'), 'inert');
514+
assert.equal(classifyPath('apps/viewer/src/assets/logo.svg'), 'inert');
515+
assert.equal(classifyPath('apps/landing/public/hero.webp'), 'inert');
516+
assert.equal(classifyPath('apps/viewer/public/fonts/inter.woff2'), 'inert');
517+
assert.equal(classifyPath('apps/viewer/public/fonts/inter.ttf'), 'inert');
518+
assert.equal(classifyPath('scripts/perf/evidence/report.pdf'), 'inert');
519+
assert.equal(classifyPath('scripts/perf/evidence/archive.zip'), 'inert');
520+
});
521+
522+
test('classifyPath: inertness is about the file kind, not the operation — a deleted image is still inert', () => {
523+
// classifyPath is purely path-based; classifyDiff carries the `status` field
524+
// through unchanged. A pure deletion of an inert file must not become
525+
// production just because the "operation" is a delete (must-not-regress in
526+
// #4137: inertness is about the file kind, not about the operation).
527+
const { production, inert } = classifyDiff(parseNameStatus('D\tapps/viewer/public/favicon.png'));
528+
assert.deepEqual(production, []);
529+
assert.deepEqual(inert.map((e) => e.path), ['apps/viewer/public/favicon.png']);
530+
});
531+
532+
test('classifyPath: inertness must NOT cover anything a runner compiles or executes', () => {
533+
// Extensions a runner in this repo actually builds/executes stay production
534+
// even though they sound "asset-like" or are commonly bundled alongside
535+
// assets — this is the narrowing #4137 must preserve.
536+
assert.equal(classifyPath('packages/core/package.json'), 'production');
537+
assert.equal(classifyPath('crates/ifc-lite-geom/src/walk.rs'), 'production');
538+
assert.equal(classifyPath('packages/renderer/src/device.ts'), 'production');
539+
assert.equal(classifyPath('tools/ifcopenshell_reference/canonical.py'), 'production');
540+
});
541+
542+
test('classifyDiff: a mixed diff keeps inert files out of BOTH production and test, without giving the real change a free pass', () => {
543+
const entries = parseNameStatus(
544+
[
545+
'M\tpackages/renderer/src/device.ts',
546+
'A\tapps/viewer/public/favicon-512x512.png',
547+
].join('\n'),
548+
);
549+
const { production, test: tests, inert } = classifyDiff(entries);
550+
assert.deepEqual(production.map((e) => e.path), ['packages/renderer/src/device.ts']);
551+
assert.deepEqual(tests, []);
552+
assert.deepEqual(inert.map((e) => e.path), ['apps/viewer/public/favicon-512x512.png']);
553+
});
554+
555+
test('classifyDiff: a diff of ONLY inert files has no production entries (script-level effect: NOT APPLICABLE, same as #4024 for test-only diffs)', () => {
556+
const entries = parseNameStatus(
557+
['D\tapps/viewer/public/favicon-192x192.png', 'D\tapps/viewer/public/favicon-512x512.png'].join('\n'),
558+
);
559+
const { production, inert } = classifyDiff(entries);
560+
assert.deepEqual(production, []);
561+
assert.equal(inert.length, 2);
562+
});
563+
501564
test('classifyDiff splits a real branch shape and never puts a test in production', () => {
502565
const entries = parseNameStatus(
503566
[

0 commit comments

Comments
 (0)