From 0c90be9072116d339083678dca8164548425c5f2 Mon Sep 17 00:00:00 2001
From: Colin Ihrig <cjihrig@gmail.com>
Date: Fri, 24 Feb 2023 09:13:24 -0500
Subject: [PATCH] test_runner: better handle async bootstrap errors

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 https://github.com/nodejs/node/pull/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: https://github.com/nodejs/node/pull/46720
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
---
 lib/internal/errors.js                 |  4 ++--
 lib/internal/test_runner/harness.js    |  8 ++++++++
 lib/internal/test_runner/utils.js      | 17 ++++++++++++-----
 test/parallel/test-runner-reporters.js | 12 ++++++++++++
 4 files changed, 34 insertions(+), 7 deletions(-)

diff --git a/lib/internal/errors.js b/lib/internal/errors.js
index 4f833741cf999f..bd68c9bd554582 100644
--- a/lib/internal/errors.js
+++ b/lib/internal/errors.js
@@ -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;
 
diff --git a/lib/internal/test_runner/harness.js b/lib/internal/test_runner/harness.js
index e88faf1e1694dc..2ee09c66b3e3c3 100644
--- a/lib/internal/test_runner/harness.js
+++ b/lib/internal/test_runner/harness.js
@@ -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');
@@ -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());
 
diff --git a/lib/internal/test_runner/utils.js b/lib/internal/test_runner/utils.js
index ba7dbe57ba1784..72270e464248a5 100644
--- a/lib/internal/test_runner/utils.js
+++ b/lib/internal/test_runner/utils.js
@@ -7,6 +7,7 @@ const {
   RegExp,
   RegExpPrototypeExec,
   SafeMap,
+  Symbol,
 } = primordials;
 const { basename } = require('path');
 const { createWriteStream } = require('fs');
@@ -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$/;
@@ -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);
   }
 }
 
@@ -219,6 +225,7 @@ module.exports = {
   doesPathMatchFilter,
   isSupportedFileType,
   isTestFailureError,
+  kAsyncBootstrapFailure,
   parseCommandLine,
   setupTestReporters,
 };
diff --git a/test/parallel/test-runner-reporters.js b/test/parallel/test-runner-reporters.js
index 5961fd8752d176..81074abc9fc838 100644
--- a/test/parallel/test-runner-reporters.js
+++ b/test/parallel/test-runner-reporters.js
@@ -116,4 +116,16 @@ describe('node:test reporters', { concurrency: true }, () => {
       /^package: reporter-esm{"test:start":4,"test:pass":2,"test:fail":2,"test:plan":2,"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/);
+  });
 });