[2.19.2] Regression with deepEqual on hashes

[boris-petrov]

Tell us about your runtime:

• QUnit version: 2.19.2
• Which environment are you using? (e.g., browser, Node): Node
• How are you running QUnit? (e.g., QUnit CLI, Grunt, Karma, manually in browser): ember-cli

Node v18.11.0

What are you trying to do?

Code that reproduces the problem:

const hash = { a: 1 };
assert.deepEqual(new Proxy(hash, {}), hash);

What did you expect to happen?

The assert to pass

What actually happened?

It doesn't.

That's a change since 2.19.1. I'm not sure it's a bad change - that is, perhaps that's "correct", but it is a breaking change which I'm not sure you wanted to introduce. Just wanted to mention it.

[Krinkle]

@boris-petrov Thanks for filing this issue. I agree it would be inappropiate in a patch release and not intended indeed.

I'm not yet able to reproduce the issue in isolation.

QUnit.module('example', function () {
	QUnit.test('test', function (assert) {
		const hash = { a: 1 };
		const prox = new Proxy(hash, {});
		assert.deepEqual(prox, hash);
		assert.deepEqual(hash, prox);
	});
});

• Passes in QUnit 2.19.1 and QUnit 2.19.2 in latest Firefox and Chrome, via
  https://codepen.io/Krinkle/pen/WNJWyxx?editors=1100 (temporarily edit the "HTML" panel in CodePen to switch versions).
• Passes via QUnit 2.19.2, using the QUnit CLI to run qunit example.js, on Node.js v18.11.0 (macOS), Node.js v18.4.0 (macOS), and v16.16.0 (Linux).

[Krinkle]

@boris-petrov Could you share the CLI output that shows the failure? I'm hoping that the way it describes the difference between actual/expected might point to where this is going wrong.

[boris-petrov]

@Krinkle thanks for taking the time to look into the issue! And sorry I didn't add a complete reproduction. It wasn't only the proxy part:

class ProxyObject {
  static of(obj) {
    return new Proxy(obj, {
      getPrototypeOf() {
        return ProxyObject.prototype;
      },
    });
  }
}

QUnit.module('example', function() {
  QUnit.test('test', function(assert) {
    const hash = { a: 1 };
    const prox = ProxyObject.of(hash);
    assert.deepEqual(prox, hash);
    assert.deepEqual(hash, prox);
  });
});

This reproduces the problem. It's because of getPrototypeOf. With it, this test fails on 2.19.2 (but passes on 2.19.1). Without it it passes on both versions (as you've seen with your test).

[Krinkle]

@boris-petrov Thanks. I found the problem, quite subtle. I failed to notice the difference during code review of #1700.

It's intentional and documented that assert.deepEqual requires A and B to be instances of the same exact class. In other words, being an instance of a different class (even a subclass) is considered not equal.

QUnit does offer easy assertion of own properties only, which lets you compare a complex object using an object literal which we effectively downcast to plain object for comparison purposes. This is known as assert.propEqual and maybe useful for your test case in the long-term.

Having said all that, this is still an unforeseen regression and we'll fix it as such. Below is how we compared inherited prototypes ("the class") before, upto 2.19.1 (simplified from source)

(a, b) {
    let protoA = Object.getPrototypeOf(a);
    let protoB = Object.getPrototypeOf(b);
    // Special case: Consider null-prototype object from Object.create(null) equal to plain object
    if (…) {
      return true;
    }

    return (a.constructor === b.constructor);
  }

And after, in 2.19.2 (simplified from source):

(a, b) {
    let protoA = Object.getPrototypeOf(a);
    let protoB = Object.getPrototypeOf(b);
    // Special case: Handle null-prototype case
    if (…) {
      return true;
    }

    return (protoA.constructor === protoB.constructor);
}

We're now comparing xProto.constructor instead of x.constructor. I actually think both of these are wrong, as we should be comparing xProto to determine confidently and quickly by reference whether they inherit the same prototype. This is also how the instanceof operator works in JavaScript. x instanceof Foo works by effectively checking that getPrototypeOf(x) === Foo.prototype, with a loop to effectively keep walking up x like getProto(getProto((x)) === Foo.prototype until we either find a match or until we reach the null prototype.

The constructor property, while conventionally a way to signal how to create similar objects and what name to use for logging purposes, doesn't really factor into any this. It doesn't control inheritence, doesn't decide which constructor function runs during new operator, doesn't affect instanceof, doesn't affect getProto, etc.

