From 5f38e70ff9a954f0f29f981375c8ac3c19f07642 Mon Sep 17 00:00:00 2001 From: Sergey Chernyshev Date: Thu, 25 Apr 2024 04:22:11 +0200 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 graal-nodejs/test/fixtures/test-runner/custom_reporters/throwing-async.js create mode 100644 graal-nodejs/test/fixtures/test-runner/custom_reporters/throwing.js diff --git a/graal-nodejs/lib/internal/test_runner/harness.js b/graal-nodejs/lib/internal/test_runner/harness.js index 3ebf4769753..6d11583c42b 100644 --- a/graal-nodejs/lib/internal/test_runner/harness.js +++ b/graal-nodejs/lib/internal/test_runner/harness.js @@ -19,12 +19,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: '' })); } @@ -38,9 +41,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/graal-nodejs/lib/internal/test_runner/utils.js b/graal-nodejs/lib/internal/test_runner/utils.js index 0c685435a1d..326b6aba16e 100644 --- a/graal-nodejs/lib/internal/test_runner/utils.js +++ b/graal-nodejs/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; @@ -420,6 +421,7 @@ module.exports = { isSupportedFileType, isTestFailureError, parseCommandLine, + reporterScope, setupTestReporters, getCoverageReport, }; diff --git a/graal-nodejs/test/fixtures/test-runner/custom_reporters/throwing-async.js b/graal-nodejs/test/fixtures/test-runner/custom_reporters/throwing-async.js new file mode 100644 index 00000000000..b24a632e697 --- /dev/null +++ b/graal-nodejs/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/graal-nodejs/test/fixtures/test-runner/custom_reporters/throwing.js b/graal-nodejs/test/fixtures/test-runner/custom_reporters/throwing.js new file mode 100644 index 00000000000..8d04901c773 --- /dev/null +++ b/graal-nodejs/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/graal-nodejs/test/parallel/test-runner-reporters.js b/graal-nodejs/test/parallel/test-runner-reporters.js index f25e1a18b9d..e5d41f0f21f 100644 --- a/graal-nodejs/test/parallel/test-runner-reporters.js +++ b/graal-nodejs/test/parallel/test-runner-reporters.js @@ -135,4 +135,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/); + }); });