assert: use object argument in innerFail

Right now it is difficult to know what argument stands for what property. By refactoring the arguments into a object it is clear what stands for what. Backport-PR-URL: #19230 PR-URL: #17582 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]>
nodejs · Mar 20, 2018 · db73d1c · db73d1c
1 parent bae5de1
commit db73d1c
Show file tree

Hide file tree

Showing 4 changed files with 104 additions and 29 deletions.
diff --git a/lib/assert.js b/lib/assert.js
@@ -36,19 +36,13 @@ const assert = module.exports = ok;
 // both the actual and expected values to the assertion error for
 // display purposes.
 
-function innerFail(actual, expected, message, operator, stackStartFunction) {
-  if (message instanceof Error) throw message;
+function innerFail(obj) {
+  if (obj.message instanceof Error) throw obj.message;
 
-  throw new errors.AssertionError({
-    message,
-    actual,
-    expected,
-    operator,
-    stackStartFunction
-  });
+  throw new errors.AssertionError(obj);
 }
 
-function fail(actual, expected, message, operator, stackStartFunction) {
+function fail(actual, expected, message, operator, stackStartFn) {
   const argsLen = arguments.length;
 
   if (argsLen === 0) {
@@ -60,7 +54,13 @@ function fail(actual, expected, message, operator, stackStartFunction) {
     operator = '!=';
   }
 
-  innerFail(actual, expected, message, operator, stackStartFunction || fail);
+  innerFail({
+    actual,
+    expected,
+    message,
+    operator,
+    stackStartFn: stackStartFn || fail
+  });
 }
 
 assert.fail = fail;
@@ -75,67 +75,124 @@ assert.AssertionError = errors.AssertionError;
 // Pure assertion tests whether a value is truthy, as determined
 // by !!value.
 function ok(value, message) {
-  if (!value) innerFail(value, true, message, '==', ok);
+  if (!value) {
+    innerFail({
+      actual: value,
+      expected: true,
+      message,
+      operator: '==',
+      stackStartFn: ok
+    });
+  }
 }
 assert.ok = ok;
 
 // The equality assertion tests shallow, coercive equality with ==.
 /* eslint-disable no-restricted-properties */
 assert.equal = function equal(actual, expected, message) {
   // eslint-disable-next-line eqeqeq
-  if (actual != expected) innerFail(actual, expected, message, '==', equal);
+  if (actual != expected) {
+    innerFail({
+      actual,
+      expected,
+      message,
+      operator: '==',
+      stackStartFn: equal
+    });
+  }
 };
 
 // The non-equality assertion tests for whether two objects are not
 // equal with !=.
 assert.notEqual = function notEqual(actual, expected, message) {
   // eslint-disable-next-line eqeqeq
   if (actual == expected) {
-    innerFail(actual, expected, message, '!=', notEqual);
+    innerFail({
+      actual,
+      expected,
+      message,
+      operator: '!=',
+      stackStartFn: notEqual
+    });
   }
 };
 
 // The equivalence assertion tests a deep equality relation.
 assert.deepEqual = function deepEqual(actual, expected, message) {
   if (!isDeepEqual(actual, expected)) {
-    innerFail(actual, expected, message, 'deepEqual', deepEqual);
+    innerFail({
+      actual,
+      expected,
+      message,
+      operator: 'deepEqual',
+      stackStartFn: deepEqual
+    });
   }
 };
 
 // The non-equivalence assertion tests for any deep inequality.
 assert.notDeepEqual = function notDeepEqual(actual, expected, message) {
   if (isDeepEqual(actual, expected)) {
-    innerFail(actual, expected, message, 'notDeepEqual', notDeepEqual);
+    innerFail({
+      actual,
+      expected,
+      message,
+      operator: 'notDeepEqual',
+      stackStartFn: notDeepEqual
+    });
   }
 };
 /* eslint-enable */
 
 assert.deepStrictEqual = function deepStrictEqual(actual, expected, message) {
   if (!isDeepStrictEqual(actual, expected)) {
-    innerFail(actual, expected, message, 'deepStrictEqual', deepStrictEqual);
+    innerFail({
+      actual,
+      expected,
+      message,
+      operator: 'deepStrictEqual',
+      stackStartFn: deepStrictEqual
+    });
   }
 };
 
 assert.notDeepStrictEqual = notDeepStrictEqual;
 function notDeepStrictEqual(actual, expected, message) {
   if (isDeepStrictEqual(actual, expected)) {
-    innerFail(actual, expected, message, 'notDeepStrictEqual',
-              notDeepStrictEqual);
+    innerFail({
+      actual,
+      expected,
+      message,
+      operator: 'notDeepStrictEqual',
+      stackStartFn: notDeepStrictEqual
+    });
   }
 }
 
 // The strict equality assertion tests strict equality, as determined by ===.
 assert.strictEqual = function strictEqual(actual, expected, message) {
   if (actual !== expected) {
-    innerFail(actual, expected, message, '===', strictEqual);
+    innerFail({
+      actual,
+      expected,
+      message,
+      operator: '===',
+      stackStartFn: strictEqual
+    });
   }
 };
 
 // The strict non-equality assertion tests for strict inequality, as
 // determined by !==.
 assert.notStrictEqual = function notStrictEqual(actual, expected, message) {
   if (actual === expected) {
-    innerFail(actual, expected, message, '!==', notStrictEqual);
+    innerFail({
+      actual,
+      expected,
+      message,
+      operator: '!==',
+      stackStartFn: notStrictEqual
+    });
   }
 };
 
@@ -183,18 +240,27 @@ function innerThrows(shouldThrow, block, expected, message) {
         details += ` (${expected.name})`;
       }
       details += message ? `: ${message}` : '.';
-      fail(actual, expected, `Missing expected exception${details}`, 'throws');
+      innerFail({
+        actual,
+        expected,
+        operator: 'throws',
+        message: `Missing expected exception${details}`,
+        stackStartFn: innerThrows
+      });
     }
     if (expected && expectedException(actual, expected) === false) {
       throw actual;
     }
   } else if (actual !== undefined) {
     if (!expected || expectedException(actual, expected)) {
       details = message ? `: ${message}` : '.';
-      fail(actual,
-           expected,
-           `Got unwanted exception${details}\n${actual.message}`,
-           'doesNotThrow');
+      innerFail({
+        actual,
+        expected,
+        operator: 'doesNotThrow',
+        message: `Got unwanted exception${details}\n${actual.message}`,
+        stackStartFn: innerThrows
+      });
     }
     throw actual;
   }

diff --git a/lib/internal/errors.js b/lib/internal/errors.js
@@ -83,7 +83,7 @@ class AssertionError extends Error {
     if (typeof options !== 'object' || options === null) {
       throw new exports.TypeError('ERR_INVALID_ARG_TYPE', 'options', 'Object');
     }
-    var { actual, expected, message, operator, stackStartFunction } = options;
+    var { actual, expected, message, operator, stackStartFn } = options;
     if (message) {
       super(message);
     } else {
@@ -102,7 +102,7 @@ class AssertionError extends Error {
     this.actual = actual;
     this.expected = expected;
     this.operator = operator;
-    Error.captureStackTrace(this, stackStartFunction);
+    Error.captureStackTrace(this, stackStartFn);
   }
 }
 

diff --git a/test/message/error_exit.out b/test/message/error_exit.out
@@ -1,6 +1,6 @@
 Exiting with code=1
 assert.js:*
-  throw new errors.AssertionError({
+  throw new errors.AssertionError(obj);
   ^
 
 AssertionError [ERR_ASSERTION]: 1 === 2

diff --git a/test/parallel/test-assert.js b/test/parallel/test-assert.js
@@ -780,3 +780,12 @@ common.expectsError(
   /* eslint-enable no-restricted-properties */
   assert(7);
 }
+
+common.expectsError(
+  () => assert.ok(null),
+  {
+    code: 'ERR_ASSERTION',
+    type: assert.AssertionError,
+    message: 'null == true'
+  }
+);