697381 - Reassess equal and deepEqual implementations

Status: RESOLVED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

With bug 696484 we have combined the equal and deepEqual methods of the assertion class. Geo has been raised some concerns about that on bug 683294. We should reassess that change and find a proper solution also for Mozmill 2.

A general question which is part of this bug, is how we want to handle external code, which we got from other projects. What do modifications mean in terms of licenses?  

Geo, can you please give an overview of all your points on this bug? Thanks.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Heather Arthur [:harth] from bug 696484 comment #5)
> fyi, you can't test if two objects are the same object in memory now.

How often will that be the case? I think that this case is really rarely. We will probably never do it. If we have to, why can't we use:

equal.ok(obj1 === obj2, ...)

Comment 2

[Heather Arthur [:harth]]

In the node module there's parity between:

equal()       and js's ==
strictEqual() and js's ===
deepEqual()   and object equivalence

Just pointing it out in case you want to have something congruent like that.

Comment 3

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

This was my comment on the checkin where I raised issues. I think it's more or less valid verbatim. If not, ask for clarification.

BTW, I'd be totally fine with what's in https://bugzilla.mozilla.org/show_bug.cgi?id=697381#c2. 

I think we're overthinking the whole "always use ===" thing to death, and it's the wrong way to go in something like an assertion API where people are used to different behavior from functions named similarly in other, similar APIs.

Comment on attachment 569201 [details] [diff] [review] [diff] [details] [review]
Patch v1 (old modules)

> // THIS IS NOT TESTED NOR LIKELY TO WORK OUTSIDE V8!

That's comforting. :/

> } else if (typeof actual != 'object' && typeof expected != 'object') {
>    return actual == expected;

Not saying this is wrong, but not sure what behavior this produces. Strict identity was covered above, so I think what this'll do is essentially:

if (actual === expected) {
  return true
} else if neither is an object && (actual == expected) {
  return true
}

...which is equivalent for most non-objects to just testing actual == expected. This isn't in line with our previous semantics, which are strict ===.

  // 7.4. For all other Object pairs, including Array objects, equivalence is

...which hands off to _objEquiv, which is documented as potentially expensive. Also, assuming keys give back properties, it'll hit those and it may cause side effects (though writing a get property with a side effect is a design bug itself). If it doesn't see properties, it won't properly verify value equality.

So here's my take:

The code's OK, as far as it goes. It's mostly c/p from a known-good project.

I don't fully understand the behavior. I do know it won't be the same as the function it's replacing, which is a strict ===.

Calling this may be expensive, and might cause side effects due to the key access issue.

Right now, because of those issues, I think I'm in favor of keeping things the way they were and introducing deepEqual to be used only when a deep equal is needed.

Once we've answered this review, I'll review the one for the API refactor project. We're going to want them to act the same.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Given several discussions on IRC and other channels, do we want to revert the changes made on bug 696484 and only leave in the improvements for the assert message? If we agree on that I can work on that patch. That way we wouldn't introduce new features or API changes.

Comment 5

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Yeah, I'd prefer to revert bug 696484's elimination of the distinction between equal and deepEqual. Aside from the specific problems listed, I distrust heavy logic in an assertion, and only like to use it when strictly necessary.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Ok, so lets do it. I will use the original bug 696484 for it.