From 0e27a7a35ab0e236f1ae36305c4aa1f0b306e43d Mon Sep 17 00:00:00 2001 From: Moshe Atlow Date: Thu, 21 Sep 2023 09:17:23 +0300 Subject: [PATCH] test_runner: catch reporter errors PR-URL: https://github.com/nodejs/node/pull/49646 Fixes: https://github.com/nodejs/node/issues/48937 Reviewed-By: Chemi Atlow Reviewed-By: Benjamin Gruenbaum Reviewed-By: Colin Ihrig --- lib/internal/test_runner/harness.js | 10 ++++++++- lib/internal/test_runner/utils.js | 8 ++++--- .../custom_reporters/throwing-async.js | 8 +++++++ .../test-runner/custom_reporters/throwing.js | 6 ++++++ test/parallel/test-runner-reporters.js | 21 +++++++++++++++++++ 5 files changed, 49 insertions(+), 4 deletions(-) create mode 100644 test/fixtures/test-runner/custom_reporters/throwing-async.js create mode 100644 test/fixtures/test-runner/custom_reporters/throwing.js diff --git a/lib/internal/test_runner/harness.js b/lib/internal/test_runner/harness.js index 357347627fcc2b..2f18b0bcf091ac 100644 --- a/lib/internal/test_runner/harness.js +++ b/lib/internal/test_runner/harness.js @@ -21,12 +21,15 @@ const { kEmptyObject } = require('internal/util'); const { kCancelledByParent, Test, Suite } = require('internal/test_runner/test'); const { parseCommandLine, + reporterScope, setupTestReporters, } = require('internal/test_runner/utils'); const { bigint: hrtime } = process.hrtime; const testResources = new SafeMap(); +testResources.set(reporterScope.asyncId(), reporterScope); + function createTestTree(options = kEmptyObject) { return setup(new Test({ __proto__: null, ...options, name: '' })); } @@ -40,9 +43,14 @@ function createProcessEventHandler(eventName, rootTest) { throw err; } - // Check if this error is coming from a test. If it is, fail the test. const test = testResources.get(executionAsyncId()); + // Check if this error is coming from a reporter. If it is, throw it. + if (test === reporterScope) { + throw err; + } + + // Check if this error is coming from a test. If it is, fail the test. if (!test || test.finished) { // If the test is already finished or the resource that created the error // is not mapped to a Test, report this as a top level diagnostic. diff --git a/lib/internal/test_runner/utils.js b/lib/internal/test_runner/utils.js index ba1b4f0fa10869..6b2620601e41ef 100644 --- a/lib/internal/test_runner/utils.js +++ b/lib/internal/test_runner/utils.js @@ -20,6 +20,7 @@ const { StringPrototypeSlice, } = primordials; +const { AsyncResource } = require('async_hooks'); const { basename, relative } = require('path'); const { createWriteStream } = require('fs'); const { pathToFileURL } = require('internal/url'); @@ -169,15 +170,15 @@ async function getReportersMap(reporters, destinations, rootTest) { }); } - -async function setupTestReporters(rootTest) { +const reporterScope = new AsyncResource('TestReporterScope'); +const setupTestReporters = reporterScope.bind(async (rootTest) => { const { reporters, destinations } = parseCommandLine(); const reportersMap = await getReportersMap(reporters, destinations, rootTest); for (let i = 0; i < reportersMap.length; i++) { const { reporter, destination } = reportersMap[i]; compose(rootTest.reporter, reporter).pipe(destination); } -} +}); let globalTestOptions; @@ -424,6 +425,7 @@ module.exports = { isSupportedFileType, isTestFailureError, parseCommandLine, + reporterScope, setupTestReporters, getCoverageReport, }; diff --git a/test/fixtures/test-runner/custom_reporters/throwing-async.js b/test/fixtures/test-runner/custom_reporters/throwing-async.js new file mode 100644 index 00000000000000..b24a632e697e4f --- /dev/null +++ b/test/fixtures/test-runner/custom_reporters/throwing-async.js @@ -0,0 +1,8 @@ +'use strict'; + +module.exports = async function * customReporter() { + yield 'Going to throw an error\n'; + setImmediate(() => { + throw new Error('Reporting error'); + }); +}; diff --git a/test/fixtures/test-runner/custom_reporters/throwing.js b/test/fixtures/test-runner/custom_reporters/throwing.js new file mode 100644 index 00000000000000..8d04901c773ac8 --- /dev/null +++ b/test/fixtures/test-runner/custom_reporters/throwing.js @@ -0,0 +1,6 @@ +'use strict'; + +module.exports = async function * customReporter() { + yield 'Going to throw an error\n'; + throw new Error('Reporting error'); +}; diff --git a/test/parallel/test-runner-reporters.js b/test/parallel/test-runner-reporters.js index c7c883286c70c0..bb831491366dfc 100644 --- a/test/parallel/test-runner-reporters.js +++ b/test/parallel/test-runner-reporters.js @@ -134,4 +134,25 @@ describe('node:test reporters', { concurrency: true }, () => { assert.strictEqual(child.stdout.toString(), ''); assert.match(child.stderr.toString(), /ERR_INVALID_ARG_TYPE/); }); + + it('should throw when reporter errors', async () => { + const child = spawnSync(process.execPath, + ['--test', '--test-reporter', fixtures.fileURL('test-runner/custom_reporters/throwing.js'), + fixtures.path('test-runner/default-behavior/index.test.js')]); + assert.strictEqual(child.status, 7); + assert.strictEqual(child.signal, null); + assert.strictEqual(child.stdout.toString(), 'Going to throw an error\n'); + assert.match(child.stderr.toString(), /Error: Reporting error\r?\n\s+at customReporter/); + }); + + it('should throw when reporter errors asynchronously', async () => { + const child = spawnSync(process.execPath, + ['--test', '--test-reporter', + fixtures.fileURL('test-runner/custom_reporters/throwing-async.js'), + fixtures.path('test-runner/default-behavior/index.test.js')]); + assert.strictEqual(child.status, 7); + assert.strictEqual(child.signal, null); + assert.strictEqual(child.stdout.toString(), 'Going to throw an error\n'); + assert.match(child.stderr.toString(), /Emitted 'error' event on Duplex instance/); + }); });