assert: fix AssertionError, assign error code

Using `assert.AssertionError()` without the `new` keyword results in a non-intuitive error: ```js > assert.AssertionError({}) TypeError: Cannot assign to read only property 'name' of function 'function ok(value, message) { if (!value) fail(value, true, message, '==', assert.ok); }' at Function.AssertionError (assert.js:45:13) at repl:1:8 at realRunInThisContextScript (vm.js:22:35) at sigintHandlersWrap (vm.js:98:12) at ContextifyScript.Script.runInThisContext (vm.js:24:12) at REPLServer.defaultEval (repl.js:346:29) at bound (domain.js:280:14) at REPLServer.runBound [as eval] (domain.js:293:12) at REPLServer.onLine (repl.js:545:10) at emitOne (events.js:101:20) > ``` The `assert.AssertionError()` can only be used correctly with `new`, so this converts it into a proper ES6 class that will give an appropriate error message. This also associates the appropriate internal/errors code with all `assert.AssertionError` instances and updates the appropriate test cases. PR-URL: #12651 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
nodejs · May 4, 2017 · e48d58b · e48d58b
1 parent c1b3b95
commit e48d58b
Show file tree

Hide file tree

Showing 12 changed files with 279 additions and 109 deletions.
diff --git a/lib/assert.js b/lib/assert.js
@@ -27,6 +27,13 @@ const { isSet, isMap } = process.binding('util');
 const objectToString = require('internal/util').objectToString;
 const Buffer = require('buffer').Buffer;
 
+var errors;
+function lazyErrors() {
+  if (!errors)
+    errors = require('internal/errors');
+  return errors;
+}
+
 // The assert module provides functions that throw
 // AssertionError's when particular conditions are not met. The
 // assert module must conform to the following interface.
@@ -38,34 +45,33 @@ const assert = module.exports = ok;
 //                             actual: actual,
 //                             expected: expected });
 
-assert.AssertionError = function AssertionError(options) {
-  this.name = 'AssertionError';
-  this.actual = options.actual;
-  this.expected = options.expected;
-  this.operator = options.operator;
-  if (options.message) {
-    this.message = options.message;
-    this.generatedMessage = false;
-  } else {
-    this.message = getMessage(this);
-    this.generatedMessage = true;
-  }
-  var stackStartFunction = options.stackStartFunction || fail;
-  Error.captureStackTrace(this, stackStartFunction);
-};
-
-// assert.AssertionError instanceof Error
-util.inherits(assert.AssertionError, Error);
-
-function truncate(s, n) {
-  return s.slice(0, n);
+// TODO(jasnell): Consider moving AssertionError into internal/errors.js
+class AssertionError extends Error {
+  constructor(options = {}) {
+    if (typeof options !== 'object' || options === null) {
+      // Lazy because the errors module itself uses assertions, leading to
+      // a circular dependency. This can be eliminated by moving this class
+      // into internal/errors.js
+      const errors = lazyErrors();
+      throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'options', 'object');
+    }
+    const message = options.message ||
+                    `${util.inspect(options.actual).slice(0, 128)} ` +
+                    `${options.operator} ` +
+                    util.inspect(options.expected).slice(0, 128);
+    super(message);
+    this.generatedMessage = !options.message;
+    this.name = 'AssertionError [ERR_ASSERTION]';
+    this.code = 'ERR_ASSERTION';
+    this.actual = options.actual;
+    this.expected = options.expected;
+    this.operator = options.operator;
+    var stackStartFunction = options.stackStartFunction || fail;
+    Error.captureStackTrace(this, stackStartFunction);
+  }
 }
 
-function getMessage(self) {
-  return truncate(util.inspect(self.actual), 128) + ' ' +
-         self.operator + ' ' +
-         truncate(util.inspect(self.expected), 128);
-}
+assert.AssertionError = AssertionError;
 
 // At present only the three keys mentioned above are used and
 // understood by the spec. Implementations or sub modules can pass
