From 5504724d7a85024b84398d227180ce38cb3b55dd Mon Sep 17 00:00:00 2001 From: sim Date: Fri, 28 Aug 2026 19:29:51 -0400 Subject: [PATCH] fix(#4012): the reporter body must return nully, not an iterable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fourth failure on this feature, and this one was caused by the previous fix's lint workaround. TypeError [ERR_INVALID_RETURN_VALUE]: Expected nully to be returned from the "body" function but got an instance of Array. When stream.compose is given an async FUNCTION as the body, that function must return nully. The reporter ended with 'return []' under a comment asserting Node "still requires the exported function to return an iterable" — exactly backwards, and the direct cause of 41 failures across every run-tests.cjs invocation. That return existed only to dodge ESLint's require-yield after the previous commit converted async function* to async function. A lint workaround became a runtime crash, and the comment written to justify it stated the opposite of the contract. Both are now corrected to what the runtime actually does. Verified by EXECUTION rather than by reading: composing the real reporter against a fake event stream completes with no error and leaves both handled events durably on disk, with the ignored event type skipped. The failing form was reproduced the same way first, so the diagnosis is not inferred from the stack trace alone. The new unit test requires the reporter directly and asserts the returned value is nully — the assertion that would have caught this before it reached the runner — plus the exact NDJSON written. No subprocess, so this half of the feature is verifiable without a full runner pass, which matters because every defect in this feature so far has only been observable remotely. T1 remains untouched and red. Verification runs on the remote runner. Refs #4012 --- scripts/lib/ndjson-reporter.cjs | 31 ++++++++++++++-------- tests/run-tests-harness.test.cjs | 45 ++++++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+), 11 deletions(-) diff --git a/scripts/lib/ndjson-reporter.cjs b/scripts/lib/ndjson-reporter.cjs index fcc3f7663..dba6c2348 100644 --- a/scripts/lib/ndjson-reporter.cjs +++ b/scripts/lib/ndjson-reporter.cjs @@ -34,13 +34,20 @@ // Contract targeted: Node's "Custom reporters" contract // (https://nodejs.org/api/test.html#custom-reporters) — a reporter module's // default export is a function receiving the test runner's event stream (an -// AsyncIterable of `{ type, data }` objects) and returning an iterable (sync -// or async) of the reporter's output. This one intentionally emits no output -// (see the durability note above) — a plain `async function` that returns an -// empty array satisfies the contract without an `async function*` generator -// that would otherwise never `yield` (require-yield). This repo's -// `engines.node` requires >=24.0.0 (package.json), where this contract has -// been stable since Node 20. +// AsyncIterable of `{ type, data }` objects). Node feeds that function to +// `stream.compose` as the stream's "body". When the body is an async +// FUNCTION (not an `async function*` generator), `stream.compose`'s own +// contract (https://nodejs.org/api/stream.html#streamcomposestreams) +// requires it to return nully (undefined/null) — returning anything else, +// including an array, throws `ERR_INVALID_RETURN_VALUE` ("Expected nully to +// be returned from the 'body' function but got an instance of Array") +// exactly once the promise resolves. Verified directly against +// `stream.compose` in this repo's Node: calling it with a body that +// `return`s `[]` reproduces that same TypeError. A generator form +// (`async function*`) is the one that yields an iterable; the plain +// `async function` form used here is the one that must return nully. This +// repo's `engines.node` requires >=24.0.0 (package.json), where both +// contracts have been stable since Node 20. // // Kept intentionally tiny: only the three event types run-tests.cjs needs to // pair start/completion are handled; everything else (diagnostics, plans, @@ -74,8 +81,10 @@ module.exports = async function ndjsonEventReporter(source) { } } } - // Intentionally empty: this reporter is a pure side-effecting sink (see the - // durability note above), never a source of reporter OUTPUT. Node still - // requires the exported function to return an iterable. - return []; + // Falls through to an implicit `return undefined` (nully): this reporter is + // a pure side-effecting sink (see the durability note above), never a + // source of reporter OUTPUT, and `stream.compose` requires its async + // FUNCTION body to return nully — returning an iterable (e.g. `[]`) here + // raises `ERR_INVALID_RETURN_VALUE` (observed live: this exact `return []` + // crashed every chunk on the real remote run this regresses). }; diff --git a/tests/run-tests-harness.test.cjs b/tests/run-tests-harness.test.cjs index 68d856bfc..7b4d42ddf 100644 --- a/tests/run-tests-harness.test.cjs +++ b/tests/run-tests-harness.test.cjs @@ -24,6 +24,7 @@ const path = require('path'); const { runNode } = require('./helpers/process-seam.cjs'); const { toLegacyResult } = require('./helpers/git-fixture.cjs'); const { createTempDir, cleanup, CONFIG_LOCATION_ENV_KEYS } = require('./helpers.cjs'); +const { splitLines } = require('../gsd-core/bin/lib/text-lines.cjs'); const HARNESS = path.join(__dirname, '..', 'scripts', 'run-tests.cjs'); @@ -905,6 +906,50 @@ test('hangs forever', () => new Promise(() => {})); ); }); + // Regression (#3889 recurrence): the reporter module itself, called + // directly with no subprocess, must return nully — this is the exact + // contract violation (`return []`) that crashed every chunk on the real + // remote run this file regresses ("Expected nully to be returned from + // the 'body' function but got an instance of Array", thrown by + // node:stream's `compose` when its async-function body returns an + // iterable instead of undefined/null). Also pins the NDJSON side effect: + // only the two handled event types are appended, verbatim, one per line. + test('the reporter returns nully and appends only the handled event types as NDJSON', async () => { + const reporter = require('../scripts/lib/ndjson-reporter.cjs'); + const eventsFile = path.join(tmpDir, 'ndjson-reporter-events.ndjson'); + const savedEventsFile = process.env.GSD_RUN_TESTS_EVENTS_FILE; + process.env.GSD_RUN_TESTS_EVENTS_FILE = eventsFile; + try { + async function* fakeEvents() { + yield { type: 'test:start', data: { file: 'a.test.cjs', name: 't', nesting: 0, testNumber: 1 } }; + yield { type: 'test:diagnostic', data: { message: 'ignored' } }; + yield { type: 'test:pass', data: { file: 'a.test.cjs', name: 't', nesting: 0, testNumber: 1 } }; + } + const result = await reporter(fakeEvents()); + assert.strictEqual( + result ?? null, + null, + `expected the reporter to return nully (undefined/null) per stream.compose's ` + + `async-function body contract; got ${JSON.stringify(result)}`, + ); + const rawContent = fs.readFileSync(eventsFile, 'utf8'); + const lines = splitLines(rawContent.trim()).filter((l) => l.length > 0); + assert.strictEqual(lines.length, 2, `expected exactly 2 NDJSON lines; got:\n${lines.join('\n')}`); + const [start, pass] = lines.map((l) => JSON.parse(l)); + assert.strictEqual(start.type, 'test:start'); + assert.strictEqual(start.file, 'a.test.cjs'); + assert.strictEqual(pass.type, 'test:pass'); + assert.strictEqual(pass.file, 'a.test.cjs'); + } finally { + if (savedEventsFile === undefined) { + delete process.env.GSD_RUN_TESTS_EVENTS_FILE; + } else { + process.env.GSD_RUN_TESTS_EVENTS_FILE = savedEventsFile; + } + cleanup(eventsFile); + } + }); + // T4: the pre-existing timeout / abort / force-exit behavior (#1051) // still holds with the reporter instrumentation wired in — the new // --test-reporter flags must not change detection, the abort-on-timeout