Skip to content

Commit

Permalink
Core: Ensure inherited getters are not invoked by assert.deepEqual.
Browse files Browse the repository at this point in the history
Previously, given the following:

```js
class Foo {
  constructor(a = 1) {
    this.a = a;
  }
}

Object.defineProperty(Foo.prototype, 'b', {
  enumerable: true,
  get() {
    return new Foo(this.a + 1);
  }
})

QUnit.test( "hello test", function( assert ) {
  assert.deepEqual(new Foo(), new Foo());
});
```

The `assert.deepEqual` invocation would never complete (ultimately
crashing the tab or node process).

The changes here ensure that inherited descriptors (getter / setter _or_
value only) are compared without invoking the getter therefore
preventing the issue mentioned above.
  • Loading branch information
rwjblue committed Oct 24, 2018
1 parent 112d782 commit 54aad99
Show file tree
Hide file tree
Showing 2 changed files with 59 additions and 5 deletions.
49 changes: 44 additions & 5 deletions src/equiv.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,31 @@ export default ( function() {
return obj.__proto__;
};

function lookupDescriptor( obj, keyName ) {
let current = obj;
do {
const descriptor = Object.getOwnPropertyDescriptor( current, keyName );
if ( descriptor !== undefined ) {
return descriptor;
}
current = getProto( current );
} while ( current !== null );
return null;
}

function hasSharedDescriptor( a, b, keyName ) {
var aDescriptor = lookupDescriptor( a, keyName );
var bDescriptor = lookupDescriptor( b, keyName );

return ( aDescriptor !== null && bDescriptor !== null ) &&
aDescriptor.value === bDescriptor.value &&
aDescriptor.get === bDescriptor.get &&
aDescriptor.set === bDescriptor.set &&
aDescriptor.writable === bDescriptor.writable &&
aDescriptor.configurable === bDescriptor.configurable &&
aDescriptor.enumerable === bDescriptor.enumerable;
}

function useStrictEquality( a, b ) {

// This only gets called if a and b are not strict equal, and is used to compare on
Expand Down Expand Up @@ -265,16 +290,30 @@ export default ( function() {
aProperties.push( i );

// Skip OOP methods that look the same
var hasValidConstructor = a.constructor !== Object &&
typeof a.constructor !== "undefined";

if (
a.constructor !== Object &&
typeof a.constructor !== "undefined" &&
typeof a[ i ] === "function" &&
typeof b[ i ] === "function" &&
a[ i ].toString() === b[ i ].toString()
hasValidConstructor &&
(
(

// Skip own functions with same definition
hasOwnProperty.call( a, i ) &&
typeof a[ i ] === "function" &&
typeof b[ i ] === "function" &&
a[ i ].toString() === b[ i ].toString()
) || (

// Skip shared inherited functions
hasSharedDescriptor( a, b, i )
)
)
) {
continue;
}


// Compare non-containers; queue non-reference-equal containers
if ( !breadthFirstCompareChild( a[ i ], b[ i ] ) ) {
return false;
Expand Down
15 changes: 15 additions & 0 deletions test/main/deepEqual.js
Original file line number Diff line number Diff line change
Expand Up @@ -1664,6 +1664,21 @@ QUnit.test( "Compare structures with multiple references to the same containers"
assert.equal( QUnit.equiv( { big: x, z: [ true ] }, { big: y, z: [ false ] } ), false, "Nonequivalent structures" );
} );

QUnit.test( "Compare instances with getters", function( assert ) {
function Foo( a ) {
this.a = a === undefined ? 1 : a;
}

Object.defineProperty( Foo.prototype, "b", {
enumerable: true,
get() {
return new Foo( this.a + 1 );
}
} );

assert.deepEqual( new Foo(), new Foo(), "inherited getters are not included in computation" );
} );

QUnit.test( "Test that must be done at the end because they extend some primitive's prototype",
function( assert ) {

Expand Down

0 comments on commit 54aad99

Please sign in to comment.