From 065f8575e142c794bdab7acece13170bf37c806b Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Thu, 8 Oct 2026 08:45:48 -0600 Subject: [PATCH 1/5] fix(run): don't fail the run on a failing todo or skip test node:test semantics: a failing `todo` test (and a `skip` test, for consistency) doesn't fail the overall run. The `test:fail` handler was unconditionally setting process.exitCode=1 and recording the file in failedFiles, turning any todo-marked known failure into a red CI job. Dispatch-Task: integration-testing-todo-not-failure Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01FRu37LkthRcNpozRAsiodp --- src/run.ts | 4 +++ test/run.test.ts | 75 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 79 insertions(+) create mode 100644 test/run.test.ts diff --git a/src/run.ts b/src/run.ts index d9d6bea..6a04d4c 100755 --- a/src/run.ts +++ b/src/run.ts @@ -125,6 +125,10 @@ runner.on('test:stdout', (data: any) => { }); runner.on('test:fail', (data: any) => { + // node:test semantics: a failing `todo` or `skip` test doesn't fail the run. + if (data.todo || data.skip) { + return; + } process.exitCode = 1; if (data.file) { failedFiles.add(resolve(data.file)); diff --git a/test/run.test.ts b/test/run.test.ts new file mode 100644 index 0000000..ecca117 --- /dev/null +++ b/test/run.test.ts @@ -0,0 +1,75 @@ +import { test, before, after } from 'node:test'; +import { strictEqual } from 'node:assert'; +import { spawn } from 'node:child_process'; +import { once } from 'node:events'; +import { mkdtempSync, writeFileSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { fileURLToPath } from 'node:url'; + +const RUN_SCRIPT = fileURLToPath(new URL('../src/run.ts', import.meta.url)); + +// A todo-marked failure and a real failure, run through the actual CLI to exercise its exit-code +// semantics end to end (node:test's `test:fail` event fires for both; only the real one must fail the run). +const FIXTURE_SOURCES: Record = { + 'todo-fail.test.mjs': ` +import { test } from 'node:test'; +import assert from 'node:assert'; + +test('a todo test that fails', { todo: true }, () => { + assert.strictEqual(1, 2); +}); +`, + 'skip-fail.test.mjs': ` +import { test } from 'node:test'; +import assert from 'node:assert'; + +test('a skipped test that fails', { skip: true }, () => { + assert.strictEqual(1, 2); +}); +`, + 'real-fail.test.mjs': ` +import { test } from 'node:test'; +import assert from 'node:assert'; + +test('a real failure', () => { + assert.strictEqual(1, 2); +}); +`, +}; + +let fixtureDir: string; +const fixtures: Record = {}; + +before(() => { + fixtureDir = mkdtempSync(join(tmpdir(), 'harper-it-run-fixtures-')); + for (const [name, src] of Object.entries(FIXTURE_SOURCES)) { + const path = join(fixtureDir, name); + writeFileSync(path, src); + fixtures[name] = path; + } +}); + +after(() => { + rmSync(fixtureDir, { recursive: true, force: true }); +}); + +async function runCli(globPattern: string): Promise { + const child = spawn(process.execPath, [RUN_SCRIPT, '--isolation=none', globPattern], { + stdio: 'ignore', + }); + const [code] = await once(child, 'exit'); + return code ?? 0; +} + +test('a suite whose only failure is a todo test exits 0', async () => { + strictEqual(await runCli(fixtures['todo-fail.test.mjs']), 0); +}); + +test('a suite whose only failure is a skipped test exits 0', async () => { + strictEqual(await runCli(fixtures['skip-fail.test.mjs']), 0); +}); + +test('a suite with a real failure exits 1 (control)', async () => { + strictEqual(await runCli(fixtures['real-fail.test.mjs']), 1); +}); From f41f56eeb886a533a7e4059191ed1138b9ec57c2 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Thu, 8 Oct 2026 09:00:04 -0600 Subject: [PATCH 2/5] fix(run): match node's own todo/skip classification, harden the self-test - data.todo/data.skip can be a falsy-but-present reason string (e.g. { todo: '' }); the || check treated that as a real failure. Match node's own CLI logic (todo/skip !== undefined && !== false). - The skip fixture used { skip: true }, which skips the body entirely and never reaches the data.skip branch; drive it with t.skip() from inside the body instead. - runCli silently turned a signal-killed child into exit 0; assert no signal. - Guard the after() hook against fixtureDir being unset if before() throws. - Strip HARPER_INTEGRATION_TEST_* from the spawned CLI's env so an inherited HARPER_INTEGRATION_TEST_SHARD/ONLY can't change file selection. - Bound each test with an explicit timeout instead of an unbounded wait. Found by the pre-push cross-model review (codex + gemini + cursor-composer). Dispatch-Task: integration-testing-todo-not-failure Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01FRu37LkthRcNpozRAsiodp --- src/run.ts | 6 ++++-- test/run.test.ts | 29 ++++++++++++++++++----------- 2 files changed, 22 insertions(+), 13 deletions(-) diff --git a/src/run.ts b/src/run.ts index 6a04d4c..6204b3a 100755 --- a/src/run.ts +++ b/src/run.ts @@ -125,8 +125,10 @@ runner.on('test:stdout', (data: any) => { }); runner.on('test:fail', (data: any) => { - // node:test semantics: a failing `todo` or `skip` test doesn't fail the run. - if (data.todo || data.skip) { + // node:test semantics: a failing `todo` or `skip` test doesn't fail the run. Match node's own + // CLI classification (not `|| data.todo`) so a falsy-but-present reason, e.g. `{ todo: '' }`, still counts. + const isTodoOrSkip = (data.todo !== undefined && data.todo !== false) || (data.skip !== undefined && data.skip !== false); + if (isTodoOrSkip) { return; } process.exitCode = 1; diff --git a/test/run.test.ts b/test/run.test.ts index ecca117..55da528 100644 --- a/test/run.test.ts +++ b/test/run.test.ts @@ -1,5 +1,5 @@ import { test, before, after } from 'node:test'; -import { strictEqual } from 'node:assert'; +import { strictEqual, ok } from 'node:assert'; import { spawn } from 'node:child_process'; import { once } from 'node:events'; import { mkdtempSync, writeFileSync, rmSync } from 'node:fs'; @@ -9,8 +9,8 @@ import { fileURLToPath } from 'node:url'; const RUN_SCRIPT = fileURLToPath(new URL('../src/run.ts', import.meta.url)); -// A todo-marked failure and a real failure, run through the actual CLI to exercise its exit-code -// semantics end to end (node:test's `test:fail` event fires for both; only the real one must fail the run). +// The skip fixture calls `t.skip()` from inside the body instead of passing `{ skip: true }`, which +// skips the body entirely and would never reach the `data.skip` branch under test. const FIXTURE_SOURCES: Record = { 'todo-fail.test.mjs': ` import { test } from 'node:test'; @@ -24,7 +24,8 @@ test('a todo test that fails', { todo: true }, () => { import { test } from 'node:test'; import assert from 'node:assert'; -test('a skipped test that fails', { skip: true }, () => { +test('a skipped test that fails', (t) => { + t.skip('skipped for testing'); assert.strictEqual(1, 2); }); `, @@ -38,7 +39,7 @@ test('a real failure', () => { `, }; -let fixtureDir: string; +let fixtureDir: string | undefined; const fixtures: Record = {}; before(() => { @@ -51,25 +52,31 @@ before(() => { }); after(() => { - rmSync(fixtureDir, { recursive: true, force: true }); + if (fixtureDir) rmSync(fixtureDir, { recursive: true, force: true }); }); +// Strip the runner's own env vars so a shell that happens to export e.g. HARPER_INTEGRATION_TEST_SHARD +// for an outer CI job can't change which fixture files the spawned CLI selects. +const cleanEnv = Object.fromEntries(Object.entries(process.env).filter(([key]) => !key.startsWith('HARPER_INTEGRATION_TEST_'))); + async function runCli(globPattern: string): Promise { const child = spawn(process.execPath, [RUN_SCRIPT, '--isolation=none', globPattern], { stdio: 'ignore', + env: cleanEnv, }); - const [code] = await once(child, 'exit'); - return code ?? 0; + const [code, signal] = await once(child, 'exit'); + ok(signal === null, `run.ts was killed by signal ${signal}`); + return code; } -test('a suite whose only failure is a todo test exits 0', async () => { +test('a suite whose only failure is a todo test exits 0', { timeout: 15_000 }, async () => { strictEqual(await runCli(fixtures['todo-fail.test.mjs']), 0); }); -test('a suite whose only failure is a skipped test exits 0', async () => { +test('a suite whose only failure is a skipped test exits 0', { timeout: 15_000 }, async () => { strictEqual(await runCli(fixtures['skip-fail.test.mjs']), 0); }); -test('a suite with a real failure exits 1 (control)', async () => { +test('a suite with a real failure exits 1 (control)', { timeout: 15_000 }, async () => { strictEqual(await runCli(fixtures['real-fail.test.mjs']), 1); }); From 924b17967e80fd73886bdb8810b629c4cedc286c Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Thu, 8 Oct 2026 09:10:42 -0600 Subject: [PATCH 3/5] fix(run): kill the spawned CLI when a run.test.ts timeout fires - Wire each test's t.signal into runCli -> spawn, so an abort (the test timeout, or a future node:test cancellation) sends SIGTERM to the child instead of leaving it running and holding its process handle, which was the one way these new timeouts could still hang npm test. - Add a regression fixture for the exact edge case this fix closes: a todo test with a falsy-but-present reason ({ todo: '' }). - Trim two comments that spoke to this review round rather than the next reader. Found by round 2 of the pre-push cross-model review (codex + gemini). Dispatch-Task: integration-testing-todo-not-failure Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01FRu37LkthRcNpozRAsiodp --- src/run.ts | 4 ++-- test/run.test.ts | 33 ++++++++++++++++++++++----------- 2 files changed, 24 insertions(+), 13 deletions(-) diff --git a/src/run.ts b/src/run.ts index 6204b3a..ae2c11d 100755 --- a/src/run.ts +++ b/src/run.ts @@ -125,8 +125,8 @@ runner.on('test:stdout', (data: any) => { }); runner.on('test:fail', (data: any) => { - // node:test semantics: a failing `todo` or `skip` test doesn't fail the run. Match node's own - // CLI classification (not `|| data.todo`) so a falsy-but-present reason, e.g. `{ todo: '' }`, still counts. + // node:test semantics: a failing `todo` or `skip` test doesn't fail the run. A reason can be a + // falsy string, e.g. `{ todo: '' }`, so classify by marker presence, not truthiness. const isTodoOrSkip = (data.todo !== undefined && data.todo !== false) || (data.skip !== undefined && data.skip !== false); if (isTodoOrSkip) { return; diff --git a/test/run.test.ts b/test/run.test.ts index 55da528..f76350a 100644 --- a/test/run.test.ts +++ b/test/run.test.ts @@ -28,6 +28,14 @@ test('a skipped test that fails', (t) => { t.skip('skipped for testing'); assert.strictEqual(1, 2); }); +`, + 'empty-reason-todo-fail.test.mjs': ` +import { test } from 'node:test'; +import assert from 'node:assert'; + +test('a todo test with a falsy reason that fails', { todo: '' }, () => { + assert.strictEqual(1, 2); +}); `, 'real-fail.test.mjs': ` import { test } from 'node:test'; @@ -55,28 +63,31 @@ after(() => { if (fixtureDir) rmSync(fixtureDir, { recursive: true, force: true }); }); -// Strip the runner's own env vars so a shell that happens to export e.g. HARPER_INTEGRATION_TEST_SHARD -// for an outer CI job can't change which fixture files the spawned CLI selects. const cleanEnv = Object.fromEntries(Object.entries(process.env).filter(([key]) => !key.startsWith('HARPER_INTEGRATION_TEST_'))); -async function runCli(globPattern: string): Promise { +async function runCli(globPattern: string, signal: AbortSignal): Promise { const child = spawn(process.execPath, [RUN_SCRIPT, '--isolation=none', globPattern], { stdio: 'ignore', env: cleanEnv, + signal, }); - const [code, signal] = await once(child, 'exit'); - ok(signal === null, `run.ts was killed by signal ${signal}`); + const [code, exitSignal] = await once(child, 'exit'); + ok(exitSignal === null, `run.ts was killed by signal ${exitSignal}`); return code; } -test('a suite whose only failure is a todo test exits 0', { timeout: 15_000 }, async () => { - strictEqual(await runCli(fixtures['todo-fail.test.mjs']), 0); +test('a suite whose only failure is a todo test exits 0', { timeout: 15_000 }, async (t) => { + strictEqual(await runCli(fixtures['todo-fail.test.mjs'], t.signal), 0); +}); + +test('a suite whose only failure is a skipped test exits 0', { timeout: 15_000 }, async (t) => { + strictEqual(await runCli(fixtures['skip-fail.test.mjs'], t.signal), 0); }); -test('a suite whose only failure is a skipped test exits 0', { timeout: 15_000 }, async () => { - strictEqual(await runCli(fixtures['skip-fail.test.mjs']), 0); +test('a suite whose only failure is a todo test with a falsy reason exits 0', { timeout: 15_000 }, async (t) => { + strictEqual(await runCli(fixtures['empty-reason-todo-fail.test.mjs'], t.signal), 0); }); -test('a suite with a real failure exits 1 (control)', { timeout: 15_000 }, async () => { - strictEqual(await runCli(fixtures['real-fail.test.mjs']), 1); +test('a suite with a real failure exits 1 (control)', { timeout: 15_000 }, async (t) => { + strictEqual(await runCli(fixtures['real-fail.test.mjs'], t.signal), 1); }); From 949e8552befa2fbc562caba6fa1ab8721d0d3dd3 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Thu, 8 Oct 2026 15:18:58 -0600 Subject: [PATCH 4/5] refactor(run): spell out which todo/skip marker values skip a failure node:test emits the todo/skip marker as `reason ?? true`, so the values a failing todo/skip test carries are `true` or the reason string, which can be empty. Accept exactly those, inline, instead of every non-undefined, non-false value (per review). Dispatch-Task: fix-kriszyp_integration-testing_41-da7fa633 Co-Authored-By: Claude Opus 5.5 --- src/run.ts | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/run.ts b/src/run.ts index ae2c11d..7ade731 100755 --- a/src/run.ts +++ b/src/run.ts @@ -125,10 +125,9 @@ runner.on('test:stdout', (data: any) => { }); runner.on('test:fail', (data: any) => { - // node:test semantics: a failing `todo` or `skip` test doesn't fail the run. A reason can be a - // falsy string, e.g. `{ todo: '' }`, so classify by marker presence, not truthiness. - const isTodoOrSkip = (data.todo !== undefined && data.todo !== false) || (data.skip !== undefined && data.skip !== false); - if (isTodoOrSkip) { + // node:test semantics: a failing `todo` or `skip` test doesn't fail the run. The marker is `true` + // or the reason string, which can be empty (`{ todo: '' }`). + if (data.todo || data.todo === '' || data.skip || data.skip === '') { return; } process.exitCode = 1; From 324c7f6b4e22adc56a1a9c131f1fae8255788aee Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Thu, 8 Oct 2026 15:18:58 -0600 Subject: [PATCH 5/5] test(run): move the run.test.ts fixtures into test/fixtures/run Fixture tests as real files instead of JS inside template strings, so they read and edit like any other test file (per review). Drops the mkdtemp/write/cleanup hooks they needed. Dispatch-Task: fix-kriszyp_integration-testing_41-da7fa633 Co-Authored-By: Claude Opus 5.5 --- test/fixtures/run/empty-reason-todo-fail.ts | 6 ++ test/fixtures/run/real-fail.ts | 6 ++ test/fixtures/run/skip-fail.ts | 9 +++ test/fixtures/run/todo-fail.ts | 6 ++ test/run.test.ts | 71 +++------------------ 5 files changed, 36 insertions(+), 62 deletions(-) create mode 100644 test/fixtures/run/empty-reason-todo-fail.ts create mode 100644 test/fixtures/run/real-fail.ts create mode 100644 test/fixtures/run/skip-fail.ts create mode 100644 test/fixtures/run/todo-fail.ts diff --git a/test/fixtures/run/empty-reason-todo-fail.ts b/test/fixtures/run/empty-reason-todo-fail.ts new file mode 100644 index 0000000..ca60716 --- /dev/null +++ b/test/fixtures/run/empty-reason-todo-fail.ts @@ -0,0 +1,6 @@ +import { test } from 'node:test'; +import assert from 'node:assert'; + +test('a todo test with an empty reason that fails', { todo: '' }, () => { + assert.strictEqual(1, 2); +}); diff --git a/test/fixtures/run/real-fail.ts b/test/fixtures/run/real-fail.ts new file mode 100644 index 0000000..be2a719 --- /dev/null +++ b/test/fixtures/run/real-fail.ts @@ -0,0 +1,6 @@ +import { test } from 'node:test'; +import assert from 'node:assert'; + +test('a real failure', () => { + assert.strictEqual(1, 2); +}); diff --git a/test/fixtures/run/skip-fail.ts b/test/fixtures/run/skip-fail.ts new file mode 100644 index 0000000..a9a8a19 --- /dev/null +++ b/test/fixtures/run/skip-fail.ts @@ -0,0 +1,9 @@ +import { test } from 'node:test'; +import assert from 'node:assert'; + +// `t.skip()` from inside the body, not `{ skip: true }`: the option skips the body entirely, so the +// test never fails and never reaches the runner's `data.skip` branch. +test('a skipped test that fails', (t) => { + t.skip('skipped for testing'); + assert.strictEqual(1, 2); +}); diff --git a/test/fixtures/run/todo-fail.ts b/test/fixtures/run/todo-fail.ts new file mode 100644 index 0000000..0ce32c5 --- /dev/null +++ b/test/fixtures/run/todo-fail.ts @@ -0,0 +1,6 @@ +import { test } from 'node:test'; +import assert from 'node:assert'; + +test('a todo test that fails', { todo: true }, () => { + assert.strictEqual(1, 2); +}); diff --git a/test/run.test.ts b/test/run.test.ts index f76350a..ee18f2b 100644 --- a/test/run.test.ts +++ b/test/run.test.ts @@ -1,67 +1,14 @@ -import { test, before, after } from 'node:test'; +import { test } from 'node:test'; import { strictEqual, ok } from 'node:assert'; import { spawn } from 'node:child_process'; import { once } from 'node:events'; -import { mkdtempSync, writeFileSync, rmSync } from 'node:fs'; -import { tmpdir } from 'node:os'; -import { join } from 'node:path'; import { fileURLToPath } from 'node:url'; const RUN_SCRIPT = fileURLToPath(new URL('../src/run.ts', import.meta.url)); -// The skip fixture calls `t.skip()` from inside the body instead of passing `{ skip: true }`, which -// skips the body entirely and would never reach the `data.skip` branch under test. -const FIXTURE_SOURCES: Record = { - 'todo-fail.test.mjs': ` -import { test } from 'node:test'; -import assert from 'node:assert'; - -test('a todo test that fails', { todo: true }, () => { - assert.strictEqual(1, 2); -}); -`, - 'skip-fail.test.mjs': ` -import { test } from 'node:test'; -import assert from 'node:assert'; - -test('a skipped test that fails', (t) => { - t.skip('skipped for testing'); - assert.strictEqual(1, 2); -}); -`, - 'empty-reason-todo-fail.test.mjs': ` -import { test } from 'node:test'; -import assert from 'node:assert'; - -test('a todo test with a falsy reason that fails', { todo: '' }, () => { - assert.strictEqual(1, 2); -}); -`, - 'real-fail.test.mjs': ` -import { test } from 'node:test'; -import assert from 'node:assert'; - -test('a real failure', () => { - assert.strictEqual(1, 2); -}); -`, -}; - -let fixtureDir: string | undefined; -const fixtures: Record = {}; - -before(() => { - fixtureDir = mkdtempSync(join(tmpdir(), 'harper-it-run-fixtures-')); - for (const [name, src] of Object.entries(FIXTURE_SOURCES)) { - const path = join(fixtureDir, name); - writeFileSync(path, src); - fixtures[name] = path; - } -}); - -after(() => { - if (fixtureDir) rmSync(fixtureDir, { recursive: true, force: true }); -}); +function fixture(name: string): string { + return fileURLToPath(new URL(`./fixtures/run/${name}`, import.meta.url)); +} const cleanEnv = Object.fromEntries(Object.entries(process.env).filter(([key]) => !key.startsWith('HARPER_INTEGRATION_TEST_'))); @@ -77,17 +24,17 @@ async function runCli(globPattern: string, signal: AbortSignal): Promise } test('a suite whose only failure is a todo test exits 0', { timeout: 15_000 }, async (t) => { - strictEqual(await runCli(fixtures['todo-fail.test.mjs'], t.signal), 0); + strictEqual(await runCli(fixture('todo-fail.ts'), t.signal), 0); }); test('a suite whose only failure is a skipped test exits 0', { timeout: 15_000 }, async (t) => { - strictEqual(await runCli(fixtures['skip-fail.test.mjs'], t.signal), 0); + strictEqual(await runCli(fixture('skip-fail.ts'), t.signal), 0); }); -test('a suite whose only failure is a todo test with a falsy reason exits 0', { timeout: 15_000 }, async (t) => { - strictEqual(await runCli(fixtures['empty-reason-todo-fail.test.mjs'], t.signal), 0); +test('a suite whose only failure is a todo test with an empty reason exits 0', { timeout: 15_000 }, async (t) => { + strictEqual(await runCli(fixture('empty-reason-todo-fail.ts'), t.signal), 0); }); test('a suite with a real failure exits 1 (control)', { timeout: 15_000 }, async (t) => { - strictEqual(await runCli(fixtures['real-fail.test.mjs'], t.signal), 1); + strictEqual(await runCli(fixture('real-fail.ts'), t.signal), 1); });