test: make crashOnUnhandleRejection opt-out

This commit removes `common.crashOnUnhandledRejection()` and adds
`common.disableCrashOnUnhandledRejection()`.

To reduce the risk of mistakes and make writing tests that involve
promises simpler, always install the unhandledRejection hook in tests
and provide a way to disable it for the rare cases where it's needed.

PR-URL: #21849
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

Author: targos

doc/guides/writing-tests.md

@@ -225,34 +225,24 @@ countdown.dec(); // The countdown callback will be invoked now.
 
 #### Testing promises
 
-When writing tests involving promises, either make sure that the
-`onFulfilled` or the `onRejected` handler is wrapped in
-`common.mustCall()` or `common.mustNotCall()` accordingly, or
-call `common.crashOnUnhandledRejection()` in the top level of the
-test to make sure that unhandled rejections would result in a test
-failure. For example:
+When writing tests involving promises, it is generally good to wrap the
+`onFulfilled` handler, otherwise the test could successfully finish if the
+promise never resolves (pending promises do not keep the event loop alive). The
+`common` module automatically adds a handler that makes the process crash - and
+hence, the test fail - in the case of an `unhandledRejection` event. It is
+possible to disable it with `common.disableCrashOnUnhandledRejection()` if
+needed.
 
 ```javascript
 const common = require('../common');
 const assert = require('assert');
 const fs = require('fs').promises;
 
-// Use `common.crashOnUnhandledRejection()` to make sure unhandled rejections
-// will fail the test.
-common.crashOnUnhandledRejection();
-
-// Or, wrap the `onRejected` handler in `common.mustNotCall()`.
-fs.writeFile('test-file', 'test').catch(common.mustNotCall());
-
-// Or, wrap the `onFulfilled` handler in `common.mustCall()`.
-// If there are assertions in the `onFulfilled` handler, wrap
-// the next `onRejected` handler in `common.mustNotCall()`
-// to handle potential failures.
+// Wrap the `onFulfilled` handler in `common.mustCall()`.
 fs.readFile('test-file').then(
   common.mustCall(
     (content) => assert.strictEqual(content.toString(), 'test2')
-  ))
-  .catch(common.mustNotCall());
+  ));
 ```
 
 ### Flags

test/addons-napi/test_promise/test.js

@@ -7,8 +7,6 @@ const common = require('../../common');
 const assert = require('assert');
 const test_promise = require(`./build/${common.buildType}/test_promise`);
 
-common.crashOnUnhandledRejection();
-
 // A resolution
 {
   const expected_result = 42;

test/addons-napi/test_threadsafe_function/test.js

@@ -12,8 +12,6 @@ const expectedArray = (function(arrayLength) {
   return result;
 })(binding.ARRAY_LENGTH);
 
-common.crashOnUnhandledRejection();
-
 // Handle the rapid teardown test case as the child process. We unref the
 // thread-safe function after we have received two values. This causes the
 // process to exit and the environment cleanup handler to be invoked.

test/addons/callback-scope/test-resolve-async.js

@@ -4,8 +4,6 @@ const common = require('../../common');
 const assert = require('assert');
 const { testResolveAsync } = require(`./build/${common.buildType}/binding`);
 
-common.crashOnUnhandledRejection();
-
 let called = false;
 testResolveAsync().then(() => { called = true; });
 

test/addons/make-callback-recurse/test.js

@@ -9,8 +9,6 @@ const makeCallback = binding.makeCallback;
 // Make sure this is run in the future.
 const mustCallCheckDomains = common.mustCall(checkDomains);
 
-common.crashOnUnhandledRejection();
-
 // Make sure that using MakeCallback allows the error to propagate.
 assert.throws(function() {
   makeCallback({}, function() {

test/async-hooks/test-promise.chain-promise-before-init-hooks.js

@@ -8,8 +8,6 @@ const { checkInvocations } = require('./hook-checks');
 if (!common.isMainThread)
   common.skip('Worker bootstrapping works differently -> different async IDs');
 
-common.crashOnUnhandledRejection();
-
 const p = new Promise(common.mustCall(function executor(resolve, reject) {
   resolve(5);
 }));

test/async-hooks/test-promise.js

@@ -9,8 +9,6 @@ const { checkInvocations } = require('./hook-checks');
 if (!common.isMainThread)
   common.skip('Worker bootstrapping works differently -> different async IDs');
 
-common.crashOnUnhandledRejection();
-
 const hooks = initHooks();
 
 hooks.enable();

test/common/README.md

@@ -55,18 +55,19 @@ symlinks
 ([SeCreateSymbolicLinkPrivilege](https://msdn.microsoft.com/en-us/library/windows/desktop/bb530716(v=vs.85).aspx)).
 On non-Windows platforms, this always returns `true`.
 
-### crashOnUnhandledRejection()
-
-Installs a `process.on('unhandledRejection')` handler that crashes the process
-after a tick. This is useful for tests that use Promises and need to make sure
-no unexpected rejections occur, because currently they result in silent
-failures.
-
 ### ddCommand(filename, kilobytes)
 * return [<Object>]
 
 Platform normalizes the `dd` command
 
+### disableCrashOnUnhandledRejection()
+
+Removes the `process.on('unhandledRejection')` handler that crashes the process
+after a tick. The handler is useful for tests that use Promises and need to make
+sure no unexpected rejections occur, because currently they result in silent
+failures. However, it is useful in some rare cases to disable it, for example if
+the `unhandledRejection` hook is directly used by the test.
+
 ### enoughTestMem
 * [<boolean>]
 

test/common/index.js

@@ -815,9 +815,10 @@ exports.getBufferSources = function getBufferSources(buf) {
 };
 
 // Crash the process on unhandled rejections.
-exports.crashOnUnhandledRejection = function() {
-  process.on('unhandledRejection',
-             (err) => process.nextTick(() => { throw err; }));
+const crashOnUnhandledRejection = (err) => { throw err; };
+process.on('unhandledRejection', crashOnUnhandledRejection);
+exports.disableCrashOnUnhandledRejection = function() {
+  process.removeListener('unhandledRejection', crashOnUnhandledRejection);
 };
 
 exports.getTTYfd = function getTTYfd() {

test/common/index.mjs

@@ -52,7 +52,7 @@ const {
   skipIf32Bits,
   getArrayBufferViews,
   getBufferSources,
-  crashOnUnhandledRejection,
+  disableCrashOnUnhandledRejection,
   getTTYfd,
   runWithInvalidFD,
   hijackStdout,
@@ -112,7 +112,7 @@ export {
   skipIf32Bits,
   getArrayBufferViews,
   getBufferSources,
-  crashOnUnhandledRejection,
+  disableCrashOnUnhandledRejection,
   getTTYfd,
   runWithInvalidFD,
   hijackStdout,