From 1cd7c4ddab3b9ddeb46d86bc00ca3b50d9f194b9 Mon Sep 17 00:00:00 2001
From: Ruben Bridgewater <ruben@bridgewater.de>
Date: Sat, 9 Dec 2017 17:15:19 -0200
Subject: [PATCH] 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.
---
 lib/assert.js                | 118 +++++++++++++++++++++++++++--------
 lib/internal/errors.js       |   4 +-
 test/message/error_exit.out  |   2 +-
 test/parallel/test-assert.js |   9 +++
 4 files changed, 104 insertions(+), 29 deletions(-)

diff --git a/lib/assert.js b/lib/assert.js
index 91b2e56fb81618..aca8313a09cbfe 100644
--- 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,7 +75,15 @@ 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;
 
@@ -83,7 +91,15 @@ assert.ok = ok;
 /* 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
@@ -91,48 +107,89 @@ assert.equal = function equal(actual, expected, message) {
 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
+    });
   }
 }
 
 assert.strictEqual = function strictEqual(actual, expected, message) {
   if (!Object.is(actual, expected)) {
-    innerFail(actual, expected, message, 'strictEqual', strictEqual);
+    innerFail({
+      actual,
+      expected,
+      message,
+      operator: 'strictEqual',
+      stackStartFn: strictEqual
+    });
   }
 };
 
 assert.notStrictEqual = function notStrictEqual(actual, expected, message) {
   if (Object.is(actual, expected)) {
-    innerFail(actual, expected, message, 'notStrictEqual', notStrictEqual);
+    innerFail({
+      actual,
+      expected,
+      message,
+      operator: 'notStrictEqual',
+      stackStartFn: notStrictEqual
+    });
   }
 };
 
@@ -180,7 +237,13 @@ 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;
@@ -188,10 +251,13 @@ function innerThrows(shouldThrow, block, expected, message) {
   } 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
index dadba400b500bc..ab0d2fe54eb5da 100644
--- a/lib/internal/errors.js
+++ b/lib/internal/errors.js
@@ -136,7 +136,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 {
@@ -155,7 +155,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
index 833bc74587484e..0fc93306204942 100644
--- 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 strictEqual 2
diff --git a/test/parallel/test-assert.js b/test/parallel/test-assert.js
index c51cefc3a73510..b64c777bddee9a 100644
--- 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'
+  }
+);