-
Notifications
You must be signed in to change notification settings - Fork 781
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
Core: Ensure inherited getters are not invoked by assert.deepEqual #1326
base: main
Are you sure you want to change the base?
Conversation
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.
5b7b877
to
54aad99
Compare
Hmm, weird, CI is done (and green see here) but still shows as in progress in the GitHub UI. |
) || ( | ||
|
||
// Skip shared inherited functions | ||
hasSharedDescriptor( a, b, i ) |
There was a problem hiding this comment.
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.
|
||
function hasSharedDescriptor( a, b, keyName ) { | ||
var aDescriptor = lookupDescriptor( a, keyName ); | ||
var bDescriptor = lookupDescriptor( b, keyName ); |
There was a problem hiding this comment.
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.
I'll do the close/re-open trick here to make Travis CI re-valuate this PR. |
I agree with @gibson042 that treating these as equal is not obviously better and indeed has potential to wrongly mask other genuine cases and/or cause a different kind of confusion instead. I'm closing this for now, but I do want to acknowledge that #1325 is a bug that should be fixed. I'll respond there with some ideas. |
@Krinkle weirdly, I just hit this issue today. I was surprised by the behavior and it took me quite some time to track down. I would be pretty happy with a mode that required me to specify a comparison function for instances of specific classes. |
@wycats Could you elaborate a bit with an example or pointer to your test? Was it a case of something where you'd like to be able to succesfully compare as equal to lazily-initialised recursive structures, or was it case were something accidentally made it into the comparison and you'd like QUnit to surface that quickly and clearly rather than with a timeout? |
Previously, given the following:
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.
Fixes #1325