Skip to content

Commit

Permalink
test_runner: catch reporter errors
Browse files Browse the repository at this point in the history
PR-URL: nodejs#49646
Fixes: nodejs#48937
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
  • Loading branch information
MoLow committed Sep 28, 2023
1 parent be211ef commit 0e27a7a
Show file tree
Hide file tree
Showing 5 changed files with 49 additions and 4 deletions.
10 changes: 9 additions & 1 deletion lib/internal/test_runner/harness.js
Original file line number Diff line number Diff line change
Expand Up @@ -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: '<root>' }));
}
Expand All @@ -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.
Expand Down
8 changes: 5 additions & 3 deletions lib/internal/test_runner/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -424,6 +425,7 @@ module.exports = {
isSupportedFileType,
isTestFailureError,
parseCommandLine,
reporterScope,
setupTestReporters,
getCoverageReport,
};
8 changes: 8 additions & 0 deletions test/fixtures/test-runner/custom_reporters/throwing-async.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
'use strict';

module.exports = async function * customReporter() {
yield 'Going to throw an error\n';
setImmediate(() => {
throw new Error('Reporting error');
});
};
6 changes: 6 additions & 0 deletions test/fixtures/test-runner/custom_reporters/throwing.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
'use strict';

module.exports = async function * customReporter() {
yield 'Going to throw an error\n';
throw new Error('Reporting error');
};
21 changes: 21 additions & 0 deletions test/parallel/test-runner-reporters.js
Original file line number Diff line number Diff line change
Expand Up @@ -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/);
});
});

0 comments on commit 0e27a7a

Please sign in to comment.