@@ -83,7 +89,7 @@ function fail(actual, expected, message, operator, stackStartFunction) {
     message = actual;
   if (arguments.length === 2)
     operator = '!=';
-  throw new assert.AssertionError({
+  throw new AssertionError({
     message: message,
     actual: actual,
     expected: expected,

diff --git a/test/message/error_exit.out b/test/message/error_exit.out
@@ -1,9 +1,9 @@
 Exiting with code=1
-
 assert.js:*
-  throw new assert.AssertionError({
+  throw new AssertionError({
   ^
-AssertionError: 1 === 2
+
+AssertionError [ERR_ASSERTION]: 1 === 2
     at Object.<anonymous> (*test*message*error_exit.js:*:*)
     at Module._compile (module.js:*:*)
     at Object.Module._extensions..js (module.js:*:*)

diff --git a/test/parallel/test-assert-checktag.js b/test/parallel/test-assert-checktag.js
@@ -1,5 +1,5 @@
 'use strict';
-require('../common');
+const common = require('../common');
 const assert = require('assert');
 const util = require('util');
 
@@ -13,7 +13,10 @@ function re(literals, ...values) {
     result += str.replace(/[\\^$.*+?()[\]{}|=!<>:-]/g, '\\$&');
     result += literals[i + 1];
   }
-  return new RegExp('^AssertionError: ' + result + '$');
+  return common.expectsError({
+    code: 'ERR_ASSERTION',
+    message: new RegExp(`^${result}$`)
+  });
 }
 
 // Turn off no-restricted-properties because we are testing deepEqual!

diff --git a/test/parallel/test-assert-deep.js b/test/parallel/test-assert-deep.js
@@ -1,5 +1,5 @@
 'use strict';
-require('../common');
+const common = require('../common');
 const assert = require('assert');
 const util = require('util');
 
@@ -13,7 +13,10 @@ function re(literals, ...values) {
     result += str.replace(/[\\^$.*+?()[\]{}|=!<>:-]/g, '\\$&');
     result += literals[i + 1];
   }
-  return new RegExp(`^AssertionError: ${result}$`);
+  return common.expectsError({
+    code: 'ERR_ASSERTION',
+    message: new RegExp(`^${result}$`)
+  });
 }
 
 // The following deepEqual tests might seem very weird.
@@ -112,8 +115,10 @@ for (const a of similar) {
 
 assert.throws(
   () => { assert.deepEqual(new Set([{a: 0}]), new Set([{a: 1}])); },
-  /^AssertionError: Set { { a: 0 } } deepEqual Set { { a: 1 } }$/
-);
+  common.expectsError({
+    code: 'ERR_ASSERTION',
+    message: /^Set { { a: 0 } } deepEqual Set { { a: 1 } }$/
+  }));
 
 function assertDeepAndStrictEqual(a, b) {
   assert.deepEqual(a, b);

diff --git a/test/parallel/test-assert-fail.js b/test/parallel/test-assert-fail.js
@@ -1,33 +1,53 @@
 'use strict';
-require('../common');
+const common = require('../common');
 const assert = require('assert');
 
 // no args
 assert.throws(
   () => { assert.fail(); },
-  /^AssertionError: undefined undefined undefined$/
+  common.expectsError({
+    code: 'ERR_ASSERTION',
+    type: assert.AssertionError,
+    message: 'undefined undefined undefined'
+  })
 );
 
 // one arg = message
 assert.throws(
   () => { assert.fail('custom message'); },
-  /^AssertionError: custom message$/
+  common.expectsError({
+    code: 'ERR_ASSERTION',
+    type: assert.AssertionError,
+    message: 'custom message'
+  })
 );
 
 // two args only, operator defaults to '!='
 assert.throws(
   () => { assert.fail('first', 'second'); },
-  /^AssertionError: 'first' != 'second'$/
+  common.expectsError({
+    code: 'ERR_ASSERTION',
+    type: assert.AssertionError,
+    message: '\'first\' != \'second\''
+  })
 );
 
 // three args
 assert.throws(
   () => { assert.fail('ignored', 'ignored', 'another custom message'); },
-  /^AssertionError: another custom message$/
+  common.expectsError({
+    code: 'ERR_ASSERTION',
+    type: assert.AssertionError,
+    message: 'another custom message'
+  })
 );
 
 // no third arg (but a fourth arg)
 assert.throws(
   () => { assert.fail('first', 'second', undefined, 'operator'); },
-  /^AssertionError: 'first' operator 'second'$/
+  common.expectsError({
+    code: 'ERR_ASSERTION',
+    type: assert.AssertionError,
+    message: '\'first\' operator \'second\''
+  })
 );