In QUnit 3.0 (\cc @izelnakri), after a bit of research into what other assertion libraries do, I think we might want to go for comparing strictly the result of getProto and not look at the constructor property. We can afford to not compare the constructor property explicitly, as it'll naturally come up when we compare properties of A and B — after establishing that they are instances of the same prototype — which is indeed possible (example below).

For now I'll restore what we did before, whilst trying to keep the performance optimisation that 2.19.2 introduced. handling we had before.

The below example shows all these silly edge cases at the same time:

class Foo1{}
class Foo2{}
class Foo3{}

class Quux {
  constructor() {
    this.constructor = (Math.random() < 0.5 ? Foo1 : Foo2);
  }
}
console.log(Quux.prototype.constructor); // Quux
Quux.prototype.constructor = Foo3;

x = new Quux();
console.log(x instanceof Foo1); // false
console.log(x instanceof Foo2); // false
console.log(x instanceof Foo3); // false
console.log(x instanceof Quux); // true
console.log(x.constructor); // Foo1 or Foo2

[boris-petrov]

@Krinkle thank you for the awesome support!

I managed to fix my tests using propEqual as per your recommendation (so now I'm using 2.19.2). There is one question remaining however - how do I know whether to use deepEqual or propEqual once I upgrade to 2.19.3 which would have "fixed" this issue? I mean without having to think too hard about the code at hand - i.e. whether the two things are of different types. Because it will be simpler for me to just write deepEqual but that would fail in QUnit 3. So perhaps some flag could be introduced (that restores the behavior of 2.19.2/3.x for deepEqual) or something? Any other ideas?

[Krinkle]

@boris-petrov In general, if you're not concerned with re-creating the exact inheritence for your actual value and want to assert the observed properties only, then propEqual should work on all recent, current, and future versions.

If you expect there to be an inherited prototype chain that you want to validate as well, then deepEqual is your friend.

Which one you pick is up to you. Sometimes it's obvious which one to pick. Other times it may be a trade-off between what's convenient to reproduce in a test context, and what your applications' promised behaviour is. For example, in the below example, let's say Coord is a public class that you expose directly to downstream users, has useful methods, and it requires no dependencies to construct. Here it's easy to re-create the exact value you expect, and so you can get the bonus of deeper validation (e.g. if it returned a different class with x and y props, the test would fail).

function Coord(x, y) {
  this.x = x;
  this.y = y;
}

function getHomeCoord() {
  return new Coord(4, 10);
}

assert.deepEqual(getHomeCoord(), new Coord(4, 10));

On the other hand, if Coord is a private class, then it's a little hard to do this. Perhaps the only documented API is that getHomeCoord will return an object with x and y properties, and the rest are private methods or other implementation details that benefit the upstream maintainer only. In that case, either because you can't construct Coord or because it's not important, you might go for propEqual instead and not worry about the prototype:

assert.propEqual(getHomeCoord(), { x: 4, y: 10 });

Another reason to use propEqual might be if the class in question is difficult to instantiate where it can be a quicker alternative to elaborate dependency mocking. There is also assert.propContains() if you need to skip certain internal properties that don't need to be asserted.

/** @private */
function Position(grid, player, x, y, secret) {
  this.x = x;
  this.y = y;
}

function getSpecialPos() {
  return new Position(private.grid, private.player, 4, 10, private.token);
}

const pos = getSpecialPos();
assert.propEqual(pos, { x: 4, y: 10 });

Long story short, I think for your case, assert.propEqual() might be the right fit regardless of this bug. It seems your intention is to compare objects that are effectively plain objects with no (publicly significant) inheritence for deepEqual to look at, or possibly you're not looking to re-create the exact way the object was constructed, but compare its observed properties only. Those cases are precisely what propEqual is for, and also helps you to keep the contract flexible in the future.

E.g. you might one day decide to promote an object literal to a class, or introduce a proxy to help with deprecation warnings. That shouldn't be observable to normal code, but would be observable to the test if you use deepEqual. Some developers will prefer a failing test if behaviour changes, and then often change tests to match new code. Others prefer to have the test only assert the public contract, and thus only fail or require changes in tests if a semver-breaking change is made. If you prefer the former of those two, then you might want to use deepEqual more often.

[boris-petrov]

Thank you again for the detailed explanations! I'll go with propEqual I guess, it suits my needs best. Thanks!