assert: improve ifError

doc/api/assert.md

@@ -433,25 +433,32 @@ suppressFrame();
 ## assert.ifError(value)
 <!-- YAML
 added: v0.1.97
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/18247
+    description: Instead of throwing the original error it is now wrapped into
+                 a AssertionError that contains the full stack trace.
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/18247
+    description: Value may now only be `undefined` or `null`. Before any truthy
+                 input was accepted.
 -->
 * `value` {any}
 
-Throws `value` if `value` is truthy. This is useful when testing the `error`
-argument in callbacks.
+Throws `value` if `value` is not `undefined` or `null`. This is useful when
+testing the `error` argument in callbacks.
 
 ```js
 const assert = require('assert').strict;
 
 assert.ifError(null);
 // OK
 assert.ifError(0);
-// OK
-assert.ifError(1);
-// Throws 1
+// AssertionError [ERR_ASSERTION]: ifError got unwanted exception: 0
 assert.ifError('error');
-// Throws 'error'
+// AssertionError [ERR_ASSERTION]: ifError got unwanted exception: 'error'
 assert.ifError(new Error());
-// Throws Error
+// AssertionError [ERR_ASSERTION]: ifError got unwanted exception: Error
 ```
 
 ## assert.notDeepEqual(actual, expected[, message])

lib/assert.js

@@ -25,7 +25,7 @@ const {
   isDeepEqual,
   isDeepStrictEqual
 } = require('internal/util/comparisons');
-const errors = require('internal/errors');
+const { AssertionError, TypeError } = require('internal/errors');
 const { openSync, closeSync, readSync } = require('fs');
 const { parseExpressionAt } = require('internal/deps/acorn/dist/acorn');
 const { inspect } = require('util');
