assert.deepEqual doing inadequate comparison

[joshwilsdon]

I was bitten originally by what I believe to be a bug in nodeunit (I will create a pull request there if behavior is changed in node). Looking further nodeunit got this because it appears to have copied the assert.js code from node. The issue I specifically ran into is that the following sorts of values are considered equal (tested on 0.10.24 on OS X):

> var assert = require('assert');
undefined
> assert.deepEqual('1', 1);
undefined
> assert.deepEqual('42', 42);
undefined
> assert.deepEqual(1, true);
undefined
> assert.deepEqual(0, false);
undefined
> assert.deepEqual({a: {b: [0, 1]}}, {a: {b: ["0", "1"]}});
undefined
> 

I then found that Mozilla had this problem previously with a Utils.deepEquals function as described in https://bugzilla.mozilla.org/show_bug.cgi?id=493363 so I modified the same test code from that bug, to work with node and adding 2 additional tests:

var assert = require('./assert');

d1 = [NaN, undefined, null, true, false, Infinity, 0, 1, "a", "b", {a: 1}, {a: "a"}, [{a: 1}], [{a: true}], {a: 1, b: 2}, [1, 2], [1, 2, 3], {a: "1"}, {a: "1", b: "2"}];
d2 = [NaN, undefined, null, true, false, Infinity, 0, 1, "a", "b", {a: 1}, {a: "a"}, [{a: 1}], [{a: true}], {a: 1, b: 2}, [1, 2], [1, 2, 3], {a: "1"}, {a: "1", b: "2"}];

d1.forEach(function(a) {
    var o = [];
    d2.forEach(function(b) {
        try {
            assert.deepEqual(a,b);
            // no error
            o.push("1");
        } catch (e) {
            o.push(" ");
        }
        // o.push(assert.deepEqual(a,b) ? 1 : " ");
    });
    console.log(o.concat([typeof (a), a, JSON.stringify([a])]).join(" "));
});

and found output:

                                      number NaN [null]
  1                                   undefined  [null]
    1                                 object  [null]
      1       1                       boolean true [true]
        1   1                         boolean false [false]
          1                           number Infinity [null]
        1   1                         number 0 [0]
      1       1                       number 1 [1]
                1                     string a ["a"]
                  1                   string b ["b"]
                    1             1   object [object Object] [{"a":1}]
                      1               object [object Object] [{"a":"a"}]
                        1 1           object [object Object] [[{"a":1}]]
                        1 1           object [object Object] [[{"a":true}]]
                            1       1 object [object Object] [{"a":1,"b":2}]
                              1       object 1,2 [[1,2]]
                                1     object 1,2,3 [[1,2,3]]
                    1             1   object [object Object] [{"a":"1"}]
                            1       1 object [object Object] [{"a":"1","b":"2"}]

which shows several things matching only because we're doing == instead of ===. Taking the assert.js from the v0.10 branch and applying this patch:

--- assert.js.orig  2014-02-20 23:00:58.000000000 -0800
+++ assert.js   2014-02-20 23:02:51.000000000 -0800
@@ -169,9 +169,9 @@
            actual.ignoreCase === expected.ignoreCase;

   // 7.4. Other pairs that do not both pass typeof value == 'object',
