Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Check __proto__ instead of .prototype in assert.deepEqual() #5288

Closed
wants to merge 1 commit into from

3 participants

@hackedy

Fixes joyent/node#4523, but that might not be the right thing to do here.

A test case for the assert module depends on the original bug. I remove it with this patch, but perhaps removing the prototype check altogether and leaving the test case and bug as they currently stand is a good idea. The docs do say the assert API is locked.

@Nodejs-Jenkins
Collaborator

Thank you for contributing this pull request! Here are a few pointers to make sure your submission will be considered for inclusion.

Commit hackedy/node@4f141d2 has the following error(s):

  • First line of commit message must be no longer than 50 characters
  • Commit message line too long: 2
  • Commit message line too long: 3
  • Commit message line too long: 4

The following commiters were not found in the CLA:

  • Ryan Doenges

Please see CONTRIBUTING.md for more information

@hackedy hackedy assert: actually check prototype with deepEqual()
Originally, assert.deepEqual() checked whether the two objects'
prototypes were equal by doing `a.prototype === b.prototype`. `a` and
`b` are instances, not constructors, which means their `.prototype` will
always be `undefined` anyway.  This makes the conditional useless.

Use `.__proto__` instead to access the real constructor.

Fixes joyent/node#4523.
617eb9a
@hackedy

Sorry for the noise, forgot to wrap my text at 72 columns.

@isaacs

Can't do that for the reason given in #4523. Breaking change to a locked API.

@isaacs isaacs closed this
@hackedy hackedy referenced this pull request
Closed

Lib/assert #5301

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Apr 13, 2013
  1. @hackedy

    assert: actually check prototype with deepEqual()

    hackedy authored
    Originally, assert.deepEqual() checked whether the two objects'
    prototypes were equal by doing `a.prototype === b.prototype`. `a` and
    `b` are instances, not constructors, which means their `.prototype` will
    always be `undefined` anyway.  This makes the conditional useless.
    
    Use `.__proto__` instead to access the real constructor.
    
    Fixes joyent/node#4523.
This page is out of date. Refresh to see the latest.
Showing with 5 additions and 2 deletions.
  1. +1 −1  lib/assert.js
  2. +4 −1 test/simple/test-assert.js
View
2  lib/assert.js
@@ -202,7 +202,7 @@ function objEquiv(a, b) {
if (isUndefinedOrNull(a) || isUndefinedOrNull(b))
return false;
// an identical 'prototype' property.
- if (a.prototype !== b.prototype) return false;
+ if (a.__proto__ !== b.__proto__) return false;
//~~~I've managed to break Object.keys through screwy arguments passing.
// Converting to array solves the problem.
if (isArguments(a)) {
View
5 test/simple/test-assert.js
@@ -112,7 +112,6 @@ assert.doesNotThrow(makeBlock(a.deepEqual, {a: 4, b: '2'}, {a: 4, b: '2'}));
assert.doesNotThrow(makeBlock(a.deepEqual, [4], ['4']));
assert.throws(makeBlock(a.deepEqual, {a: 4}, {a: 4, b: true}),
a.AssertionError);
-assert.doesNotThrow(makeBlock(a.deepEqual, ['a'], {0: 'a'}));
//(although not necessarily the same order),
assert.doesNotThrow(makeBlock(a.deepEqual, {a: 4, b: '1'}, {b: '1', a: 4}));
var a1 = [1, 2, 3];
@@ -293,3 +292,7 @@ try {
assert.equal(e.message, 'Missing expected exception..');
}
assert.ok(threw);
+
+// #4523
+assert.throws(makeBlock(a.deepEqual, [], {}), a.AssertionError, 'deepEqual prototypes');
+assert.doesNotThrow(makeBlock(a.deepEqual, [], []));
Something went wrong with that request. Please try again.