assert.deepEqual has bad test

[othiym23]

If you look at https://github.com/joyent/node/blob/master/lib/assert.js#L208-L209

  // an identical 'prototype' property.
  if (a.prototype !== b.prototype) return false;

is incorrect. a and b are instances, not constructor functions, and as such, will never have prototypes set directly on them. Similar deep equality tests do the following instead:

  // a and b are on the same constructor chain
  if (a.constructor !== b.constructor) return false;

objEquiv should probably be changed similarly, although a case could be made that this test could be junked, seeing as how this hasn't been noticed until now.

[othiym23]

The simplest possible demonstration that this is a bug is that this doesn't throw:

var assert = require('assert')
assert.deepEqual([], {})

[bnoordhuis]

If you submit a pull request and include regression tests, I'll merge it.

[othiym23]

will do.

[medikoo]

It most likely should be:

if (a.__proto__ !== b.__proto__) return false;

[Lewuathe]

@medikoo Though __proto__ is supported by node.js, it is not ECMAScript standard. So I think it should not be put on node.js original source code.

[medikoo]

@Lewuathe __proto__ is already used in node.js original source code, and is already accepted for ECMAScript standard (wiil be standardized with version 6). Still if you want to be less controversial, you can go ECMAScript 5 way:

if (Object.getPrototypeOf(a) !== Object.getPrototypeOf(b)) return false;

[Lewuathe]

Oh, sorry, lack my knowledge. Thanks @medikoo.

[hackedy]

If you fix this, you break https://github.com/joyent/node/blob/master/test/simple/test-assert.js#L115 😖

[hackedy]

That's especially bad considering the API is locked

[isaacs]

Yeah, even if they do have a "prototype" prop, then it'll check later
anyway. Best to just remove that line, or ignore it. We explicitly don't
require same-class in many tests using deepEquals assertion.

Patch welcome.

On Friday, April 12, 2013, Ryan Doenges wrote:

That's especially bad considering the API is lockedhttp://nodejs.org/docs/latest/api/assert.html#assert_assert

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/4523#issuecomment-16328259
.

[hackedy]

Uh oh, removing if (a.prototype !== b.prototype) return false; breaks https://github.com/joyent/node/blob/master/test/simple/test-assert.js#L154

😦

[bnoordhuis]

Uhm, looks like this is still broken? What happened?

[jokeyrhyme]

I just noticed that this does NOT throw:

assert.deepEqual(Object.create({tea:'chai'}), Object.create({tea:'black'}))

Weird.

[cjihrig]

This is a shortcoming of assert.deepEqual(). It was fixed in io.js in nodejs/node@3f473ef by adding the assert.deepStrictEqual().

[avdg]

Can't reproduce in node v0.12.3, any need to keep this issue open?

[jasnell]

At this point someone could take the step of backporting nodejs/node@3f473ef to v0.12 if they feel it's necessary. But given that this is addressed in v3.x, I'm inclined to close this. Can reopen if necessary.