1300385 - SimpleTest.isDeeply is deeply flawed

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Rob Wu [:robwu]]

There are multiple SimpleTest.js in the tree (???), so to be clear I am referring to the one in testing/mochitest/tests/SimpleTest/SimpleTest.js

SimpleTest.isDeeply is flawed:
- isDeeply(undefined, anyObject) does NOT raise test failures.
- isDeeply uses *some* form of weak equality, while the other SimpleTest.is* methods use Object.is (bug 1165533) (and previously ===, bug 949614).
- The log message formatter contains a dead loop (I guess the intention was to turn DNE in the "Does not exist" string?): http://searchfox.org/mozilla-central/rev/3582398bc9db5a83e25c2a8299cc780a52ad7ca8/testing/mochitest/tests/SimpleTest/SimpleTest.js#1523

Comment 1

[Rob Wu [:robwu]]

Attachment: Bug 1300385 - Fix flaws in SimpleTest.isDeeply

- When one of the parameters to isDeeply is an object/function
  and the other is not, isDeeply returned false. Well, isDeeply
  is supposed to report an error instead of returning a value...

- Change the implementation of isDeeply to have SameValue semantics
  instead of weak equality.

- Change the representation of arrays to look like an array, instead
  of its default toString() value which is indistinguishable from a
  string due to the lack of brackets and quotes.

Review commit: https://reviewboard.mozilla.org/r/76512/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/76512/

Comment 2

[Rob Wu [:robwu]]

Comment on attachment 8787931 [details]
Bug 1300385 - Fix flaws in SimpleTest.isDeeply

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/76512/diff/1-2/

Comment 3

[Rob Wu [:robwu]]

Comment on attachment 8787931 [details]
Bug 1300385 - Fix flaws in SimpleTest.isDeeply

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/76512/diff/2-3/

Comment 4

[Rob Wu [:robwu]]

Comment on attachment 8787931 [details]
Bug 1300385 - Fix flaws in SimpleTest.isDeeply

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/76512/diff/3-4/

Comment 5

[Martijn Wargers (dead)]

