assert: wrap original error in ifError

It is hard to know where ifError is actually triggered due to the
original error being thrown.
This changes it by wrapping the original error in a AssertionError.
This has the positive effect of also making clear that it is indeed
a assertion function that triggered that error.

The original stack can still be accessed by checking the `actual`
property.

PR-URL: #18247
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

Author: BridgeAR

doc/api/assert.md

@@ -469,6 +469,11 @@ 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.
 -->
 * `value` {any}
 
@@ -483,11 +488,11 @@ assert.ifError(null);
 assert.ifError(0);
 // OK
 assert.ifError(1);
-// Throws 1
+// AssertionError [ERR_ASSERTION]: ifError got unwanted exception: 1
 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

@@ -452,7 +452,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) {
+    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 assert.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/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,82 @@
+'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.doesNotThrow(() => { assert.ifError(null); });
+assert.doesNotThrow(() => { assert.ifError(); });
+assert.doesNotThrow(() => { assert.ifError(undefined); });
+assert.doesNotThrow(() => { assert.ifError(false); });
+
+// 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, '');
     })
   );