Skip to content

Commit

Permalink
test_runner: better handle async bootstrap errors
Browse files Browse the repository at this point in the history
The test runner is bootstrapped synchronously, with the exception
of importing custom reporters. To better handle asynchronously
throw errors, this commit introduces an internal error type that
can be checked for from the test runner's uncaughtException
handler.

After #46707 and this change
land, the other throw statement in the uncaughtException handler
can be removed. This will allow the test runner to handle errors
thrown from outside of tests (which currently prevents the test
runner from reporting results).

PR-URL: #46720
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
  • Loading branch information
cjihrig authored and targos committed Mar 13, 2023
1 parent 93e91f3 commit dfe529b
Show file tree
Hide file tree
Showing 4 changed files with 34 additions and 7 deletions.
4 changes: 2 additions & 2 deletions lib/internal/errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -1604,8 +1604,8 @@ E('ERR_TAP_VALIDATION_ERROR', function(errorMsg) {
}, Error);
E('ERR_TEST_FAILURE', function(error, failureType) {
hideInternalStackFrames(this);
assert(typeof failureType === 'string',
"The 'failureType' argument must be of type string.");
assert(typeof failureType === 'string' || typeof failureType === 'symbol',
"The 'failureType' argument must be of type string or symbol.");

let msg = error?.message ?? error;

Expand Down
8 changes: 8 additions & 0 deletions lib/internal/test_runner/harness.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ const { exitCodes: { kGenericUserError } } = internalBinding('errors');
const { kEmptyObject } = require('internal/util');
const { kCancelledByParent, Test, ItTest, Suite } = require('internal/test_runner/test');
const {
kAsyncBootstrapFailure,
parseCommandLine,
setupTestReporters,
} = require('internal/test_runner/utils');
Expand All @@ -32,6 +33,13 @@ function createTestTree(options = kEmptyObject) {

function createProcessEventHandler(eventName, rootTest) {
return (err) => {
if (err?.failureType === kAsyncBootstrapFailure) {
// Something went wrong during the asynchronous portion of bootstrapping
// the test runner. Since the test runner is not setup properly, we can't
// do anything but throw the error.
throw err.cause;
}

// Check if this error is coming from a test. If it is, fail the test.
const test = testResources.get(executionAsyncId());

Expand Down
17 changes: 12 additions & 5 deletions lib/internal/test_runner/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ const {
RegExp,
RegExpPrototypeExec,
SafeMap,
Symbol,
} = primordials;
const { basename } = require('path');
const { createWriteStream } = require('fs');
Expand All @@ -23,6 +24,7 @@ const {
} = require('internal/errors');
const { compose } = require('stream');

const kAsyncBootstrapFailure = Symbol('asyncBootstrapFailure');
const kMultipleCallbackInvocations = 'multipleCallbackInvocations';
const kRegExpPattern = /^\/(.*)\/([a-z]*)$/;
const kSupportedFileExtensions = /\.[cm]?js$/;
Expand Down Expand Up @@ -149,11 +151,15 @@ async function getReportersMap(reporters, destinations) {


async function setupTestReporters(testsStream) {
const { reporters, destinations } = parseCommandLine();
const reportersMap = await getReportersMap(reporters, destinations);
for (let i = 0; i < reportersMap.length; i++) {
const { reporter, destination } = reportersMap[i];
compose(testsStream, reporter).pipe(destination);
try {
const { reporters, destinations } = parseCommandLine();
const reportersMap = await getReportersMap(reporters, destinations);
for (let i = 0; i < reportersMap.length; i++) {
const { reporter, destination } = reportersMap[i];
compose(testsStream, reporter).pipe(destination);
}
} catch (err) {
throw new ERR_TEST_FAILURE(err, kAsyncBootstrapFailure);
}
}

Expand Down Expand Up @@ -219,6 +225,7 @@ module.exports = {
doesPathMatchFilter,
isSupportedFileType,
isTestFailureError,
kAsyncBootstrapFailure,
parseCommandLine,
setupTestReporters,
};
12 changes: 12 additions & 0 deletions test/parallel/test-runner-reporters.js
Original file line number Diff line number Diff line change
Expand Up @@ -114,4 +114,16 @@ describe('node:test reporters', { concurrency: true }, () => {
/^package: reporter-esm{"test:start":5,"test:pass":2,"test:fail":3,"test:plan":3,"test:diagnostic":\d+}$/,
);
});

it('should throw when reporter setup throws asynchronously', async () => {
const child = spawnSync(
process.execPath,
['--test', '--test-reporter', fixtures.fileURL('empty.js'), 'reporters.js'],
{ cwd: fixtures.path('test-runner') }
);
assert.strictEqual(child.status, 7);
assert.strictEqual(child.signal, null);
assert.strictEqual(child.stdout.toString(), '');
assert.match(child.stderr.toString(), /ERR_INVALID_ARG_TYPE/);
});
});

0 comments on commit dfe529b

Please sign in to comment.