From b191f09e6ee7d40798454e8545d83e53cab0e63c Mon Sep 17 00:00:00 2001 From: cjihrig Date: Sun, 5 Mar 2023 12:38:32 -0500 Subject: [PATCH 1/4] test_runner: give the root test a harness reference This commit replaces the 'coverage' reference inside of the Test class with a more generic harness reference which includes coverage. This will let the root test more easily track process level state such as code coverage, uncaughtException handlers, and the state of bootstrapping. PR-URL: https://github.com/nodejs/node/pull/46962 Reviewed-By: Moshe Atlow Reviewed-By: Yagiz Nizipli --- lib/internal/test_runner/harness.js | 7 ++++++- lib/internal/test_runner/test.js | 6 +++--- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/lib/internal/test_runner/harness.js b/lib/internal/test_runner/harness.js index a5cec210df2f..42cc5613f2a2 100644 --- a/lib/internal/test_runner/harness.js +++ b/lib/internal/test_runner/harness.js @@ -139,7 +139,7 @@ function setup(root) { createProcessEventHandler('unhandledRejection', root); const coverage = configureCoverage(root, globalOptions); const exitHandler = () => { - root.coverage = collectCoverage(root, coverage); + root.harness.coverage = collectCoverage(root, coverage); root.postRun(new ERR_TEST_FAILURE( 'Promise resolution is still pending but the event loop has already resolved', kCancelledByParent)); @@ -163,6 +163,11 @@ function setup(root) { process.on('SIGTERM', terminationHandler); } + root.harness = { + __proto__: null, + bootstrapComplete: false, + coverage: null, + }; root.startTime = hrtime(); wasRootSetup.add(root); diff --git a/lib/internal/test_runner/test.js b/lib/internal/test_runner/test.js index b165808686e0..ffbf2d257aed 100644 --- a/lib/internal/test_runner/test.js +++ b/lib/internal/test_runner/test.js @@ -252,7 +252,7 @@ class Test extends AsyncResource { this.#outerSignal?.addEventListener('abort', this.#abortHandler); this.fn = fn; - this.coverage = null; // Configured on the root test by the test harness. + this.harness = null; // Configured on the root test by the test harness. this.mock = null; this.name = name; this.parent = parent; @@ -651,8 +651,8 @@ class Test extends AsyncResource { this.reporter.diagnostic(this.nesting, kFilename, `todo ${counters.todo}`); this.reporter.diagnostic(this.nesting, kFilename, `duration_ms ${this.#duration()}`); - if (this.coverage) { - this.reporter.coverage(this.nesting, kFilename, this.coverage); + if (this.harness?.coverage) { + this.reporter.coverage(this.nesting, kFilename, this.harness.coverage); } this.reporter.push(null); From 4e3bc06e6547c1d640111b73e80e198ba03f7e45 Mon Sep 17 00:00:00 2001 From: cjihrig Date: Sun, 5 Mar 2023 12:46:14 -0500 Subject: [PATCH 2/4] test_runner: track bootstrapping process This commit updates the test harness and root test to track when bootstrapping has completed. PR-URL: https://github.com/nodejs/node/pull/46962 Reviewed-By: Moshe Atlow Reviewed-By: Yagiz Nizipli --- lib/internal/test_runner/harness.js | 3 ++- lib/internal/test_runner/runner.js | 15 +++++++++------ lib/internal/test_runner/utils.js | 4 ++-- 3 files changed, 13 insertions(+), 9 deletions(-) diff --git a/lib/internal/test_runner/harness.js b/lib/internal/test_runner/harness.js index 42cc5613f2a2..e30d6242da91 100644 --- a/lib/internal/test_runner/harness.js +++ b/lib/internal/test_runner/harness.js @@ -182,13 +182,14 @@ function getGlobalRoot() { globalRoot.reporter.once('test:fail', () => { process.exitCode = kGenericUserError; }); - reportersSetup = setupTestReporters(globalRoot.reporter); + reportersSetup = setupTestReporters(globalRoot); } return globalRoot; } async function startSubtest(subtest) { await reportersSetup; + getGlobalRoot().harness.bootstrapComplete = true; await subtest.start(); } diff --git a/lib/internal/test_runner/runner.js b/lib/internal/test_runner/runner.js index 5108fc3283f6..5b772251be98 100644 --- a/lib/internal/test_runner/runner.js +++ b/lib/internal/test_runner/runner.js @@ -387,13 +387,16 @@ function run(options) { filesWatcher = watchFiles(testFiles, root, inspectPort); postRun = undefined; } - const runFiles = () => SafePromiseAllSettledReturnVoid(testFiles, (path) => { - const subtest = runTestFile(path, root, inspectPort, filesWatcher); - runningSubtests.set(path, subtest); - return subtest; - }); + const runFiles = () => { + root.harness.bootstrapComplete = true; + return SafePromiseAllSettledReturnVoid(testFiles, (path) => { + const subtest = runTestFile(path, root, inspectPort, filesWatcher); + runningSubtests.set(path, subtest); + return subtest; + }); + }; - PromisePrototypeThen(PromisePrototypeThen(PromiseResolve(setup?.(root.reporter)), runFiles), postRun); + PromisePrototypeThen(PromisePrototypeThen(PromiseResolve(setup?.(root)), runFiles), postRun); return root.reporter; } diff --git a/lib/internal/test_runner/utils.js b/lib/internal/test_runner/utils.js index 72270e464248..e4fd23c8e8cb 100644 --- a/lib/internal/test_runner/utils.js +++ b/lib/internal/test_runner/utils.js @@ -150,13 +150,13 @@ async function getReportersMap(reporters, destinations) { } -async function setupTestReporters(testsStream) { +async function setupTestReporters(rootTest) { 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); + compose(rootTest.reporter, reporter).pipe(destination); } } catch (err) { throw new ERR_TEST_FAILURE(err, kAsyncBootstrapFailure); From e079aa80f05be11f4674f3caeb5d63c298126a6e Mon Sep 17 00:00:00 2001 From: cjihrig Date: Sun, 5 Mar 2023 12:49:25 -0500 Subject: [PATCH 3/4] test_runner: throw if harness is not bootstrapped This commit updates the test harness to re-throw uncaught errors if bootstrapping has not completed. This updates the existing logic which tried to detect a specific error code. PR-URL: https://github.com/nodejs/node/pull/46962 Reviewed-By: Moshe Atlow Reviewed-By: Yagiz Nizipli --- lib/internal/test_runner/harness.js | 5 ++--- lib/internal/test_runner/utils.js | 17 +++++------------ 2 files changed, 7 insertions(+), 15 deletions(-) diff --git a/lib/internal/test_runner/harness.js b/lib/internal/test_runner/harness.js index e30d6242da91..4bf61379e7af 100644 --- a/lib/internal/test_runner/harness.js +++ b/lib/internal/test_runner/harness.js @@ -19,7 +19,6 @@ const { exitCodes: { kGenericUserError } } = internalBinding('errors'); const { kEmptyObject } = require('internal/util'); const { kCancelledByParent, Test, Suite } = require('internal/test_runner/test'); const { - kAsyncBootstrapFailure, parseCommandLine, setupTestReporters, } = require('internal/test_runner/utils'); @@ -34,11 +33,11 @@ function createTestTree(options = kEmptyObject) { function createProcessEventHandler(eventName, rootTest) { return (err) => { - if (err?.failureType === kAsyncBootstrapFailure) { + if (!rootTest.harness.bootstrapComplete) { // 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; + throw err; } // Check if this error is coming from a test. If it is, fail the test. diff --git a/lib/internal/test_runner/utils.js b/lib/internal/test_runner/utils.js index e4fd23c8e8cb..6d5e474a59c4 100644 --- a/lib/internal/test_runner/utils.js +++ b/lib/internal/test_runner/utils.js @@ -7,7 +7,6 @@ const { RegExp, RegExpPrototypeExec, SafeMap, - Symbol, } = primordials; const { basename } = require('path'); const { createWriteStream } = require('fs'); @@ -24,7 +23,6 @@ const { } = require('internal/errors'); const { compose } = require('stream'); -const kAsyncBootstrapFailure = Symbol('asyncBootstrapFailure'); const kMultipleCallbackInvocations = 'multipleCallbackInvocations'; const kRegExpPattern = /^\/(.*)\/([a-z]*)$/; const kSupportedFileExtensions = /\.[cm]?js$/; @@ -151,15 +149,11 @@ async function getReportersMap(reporters, destinations) { async function setupTestReporters(rootTest) { - 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(rootTest.reporter, reporter).pipe(destination); - } - } catch (err) { - throw new ERR_TEST_FAILURE(err, kAsyncBootstrapFailure); + const { reporters, destinations } = parseCommandLine(); + const reportersMap = await getReportersMap(reporters, destinations); + for (let i = 0; i < reportersMap.length; i++) { + const { reporter, destination } = reportersMap[i]; + compose(rootTest.reporter, reporter).pipe(destination); } } @@ -225,7 +219,6 @@ module.exports = { doesPathMatchFilter, isSupportedFileType, isTestFailureError, - kAsyncBootstrapFailure, parseCommandLine, setupTestReporters, }; From 3d63d5333ddf6b08e123e1ebdace1a5600f24c80 Mon Sep 17 00:00:00 2001 From: cjihrig Date: Thu, 16 Feb 2023 17:58:37 -0500 Subject: [PATCH 4/4] test_runner: handle errors not bound to tests This commit addresses a previously untested branch of the code. It is possible when using the test runner that an error occurs outside of a test. In this case, the test runner would simply rethrow the error. This commit updates the logic to handle the error in the same fashion as other uncaughtExceptions. PR-URL: https://github.com/nodejs/node/pull/46962 Reviewed-By: Moshe Atlow Reviewed-By: Yagiz Nizipli --- lib/internal/test_runner/harness.js | 26 +++++++++++-------- test/message/test_runner_output.js | 6 +++++ test/message/test_runner_output.out | 1 + test/message/test_runner_output_cli.out | 1 + .../test_runner_output_spec_reporter.out | 1 + 5 files changed, 24 insertions(+), 11 deletions(-) diff --git a/lib/internal/test_runner/harness.js b/lib/internal/test_runner/harness.js index 4bf61379e7af..168310ff3d93 100644 --- a/lib/internal/test_runner/harness.js +++ b/lib/internal/test_runner/harness.js @@ -43,17 +43,21 @@ function createProcessEventHandler(eventName, rootTest) { // Check if this error is coming from a test. If it is, fail the test. const test = testResources.get(executionAsyncId()); - if (!test) { - throw err; - } - - if (test.finished) { - // If the test is already finished, report this as a top level - // diagnostic since this is a malformed test. - const msg = `Warning: Test "${test.name}" generated asynchronous ` + - 'activity after the test ended. This activity created the error ' + - `"${err}" and would have caused the test to fail, but instead ` + - `triggered an ${eventName} event.`; + 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. + let msg; + + if (test) { + msg = `Warning: Test "${test.name}" generated asynchronous ` + + 'activity after the test ended. This activity created the error ' + + `"${err}" and would have caused the test to fail, but instead ` + + `triggered an ${eventName} event.`; + } else { + msg = 'Warning: A resource generated asynchronous activity after ' + + `the test ended. This activity created the error "${err}" which ` + + `triggered an ${eventName} event, caught by the test runner.`; + } rootTest.diagnostic(msg); process.exitCode = kGenericUserError; diff --git a/test/message/test_runner_output.js b/test/message/test_runner_output.js index 593f069d8f46..a53ded3dfde3 100644 --- a/test/message/test_runner_output.js +++ b/test/message/test_runner_output.js @@ -383,3 +383,9 @@ test('unfinished test with unhandledRejection', async () => { setTimeout(() => Promise.reject(new Error('bar'))); }); }); + +// Verify that uncaught exceptions outside of any tests are handled after the +// test harness has finished bootstrapping itself. +setImmediate(() => { + throw new Error('uncaught from outside of a test'); +}); diff --git a/test/message/test_runner_output.out b/test/message/test_runner_output.out index 2609833304e2..b6f254708010 100644 --- a/test/message/test_runner_output.out +++ b/test/message/test_runner_output.out @@ -637,6 +637,7 @@ not ok 65 - invalid subtest fail 1..65 # Warning: Test "unhandled rejection - passes but warns" generated asynchronous activity after the test ended. This activity created the error "Error: rejected from unhandled rejection fail" and would have caused the test to fail, but instead triggered an unhandledRejection event. # Warning: Test "async unhandled rejection - passes but warns" generated asynchronous activity after the test ended. This activity created the error "Error: rejected from async unhandled rejection fail" and would have caused the test to fail, but instead triggered an unhandledRejection event. +# Warning: A resource generated asynchronous activity after the test ended. This activity created the error "Error: uncaught from outside of a test" which triggered an uncaughtException event, caught by the test runner. # Warning: Test "immediate throw - passes but warns" generated asynchronous activity after the test ended. This activity created the error "Error: thrown from immediate throw fail" and would have caused the test to fail, but instead triggered an uncaughtException event. # Warning: Test "immediate reject - passes but warns" generated asynchronous activity after the test ended. This activity created the error "Error: rejected from immediate reject fail" and would have caused the test to fail, but instead triggered an unhandledRejection event. # Warning: Test "callback called twice in different ticks" generated asynchronous activity after the test ended. This activity created the error "Error [ERR_TEST_FAILURE]: callback invoked multiple times" and would have caused the test to fail, but instead triggered an uncaughtException event. diff --git a/test/message/test_runner_output_cli.out b/test/message/test_runner_output_cli.out index 72957397c054..3baeb534704b 100644 --- a/test/message/test_runner_output_cli.out +++ b/test/message/test_runner_output_cli.out @@ -636,6 +636,7 @@ not ok 65 - invalid subtest fail ... # Warning: Test "unhandled rejection - passes but warns" generated asynchronous activity after the test ended. This activity created the error "Error: rejected from unhandled rejection fail" and would have caused the test to fail, but instead triggered an unhandledRejection event. # Warning: Test "async unhandled rejection - passes but warns" generated asynchronous activity after the test ended. This activity created the error "Error: rejected from async unhandled rejection fail" and would have caused the test to fail, but instead triggered an unhandledRejection event. +# Warning: A resource generated asynchronous activity after the test ended. This activity created the error "Error: uncaught from outside of a test" which triggered an uncaughtException event, caught by the test runner. # Warning: Test "immediate throw - passes but warns" generated asynchronous activity after the test ended. This activity created the error "Error: thrown from immediate throw fail" and would have caused the test to fail, but instead triggered an uncaughtException event. # Warning: Test "immediate reject - passes but warns" generated asynchronous activity after the test ended. This activity created the error "Error: rejected from immediate reject fail" and would have caused the test to fail, but instead triggered an unhandledRejection event. # Warning: Test "callback called twice in different ticks" generated asynchronous activity after the test ended. This activity created the error "Error [ERR_TEST_FAILURE]: callback invoked multiple times" and would have caused the test to fail, but instead triggered an uncaughtException event. diff --git a/test/message/test_runner_output_spec_reporter.out b/test/message/test_runner_output_spec_reporter.out index 591dc9a76d64..3ff9aefe7a42 100644 --- a/test/message/test_runner_output_spec_reporter.out +++ b/test/message/test_runner_output_spec_reporter.out @@ -270,6 +270,7 @@ Warning: Test "unhandled rejection - passes but warns" generated asynchronous activity after the test ended. This activity created the error "Error: rejected from unhandled rejection fail" and would have caused the test to fail, but instead triggered an unhandledRejection event. Warning: Test "async unhandled rejection - passes but warns" generated asynchronous activity after the test ended. This activity created the error "Error: rejected from async unhandled rejection fail" and would have caused the test to fail, but instead triggered an unhandledRejection event. + Warning: A resource generated asynchronous activity after the test ended. This activity created the error "Error: uncaught from outside of a test" which triggered an uncaughtException event, caught by the test runner. Warning: Test "immediate throw - passes but warns" generated asynchronous activity after the test ended. This activity created the error "Error: thrown from immediate throw fail" and would have caused the test to fail, but instead triggered an uncaughtException event. Warning: Test "immediate reject - passes but warns" generated asynchronous activity after the test ended. This activity created the error "Error: rejected from immediate reject fail" and would have caused the test to fail, but instead triggered an unhandledRejection event. Warning: Test "callback called twice in different ticks" generated asynchronous activity after the test ended. This activity created the error "Error [ERR_TEST_FAILURE]: callback invoked multiple times" and would have caused the test to fail, but instead triggered an uncaughtException event.