mirror of
https://github.com/garrytan/gstack.git
synced 2026-11-04 15:57:33 +01:00
fix(pregate): timing reads a native diff and never passes an unread one
The timing check's git diff honored diff.external, textconv and diff.noprefix, so a configured driver hid every added line and the check passed. Run it with --no-ext-diff --no-textconv and explicit a/ b/ prefixes, and report a git diff that fails or does not finish as incomplete (PREGATE_INCOMPLETE), which a warn downgrade does not soften. Review of #3235, sibling of P1-4 (lib/pregate/checks/timing.ts:189). Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
This commit is contained in:
1 parent
aa8a1195b4
commit
66aad71726
2 files changed
+34
-5
No files matched your search
@@ -183,13 +183,14 @@ export function scanTiming(file: string, text: string, added: ReadonlySet<number
|
||||
}
|
||||
|
||||
/** Added line numbers per test file, merge-base to the working tree; an untracked test file counts whole. */
|
||||
export function addedTestLines(ctx: PregateContext): Map<string, Set<number> | 'all'> {
|
||||
export function addedTestLines(ctx: PregateContext): Map<string, Set<number> | 'all'> | { error: string } {
|
||||
const out = new Map<string, Set<number> | 'all'>();
|
||||
const files = ctx.touched.filter(t => t.status !== 'D' && scansTiming(t.path));
|
||||
if (!files.length) return out;
|
||||
const diff = spawnSync('git', ['diff', '--no-color', '--unified=0', '--diff-filter=ACMR', '-M', ctx.mergeBase, '--', ...files.map(t => t.path)], { cwd: ctx.repoRoot, encoding: 'utf8', timeout: 60_000, maxBuffer: 256 * 1024 * 1024, env: ctx.env });
|
||||
const diff = spawnSync('git', ['diff', '--no-ext-diff', '--no-textconv', '--no-color', '--src-prefix=a/', '--dst-prefix=b/', '--unified=0', '--diff-filter=ACMR', '-M', ctx.mergeBase, '--', ...files.map(t => t.path)], { cwd: ctx.repoRoot, encoding: 'utf8', timeout: 60_000, maxBuffer: 256 * 1024 * 1024, env: ctx.env });
|
||||
if (diff.error || diff.status !== 0) return { error: 'git diff ' + (diff.error ? 'did not finish (' + diff.error.message + ')' : 'exited ' + diff.status + ': ' + (diff.stderr ?? '').trim().split('\n').pop()) };
|
||||
let current: Set<number> | null = null;
|
||||
for (const line of (diff.stdout ?? '').split('\n')) {
|
||||
for (const line of diff.stdout.split('\n')) {
|
||||
const header = /^\+\+\+ b\/(.+)$/.exec(line);
|
||||
if (header) { current = new Set(); out.set(header[1]!, current); continue; }
|
||||
const hunk = /^@@ -\d+(?:,\d+)? \+(\d+)(?:,(\d+))? @@/.exec(line);
|
||||
@@ -201,6 +202,7 @@ export function addedTestLines(ctx: PregateContext): Map<string, Set<number> | '
|
||||
|
||||
function run(ctx: PregateContext): CheckResult {
|
||||
const added = addedTestLines(ctx);
|
||||
if (!(added instanceof Map)) return { id: 'timing', stage: 'preflight', status: 'incomplete', inputs: baseInputs(ctx), code: 'PREGATE_INCOMPLETE', detail: added.error + '; the added test lines were not read', fix: 'fix the git error and rerun gstack-pregate; an unread diff never passes' };
|
||||
const hits: TimingHit[] = [];
|
||||
for (const [file, lines] of added) {
|
||||
let text: string;
|
||||
|
||||
@@ -11,8 +11,9 @@ import { afterEach, describe, expect, test } from 'bun:test';
|
||||
import * as fs from 'node:fs';
|
||||
import * as path from 'node:path';
|
||||
import { EXIT } from '../lib/headless-artifacts';
|
||||
import { scanTiming, scansTiming, type TimingHit } from '../lib/pregate/checks/timing';
|
||||
import { CHECK_IDS } from '../lib/pregate/index';
|
||||
import { scanTiming, scansTiming, timingCheck, type TimingHit } from '../lib/pregate/checks/timing';
|
||||
import { CHECK_IDS, runPregate } from '../lib/pregate/index';
|
||||
import { buildContext } from '../lib/pregate/context';
|
||||
import { validateConfig } from '../lib/pregate/config';
|
||||
import { BASE_PREGATE, cleanup, commit, makeFixtureRepo, runBin, write, type FixtureRepo } from './helpers/pregate-fixture';
|
||||
|
||||
@@ -106,6 +107,32 @@ describe('gstack-pregate --only timing (real subprocess, fixture repo)', () => {
|
||||
expect(r.out).toContain('TIMING_ASSERTIONS: found=1 escaped=0 files=2');
|
||||
});
|
||||
|
||||
test('diff configuration cannot hide added lines: an external driver, a textconv or noprefix still gets the hit', () => {
|
||||
const repo = makeFixtureRepo(); repos.push(repo);
|
||||
commit(repo, { 'test/speed.test.js': "const t0 = Date.now();\nexpect(Date.now() - t0).toBeLessThan(400);\n" });
|
||||
fs.writeFileSync(path.join(repo.work, '.git', 'info', 'attributes'), '*.js diff=hide\n');
|
||||
for (const [key, value] of [['diff.external', '/bin/true'], ['diff.hide.textconv', 'true'], ['diff.noprefix', 'true']] as const) {
|
||||
repo.git('config', key, value);
|
||||
const r = pregate(repo, []);
|
||||
expect(r.out).toContain('timing: TIMING_ASSERTION: test/speed.test.js:2 wall-time-bound');
|
||||
expect(r.out).toContain('TIMING_ASSERTIONS: found=1 escaped=0 files=1');
|
||||
repo.git('config', '--unset', key);
|
||||
}
|
||||
});
|
||||
|
||||
test('a diff that cannot be read is incomplete, never a pass, even when timing is downgraded to warn', async () => {
|
||||
const repo = makeFixtureRepo(); repos.push(repo);
|
||||
commit(repo, { 'test/speed.test.js': "const t0 = Date.now();\nexpect(Date.now() - t0).toBeLessThan(400);\n" });
|
||||
const loaded = buildContext({ repoRoot: repo.work, base: 'main', policyFrom: 'base', allowRepoCommands: false, env: { ...process.env, ...repo.env }, progress: () => {} });
|
||||
if (!('ctx' in loaded)) throw new Error('fixture context refused');
|
||||
const ctx = { ...loaded.ctx, mergeBase: '0'.repeat(40), config: { ...loaded.ctx.config, warn: ['timing'] } };
|
||||
const direct = await timingCheck.run(ctx);
|
||||
expect(direct).toMatchObject({ id: 'timing', status: 'incomplete', code: 'PREGATE_INCOMPLETE' });
|
||||
expect(direct.detail).toMatch(/^git diff exited 128: .*; the added test lines were not read$/);
|
||||
const file = await runPregate(ctx, { stage: 'preflight', only: ['timing'] });
|
||||
expect(file.checks.find(c => c.id === 'timing')!.status).toBe('incomplete');
|
||||
});
|
||||
|
||||
test('a docs-only branch passes and timing is a known, downgradable check id', () => {
|
||||
const repo = makeFixtureRepo(); repos.push(repo);
|
||||
commit(repo, { 'docs/README.md': '# docs\nmore\n' });
|
||||
|
||||
Reference in new issue
Block a user