Automattic / expect.js

Minimalistic BDD-style assertions for Node.JS and the browser.
2.11k stars 209 forks source link

fix "sort of comparison" for dom nodes #37

Open givankin opened 12 years ago

givankin commented 12 years ago

expect.eql failed to perform "sort of comparison" for DOM nodes, always returning true for nodes even if they were of different types (say, text node and div). I stumbled upon this recently when saw this test passing:

var div = document.createElement('div');
var a = document.createTextNode('a');
var b = document.createTextNode('b');
div.innerHTML = '<c></c><d></d>';
var c = div.firstChild;
var d = div.lastChild;
expect([a, b]).to.be.eql([c, d]);

After digging through expect.js code with breakpoints and console.logs I understood that basically the CommonJS Unit Testing 1.0 spec implemented by expect.js has no special algo for comparing nodes. So they are treated as mere objects and as such are considered "sort of equal" because their prototypes are undefined and their keys are empty (in fact, sometimes Firefox returns "constructor" as as a result of keys check, in this case this works better, but this unreliable and insufficient and also quite strange).

What I've implemented is deep for-in check for nodes (this in fact is what QUnit does and why comparing nodes with deepEqual works well there).

P. S. Sorry for multiple commits, I'm a github noob. If this needs to be cleaned up, I can create another pull request.

skeggse commented 10 years ago

@abbakym could you squash these commits? It's pretty straightforward.

More info about git and rebasing/squashing:

givankin commented 10 years ago

@skeggse done, thanks!

truongsinh commented 10 years ago

@abbakym What browser and version were you using? I cannot reproduce the error with your test case

givankin commented 10 years ago

@truongsinh, I see this with latest expect.js in all modern browsers (re-checked in FF just now, Chrome and IE choke on the Fiddle for some reason). See http://jsfiddle.net/Hq8eK/1/. Just open the console and see:

Error: expected [ {}, {} ] to sort of not equal [ <c></c>, <d></d> ]

Note that the test in my initial comment will pass, but it shouldn't do so. Maybe this confused you.

truongsinh commented 10 years ago

"for some reason" is that you should change https://raw2.github.com/LearnBoost/expect.js/master/index.js to https://rawgithub.com/LearnBoost/expect.js/master/index.js . Long story short, security mime. After that it works fine on Chrome. I'll try FF in a minute.

truongsinh commented 10 years ago

Some thing todo with objEquiv, which is taken from Nodejs' util i guess