@@ -65,7 +65,7 @@ const NO_EXCEPTION_SENTINEL = {};
 function innerFail(obj) {
   if (obj.message instanceof Error) throw obj.message;
 
-  throw new errors.AssertionError(obj);
+  throw new AssertionError(obj);
 }
 
 function fail(actual, expected, message, operator, stackStartFn) {
@@ -95,7 +95,7 @@ assert.fail = fail;
 // new assert.AssertionError({ message: message,
 //                             actual: actual,
 //                             expected: expected });
-assert.AssertionError = errors.AssertionError;
+assert.AssertionError = AssertionError;
 
 function getBuffer(fd, assertLine) {
   var lines = 0;
@@ -134,7 +134,7 @@ function innerOk(args, fn) {
   var [value, message] = args;
 
   if (args.length === 0)
-    throw new errors.TypeError('ERR_MISSING_ARGS', 'value');
+    throw new TypeError('ERR_MISSING_ARGS', 'value');
 
   if (!value) {
     if (message == null) {
@@ -338,8 +338,8 @@ function expectedException(actual, expected, msg) {
       return expected.test(actual);
     // assert.doesNotThrow does not accept objects.
     if (arguments.length === 2) {
-      throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'expected',
-                                 ['Function', 'RegExp'], expected);
+      throw new TypeError('ERR_INVALID_ARG_TYPE', 'expected',
+                          ['Function', 'RegExp'], expected);
     }
     // The name and message could be non enumerable. Therefore test them
     // explicitly.
@@ -376,8 +376,8 @@ function expectedException(actual, expected, msg) {
 
 function getActual(block) {
   if (typeof block !== 'function') {
-    throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'block', 'Function',
-                               block);
+    throw new TypeError('ERR_INVALID_ARG_TYPE', 'block', 'Function',
+                        block);
   }
   try {
     block();
@@ -393,10 +393,10 @@ assert.throws = function throws(block, error, message) {
 
   if (typeof error === 'string') {
     if (arguments.length === 3)
-      throw new errors.TypeError('ERR_INVALID_ARG_TYPE',
-                                 'error',
-                                 ['Function', 'RegExp'],
-                                 error);
+      throw new TypeError('ERR_INVALID_ARG_TYPE',
+                          'error',
+                          ['Function', 'RegExp'],
+                          error);
 
     message = error;
     error = null;
@@ -444,7 +444,53 @@ assert.doesNotThrow = function doesNotThrow(block, error, message) {
   throw actual;
 };
 
-assert.ifError = function ifError(err) { if (err) throw err; };
+assert.ifError = function ifError(err) {
+  if (err !== null && err !== undefined) {
+    let message = 'ifError got unwanted exception: ';
+    if (typeof err === 'object' && typeof err.message === 'string') {
+      if (err.message.length === 0 && err.constructor) {
+        message += err.constructor.name;
+      } else {
+        message += err.message;
+      }
+    } else {
+      message += inspect(err);
+    }
+
+    const newErr = new AssertionError({
+      actual: err,
+      expected: null,
+      operator: 'ifError',
+      message,
+      stackStartFn: ifError
+    });
+
+    // Make sure we actually have a stack trace!
+    const origStack = err.stack;
+
+    if (typeof origStack === 'string') {
+      // This will remove any duplicated frames from the error frames taken
+      // from within `ifError` and add the original error frames to the newly
+      // created ones.
+      const tmp2 = origStack.split('\n');
+      tmp2.shift();
+      // Filter all frames existing in err.stack.
+      let tmp1 = newErr.stack.split('\n');
+      for (var i = 0; i < tmp2.length; i++) {
+        // Find the first occurrence of the frame.
+        const pos = tmp1.indexOf(tmp2[i]);
+        if (pos !== -1) {
+          // Only keep new frames.
+          tmp1 = tmp1.slice(0, pos);
+          break;
+        }
+      }
+      newErr.stack = `${tmp1.join('\n')}\n${tmp2.join('\n')}`;
+    }
+
+    throw newErr;
+  }
+};
 
 // Expose a strict only variant of assert
 function strict(...args) {

test/fixtures/uncaught-exceptions/callbackify2.js

@@ -8,13 +8,13 @@ const { callbackify } = require('util');
 {
   const sentinel = new Error(__filename);
   process.once('uncaughtException', (err) => {
-    assert.strictEqual(err, sentinel);
+    assert.notStrictEqual(err, sentinel);
     // Calling test will use `stdout` to assert value of `err.message`
     console.log(err.message);
   });
 
-  async function fn() {
-    return await Promise.reject(sentinel);
+  function fn() {
+    return Promise.reject(sentinel);
   }
 
   const cbFn = callbackify(fn);

test/message/error_exit.out

@@ -1,6 +1,6 @@
 Exiting with code=1
 assert.js:*
-  throw new errors.AssertionError(obj);
+  throw new AssertionError(obj);
   ^
 
 AssertionError [ERR_ASSERTION]: 1 strictEqual 2

test/message/if-error-has-good-stack.js

@@ -0,0 +1,22 @@
+'use strict';
+
+require('../common');
+const assert = require('assert');
+
+let err;
+// Create some random error frames.
+(function a() {
+  (function b() {
+    (function c() {
+      err = new Error('test error');
+    })();
+  })();
+})();
+
+(function x() {
+  (function y() {
+    (function z() {
+      assert.ifError(err);
+    })();
+  })();
+})();

test/message/if-error-has-good-stack.out

@@ -0,0 +1,19 @@
+assert.js:*
+    throw newErr;
+    ^
+
+AssertionError [ERR_ASSERTION]: ifError got unwanted exception: test error
+    at z (*/if-error-has-good-stack.js:*:*
+    at y (*/if-error-has-good-stack.js:*:*)
+    at x (*/if-error-has-good-stack.js:*:*)
+    at Object.<anonymous> (*/if-error-has-good-stack.js:*:*)
+    at c (*/if-error-has-good-stack.js:*:*)
+    at b (*/if-error-has-good-stack.js:*:*)
+    at a (*/if-error-has-good-stack.js:*:*)
+    at Object.<anonymous> (*/if-error-has-good-stack.js:*:*)
+    at Module._compile (module.js:*:*)
+    at Object.Module._extensions..js (module.js:*:*)
+    at Module.load (module.js:*:*)
+    at tryModuleLoad (module.js:*:*)
+    at Function.Module._load (module.js:*:*)
+    at Function.Module.runMain (module.js:*:*)

test/parallel/test-assert-if-error.js

@@ -0,0 +1,88 @@
+'use strict';
+
+require('../common');
+const assert = require('assert').strict;
+/* eslint-disable no-restricted-properties */
+
+// Test that assert.ifError has the correct stack trace of both stacks.
+
+let err;
+// Create some random error frames.
+(function a() {
+  (function b() {
+    (function c() {
+      err = new Error('test error');
+    })();
+  })();
+})();
+
+const msg = err.message;
+const stack = err.stack;
+
+(function x() {
+  (function y() {
+    (function z() {
+      let threw = false;
+      try {
+        assert.ifError(err);
+      } catch (e) {
+        assert.equal(e.message, 'ifError got unwanted exception: test error');
+        assert.equal(err.message, msg);
+        assert.equal(e.actual, err);
+        assert.equal(e.actual.stack, stack);
+        assert.equal(e.expected, null);
+        assert.equal(e.operator, 'ifError');
+        threw = true;
+      }
+      assert(threw);
+    })();
+  })();
+})();
+
+assert.throws(
+  () => assert.ifError(new TypeError()),
+  {
+    message: 'ifError got unwanted exception: TypeError'
+  }
+);
+
+assert.throws(
+  () => assert.ifError({ stack: false }),
+  {
+    message: 'ifError got unwanted exception: { stack: false }'
+  }
+);
+
+assert.throws(
+  () => assert.ifError({ constructor: null, message: '' }),
+  {
+    message: 'ifError got unwanted exception: '
+  }
+);
+
+assert.throws(
+  () => { assert.ifError(false); },
+  {
+    message: 'ifError got unwanted exception: false'
+  }
+);
+
+assert.doesNotThrow(() => { assert.ifError(null); });
+assert.doesNotThrow(() => { assert.ifError(); });
+assert.doesNotThrow(() => { assert.ifError(undefined); });
+
+// https://github.com/nodejs/node-v0.x-archive/issues/2893
+{
+  let threw = false;
+  try {
+    // eslint-disable-next-line no-restricted-syntax
+    assert.throws(() => {
+      assert.ifError(null);
+    });
+  } catch (e) {
+    threw = true;
+    assert.strictEqual(e.message, 'Missing expected exception.');
+    assert(!e.stack.includes('throws'), e.stack);
+  }
+  assert(threw);
+}

test/parallel/test-assert.js

@@ -452,11 +452,6 @@ assert.throws(makeBlock(thrower, TypeError));
                      'a.doesNotThrow is not catching type matching errors');
 }
 
-assert.throws(() => { assert.ifError(new Error('test error')); },
-              /^Error: test error$/);
-assert.doesNotThrow(() => { assert.ifError(null); });
-assert.doesNotThrow(() => { assert.ifError(); });
-
 common.expectsError(
   () => assert.doesNotThrow(makeBlock(thrower, Error), 'user message'),
   {
@@ -602,22 +597,6 @@ testAssertionMessage({ a: undefined, b: null }, '{ a: undefined, b: null }');
 testAssertionMessage({ a: NaN, b: Infinity, c: -Infinity },
                      '{ a: NaN, b: Infinity, c: -Infinity }');
 
-// https://github.com/nodejs/node-v0.x-archive/issues/2893
-{
-  let threw = false;
-  try {
-    // eslint-disable-next-line no-restricted-syntax
-    assert.throws(() => {
-      assert.ifError(null);
-    });
-  } catch (e) {
-    threw = true;
-    assert.strictEqual(e.message, 'Missing expected exception.');
-    assert.ok(!e.stack.includes('throws'), e.stack);
-  }
-  assert.ok(threw);
-}
-
 // https://github.com/nodejs/node-v0.x-archive/issues/5292
 try {
   assert.strictEqual(1, 2);

test/parallel/test-domain-implicit-fs.js

@@ -34,9 +34,9 @@ d.on('error', common.mustCall(function(er) {
   assert.strictEqual(er.domain, d);
   assert.strictEqual(er.domainThrown, true);
   assert.ok(!er.domainEmitter);
-  assert.strictEqual(er.code, 'ENOENT');
-  assert.ok(/\bthis file does not exist\b/i.test(er.path));
-  assert.strictEqual(typeof er.errno, 'number');
+  assert.strictEqual(er.actual.code, 'ENOENT');
+  assert.ok(/\bthis file does not exist\b/i.test(er.actual.path));
+  assert.strictEqual(typeof er.actual.errno, 'number');
 }));
 
 

test/parallel/test-util-callbackify.js

@@ -220,7 +220,9 @@ const values = [
     [fixture],
     common.mustCall((err, stdout, stderr) => {
       assert.ifError(err);
-      assert.strictEqual(stdout.trim(), fixture);
+      assert.strictEqual(
+        stdout.trim(),
+        `ifError got unwanted exception: ${fixture}`);
       assert.strictEqual(stderr, '');
     })
   );

test/sequential/test-inspector-port-zero.js

@@ -16,8 +16,8 @@ function test(arg, port = '') {
   let stderr = '';
   proc.stdout.on('data', (data) => stdout += data);
   proc.stderr.on('data', (data) => stderr += data);
-  proc.stdout.on('close', assert.ifError);
-  proc.stderr.on('close', assert.ifError);
+  proc.stdout.on('close', (hadErr) => assert(!hadErr));
+  proc.stderr.on('close', (hadErr) => assert(!hadErr));
   proc.stderr.on('data', () => {
     if (!stderr.includes('\n')) return;
     assert(/Debugger listening on (.+)/.test(stderr));