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.

Sign up for GitHub

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

Jump to bottom

Core: Ensure inherited getters are not invoked by assert.deepEqual #1326

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+59 −5
Open

Core: Ensure inherited getters are not invoked by assert.deepEqual #1326

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 44 additions & 5 deletions src/equiv.js
View file
Open in desktop
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 );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This ignores where on the prototype chain the keyName property is defined when checking if it is "shared"... if that is intentional, then it should be documented and tested.


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 )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having an identical descriptor does not make two properties equal. Consider the following:

let Identifiable = (function() {
	let ids = new WeakMap(), lastId = 0;
	class Identifiable {
		constructor() {
			ids.set(this, ++lastId);
		}
		get id () {
			return ids.get(this);
		}
	};
	Object.defineProperty(Identifiable.prototype, "id", {enumerable:true});
	return Identifiable;
})();

var a = new Identifiable();
var b = new Identifiable();
QUnit.equal(a.id, 1);
QUnit.equal(b.id, 2);
QUnit.deepEqual(a, b, "Identifiable{id: 1} deep-equals Identifiable{id: 2} ?!?");

But you have a use case where that doesn't matter. It's not wrong per se, I just see no reason why QUnit should adopt it as a general position. This is one of many opinionated esoteric decisions that I'd rather not bake in to deepEqual, which is why I opened #1327.

)
)
) {
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
View file
Open in desktop
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