assert: refactor the code

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

PR-URL: #13862
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

Backport-PR-URL: #14428
Backport-Reviewed-By: Anna Henningsen <anna@addaleax.net>

Author: BridgeAR

lib/assert.js

@@ -20,12 +20,11 @@
 
 'use strict';
 
-// UTILITY
-const compare = process.binding('buffer').compare;
+const { compare } = process.binding('buffer');
 const util = require('util');
 const { isSet, isMap } = process.binding('util');
-const objectToString = require('internal/util').objectToString;
-const Buffer = require('buffer').Buffer;
+const { objectToString } = require('internal/util');
+const { Buffer } = require('buffer');
 
 var errors;
 function lazyErrors() {
@@ -47,26 +46,28 @@ 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 === 1)
     message = actual;
   if (arguments.length === 2)
     operator = '!=';
-  const errors = lazyErrors();
-  throw new errors.AssertionError({
-    message: message,
-    actual: actual,
-    expected: expected,
-    operator: operator,
-    stackStartFunction: stackStartFunction
-  });
+  innerFail(actual, expected, message, operator, stackStartFunction || fail);
 }
-
-// EXTENSION! allows for well behaved errors defined elsewhere.
 assert.fail = fail;
 
 // The AssertionError is defined in internal/error.
@@ -77,50 +78,39 @@ assert.AssertionError = lazyErrors().AssertionError;
 
 
 // Pure assertion tests whether a value is truthy, as determined
-// by !!guard.
-// 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);.
-
+// by !!value.
 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
-// ==.
-// assert.equal(actual, expected, message_opt);
+// 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) 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)) {
-    fail(actual, expected, message, 'deepEqual', assert.deepEqual);
+  if (!innerDeepEqual(actual, expected, false)) {
+    innerFail(actual, expected, message, 'deepEqual', deepEqual);
   }
 };
 /* eslint-enable */
 
 assert.deepStrictEqual = function deepStrictEqual(actual, expected, message) {
-  if (!_deepEqual(actual, expected, true)) {
-    fail(actual, expected, message, 'deepStrictEqual', assert.deepStrictEqual);
+  if (!innerDeepEqual(actual, expected, true)) {
+    innerFail(actual, expected, message, 'deepStrictEqual', deepStrictEqual);
   }
 };
 
@@ -149,7 +139,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;
@@ -302,7 +292,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;
@@ -359,7 +349,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;
@@ -376,8 +366,8 @@ function mapHasSimilarEntry(map, key1, item1, usedEntries, strict, memo) {
     if (usedEntries && usedEntries.has(key2))
       continue;
 
-    if (_deepEqual(key1, key2, strict, memo) &&
-        _deepEqual(item1, item2, strict, memo)) {
+    if (innerDeepEqual(key1, key2, strict, memo) &&
+        innerDeepEqual(item1, item2, strict, memo)) {
       if (usedEntries)
         usedEntries.add(key2);
       return true;
@@ -454,44 +444,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)) {
-    fail(actual, expected, message, 'notDeepEqual', assert.notDeepEqual);
+  if (innerDeepEqual(actual, expected, false)) {
+    innerFail(actual, expected, message, 'notDeepEqual', notDeepEqual);
   }
 };
 
 assert.notDeepStrictEqual = notDeepStrictEqual;
 function notDeepStrictEqual(actual, expected, message) {
-  if (_deepEqual(actual, expected, true)) {
-    fail(actual, expected, message, 'notDeepStrictEqual', notDeepStrictEqual);
+  if (innerDeepEqual(actual, expected, true)) {
+    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);
   }
 };
 
@@ -520,7 +505,7 @@ function expectedException(actual, expected) {
   return expected.call({}, actual) === true;
 }
 
-function _tryBlock(block) {
+function tryBlock(block) {
   var error;
   try {
     block();
@@ -530,7 +515,7 @@ function _tryBlock(block) {
   return error;
 }
 
-function _throws(shouldThrow, block, expected, message) {
+function innerThrows(shouldThrow, block, expected, message) {
   var actual;
 
   if (typeof block !== 'function') {
@@ -544,13 +529,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';
@@ -561,7 +546,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 &&
@@ -571,16 +556,12 @@ function _throws(shouldThrow, block, expected, message) {
 }
 
 // Expected to throw an error.
-// assert.throws(block, Error_opt, message_opt);
-
-assert.throws = function throws(block, /*optional*/error, /*optional*/message) {
-  _throws(true, block, error, message);
+assert.throws = function throws(block, error, message) {
+  innerThrows(true, block, error, message);
 };
 
-// EXTENSION! This is annoying to write outside this module.
-assert.doesNotThrow = doesNotThrow;
-function doesNotThrow(block, /*optional*/error, /*optional*/message) {
-  _throws(false, block, error, message);
-}
+assert.doesNotThrow = function doesNotThrow(block, error, message) {
+  innerThrows(false, block, error, message);
+};
 
 assert.ifError = function ifError(err) { if (err) throw err; };

test/parallel/test-assert-fail.js

@@ -1,53 +1,75 @@
 '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,
+    operator: undefined,
+    actual: undefined,
+    expected: undefined,
     message: 'undefined undefined undefined'
   })
 );
 
-// one arg = message
+// One arg = message
 assert.throws(
   () => { assert.fail('custom message'); },
   common.expectsError({
     code: 'ERR_ASSERTION',
     type: assert.AssertionError,
+    operator: undefined,
+    actual: undefined,
+    expected: undefined,
     message: 'custom message'
   })
 );
 
-// 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,
+    operator: '!=',
+    actual: 'first',
+    expected: 'second',
     message: '\'first\' != \'second\''
   })
 );
 
-// three args
+// Three args
 assert.throws(
   () => { assert.fail('ignored', 'ignored', 'another custom message'); },
   common.expectsError({
     code: 'ERR_ASSERTION',
     type: assert.AssertionError,
+    operator: undefined,
+    actual: 'ignored',
+    expected: 'ignored',
     message: 'another custom message'
   })
 );
 
-// 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,
+    operator: 'operator',
+    actual: 'first',
+    expected: 'second',
     message: '\'first\' operator \'second\''
   })
 );
+
+// The stackFrameFunction should exclude the foo frame
+assert.throws(
+  function foo() { assert.fail('first', 'second', 'message', '!==', foo); },
+  (err) => !/foo/m.test(err.stack)
+);