Ignore constructor equality for t.same

[mattkrick]

Description

t.same now uses deeper, which checks the equality of the constructors of objects. I argue that a deep equal should ignore the constructor. Here's why:
Two objects created with different instances of the constructor will fail.
Constructor is not a key in Object.keys, not part of JSON, and not returned by tools like graphQL. That means one must recreate the result in order to append the constructor without adding it as a key (unless I'm doing it wrong).
Insanely difficult to debug and makes you pull your hair out because logging both objects returns two identical objects and you start to question your sanity when the diff returns nothing.

Environment

Affects all, but running on 5.7.1, OSX (Darwin 15.x)

[sindresorhus]

👍 Do you have a suggestion on how we could solve this?

[mattkrick]

Are you open to swapping out deeper for something else? I know I've used
similar packages than don't check the constructor, but can't remember the
names...

On Thu, Mar 10, 2016, 10:54 AM Sindre Sorhus [email protected]
wrote:

[image: 👍] Do you have a suggestion on how we could solve this?

—
Reply to this email directly or view it on GitHub
#630 (comment).

[sindresorhus]

Maybe, if it does the same except for the constructor checking. We previously tried with is-equal, but the package is ridiculously large.

[jamestalmage]

I guess I'm not so sure checking the constructor is a terrible idea. Two objects with different constructors are not strictly equal. Specifically, you could certainly write an instanceOf check that one object passes, and the other fails. The empty diff is a bit problematic. Maybe we should provide a special error message in that case:

AssertionError: t.same - objects differed only by constructor

The docs indicate this behavior was a specific choice in deeper. My gut tells me it's the correct choice for the default behavior. Perhaps @othiym23 can provide further rationale.

[othiym23]

If you want looser constructor checking, take a look at only-shallow, which exists so that tap can follow pretty much exactly @mattkrick's rationale in the original post. tap differentiates between looser structural equality (t.same, using only-shallow), and prototype-chain validation (t.strictSame, using deeper) for those (comparatively rarer cases) in which you want to ensure that the prototype chains match. It turns out that only-shallow has slightly more complicated logic as a result, but for a test library like ava, any performance consequences should be unnoticeable.

I'd be super amused if somebody found a use for deepest, but probably a terrible idea for ava.

[mattkrick]

I've used deep-equal and it's really lightweight. It checks for prototype
equality instead of constructor equality so I think it'd be the best of
both worlds. There's also deep-eql which is what chai uses. Thoughts?

On Thu, Mar 10, 2016, 5:39 PM Forrest L Norvell [email protected]
wrote:

If you want looser constructor checking, take a look at only-shallow
http://npm.im/only-shallow, which exists so that tap can follow pretty
much exactly @mattkrick https://github.com/mattkrick's rationale in the
original post. tap differentiates between looser structural equality (
t.same, using only-shallow), and prototype-chain validation (t.strictSame,
using deeper) for those (comparatively rarer cases) in which you want to
ensure that the prototype chains match. It turns out that only-shallow
has slightly more complicated logic as a result, but for a test library
like ava, any performance consequences should be unnoticeable.

I'd be super amused if somebody found a use for deepest
http://npm.im/deepest, but probably a terrible idea for ava.

—
Reply to this email directly or view it on GitHub
#630 (comment).

[jamestalmage]

We moved away from deep-equal because of inspect-js/node-deep-equal#19.

I see no reason not to emulate tap here, with same and strictSame just as described by @othiym23.

[othiym23]

FWIW, I wrote deeper, deepest, and only-shallow explicitly because I was horrified by how [very popular package I'm not going to name that isn't deep-equal] handled objects containing circular references. I (much later) patched tap to use deeper (and then, at Isaac's request, wrote only-shallow) entirely because I wanted to compare objects containing circular references.

The only dependency that deeper or only-shallow has is that it will use buffertools to do fast Buffer comparison if it can require() buffertools in the current environment (it's not even an optionalDependency). It's not a lot more complicated than deep-equal; it just looks that way because it was important to me that the algorithm be completely documented, because there are many packages that do something like this, but with poorly-specified functionality. As this discussion shows, the set of assumptions the implementer chooses when designing an equality test is important.

[jamestalmage]

the set of assumptions the implementer chooses when designing an equality test is important

👍 this just leaves me more convinced that we want to stick with deeper/only-shallow.

Perhaps AVA should add buffertools as a dependency if it's not too heavy. Or at least document somewhere that installing it will speed up buffer comparison.

[othiym23]

Perhaps AVA should add buffertools as a dependency if it's not too heavy. Or at least document somewhere that installing it will speed up buffer comparison.

buffertools only offers a significant performance benefit when used with large Buffers, and with versions of Node earlier than Node 0.12, which includes buffer.equals(), which is undoubtedly the fastest method available. Since it includes native code, you don't want to include it if you don't have to.

[sindresorhus]

I see no reason not to emulate tap here, with same and strictSame just as described by

I don't see the reason to have both to be honest. Same reason we don't have is and strictIs.

buffertools only offers a significant performance benefit when used with large Buffers, and with versions of Node earlier than Node 0.12, which includes buffer.equals(), which is undoubtedly the fastest method available.

👎 We definitely don't want to include more native dependencies for such a minor to none gain.

[jamestalmage]

This isn't completely analogous to is vs strictIs. You can always make your test work using the strict-only method AVA offers, you may just need to swap t.is(x, null) with t.is(x, undefined). In this case AVA forces you to write a more explicit test without creating an undue burden.

It seems @mattkrick is claiming there are cases where a looser "structurally equal" comparison is desirable.

Maybe we should keep t.same to be the strict variety, and have t.looseSame

[novemberborn]

Maybe we should keep t.same to be the strict variety, and have t.looseSame

IMHO the strictest method should be the one you think of the soonest, so +1.

I wonder if performance-wise it'd be feasible to run t.looseSame if t.same fails, and then report "this test would pass using looseSame, did you mean to use that?" It might help prevent people getting stuck debugging their assertion failures.

[mattkrick]

I'd still argue against having 2 sames simply because getting a JSON response (like GraphQL) will return undefined as the constructor for every object. So even though a warning would be a step forward, it'd still require recreating the object from scratch, which would include so much code it wouldn't be practical to use it.

[jamestalmage]

In which case you could just use looseSame directly and be done with it?

[sindresorhus]

I'm having a hard time thinking of when I would ever want it to check the equality of the constructors of objects. And I would honestly prefer one method, not two. It's very easy to add another method, but it adds a lot of overhead for users. Now they must think hard about which method to use in which cases and know about the subtle differences between them.

[mattkrick]

Personally, I avoid making decisions based on the constructor function & go off of the shape of the object, or a signature property, or the constructor.name as a last resort, but that's because the multiple instance problem has bitten me more times than i care to admit. If I really cared about constructor equality, I'd write a series of t.is tests to shallowly compare anyways. But my opinion is biased 😄

[jamestalmage]

OK,
I guess I am sold on just using the loose comparison.

@novemberborn Can you think of a reason for keeping the strict one around?

@mattkrick are you interested in making a PR swapping deeper for only-shallow?

[novemberborn]

Can you think of a reason for keeping the strict one around?

Nah, happen to loosen it up. If you're really concerned then as @mattkrick says write some t.is tests. I doubt stricter tests would catch many real word bugs.