Comments (7)
@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
from qunit.
@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).
from qunit.
@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.
from qunit.
@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).
from qunit.
@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?
from qunit.
@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.
from qunit.
Thank you again for the detailed explanations! I'll go with propEqual
I guess, it suits my needs best. Thanks!
from qunit.
Related Issues (20)
- Great convergence effort/suggestion: Making QUnit proxy to node.js and deno testing APIs in these environments HOT 2
- Add DOM hook to allow links to be added after the Rerun link
- Allow non-escaped HTML to be used as an assertion message HOT 1
- importing .css file using webpack css-loader/style-loader causes: TypeError: Unknown file extension ".css" HOT 2
- Improve assert.async Function to Handle Type Checking
- Qunit v2 has incorrectly configured exports HOT 4
- Drop support for IE9-IE10 HOT 3
- Can we move this repo to a monorepo so we can more accurately test different usage scenarios? HOT 1
- Drop support for node < 18? HOT 1
- Can we drop builtin AMD support? HOT 6
- Can we start a `next` branch so I can start PRing improvements? HOT 1
- Let simple array data in test.each() serve as automatic labels
- Facilitate "close to" number equal assertion HOT 3
- [Feature Request]: Allow more customization of how errors are handled (especially uncaughtrejection). HOT 1
- Web Test Runner and QUnit reporting problems HOT 9
- qunit cannot parse private functions and properties HOT 2
- Support multiple `module` parameters (QUnit.config.module array) HOT 2
- Unify qunitjs.com and api.qunitjs.com
- [QUESTION] How can I forcefully abort a testcase and advance to next in queue HOT 1
- Let the test.each callback access the current case key HOT 3
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from qunit.