683294 - Update assertion module for new deepEqual assertions

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

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

On bug 671290 the deepEqual assertions have been added. We should port those to our own API and make it available for our current shared modules.

Comment 1

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

Attachment: Patch v1 (old modules) [checked-in]

This patch follows the changes in Mozmill and replaces the current equal/notEqual code with the deep equal checks.

Comment 2

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

Attachment: Patch v1 (api refactor) [checked-in]

Pointer to Github pull-request

Comment 3

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

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

> // 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

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

BTW, r- based on comments, but it's really high-level discussion about whether to go this route and I have no problem based on the code. Not sure what that means so leaving it "unreviewed."

Comment 5

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

I would rather stay in sync with Mozmill code here. Otherwise we can run into issues when switching between 1.5.x and 2.0. Personally I also don't like that external code that much given the statements you already have made. 

I would kinda like to get this solved by Mozmill devs and file a follow-up bug for it.

Comment 6

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

I guess I thought the "old modules" patch was the Mozmill code, but maybe that was just the 1.5 compatibility library. 

So this change has already been made for 2.0? I agree we need to stay in sync, but I think there are some issues making this change at all.

Comment 7

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

This change has been checked-in for Mozmill 2.0 and on master. See bug 696484 where you are also CC'ed.

Comment 8

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

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

I don't agree with this checkin in general, but agree we need to stay in sync.

Comment 9

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

Comment on attachment 569204 [details]
Patch v1 (api refactor) [checked-in]

Looks fine. I merged from Github already.

Comment 10

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

So how should we proceed forward? I'm uncomfortable with this change. 

For that matter, I really don't understand why people think a group of simple assertion functions needs to be pared down this much. It's bike shed optimization at the cost of specificity.

Yeah, they're a plethora of what amounts to renames. That's what makes the test code readable. The actual maintenance on them will be close to nil.

Comment 11

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

(In reply to Geo Mealer [:geo] from comment #10)
> So how should we proceed forward? I'm uncomfortable with this change. 

I have filed bug 697381 which we can use to clarify remaining issues.

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/13fbfcb6f832 (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/2d3e4f6de7a3 (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/4856d46f8448 (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/a73cf599b7cd (release)

Comment 12

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

We want to revert this change given our discussion on bug 697381.

Comment 13

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

Attachment: Patch (revert - current modules)

This patch reverts the combination of equal and deepEqual for the current modules.

Comment 14

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

Attachment: Patch (revert - api refactor)

Pointer to Github pull-request

Comment 15

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

Comment on attachment 575469 [details]
Patch (revert - api refactor)

Patch for the API refactor project.

Comment 16

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

Comment on attachment 575468 [details] [diff] [review]
Patch (revert - current modules)

Looks good. Appreciate the revert! r+

Comment 17

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

Comment on attachment 575469 [details]
Patch (revert - api refactor)

Reviewed and merged on github, r+

Comment 18

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

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/2a9b9561315a (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/e7d16e67f2da (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/eb4a1784932e (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/db2191add6aa (release)