SimpleTest.isDeeply is deeply flawed

RESOLVED FIXED in Firefox 51

Status

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: robwu, Assigned: robwu)

Tracking

Trunk
mozilla51
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

2 years ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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 hidden (mozreview-request)
(Assignee)

Comment 7

2 years ago
(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 hidden (mozreview-request)
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

2 years ago
mozreview-review
Comment on attachment 8788001 [details]
Bug 1300385 - Do not set button if undefined.

https://reviewboard.mozilla.org/r/76532/#review75550
Attachment #8788001 - Flags: review?(jwalden+bmo) → review+

Comment 11

2 years ago
mozreview-review
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

2 years ago
mozreview-review
Comment on attachment 8787931 [details]
Bug 1300385 - Fix flaws in SimpleTest.isDeeply

https://reviewboard.mozilla.org/r/76512/#review75638
Attachment #8787931 - Flags: review?(jwalden+bmo) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 15

2 years ago
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed

Comment 16

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6a240891ba8a
https://hg.mozilla.org/mozilla-central/rev/69c9fab3db00
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.