You should add some tests to this to make sure this doesn't break (negative and positive I guess).
(In reply to Rob Wu [:robwu] from comment #0)
> There are multiple SimpleTest.js in the tree (???), so to be clear I am
> referring to the one in testing/mochitest/tests/SimpleTest/SimpleTest.js

I see 3:
https://dxr.mozilla.org/mozilla-central/search?q=file%3ASimpleTest.js&redirect=false
dom/tests/mochitest/ajax/mochikit/tests/SimpleTest/SimpleTest.js
obj-x86_64-pc-linux-gnu/_tests/testing/mochitest/mochijar/chrome/mochikit/content/tests/SimpleTest/SimpleTest.js
testing/mochitest/tests/SimpleTest/SimpleTest.js

Comment 6

[Rob Wu [:robwu]]

Comment on attachment 8787931 [details]
Bug 1300385 - Fix flaws in SimpleTest.isDeeply

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/76512/diff/4-5/

Comment 7

[Rob Wu [:robwu]]

(In reply to Martijn Wargers [:mwargers] (not working for Mozilla) from comment #5)
> You should add some tests to this to make sure this doesn't break (negative
> and positive I guess).

Automated tests would be ideal, but I don't see any existing ones for SimpleTest (not even in the two referenced bugs that altered the behavior of SimpleTest.is). Locally I verified that my patch works for the reported scenarios, and I also created a large set of tests for an unrelated bug that relies on the fixes from this bug. I believe that this, plus the other consumers of isDeeply provide reasonable test coverage, so I'm inclined to not add new tests to this bug.

> (In reply to Rob Wu [:robwu] from comment #0)
> > There are multiple SimpleTest.js in the tree (???), so to be clear I am
> > referring to the one in testing/mochitest/tests/SimpleTest/SimpleTest.js
> 
> I see 3:
> https://dxr.mozilla.org/mozilla-central/search?q=file%3ASimpleTest.
> js&redirect=false
> dom/tests/mochitest/ajax/mochikit/tests/SimpleTest/SimpleTest.js
> obj-x86_64-pc-linux-gnu/_tests/testing/mochitest/mochijar/chrome/mochikit/
> content/tests/SimpleTest/SimpleTest.js
> testing/mochitest/tests/SimpleTest/SimpleTest.js

The one in obj-x86_64-pc-linux-gnu is an automated copy of the one in testing/mochitest.
I don't really know what to do with the one in dom/tests, because it still uses == equality (not even ===, let alone Object.is). - http://searchfox.org/mozilla-central/rev/3582398bc9db5a83e25c2a8299cc780a52ad7ca8/dom/tests/mochitest/ajax/mochikit/tests/SimpleTest/SimpleTest.js#48

Comment 8

[Rob Wu [:robwu]]

Attachment: Bug 1300385 - Do not set button if undefined.

Review commit: https://reviewboard.mozilla.org/r/76532/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/76532/

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

dom/tests/mochitest/ajax/mochikit/tests/SimpleTest/SimpleTest.js is supposed to be a vanilla copy of the upstream SimpleTest library, for testing that the library itself works in Gecko.

As for our own tests, I feel we might as well stop using it entirely.

Comment 10

[Jeff Walden [:Waldo]]

Comment on attachment 8788001 [details]
Bug 1300385 - Do not set button if undefined.

https://reviewboard.mozilla.org/r/76532/#review75550

Comment 11

[Jeff Walden [:Waldo]]

Comment on attachment 8787931 [details]
Bug 1300385 - Fix flaws in SimpleTest.isDeeply

https://reviewboard.mozilla.org/r/76512/#review75588

Ms2ger's right, the other copy we shouldn't be touching because it's part of imported tests.  This version...frankly, I'm largely disinclined to touch this at all.  And I *really* don't like deep-equality sorts of operations, because they're by nature vague about what exactly they allow through.  But oh well.  :-(  This pig could be gussied up some.

::: testing/mochitest/tests/SimpleTest/SimpleTest.js:1388
(Diff revision 5)
>  
>  
>  SimpleTest._deepCheck = function (e1, e2, stack, seen) {
>      var ok = false;
> -    // Either they're both references or both not.
> -    var sameRef = !(!SimpleTest._isRef(e1) ^ !SimpleTest._isRef(e2));
> +    if (Object.is(e1, e2)) {
> +        // Handles equal primitives and identical references.

"Handles identical primitives and references."

In particular you want to use "identical" here because you distinguish -0 from +0, and NaN is not "equal" to NaN but is identical to it.

::: testing/mochitest/tests/SimpleTest/SimpleTest.js:1391
(Diff revision 5)
> -    // Either they're both references or both not.
> -    var sameRef = !(!SimpleTest._isRef(e1) ^ !SimpleTest._isRef(e2));
> +    if (Object.is(e1, e2)) {
> +        // Handles equal primitives and identical references.
> -    if (e1 == null && e2 == null) {
>          ok = true;
> -    } else if (e1 != null ^ e2 != null) {
> +    } else if (typeof e1 != "object" || typeof e2 != "object" || e1 === null || e2 === null) {
> +        // Different primitives, or different function references.

"If either argument is a primitive or function, don't consider the arguments the same."

::: testing/mochitest/tests/SimpleTest/SimpleTest.js:1427
(Diff revision 5)
>  
>      var ok = true;
>      // Only examines enumerable attributes. Only works for numeric arrays!
>      // Associative arrays return 0. So call _eqAssoc() for them, instead.
> -    var max = a1.length > a2.length ? a1.length : a2.length;
> -    if (max == 0) return SimpleTest._eqAssoc(a1, a2, stack, seen);
> +    var max = Math.max(a1.length, a2.length);
> +    if (!max) return SimpleTest._eqAssoc(a1, a2, stack, seen);

Keep max == 0 as it was -- I misread this when I look at it as asking if there's "not a max", rather than that the max is zero.

::: testing/mochitest/tests/SimpleTest/SimpleTest.js:1430
(Diff revision 5)
>      // Associative arrays return 0. So call _eqAssoc() for them, instead.
> -    var max = a1.length > a2.length ? a1.length : a2.length;
> -    if (max == 0) return SimpleTest._eqAssoc(a1, a2, stack, seen);
> +    var max = Math.max(a1.length, a2.length);
> +    if (!max) return SimpleTest._eqAssoc(a1, a2, stack, seen);
>      for (var i = 0; i < max; i++) {
> -        var e1 = i > a1.length - 1 ? SimpleTest.DNE : a1[i];
> -        var e2 = i > a2.length - 1 ? SimpleTest.DNE : a2[i];
> +        var e1 = i in a1 ? a1[i] : SimpleTest.DNE;
> +        var e2 = i in a2 ? a2[i] : SimpleTest.DNE;

|in| testing is not the same thing as length-testing.  That said, this would have been more readable as |i < a1.length ? a1[i] : SimpleTest.DNE|, so make that change to these lines.

Comment 12

[Jeff Walden [:Waldo]]

Comment on attachment 8787931 [details]
Bug 1300385 - Fix flaws in SimpleTest.isDeeply

https://reviewboard.mozilla.org/r/76512/#review75638

Comment 13

[Rob Wu [:robwu]]

Comment on attachment 8787931 [details]
Bug 1300385 - Fix flaws in SimpleTest.isDeeply

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/76512/diff/5-6/

Comment 14

[Rob Wu [:robwu]]

Comment on attachment 8788001 [details]
Bug 1300385 - Do not set button if undefined.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/76532/diff/1-2/

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/6a240891ba8a
Fix flaws in SimpleTest.isDeeply. r=Waldo
https://hg.mozilla.org/integration/fx-team/rev/69c9fab3db00
Do not set button if undefined. r=Waldo

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6a240891ba8a
https://hg.mozilla.org/mozilla-central/rev/69c9fab3db00