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>
nodejs · Jul 24, 2018 · bea1ee8 · bea1ee8
1 parent 9817e40
commit bea1ee8
Show file tree

Hide file tree

Showing 117 changed files with 58 additions and 241 deletions.
diff --git a/doc/guides/writing-tests.md b/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

diff --git a/test/addons-napi/test_promise/test.js b/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;

diff --git a/test/addons-napi/test_threadsafe_function/test.js b/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.

diff --git a/test/addons/callback-scope/test-resolve-async.js b/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; });
 

diff --git a/test/addons/make-callback-recurse/test.js b/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() {

diff --git a/test/async-hooks/test-promise.chain-promise-before-init-hooks.js b/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);
 }));

diff --git a/test/async-hooks/test-promise.js b/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();

diff --git a/test/common/README.md b/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 [&lt;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
 * [&lt;boolean>]
 

diff --git a/test/common/index.js b/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() {

diff --git a/test/common/index.mjs b/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,

diff --git a/test/common/inspector-helper.js b/test/common/inspector-helper.js
@@ -25,6 +25,7 @@ function spawnChildProcess(inspectorFlags, scriptContents, scriptFile) {
   const handler = tearDown.bind(null, child);
   process.on('exit', handler);
   process.on('uncaughtException', handler);
+  common.disableCrashOnUnhandledRejection();
   process.on('unhandledRejection', handler);
   process.on('SIGINT', handler);
 

diff --git a/test/es-module/test-esm-dynamic-import.js b/test/es-module/test-esm-dynamic-import.js
@@ -5,8 +5,6 @@ const assert = require('assert');
 const { URL } = require('url');
 const vm = require('vm');
 
-common.crashOnUnhandledRejection();
-
 const relativePath = '../fixtures/es-modules/test-esm-ok.mjs';
 const absolutePath = require.resolve('../fixtures/es-modules/test-esm-ok.mjs');
 const targetURL = new URL('file:///');

diff --git a/test/es-module/test-esm-error-cache.js b/test/es-module/test-esm-error-cache.js
@@ -2,11 +2,9 @@
 
 // Flags: --experimental-modules
 
-const common = require('../common');
+require('../common');
 const assert = require('assert');
 
-common.crashOnUnhandledRejection();
-
 const file = '../fixtures/syntax/bad_syntax.js';
 
 let error;

diff --git a/test/es-module/test-esm-loader-missing-dynamic-instantiate-hook.mjs b/test/es-module/test-esm-loader-missing-dynamic-instantiate-hook.mjs
@@ -1,11 +1,6 @@
 // Flags: --experimental-modules --loader ./test/fixtures/es-module-loaders/missing-dynamic-instantiate-hook.mjs
 
-import {
-  crashOnUnhandledRejection,
-  expectsError
-} from '../common';
-
-crashOnUnhandledRejection();
+import { expectsError } from '../common';
 
 import('test').catch(expectsError({
   code: 'ERR_MISSING_DYNAMIC_INSTANTIATE_HOOK',

diff --git a/test/es-module/test-esm-throw-undefined.mjs b/test/es-module/test-esm-throw-undefined.mjs
@@ -1,6 +1,5 @@
 // Flags: --experimental-modules
-/* eslint-disable node-core/required-modules */
-import common from '../common/index.js';
+import '../common';
 import assert from 'assert';
 
 async function doTest() {
@@ -12,5 +11,4 @@ async function doTest() {
   );
 }
 
-common.crashOnUnhandledRejection();
 doTest();
diff --git a/test/internet/test-dns-any.js b/test/internet/test-dns-any.js
@@ -9,8 +9,6 @@ const net = require('net');
 let running = false;
 const queue = [];
 
-common.crashOnUnhandledRejection();
-
 const dnsPromises = dns.promises;
 const isIPv4 = net.isIPv4;
 const isIPv6 = net.isIPv6;

diff --git a/test/internet/test-dns-ipv4.js b/test/internet/test-dns-ipv4.js
@@ -7,8 +7,6 @@ const net = require('net');
 const util = require('util');
 const isIPv4 = net.isIPv4;
 
-common.crashOnUnhandledRejection();
-
 const dnsPromises = dns.promises;
 let running = false;
 const queue = [];

diff --git a/test/internet/test-dns-ipv6.js b/test/internet/test-dns-ipv6.js
@@ -4,8 +4,6 @@ const { addresses } = require('../common/internet');
 if (!common.hasIPv6)
   common.skip('this test, no IPv6 support');
 
-common.crashOnUnhandledRejection();
-
 const assert = require('assert');
 const dns = require('dns');
 const net = require('net');

diff --git a/test/internet/test-dns-promises-resolve.js b/test/internet/test-dns-promises-resolve.js
@@ -4,8 +4,6 @@ const assert = require('assert');
 
 const dnsPromises = require('dns').promises;
 
-common.crashOnUnhandledRejection();
-
 // Error when rrtype is invalid.
 {
   const rrtype = 'DUMMY';

diff --git a/test/internet/test-dns-txt-sigsegv.js b/test/internet/test-dns-txt-sigsegv.js
@@ -1,11 +1,9 @@
 'use strict';
-const common = require('../common');
+require('../common');
 const assert = require('assert');
 const dns = require('dns');
 const dnsPromises = dns.promises;
 
-common.crashOnUnhandledRejection();
-
 (async function() {
   const result = await dnsPromises.resolveTxt('www.microsoft.com');
   assert.strictEqual(result.length, 0);

diff --git a/test/internet/test-dns.js b/test/internet/test-dns.js
@@ -30,8 +30,6 @@ const isIPv6 = net.isIPv6;
 const util = require('util');
 const dnsPromises = dns.promises;
 
-common.crashOnUnhandledRejection();
-
 let expected = 0;
 let completed = 0;
 let running = false;

diff --git a/test/known_issues/test-inspector-cluster-port-clash.js b/test/known_issues/test-inspector-cluster-port-clash.js
@@ -23,8 +23,6 @@ if (process.config.variables.v8_enable_inspector === 0) {
 const cluster = require('cluster');
 const net = require('net');
 
-common.crashOnUnhandledRejection();
-
 const ports = [process.debugPort];
 const clashPort = process.debugPort + 2;
 function serialFork() {

diff --git a/test/message/unhandled_promise_trace_warnings.js b/test/message/unhandled_promise_trace_warnings.js
@@ -1,5 +1,6 @@
 // Flags: --trace-warnings
 'use strict';
-require('../common');
+const common = require('../common');
+common.disableCrashOnUnhandledRejection();
 const p = Promise.reject(new Error('This was rejected'));
 setImmediate(() => p.catch(() => {}));
diff --git a/test/parallel/test-assert-async.js b/test/parallel/test-assert-async.js
@@ -5,8 +5,6 @@ const assert = require('assert');
 // Test assert.rejects() and assert.doesNotReject() by checking their
 // expected output and by verifying that they do not work sync
 
-common.crashOnUnhandledRejection();
-
 // Run all tests in parallel and check their outcome at the end.
 const promises = [];
 

diff --git a/test/parallel/test-async-hooks-disable-during-promise.js b/test/parallel/test-async-hooks-disable-during-promise.js
@@ -1,7 +1,6 @@
 'use strict';
 const common = require('../common');
 const async_hooks = require('async_hooks');
-common.crashOnUnhandledRejection();
 
 if (!common.isMainThread)
   common.skip('Worker bootstrapping works differently -> different AsyncWraps');

diff --git a/test/parallel/test-async-hooks-enable-during-promise.js b/test/parallel/test-async-hooks-enable-during-promise.js
@@ -2,8 +2,6 @@
 const common = require('../common');
 const async_hooks = require('async_hooks');
 
-common.crashOnUnhandledRejection();
-
 Promise.resolve(1).then(common.mustCall(() => {
   async_hooks.createHook({
     init: common.mustCall(),

diff --git a/test/parallel/test-async-hooks-promise-enable-disable.js b/test/parallel/test-async-hooks-promise-enable-disable.js
@@ -8,8 +8,6 @@ let p_resource = null;
 let p_er = null;
 let p_inits = 0;
 
-common.crashOnUnhandledRejection();
-
 // Not useful to place common.mustCall() around 'exit' event b/c it won't be
 // able to check it anyway.
 process.on('exit', (code) => {

diff --git a/test/parallel/test-async-hooks-promise-triggerid.js b/test/parallel/test-async-hooks-promise-triggerid.js
@@ -6,8 +6,6 @@ const async_hooks = require('async_hooks');
 if (!common.isMainThread)
   common.skip('Worker bootstrapping works differently -> different async IDs');
 
-common.crashOnUnhandledRejection();
-
 const promiseAsyncIds = [];
 
 async_hooks.createHook({

diff --git a/test/parallel/test-async-wrap-pop-id-during-load.js b/test/parallel/test-async-wrap-pop-id-during-load.js
@@ -1,8 +1,9 @@
 'use strict';
 
-require('../common');
+const common = require('../common');
 
 if (process.argv[2] === 'async') {
+  common.disableCrashOnUnhandledRejection();
   async function fn() {
     fn();
     throw new Error();

diff --git a/test/parallel/test-async-wrap-promise-after-enabled.js b/test/parallel/test-async-wrap-promise-after-enabled.js
@@ -12,8 +12,6 @@ const async_hooks = require('async_hooks');
 
 const seenEvents = [];
 
-common.crashOnUnhandledRejection();
-
 const p = new Promise((resolve) => resolve(1));
 p.then(() => seenEvents.push('then'));
 

diff --git a/test/parallel/test-c-ares.js b/test/parallel/test-c-ares.js
@@ -23,8 +23,6 @@
 const common = require('../common');
 const assert = require('assert');
 
-common.crashOnUnhandledRejection();
-
 const dns = require('dns');
 const dnsPromises = dns.promises;
 

diff --git a/test/parallel/test-child-process-promisified.js b/test/parallel/test-child-process-promisified.js
@@ -4,8 +4,6 @@ const assert = require('assert');
 const child_process = require('child_process');
 const { promisify } = require('util');
 
-common.crashOnUnhandledRejection();
-
 const exec = promisify(child_process.exec);
 const execFile = promisify(child_process.execFile);
 

diff --git a/test/parallel/test-dns-lookup.js b/test/parallel/test-dns-lookup.js
@@ -5,8 +5,6 @@ const cares = process.binding('cares_wrap');
 const dns = require('dns');
 const dnsPromises = dns.promises;
 
-common.crashOnUnhandledRejection();
-
 // Stub `getaddrinfo` to *always* error.
 cares.getaddrinfo = () => process.binding('uv').UV_ENOENT;
 

diff --git a/test/parallel/test-dns-resolveany-bad-ancount.js b/test/parallel/test-dns-resolveany-bad-ancount.js
@@ -6,8 +6,6 @@ const assert = require('assert');
 const dgram = require('dgram');
 const dnsPromises = dns.promises;
 
-common.crashOnUnhandledRejection();
-
 const server = dgram.createSocket('udp4');
 
 server.on('message', common.mustCall((msg, { address, port }) => {

diff --git a/test/parallel/test-dns-resolveany.js b/test/parallel/test-dns-resolveany.js
@@ -6,8 +6,6 @@ const assert = require('assert');
 const dgram = require('dgram');
 const dnsPromises = dns.promises;
 
-common.crashOnUnhandledRejection();
-
 const answers = [
   { type: 'A', address: '1.2.3.4', ttl: 123 },
   { type: 'AAAA', address: '::42', ttl: 123 },