assert.deepStrictEqual is fooled by strange dates

[akdor1154]

• Version: v6.9.1
• Platform: OS X
• Subsystem: assert

assert.deepStrictEqual (and strictEqual) is fooled by the following code:

function FakeDate() {};
FakeDate.prototype = Date.prototype;
const f = new FakeDate();
const d = new Date();

f.toString();
//TypeError: Method Date.prototype.toString called on incompatible receiver [object Object]

d.toString();
//'Wed Dec 14 2016 09:53:28 GMT+1100 (AEDT)'

assert.deepStrictEqual(d, f);
// does not throw any error

I think it's reasonable to expect that assert.deepStrictEqual should consider d and f as unequal.

I have not tested on Node master, however there are no relevant changes in assert.js between 6.9.1 and master.

[joyeecheung]

Tested on master, for strictEqual I get

assert.js:85
  throw new assert.AssertionError({
  ^
AssertionError: 2016-12-14T04:59:37.022Z === Date {}
    at Object.<anonymous> (/Users/joyee/projects/node-test/fake-date.js:16:8)
    at Module._compile (module.js:571:32)
    at Object.Module._extensions..js (module.js:580:10)
    at Module.load (module.js:488:32)
    at tryModuleLoad (module.js:447:12)
    at Function.Module._load (module.js:439:3)
    at Module.runMain (module.js:605:10)
    at run (bootstrap_node.js:420:7)
    at startup (bootstrap_node.js:139:9)
    at bootstrap_node.js:535:3

But deepStrictEqual does not throw indeed(I think you mean deepEqual & deepStrictEqual?). That's because Dates are not primitive values and don't have their own properties, and here f and d have the same [[Prototype]]. I think this behavior does not contradict what the doc says

assert.deepEqual(actual, expected[, message])

Tests for deep equality between the actual and expected parameters. Primitive values are compared with the equal comparison operator ( == ).
Only enumerable "own" properties are considered. The deepEqual() implementation does not test object prototypes, attached symbols, or non-enumerable properties.

assert.deepStrictEqual(actual, expected[, message])

Generally identical to assert.deepEqual() with two exceptions. First, primitive values are compared using the strict equality operator ( === ). Second, object comparisons include a strict equality check of their prototypes.

But in this case the more reasonable behavior should throw IMHO. I can open a PR adding a special case in assert it that seems reasonable to others.

[Trott]

I can open a PR adding a special case in assert it that seems reasonable to others.

I'm open to being persuaded on this, but given what we have here so far, I'm not convinced this is a bug.

The two objects have identical properties, including the toString() function. The toString() function is itself sufficiently aware of its context to throw an error on the FakeDate object. But that's a C++-land check. The toString() functions are in fact identical, even though the result of calling it are different on the two objects.

This seems basically the same as this:

const assert = require('assert');

function Foo() {}
function Bar() {}
Bar.prototype = Foo.prototype;

const f = new Foo();
const b = new Bar();

assert.deepStrictEqual(f, b);

If we decide that Date is special in this regard and should be treated differently than other objects, is there an easy way to distinguish between the situation described in this issue and a more prosaic case where two Date objects should be deepStrictEqual? I guess we can run toString() and wrap it in a try/catch, but that seems problematic.

[akdor1154]

It's not quite the same as your example, as d has actual date information that f lacks (in my example, the Date constructor is never called. If f was initialised with super() then the examples would be more comparable). I guess the issue is that some types have important "comparable" data that is not directly accessible to js. I haven't tested, but I guess that the primitive wrapper objects (String etc) might show similar annoying behaviour.

#valueOf()seems to be a standard way of getting a comparable value out of such objects (i.e. not just Date), might it be worth calling this on comparisons?

@joyeecheung, yep I meant deepEqual, sorry.

[joyeecheung]

Just did a quick research across some popular assertion libraries that I can think of(omitted power-assert because IIRC they just reuse assert underneath). Turns out everybody throws for this...expect Node core.

'use strict';
const assert = require('assert');
const chaiAssert = require('chai').assert;
const chaiExpect = require('chai').expect;
const expect = require('expect.js');
const should = require('should');
const codeExpect = require('code').expect;
const mustExpect = require('must');
const unexpectedExpect = require('unexpected');

function FakeDate() {};
FakeDate.prototype = Date.prototype;

const d = new Date();
const f = new FakeDate();

// to make sure I use these libs properly
// const d = { a: 1, b: 2 };
// const f = { a: 1, b: 2 };

let tests = {
  'assert': () => assert.deepEqual(d, f),
  'chaiAssert': () => chaiAssert.deepEqual(d, f),
  'should': () => should.deepEqual(d, f),
  'codeExpect': () => codeExpect(d).to.equal(f),
  'chaiExpect': () => chaiExpect(d).to.deep.equal(f),
  'mustExpect': () => mustExpect(d).to.eql(f),
  'unexpectedExpect': () => unexpectedExpect(d, 'to equal', f),
  'expect': () => expect(d).to.eql(f)
}

for (const name in tests) {
  try {
    tests[name]();
    console.log('\n### ' + name + ' does not throw');
  } catch(e) {
    console.log('\n### ' + name + ' throws:');
    console.log(e.message);
  }
}

The results are:

### assert does not throw

### chaiAssert throws:
expected Wed, 14 Dec 2016 05:58:25 GMT to deeply equal {}

### should throws:
expected 2016-12-14 13:58:25.447 +0800 deepEqual Date {}

### codeExpect throws:
this is not a Date object.

### chaiExpect throws:
expected Wed, 14 Dec 2016 05:58:25 GMT to deeply equal {}

### mustExpect throws:
2016-12-14T05:58:25.447Z must be equivalent to {}

### unexpectedExpect throws:

expected new Date('Wed, 14 Dec 2016 05:58:25.447 GMT') to equal Date({})


### expect throws:
this is not a Date object.

[joyeecheung]

@Trott And for the Foo Bar example nobody throws. So other libraries treat Dates somewhat differently I guess? I think it's more about making sure fake dates are not equal to a real Date, no matter conceptually what the time underneath should be.

[Trott]

d has actual date information that f lacks

Hmmm... On the one hand: For both objects, it's all tucked away behind getters and setters, and those getters and setters are all identical.

But on the other hand: You certainly have a point that this behavior is kind of surprising and non-intuitive, at least at first blush.

And the fact that six out of six userland assertion libraries throw on this would seem to back that up.

I can go either way on this one right now. A PR might help clarify things. So I'll stop getting in the way. :-D

[joyeecheung]

OK, I think I will try to read other libraries' code first and figure out why they all choose(or perhaps accidentally choose) to throw :).

[targos]

Another unexpected result:

class MyDate extends Date {
  constructor(...args) {
    super(...args)
    this.myProp = 'value'
  }
}
var date1 = new Date('2016')
console.log(date1)
// 2016-01-01T00:00:00.000Z
var date2 = new MyDate('2016')
console.log(date2)
// { 2016-01-01T00:00:00.000Z myProp: 'value' }

assert.deepStrictEqual(date1, date2) // does not throw

Relevant lines from assert:

node/lib/assert.js

Lines 152 to 153 in 4f97a14

	} else if (util.isDate(actual) && util.isDate(expected)) {
	return actual.getTime() === expected.getTime();

[joyeecheung]

Not quite relavent to this issue, this just came up to me while I am digging around the code:

assert.deepEqual(new Set([1, 2]), new Set([1, 2, 3]));  // does not throw
assert.deepEqual(new Map([['a', 1], ['b', 2]]), new Map([['a', 1], ['b', 3]]));  // does not throw

I know assert has a stability index of 3, so it's just a thought :P. Since we are using more and more ES6 in core and V8 have started to optimize iterators, adding checks for iterables might be a nice-to-have in the future.

[Fishrock123]

@joyeecheung That last example is why deepStrictEqual is preferred over deepEqual. :D

[joyeecheung]

@Fishrock123 Uh, which one do you mean?

[joyeecheung]

I looked into the source code of other libraries, turns out

• Some of them use Object.prototype.toString.call(obj) to check if the type of the two objects are the same first. A fake date would have [object Object] but a real date would have [object Date] in our current version of V8. IIRC an older version of v8 would return invalid Date but that was fixed.
• Some of them just call getTime() on Date-like objects(and throw this is not a Date object.).

And in the user-land modules, deepEqual is usually closer to the semantics of deepStrictEqual in node core.

I think in addition to checking the prototype, we need to check the types returned by Object.prototype.toString.call(obj) too (it's already done for [object Arguments], but not for other built-in types).

[joyeecheung]

I'm trying to put a PR together but there are always some really bizarre cases that I could break, like these introduced by 00a7456:

assert.doesNotThrow(makeBlock(a.deepEqual, new Number(1), {}),
                    a.AssertionError);
assert.doesNotThrow(makeBlock(a.deepEqual, new Boolean(true), {}),
                    a.AssertionError);

[akdor1154]

@joyeecheung those are really weird tests, I would assume those should throw.
@teppeis, @bnoordhuis is this really desired behaviour?

[Fishrock123]

@joyeecheung I meant assert.deepEqual(new Set([1, 2]), new Set([1, 2, 3]));

---

assert.doesNotThrow(makeBlock(a.deepEqual, new Number(1), {}),
                    a.AssertionError);
assert.doesNotThrow(makeBlock(a.deepEqual, new Boolean(true), {}),
                    a.AssertionError);

These look correct to me. deepEqual is not strict (only ==) so Number and Boolean Objects (not to be confused with Number(num) and Boolean(bool), the new makes a big difference!) just coerce to being "objects".

[akdor1154]

@Fishrock123 the docs support what you are saying (and even warn about this surprising behaviour), fair enough. I guess deepStrictEqual should throw in these cases though? (as according to the docs it includes a prototype check?)

[Fishrock123]

I guess deepStrictEqual should throw in these cases though?

Yes. This is one of many reasons why you should probably never use the Object forms of Simple Primitives.

[akdor1154]

I am thinking more on this, is there any actual use case for someone to want deepEqual to think those types are equal?
A simple (a.valueOf() ==[=] b.valueOf()) at the top of deep[Strict]Equal would change behaviour in these cases, but the new behaviour would be much less surprising IMO. I guess changing behaviour like this is probably out of the question for now, though.

[Fishrock123]

is there any actual use case for someone to want deepEqual to think those types are equal?

That is why we have deepStrictEqual. Changing the behavior is probably not viable due to existing potential ecosystem dependency.

[joyeecheung]

These look correct to me. deepEqual is not strict (only ==) so Number and Boolean Objects (not to be confused with Number(num) and Boolean(bool), the new makes a big difference!) just coerce to being "objects".

@Fishrock123 I am 50/50 on this, given it's a 50/50 result among the user land modules that I've tested :). I am currently inclined to leave deepEqual as-is and perform a more strict type check for deepStrictEqual.

Also, I think we could stress that deepEqual is in fact, a "loose"(non-strict) comparison in the docs, in case people miss the docs of deepStrictEqual.(EDIT: the docs are fine, they make a pretty good example there). Some user land modules put the name "deep equal" on a more deepStrictEqual-ish functionality, so their user can be surprised when transitioning to use assert in the core. Given that power-assert is gaining popularity I think there will be more people doing this(I know a few of my colleges are evangelizing it).

Trying to make a PR now! :)