-  // equivalence is determined by ==.
+  // equivalence is determined by ===.
   } else if (typeof actual != 'object' && typeof expected != 'object') {
-    return actual == expected;
+    return actual === expected;

   // 7.5 For all other Object pairs, including Array objects, equivalence is
   // determined by having the same number of owned properties (as verified

the results become:

                                      number NaN [null]
  1                                   undefined  [null]
    1                                 object  [null]
      1                               boolean true [true]
        1                             boolean false [false]
          1                           number Infinity [null]
            1                         number 0 [0]
              1                       number 1 [1]
                1                     string a ["a"]
                  1                   string b ["b"]
                    1                 object [object Object] [{"a":1}]
                      1               object [object Object] [{"a":"a"}]
                        1             object [object Object] [[{"a":1}]]
                          1           object [object Object] [[{"a":true}]]
                            1         object [object Object] [{"a":1,"b":2}]
                              1       object 1,2 [[1,2]]
                                1     object 1,2,3 [[1,2,3]]
                                  1   object [object Object] [{"a":"1"}]
                                    1 object [object Object] [{"a":"1","b":"2"}]

which seems correct. Back to my original examples after applying this change, they also fail as I'd expect:

> var assert = require('./assert.js');
undefined
> assert.deepEqual('1', 1);
AssertionError: "1" deepEqual 1
    at repl:1:8
    at REPLServer.self.eval (repl.js:110:21)
    at repl.js:249:20
    at REPLServer.self.eval (repl.js:122:7)
    at Interface.<anonymous> (repl.js:239:12)
    at Interface.EventEmitter.emit (events.js:95:17)
    at Interface._onLine (readline.js:202:10)
    at Interface._line (readline.js:531:8)
    at Interface._ttyWrite (readline.js:760:14)
    at ReadStream.onkeypress (readline.js:99:10)
> assert.deepEqual('42', 42);
AssertionError: "42" deepEqual 42
    at repl:1:8
    at REPLServer.self.eval (repl.js:110:21)
    at repl.js:249:20
    at REPLServer.self.eval (repl.js:122:7)
    at Interface.<anonymous> (repl.js:239:12)
    at Interface.EventEmitter.emit (events.js:95:17)
    at Interface._onLine (readline.js:202:10)
    at Interface._line (readline.js:531:8)
    at Interface._ttyWrite (readline.js:760:14)
    at ReadStream.onkeypress (readline.js:99:10)
> assert.deepEqual(1, true);
AssertionError: 1 deepEqual true
    at repl:1:8
    at REPLServer.self.eval (repl.js:110:21)
    at repl.js:249:20
    at REPLServer.self.eval (repl.js:122:7)
    at Interface.<anonymous> (repl.js:239:12)
    at Interface.EventEmitter.emit (events.js:95:17)
    at Interface._onLine (readline.js:202:10)
    at Interface._line (readline.js:531:8)
    at Interface._ttyWrite (readline.js:760:14)
    at ReadStream.onkeypress (readline.js:99:10)
> assert.deepEqual(0, false);
AssertionError: 0 deepEqual false
    at repl:1:8
    at REPLServer.self.eval (repl.js:110:21)
    at repl.js:249:20
    at REPLServer.self.eval (repl.js:122:7)
    at Interface.<anonymous> (repl.js:239:12)
    at Interface.EventEmitter.emit (events.js:95:17)
    at Interface._onLine (readline.js:202:10)
    at Interface._line (readline.js:531:8)
    at Interface._ttyWrite (readline.js:760:14)
    at ReadStream.onkeypress (readline.js:99:10)
> assert.deepEqual({a: {b: [0, 1]}}, {a: {b: ["0", "1"]}});
AssertionError: {"a":{"b":[0,1]}} deepEqual {"a":{"b":["0","1"]}}
    at repl:1:8
    at REPLServer.self.eval (repl.js:110:21)
    at repl.js:249:20
    at REPLServer.self.eval (repl.js:122:7)
    at Interface.<anonymous> (repl.js:239:12)
    at Interface.EventEmitter.emit (events.js:95:17)
    at Interface._onLine (readline.js:202:10)
    at Interface._line (readline.js:531:8)
    at Interface._ttyWrite (readline.js:760:14)
    at ReadStream.onkeypress (readline.js:99:10)
> 

Any chance that this could get applied? Looking at master, it seems the issue still exists there as well though the typeof check has changed to using util.isObject instead so the patch won't apply as-is. It appears to be the same 1 character (and comment) fix there though.

[litmit]

As I think needed counterpart to assert.deepEqual new function assert.deepStrictEqual.

Other test case that wrong for me:
assert.deepEqual(new Array(10),new Array(100))
does not throw.

I hope that assert.deepStrictEqual(new Array(10),new Array(100)) will throw.

[indutny]

assert module has stability 5 - locked and is implemented upon CommonJS standard: http://wiki.commonjs.org/wiki/Unit_Testing/1.0 . If you have any ideas - please try submitting them to CommonJS guys first!

[tjfontaine]

I think we could add a deepStrictEqual without much pain and suffering. Assigned to me, milestone 0.12

[Sannis]

Can we mark deepStrictEqual with different freeze level to make changes to it name if CommonJS will include it to spec?

[loveencounterflow]

as i see it, the CommonJS specs have a fault here—comparing two values with == is known to be "too surprising to qualify as a Good Part of JS".

even Array::indexOf uses === instead of ==, and the Principle of Least Surprise would dictate that two arrays cannot be considered 'equal' when x.indexOf(123) !== y.indexOf(123), yet assert.deepEqual(x,y) would have you believe so for pairs like x = [ 123 ]; y = [ '123' ].

considering that NodeJS assert is specifically there to perform data sanity checks, it is somewhat appalling to think of the consequences—i myself just found out about this glitch when getting illogical test reports for my module. wonder how many modules are out there that pass all tests but use a flawed testing software to do so.

i'm fully expecting someone to chime in once more with "it's frozen / 5 / locked / works as standardized", but i believe the standard is broken, NodeJS should fix that bit, and update the docs to tell people about the deviation.

and yes, such a change will certainly break many tests, but then those test suites used to fail silently before, so that's a good thing. if anyone should disagree, please point out what useful properties a non-throwing assert.deepEqual( [ 123 ], [ '123' ] ) could have in the real world.

[loveencounterflow]

to further encourage people to take it seriously when users say that assert.deepEqual is broken, three more test cases with highly unintuitive, falsely positive results:

  assert.deepEqual( 3, '3' )
  assert.deepEqual( [], {} )
  assert.deepEqual( [{}], [[]] )

the first could conceivably fail with an exception to the effect that deepEqual is not responsible for primitive values; however, that would be somewhat strange given that assert.deepEqual( 3, [ 3 ] ) is a valid usecase for this method. but it shouldn't throw, as that would look like 'the test has failed' instead of 'you used the wrong method'. ergo assert.deepEqual( 3, '3' ) must throw a "not equal" exception, and assert.deepEqual( 3, 3 ) must remain silent; this means deepEqual as a method that is separate from equal is logically flawed in the context of the assert module. the two methods should really be one.

the very least action that should be taken is to amend the docs; they currently claim that assert.depEqual "Tests for deep equality." which is demonstrably wrong. this somewhat flies into the face of a remark i made on stackoverflow: "nerds will try to tell you to read the docs, but marketing people will tell you when you sell a product that has 'deepEqual' written all over it and 'coercive equality' in the fine print, they'll still buy."—the point here is that yes, those nerds are in principle right, but in practice wrong (you shouldn't write hard-to-read-docs and then require people to fully absorb them), and, second, even reading the entire docs for assert does not tell you that assert is an implementation of the CommonJS specs, and what that means for assert.deepEqual.

i suggest to deprecate all affected equality-testing methods in red, then go and add an assert.eq method that gets all the green fat pointers so people start using it. as for an implementation—i'm currently using https://github.com/jkroso/equals, which apparently works (even for primitive values and circular references). should be easy to add and put to test.

a further suggestion is to put a non-throwing xxx.eq method into some module (util is the natural candidate) for all the use cases that require sane equality tests, and have the throwing assert.eq method rely on xxx.eq.

[seishun]

Ideally the assert module would follow CommonJS spec and mention it. The problem is that the spec is ambiguous and incomplete.

7.3. Other pairs that do not both pass typeof value == "object", equivalence is determined by ==.

Does this apply to any pair where either value fails the typeof value == "object" check? Or does it mean that both values must fail the check? In the former case, assert.deepEqual(5, new Number(5)) should pass since 5 == new Number(5), which is what QUnit does but not node. In the latter case, we get to the next point.

7.4. For all other Object pairs, including Array objects, <...>

This implies that all the remaining pairs are object pairs, which is incorrect no matter how the previous point is interpreted. For example, null and {} both pass typeof value == "object", but it's not an object pair. And what should happen when the pair doesn't fall under any of the categories? Should it throw, or maybe it's undefined behavior?

I'm surprised that whoever wrote and approved these specs didn't think of such obvious edge cases. I would suggest explicitly not following CommonJS and just doing whatever makes more sense.

[loveencounterflow]

@seishun "I would suggest explicitly not following CommonJS and just doing whatever makes more sense."—i agree. there's good precedence in the JS community to implement standards because they're standards, not because they're particularly good standard. V8 does that with the ES specs, and at least you know what you get is pretty much 100% EcmaScript, not something vaguely similar like JScript.

that said, it's up for discussion whether the relevant specs for assert are so important and 'good enough' so as to build a NodeJS core module after that model. if so, then the NodeJS docs should point out that fact (and ideally warn against pitfalls).

i tend to lean towards the observation that (1) specs are not a God-given, they're man-made; (2) this particular part is very problematic; therefore (3) NodeJS should deliver a better implementation together with a set of rules that can in turn convince the CommonJS folks to turn around their cart for a significant improvement in their specs, v2.

it just makes little sense to promote problematic software just because someone sat down and published a piece called 'the standard'. with JS that was a different story, and it's history. still, had they early on decided to ditch the earlier interpretation of == for what then was bolted on as ===, JS would be a little better today. maybe that breaking change would have weakened the progress of JS, maybe it would have benefited from it. it would be a trouble long past either way where it is a lingering spectre as it stands: the CommonJS specs should never use == at all, except to warn against its use, and except for a very small number of cases like x == null where explicitly meant to match both null and undefined.

your example 5 vs new Number( 5 ) is a hairy one. it can only meaningfully discussed with an agreement of how to determine types in JS. i suggest to do

js_type_of = function(x) {
  return Object.prototype.toString.call(x);
};

isa_number = function(x) {
  return (js_type_of(x)) === '[object Number]' && isFinite(x);
};

which yields true for both isa_number( 5 ) and isa_number( new Number( 5 ) ). someone will probably come and point out that the one is a primitive value and the other an object, but then that distinction is a pretty thorny issue as JS VMs go to lengths to insulate people from that detail. in fact, V8 gives false for 5 === new Number( 5 ), but true for 0 + 5 === 0 + new Number( 5 ), so you might say it's a little split on the issue.

i think we should be pragmatic and say that both 5 and new Number( 5 ) are both numbers (for purposes of equality testing), and that NaN and Infinity are not numbers in this sense (since they're not finite). the unholy primitive / object distinction here is really a relic from earlier models of how OOP should work and serves no practical purpose at all. Java made the mistake of letting that implementation detail creep into the language proper, let us not repeat that mistake.

sound deep and shallow equality testing is very much at the heart of every test-driven quality software, so let's do it right.

[vkurchatkin]

@seishun

Ideally the assert module should follow CommonJS spec and mention it.

I would suggest explicitly not following CommonJS

so, which one?

[seishun]

@vkurchatkin I mean in an ideal world it would follow CommonJS spec, but the spec is far from ideal.