assert: refactor the code

1. Rename private functions
2. Use destructuring
3. Remove obsolete comments

PR-URL: https://github.com/nodejs/node/pull/13862
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

lib/assert.js

@@ -20,12 +20,11 @@
 
 'use strict';
 
-// UTILITY
+const { compare } = process.binding('buffer');
-const compare = process.binding('buffer').compare;
 const util = require('util');
 const { isSet, isMap } = process.binding('util');
-const objectToString = require('internal/util').objectToString;
+const { objectToString } = require('internal/util');
-const Buffer = require('buffer').Buffer;
+const { Buffer } = require('buffer');
 
 var errors;
 function lazyErrors() {
@@ -47,10 +46,21 @@ const assert = module.exports = ok;
 
 // All of the following functions must throw an AssertionError
 // when a corresponding condition is not met, with a message that
-// may be undefined if not provided.  All assertion methods provide
+// may be undefined if not provided. All assertion methods provide
 // both the actual and expected values to the assertion error for
 // display purposes.
 
+function innerFail(actual, expected, message, operator, stackStartFunction) {
+  const errors = lazyErrors();
+  throw new errors.AssertionError({
+    message,
+    actual,
+    expected,
+    operator,
+    stackStartFunction
+  });
+}
+
 function fail(actual, expected, message, operator, stackStartFunction) {
   if (arguments.length === 0) {
     message = 'Failed';
@@ -59,19 +69,11 @@ function fail(actual, expected, message, operator, stackStartFunction) {
     message = actual;
     actual = undefined;
   }
-  if (arguments.length === 2)
+  if (arguments.length === 2) {
     operator = '!=';
-  const errors = lazyErrors();
+  }
-  throw new errors.AssertionError({
+  innerFail(actual, expected, message, operator, stackStartFunction || fail);
-    message: message,
-    actual: actual,
-    expected: expected,
-    operator: operator,
-    stackStartFunction: stackStartFunction
-  });
 }
-
-// EXTENSION! allows for well behaved errors defined elsewhere.
 assert.fail = fail;
 
 // The AssertionError is defined in internal/error.
@@ -82,50 +84,39 @@ assert.AssertionError = lazyErrors().AssertionError;
 
 
 // Pure assertion tests whether a value is truthy, as determined
-// by !!guard.
+// by !!value.
-// assert.ok(guard, message_opt);
-// This statement is equivalent to assert.equal(true, !!guard,
-// message_opt);. To test strictly for the value true, use
-// assert.strictEqual(true, guard, message_opt);.
-
 function ok(value, message) {
-  if (!value) fail(value, true, message, '==', assert.ok);
+  if (!value) innerFail(value, true, message, '==', ok);
 }
 assert.ok = ok;
 
-// The equality assertion tests shallow, coercive equality with
+// The equality assertion tests shallow, coercive equality with ==.
-// ==.
-// assert.equal(actual, expected, message_opt);
 /* eslint-disable no-restricted-properties */
 assert.equal = function equal(actual, expected, message) {
   // eslint-disable-next-line eqeqeq
-  if (actual != expected) fail(actual, expected, message, '==', assert.equal);
+  if (actual != expected) innerFail(actual, expected, message, '==', equal);
 };
 
 // The non-equality assertion tests for whether two objects are not
 // equal with !=.
-// assert.notEqual(actual, expected, message_opt);
-
 assert.notEqual = function notEqual(actual, expected, message) {
   // eslint-disable-next-line eqeqeq
   if (actual == expected) {
-    fail(actual, expected, message, '!=', assert.notEqual);
+    innerFail(actual, expected, message, '!=', notEqual);
   }
 };
 
 // The equivalence assertion tests a deep equality relation.
-// assert.deepEqual(actual, expected, message_opt);
-
 assert.deepEqual = function deepEqual(actual, expected, message) {
-  if (!_deepEqual(actual, expected, false)) {
+  if (!innerDeepEqual(actual, expected, false)) {
-    fail(actual, expected, message, 'deepEqual', assert.deepEqual);
+    innerFail(actual, expected, message, 'deepEqual', deepEqual);
   }
 };
 /* eslint-enable */
 
 assert.deepStrictEqual = function deepStrictEqual(actual, expected, message) {
-  if (!_deepEqual(actual, expected, true)) {
+  if (!innerDeepEqual(actual, expected, true)) {
-    fail(actual, expected, message, 'deepStrictEqual', assert.deepStrictEqual);
+    innerFail(actual, expected, message, 'deepStrictEqual', deepStrictEqual);
   }
 };
 
@@ -154,7 +145,7 @@ function isArguments(tag) {
   return tag === '[object Arguments]';
 }
 
-function _deepEqual(actual, expected, strict, memos) {
+function innerDeepEqual(actual, expected, strict, memos) {
   // All identical values are equivalent, as determined by ===.
   if (actual === expected) {
     return true;
@@ -307,7 +298,7 @@ function setHasSimilarElement(set, val1, usedEntries, strict, memo) {
     if (usedEntries && usedEntries.has(val2))
       continue;
 
-    if (_deepEqual(val1, val2, strict, memo)) {
+    if (innerDeepEqual(val1, val2, strict, memo)) {
       if (usedEntries)
         usedEntries.add(val2);
       return true;
@@ -364,7 +355,7 @@ function mapHasSimilarEntry(map, key1, item1, usedEntries, strict, memo) {
   // This check is not strictly necessary. The loop performs this check, but
   // doing it here improves performance of the common case when reference-equal
   // keys exist (which includes all primitive-valued keys).
-  if (map.has(key1) && _deepEqual(item1, map.get(key1), strict, memo)) {
+  if (map.has(key1) && innerDeepEqual(item1, map.get(key1), strict, memo)) {
     if (usedEntries)
       usedEntries.add(key1);
     return true;
@@ -381,8 +372,8 @@ function mapHasSimilarEntry(map, key1, item1, usedEntries, strict, memo) {
     if (usedEntries && usedEntries.has(key2))
       continue;
 
-    if (_deepEqual(key1, key2, strict, memo) &&
+    if (innerDeepEqual(key1, key2, strict, memo) &&
-        _deepEqual(item1, item2, strict, memo)) {
+        innerDeepEqual(item1, item2, strict, memo)) {
       if (usedEntries)
         usedEntries.add(key2);
       return true;
@@ -459,44 +450,39 @@ function objEquiv(a, b, strict, actualVisitedObjects) {
   // Possibly expensive deep test:
   for (i = aKeys.length - 1; i >= 0; i--) {
     key = aKeys[i];
-    if (!_deepEqual(a[key], b[key], strict, actualVisitedObjects))
+    if (!innerDeepEqual(a[key], b[key], strict, actualVisitedObjects))
       return false;
   }
   return true;
 }
 
 // The non-equivalence assertion tests for any deep inequality.
-// assert.notDeepEqual(actual, expected, message_opt);
-
 assert.notDeepEqual = function notDeepEqual(actual, expected, message) {
-  if (_deepEqual(actual, expected, false)) {
+  if (innerDeepEqual(actual, expected, false)) {
-    fail(actual, expected, message, 'notDeepEqual', assert.notDeepEqual);
+    innerFail(actual, expected, message, 'notDeepEqual', notDeepEqual);
   }
 };
 
 assert.notDeepStrictEqual = notDeepStrictEqual;
 function notDeepStrictEqual(actual, expected, message) {
-  if (_deepEqual(actual, expected, true)) {
+  if (innerDeepEqual(actual, expected, true)) {
-    fail(actual, expected, message, 'notDeepStrictEqual', notDeepStrictEqual);
+    innerFail(actual, expected, message, 'notDeepStrictEqual',
+              notDeepStrictEqual);
   }
 }
 
 // The strict equality assertion tests strict equality, as determined by ===.
-// assert.strictEqual(actual, expected, message_opt);
-
 assert.strictEqual = function strictEqual(actual, expected, message) {
   if (actual !== expected) {
-    fail(actual, expected, message, '===', assert.strictEqual);
+    innerFail(actual, expected, message, '===', strictEqual);
   }
 };
 
 // The strict non-equality assertion tests for strict inequality, as
 // determined by !==.
-// assert.notStrictEqual(actual, expected, message_opt);
-
 assert.notStrictEqual = function notStrictEqual(actual, expected, message) {
   if (actual === expected) {
-    fail(actual, expected, message, '!==', assert.notStrictEqual);
+    innerFail(actual, expected, message, '!==', notStrictEqual);
   }
 };
 
@@ -525,7 +511,7 @@ function expectedException(actual, expected) {
   return expected.call({}, actual) === true;
 }
 
-function _tryBlock(block) {
+function tryBlock(block) {
   var error;
   try {
     block();
@@ -535,7 +521,7 @@ function _tryBlock(block) {
   return error;
 }
 
-function _throws(shouldThrow, block, expected, message) {
+function innerThrows(shouldThrow, block, expected, message) {
   var actual;
 
   if (typeof block !== 'function') {
@@ -549,13 +535,13 @@ function _throws(shouldThrow, block, expected, message) {
     expected = null;
   }
 
-  actual = _tryBlock(block);
+  actual = tryBlock(block);
 
   message = (expected && expected.name ? ' (' + expected.name + ')' : '') +
             (message ? ': ' + message : '.');
 
   if (shouldThrow && !actual) {
-    fail(actual, expected, 'Missing expected exception' + message);
+    innerFail(actual, expected, 'Missing expected exception' + message, fail);
   }
 
   const userProvidedMessage = typeof message === 'string';
@@ -566,7 +552,7 @@ function _throws(shouldThrow, block, expected, message) {
       userProvidedMessage &&
       expectedException(actual, expected)) ||
       isUnexpectedException) {
-    fail(actual, expected, 'Got unwanted exception' + message);
+    innerFail(actual, expected, 'Got unwanted exception' + message, fail);
   }
 
   if ((shouldThrow && actual && expected &&
@@ -576,16 +562,12 @@ function _throws(shouldThrow, block, expected, message) {
 }
 
 // Expected to throw an error.
-// assert.throws(block, Error_opt, message_opt);
+assert.throws = function throws(block, error, message) {
-
+  innerThrows(true, block, error, message);
-assert.throws = function throws(block, /*optional*/error, /*optional*/message) {
-  _throws(true, block, error, message);
 };
 
-// EXTENSION! This is annoying to write outside this module.
+assert.doesNotThrow = function doesNotThrow(block, error, message) {
-assert.doesNotThrow = doesNotThrow;
+  innerThrows(false, block, error, message);
-function doesNotThrow(block, /*optional*/error, /*optional*/message) {
+};
-  _throws(false, block, error, message);
-}
 
 assert.ifError = function ifError(err) { if (err) throw err; };

test/parallel/test-assert-fail.js

@@ -1,53 +1,76 @@
 'use strict';
+
 const common = require('../common');
 const assert = require('assert');
 
-// no args
+// No args
 assert.throws(
   () => { assert.fail(); },
   common.expectsError({
     code: 'ERR_ASSERTION',
     type: assert.AssertionError,
-    message: 'Failed'
+    message: 'Failed',
+    operator: undefined,
+    actual: undefined,
+    expected: undefined
   })
 );
 
-// one arg = message
+// One arg = message
 assert.throws(
   () => { assert.fail('custom message'); },
   common.expectsError({
     code: 'ERR_ASSERTION',
     type: assert.AssertionError,
-    message: 'custom message'
+    message: 'custom message',
+    operator: undefined,
+    actual: undefined,
+    expected: undefined
   })
 );
 
-// two args only, operator defaults to '!='
+// Two args only, operator defaults to '!='
 assert.throws(
   () => { assert.fail('first', 'second'); },
   common.expectsError({
     code: 'ERR_ASSERTION',
     type: assert.AssertionError,
-    message: '\'first\' != \'second\''
+    message: '\'first\' != \'second\'',
+    operator: '!=',
+    actual: 'first',
+    expected: 'second'
+
   })
 );
 
-// three args
+// Three args
 assert.throws(
   () => { assert.fail('ignored', 'ignored', 'another custom message'); },
   common.expectsError({
     code: 'ERR_ASSERTION',
     type: assert.AssertionError,
-    message: 'another custom message'
+    message: 'another custom message',
+    operator: undefined,
+    actual: 'ignored',
+    expected: 'ignored'
   })
 );
 
-// no third arg (but a fourth arg)
+// No third arg (but a fourth arg)
 assert.throws(
   () => { assert.fail('first', 'second', undefined, 'operator'); },
   common.expectsError({
     code: 'ERR_ASSERTION',
     type: assert.AssertionError,
-    message: '\'first\' operator \'second\''
+    message: '\'first\' operator \'second\'',
+    operator: 'operator',
+    actual: 'first',
+    expected: 'second'
   })
 );
+
+// The stackFrameFunction should exclude the foo frame
+assert.throws(
+  function foo() { assert.fail('first', 'second', 'message', '!==', foo); },
+  (err) => !/foo/m.test(err.stack)
+);