From 26785a23bb05def0979ad09f3cea4ba400d75ec8 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Sun, 2 Jul 2017 12:06:36 -0400 Subject: [PATCH] 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 Reviewed-By: Joyee Cheung Backport-PR-URL: https://github.com/nodejs/node/pull/14428 Backport-Reviewed-By: Anna Henningsen --- lib/assert.js | 115 +++++++++++++----------------- test/parallel/test-assert-fail.js | 32 +++++++-- 2 files changed, 75 insertions(+), 72 deletions(-) diff --git a/lib/assert.js b/lib/assert.js index bd36c9a16d1b38..a751ab2f2089d9 100644 --- a/lib/assert.js +++ b/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; }; diff --git a/test/parallel/test-assert-fail.js b/test/parallel/test-assert-fail.js index 1f389933bb4a9c..d64947fa1000a6 100644 --- a/test/parallel/test-assert-fail.js +++ b/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) +);