fix(verification): red proof checks the test list against the plan
Review finding cf241526; the list is read as operator decision 20 sets it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
parent
045191f51d
commit
734b4a23ae
SSH key fingerprint:
SHA256:JakMjO6FTBBzN0Bhfj9saOoEjaFxlSdYuZQQpM/lF9Q
2 changed files with 180 additions and 5 deletions
|
|
@ -19,6 +19,14 @@
|
|||
// red and goes up to the plan. The plan's `Files:` alone decides (see
|
||||
// `testsOnly` below).
|
||||
//
|
||||
// With `--plan <plan.md> --step <N>` the test list is also checked against the
|
||||
// step's `Test first:` File line (plan-schema `testFirstFiles`: the leading
|
||||
// backticked paths, never a path in the description - operator decision 20).
|
||||
// No `--tests`: the plan's list is used. A `--tests` whose set differs from the
|
||||
// plan's is `not-run` with reason `tests-mismatch` and the plan's list in
|
||||
// `plan_tests` (review cf241526). A step whose plan list is empty (a 1.7 step
|
||||
// without `Test first:`) or cannot be read changes nothing.
|
||||
//
|
||||
// Exit 0 = proven, 1 = rejected (`not-red-at-base` or `not-green-at-result`),
|
||||
// 2 = could not run (`not-run`: no test files, an unknown start point, a command
|
||||
// outside the test-runner allowlist, a command that cannot start, a control run
|
||||
|
|
@ -61,7 +69,7 @@ import { tmpdir } from 'node:os';
|
|||
import { dirname, isAbsolute, join, resolve } from 'node:path';
|
||||
import { DEFAULT_TIMEOUT_MS, allowedCommand, runCommand, screenCommand, stoppedBy } from './criteria-runner.mjs';
|
||||
import { commandEnv } from '../util/command-env.mjs';
|
||||
import { extractPlanVersion, parseStepFields, sliceSteps, stepTestsOnlyFromFields } from '../parsers/plan-schema.mjs';
|
||||
import { extractPlanVersion, parseStepFields, sliceSteps, stepTestsOnlyFromFields, testFirstFiles } from '../parsers/plan-schema.mjs';
|
||||
import { isMain } from '../util/is-main.mjs';
|
||||
|
||||
// The one timeout, defined in criteria-runner.mjs (release review of 6.3.0, round 7).
|
||||
|
|
@ -131,6 +139,22 @@ function testsOnly(o) {
|
|||
return { tests_only: stepTestsOnlyFromFields(parseStepFields(step.body), extractPlanVersion(text)).testsOnly };
|
||||
}
|
||||
|
||||
// With `--plan` and `--step`: the step's test files as its `Test first:` File
|
||||
// line names them (testFirstFiles, operator decision 20), read as `testsOnly`
|
||||
// reads the plan. null = the plan or the step could not be read.
|
||||
function planTests(o) {
|
||||
if (!o.plan) return null;
|
||||
let text;
|
||||
try { text = readFileSync(o.plan, 'utf8'); }
|
||||
catch { return null; }
|
||||
const step = sliceSteps(text).find((s) => Number(s.n) === Number(o.step));
|
||||
if (!step) return null;
|
||||
return testFirstFiles(parseStepFields(step.body), extractPlanVersion(text));
|
||||
}
|
||||
|
||||
/** A test path as the plan's list holds it: `/` separators, no leading `./`. */
|
||||
const normTest = (p) => String(p).replace(/\\/g, '/').replace(/^\.\//, '');
|
||||
|
||||
// A program that could not start (a spawn error: not installed), or one this
|
||||
// run stopped (the timeout, output past the buffer, a signal), is not a test
|
||||
// verdict at all (criteria-runner `stoppedBy`). With no shell, an exit 126/127
|
||||
|
|
@ -185,16 +209,30 @@ function applyResult(root, tree, base) {
|
|||
}
|
||||
|
||||
/**
|
||||
* @param {{cwd: string, base: string, cmd: string, tests: string[], link?: string[], redOnly?: boolean, timeoutMs?: number, plan?: string, step?: number}} o
|
||||
* @returns {{status: 'proven'|'rejected'|'not-run', reason: string, base?: object, result?: object, control?: object}}
|
||||
* @param {{cwd: string, base: string, cmd: string, tests?: string[], link?: string[], redOnly?: boolean, timeoutMs?: number, plan?: string, step?: number}} o
|
||||
* @returns {{status: 'proven'|'rejected'|'not-run', reason: string, plan_tests?: string[], base?: object, result?: object, control?: object}}
|
||||
*/
|
||||
export function proveRed(o) {
|
||||
const given = resolve(o.cwd ?? process.cwd());
|
||||
const timeoutMs = o.timeoutMs ?? DEFAULT_TIMEOUT_MS;
|
||||
const tests = (o.tests ?? []).filter(Boolean);
|
||||
let tests = (o.tests ?? []).filter(Boolean);
|
||||
if (!existsSync(given)) return notRun(`--cwd ${given} does not exist`);
|
||||
const cwd = repoRoot(given);
|
||||
if (!cwd) return notRun(`--cwd ${given} is not inside a git repository`);
|
||||
// The step's own test list (review cf241526): read from the plan when none is
|
||||
// given, and a given list that is another set is refused. An empty or
|
||||
// unreadable plan list changes nothing.
|
||||
const fromPlan = planTests(o);
|
||||
if (fromPlan && fromPlan.length > 0) {
|
||||
if (tests.length === 0) tests = [...fromPlan];
|
||||
else {
|
||||
const a = new Set(tests.map(normTest));
|
||||
const b = new Set(fromPlan.map(normTest));
|
||||
if (a.size !== b.size || [...a].some((t) => !b.has(t))) {
|
||||
return notRun(`tests-mismatch: --tests names ${tests.join(',')}; the plan's Test first File line names ${fromPlan.join(',')}`, { plan_tests: fromPlan });
|
||||
}
|
||||
}
|
||||
}
|
||||
if (tests.length === 0) return notRun('no test files given: a step without its test cannot prove red');
|
||||
for (const t of tests) {
|
||||
if (!safeRel(t)) return notRun(`test path must be repo-relative: ${t}`);
|
||||
|
|
@ -254,7 +292,8 @@ export function proveRed(o) {
|
|||
// --- CLI ------------------------------------------------------------------------
|
||||
|
||||
const USAGE =
|
||||
'usage: red-proof.mjs --base <start_sha> --cmd "<test command>" --tests <a,b> [--plan <plan.md> --step <N>] [--red-only] [--cwd <dir>] [--link <dir,...>] [--timeout <ms>] [--json]';
|
||||
'usage: red-proof.mjs --base <start_sha> --cmd "<test command>" [--tests <a,b>] [--plan <plan.md> --step <N>] [--red-only] [--cwd <dir>] [--link <dir,...>] [--timeout <ms>] [--json]\n' +
|
||||
' --tests is required unless --plan/--step name a step whose Test first File line lists its tests; given with them, it must name the same set';
|
||||
|
||||
const list = (v) => String(v).split(',').map((s) => s.trim()).filter(Boolean);
|
||||
|
||||
|
|
|
|||
|
|
@ -13,6 +13,8 @@ import { tmpdir } from 'node:os';
|
|||
import { dirname, join } from 'node:path';
|
||||
import { fileURLToPath } from 'node:url';
|
||||
import { proveRed } from '../../lib/verification/red-proof.mjs';
|
||||
import { buildPlan } from '../helpers/plan-v18-builder.mjs';
|
||||
import { childEnv } from '../helpers/child-env.mjs';
|
||||
|
||||
const HERE = dirname(fileURLToPath(import.meta.url));
|
||||
const ROOT = join(HERE, '..', '..');
|
||||
|
|
@ -269,3 +271,137 @@ test('red proof: reproduces the 02243c6 red-first measurement (6 of 55) and the
|
|||
rmSync(tar, { force: true });
|
||||
}
|
||||
});
|
||||
|
||||
// Review cf241526 (operator decision 20): with --plan and --step, the test list
|
||||
// is checked against the step's `Test first:` File line, and read from it when
|
||||
// --tests is not given. An empty plan list (a 1.7 step without `Test first:`, a
|
||||
// 1.8 File line with no backticked path right after the marker) changes nothing.
|
||||
|
||||
const OTHER_TEST = [
|
||||
"import { test } from 'node:test';",
|
||||
"test('other', () => {});",
|
||||
'',
|
||||
].join('\n');
|
||||
|
||||
/** A plan whose Step 1 changes lib/sum.mjs; `opts` overrides Step 1's builder options. */
|
||||
function planFile(dir, opts = {}, version = '1.8') {
|
||||
const p = join(dir, 'plan.local.md');
|
||||
writeFileSync(p, buildPlan({ version, steps: [
|
||||
{ file: 'lib/sum.mjs', test: 'tests/sum.test.mjs', ...opts },
|
||||
{ endToEnd: 'bash tests/e2e.sh' },
|
||||
] }));
|
||||
return p;
|
||||
}
|
||||
|
||||
/** The CLI with the child environment the suite requires (tests/helpers/child-env.mjs). */
|
||||
function cliPlan(args) {
|
||||
const r = spawnSync(process.execPath, [CLI, ...args], { encoding: 'utf8', env: childEnv() });
|
||||
return { status: r.status, stdout: r.stdout, stderr: r.stderr };
|
||||
}
|
||||
|
||||
test('red proof --plan/--step: a --tests that differs from the step\'s Test first File line is not-run, exit 2 (tests-mismatch)', () => {
|
||||
const { dir, base } = repo('a - b');
|
||||
try {
|
||||
addTest(dir);
|
||||
writeFileSync(join(dir, 'tests', 'other.test.mjs'), OTHER_TEST);
|
||||
const plan = planFile(dir);
|
||||
for (const given of ['tests/other.test.mjs', 'tests/sum.test.mjs,tests/other.test.mjs']) {
|
||||
for (const mode of [['--red-only'], []]) {
|
||||
const c = cliPlan([...mode, '--cwd', dir, '--base', base, '--cmd', TEST_CMD, '--tests', given, '--plan', plan, '--step', '1', '--json']);
|
||||
assert.equal(c.status, 2, `${given} ${mode.join(' ')}: ${c.stdout}${c.stderr}`);
|
||||
const out = JSON.parse(c.stdout);
|
||||
assert.equal(out.status, 'not-run', JSON.stringify(out));
|
||||
assert.match(out.reason, /^tests-mismatch/, JSON.stringify(out));
|
||||
assert.deepEqual(out.plan_tests, ['tests/sum.test.mjs']);
|
||||
}
|
||||
}
|
||||
// The API says the same.
|
||||
const r = proveRed({ cwd: dir, base, cmd: TEST_CMD, tests: ['tests/other.test.mjs'], redOnly: true, plan, step: 1 });
|
||||
assert.equal(r.status, 'not-run', JSON.stringify(r));
|
||||
assert.match(r.reason, /^tests-mismatch/);
|
||||
assert.deepEqual(r.plan_tests, ['tests/sum.test.mjs']);
|
||||
} finally { rmSync(dir, { recursive: true, force: true }); }
|
||||
});
|
||||
|
||||
test('red proof --plan/--step without --tests: the step\'s Test first file is the test list', () => {
|
||||
const { dir, base } = repo('a - b');
|
||||
try {
|
||||
addTest(dir); // test written, nothing built yet: red at the start point
|
||||
const plan = planFile(dir);
|
||||
const red = cliPlan(['--red-only', '--cwd', dir, '--base', base, '--cmd', TEST_CMD, '--plan', plan, '--step', '1', '--json']);
|
||||
assert.equal(red.status, 0, red.stdout + red.stderr);
|
||||
assert.equal(JSON.parse(red.stdout).status, 'proven');
|
||||
// Built: the full run proves red, green and the control from the plan's list alone.
|
||||
writeFileSync(join(dir, 'lib', 'sum.mjs'), 'export function sum(a, b) { return a + b; }\n');
|
||||
const full = cliPlan(['--cwd', dir, '--base', base, '--cmd', TEST_CMD, '--plan', plan, '--step', '1', '--json']);
|
||||
assert.equal(full.status, 0, full.stdout + full.stderr);
|
||||
assert.equal(JSON.parse(full.stdout).status, 'proven');
|
||||
// A step whose plan list is empty still needs --tests.
|
||||
const none = planFile(dir, { testFirst: false }, '1.7');
|
||||
const empty = cliPlan(['--red-only', '--cwd', dir, '--base', base, '--cmd', TEST_CMD, '--plan', none, '--step', '1', '--json']);
|
||||
assert.equal(empty.status, 2, empty.stdout + empty.stderr);
|
||||
assert.match(JSON.parse(empty.stdout).reason, /no test files given/);
|
||||
} finally { rmSync(dir, { recursive: true, force: true }); }
|
||||
});
|
||||
|
||||
test('red proof --plan/--step: a --tests equal to the plan\'s list (normalised) behaves as before', () => {
|
||||
const { dir, base } = repo('a - b');
|
||||
try {
|
||||
addTest(dir);
|
||||
const plan = planFile(dir);
|
||||
for (const given of ['tests/sum.test.mjs', './tests/sum.test.mjs']) {
|
||||
const before = cliPlan(['--red-only', '--cwd', dir, '--base', base, '--cmd', TEST_CMD, '--tests', given, '--json']);
|
||||
const withPlan = cliPlan(['--red-only', '--cwd', dir, '--base', base, '--cmd', TEST_CMD, '--tests', given, '--plan', plan, '--step', '1', '--json']);
|
||||
assert.equal(withPlan.status, 0, withPlan.stdout + withPlan.stderr);
|
||||
assert.equal(withPlan.status, before.status);
|
||||
const a = JSON.parse(withPlan.stdout);
|
||||
const b = JSON.parse(before.stdout);
|
||||
assert.equal(a.status, 'proven');
|
||||
assert.equal(a.reason, b.reason);
|
||||
assert.equal('plan_tests' in a, false, JSON.stringify(a));
|
||||
}
|
||||
} finally { rmSync(dir, { recursive: true, force: true }); }
|
||||
});
|
||||
|
||||
test('red proof --plan/--step: a 1.8 File line naming a source in its description lists only the test (decision 20)', () => {
|
||||
const { dir, base } = repo('a - b');
|
||||
try {
|
||||
addTest(dir);
|
||||
// Built: were lib/sum.mjs on the list, it would be copied into the start
|
||||
// point and the start point would go green (not-red-at-base).
|
||||
writeFileSync(join(dir, 'lib', 'sum.mjs'), 'export function sum(a, b) { return a + b; }\n');
|
||||
const plan = planFile(dir, { testFirstRaw: ' - File: `tests/sum.test.mjs` (existing) - covers `lib/sum.mjs`' });
|
||||
const derived = cliPlan(['--red-only', '--cwd', dir, '--base', base, '--cmd', TEST_CMD, '--plan', plan, '--step', '1', '--json']);
|
||||
assert.equal(derived.status, 0, derived.stdout + derived.stderr);
|
||||
assert.equal(JSON.parse(derived.stdout).status, 'proven');
|
||||
const given = cliPlan(['--red-only', '--cwd', dir, '--base', base, '--cmd', TEST_CMD, '--tests', 'tests/sum.test.mjs', '--plan', plan, '--step', '1', '--json']);
|
||||
assert.equal(given.status, 0, given.stdout + given.stderr);
|
||||
assert.doesNotMatch(JSON.parse(given.stdout).reason, /tests-mismatch/);
|
||||
} finally { rmSync(dir, { recursive: true, force: true }); }
|
||||
});
|
||||
|
||||
test('red proof --plan/--step: an empty plan list (1.7 without Test first, 1.8 File line without a backticked path) changes nothing', () => {
|
||||
const { dir, base } = repo('a - b');
|
||||
try {
|
||||
addTest(dir);
|
||||
writeFileSync(join(dir, 'tests', 'other.test.mjs'), OTHER_TEST);
|
||||
// planFile writes one path: read each plan's text before the next one is written.
|
||||
const plans = [];
|
||||
plans.push(readFileSync(planFile(dir, { testFirst: false }, '1.7'), 'utf8'));
|
||||
plans.push(readFileSync(planFile(dir, { testFirstRaw: ' - File: see `tests/sum.test.mjs` for the test' }, '1.8'), 'utf8'));
|
||||
const args = ['--red-only', '--cwd', dir, '--base', base, '--cmd', TEST_CMD, '--tests', 'tests/other.test.mjs', '--json'];
|
||||
const before = cliPlan(args);
|
||||
const b = JSON.parse(before.stdout);
|
||||
for (const content of plans) {
|
||||
const p = join(dir, 'plan.local.md');
|
||||
writeFileSync(p, content);
|
||||
const c = cliPlan([...args, '--plan', p, '--step', '1']);
|
||||
assert.equal(c.status, before.status, c.stdout + c.stderr);
|
||||
const out = JSON.parse(c.stdout);
|
||||
assert.doesNotMatch(out.reason, /tests-mismatch/, JSON.stringify(out));
|
||||
assert.equal(out.status, b.status);
|
||||
assert.equal(out.reason, b.reason);
|
||||
assert.equal('plan_tests' in out, false);
|
||||
}
|
||||
} finally { rmSync(dir, { recursive: true, force: true }); }
|
||||
});
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue