Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Comparing errors through deepEqual #3122

Closed
rpepato opened this issue Sep 30, 2015 · 3 comments
Closed

Comparing errors through deepEqual #3122

rpepato opened this issue Sep 30, 2015 · 3 comments
Labels
assert Issues and PRs related to the assert subsystem. question Issues that look for answers.

Comments

@rpepato
Copy link

rpepato commented Sep 30, 2015

I'm implementing tests for my js project and I need to compare an error returned from a rejected promise with an expected one.

It turns out that, no mater what is the content of the error object, assert.deepEqual always returns true as long as the two instances being compared are Errors.

The same behaviour does not occur when comparing strings, for example (node version 4.1.1):

node --version
v4.1.1

> let assert = require("assert");
undefined

> assert.deepEqual(new Error("a"), new Error("b"));
undefined

> assert.deepEqual(new String("a"), new String("b"));
AssertionError: [String: 'a'] deepEqual [String: 'b']
    at repl:1:8
    at REPLServer.defaultEval (repl.js:164:27)
    at bound (domain.js:250:14)
    at REPLServer.runBound [as eval] (domain.js:263:12)
    at REPLServer.<anonymous> (repl.js:393:12)
    at emitOne (events.js:82:20)
    at REPLServer.emit (events.js:169:7)
    at REPLServer.Interface._onLine (readline.js:210:10)
    at REPLServer.Interface._line (readline.js:549:8)
    at REPLServer.Interface._ttyWrite (readline.js:826:14)

Reading the documentation for the deepEqual assertion, I didn't found any mentions regarding something special when comparing Errors.

Is this behaviour intended by design? Am I missing something important or should a consider another strategy to deeply compare two Error objects (instances + messages)?

@thefourtheye thefourtheye added question Issues that look for answers. assert Issues and PRs related to the assert subsystem. labels Sep 30, 2015
@Trott
Copy link
Member

Trott commented Sep 30, 2015

This looks like it's caused by code in lib/assert.js that uses Object.keys() to get all the keys in the Error objects and then compares them. The Error objects have non-enumerable keys so Object.keys(new Error('a')); returns an empty array. So the errors are determined to be equal.

@Trott
Copy link
Member

Trott commented Sep 30, 2015

Possible fix: #3124

Trott added a commit to Trott/io.js that referenced this issue Oct 12, 2015
Error objects have non-enumerable properties. So, unexpectedly,
assert.deepEqual(new Error('a'), new Error('b')) will not throw an
AssertionError. This commit changes that behavior.

Fixes: nodejs#3122
Trott added a commit to Trott/io.js that referenced this issue Oct 16, 2015
Trott added a commit to Trott/io.js that referenced this issue Oct 19, 2015
Assert is now locked. Userland alternatives should be used. Assert is
for testing Node.js itself.

Document potentially surprising use of enumerable properties only in
deep equality assertions.

Ref: nodejs#3124
Ref: nodejs#3122

PR-URL: nodejs#3330
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
@Trott
Copy link
Member

Trott commented Oct 19, 2015

I'm afraid a fix for this is unlikely to land in Node.js. The API is now locked and users are encouraged to employ userland assertion libraries instead.

If you don't want to use a userland assertion library for whatever reason, a workaround for your case might look like this:

assert.deepEqual(error1.message, error2.message);
assert.deepEqual(error1.name, error2.name);

Or, more thoroughly (like, if you want the stack traces to match exactly, which you typically probably don't):

var ka = Object.getOwnPropertyNames(error1).concat(Object.getOwnPropertySymbols(error1));
var kb = Object.getOwnPropertyNames(error1).concat(Object.getOwnPropertySymbols(error2));

ka.sort();
kb.sort();

assert.deepStrictEqual(ka, kb);

for (var i = ka.length - 1; i >= 0; i--) {
  console.log(ka[i]);
  assert.deepEqual(error1[ka[i]], error2[ka[i]]); // use assert.deepStrictEqual() if that's what you want
}

@Trott Trott closed this as completed Oct 19, 2015
Trott added a commit that referenced this issue Oct 21, 2015
Assert is now locked. Userland alternatives should be used. Assert is
for testing Node.js itself.

Document potentially surprising use of enumerable properties only in
deep equality assertions.

Ref: #3124
Ref: #3122

PR-URL: #3330
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Trott added a commit that referenced this issue Oct 26, 2015
Assert is now locked. Userland alternatives should be used. Assert is
for testing Node.js itself.

Document potentially surprising use of enumerable properties only in
deep equality assertions.

Ref: #3124
Ref: #3122

PR-URL: #3330
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Trott added a commit that referenced this issue Oct 29, 2015
Assert is now locked. Userland alternatives should be used. Assert is
for testing Node.js itself.

Document potentially surprising use of enumerable properties only in
deep equality assertions.

Ref: #3124
Ref: #3122

PR-URL: #3330
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assert Issues and PRs related to the assert subsystem. question Issues that look for answers.
Projects
None yet
Development

No branches or pull requests

3 participants