668728 - Teach the mochitest harness to verify that the test calling finish is the same one that we expect it to be

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Simon Montagu :smontagu]

Even after the backout of bug 664152 and the fix for bug 668267, test_reftests_with_caret seems to be bailing early. We need a way to confirm that all the tests are being run.

Comment 1

[Simon Montagu :smontagu]

Attachment: Tentative patch

I'm not sure whether this will detect failure or just get bypassed. Checking it out on tryserver now.

Comment 2

[Simon Montagu :smontagu]

Comment on attachment 543364 [details] [diff] [review]
Tentative patch

This doesn't detect the early bail -- see OSX and Windows logs at http://tbpl.mozilla.org/?tree=Try&rev=ecf46df6a8c7. Ehsan, do you have a better idea?

Comment 3

[Simon Montagu :smontagu]

Attachment: Alternative patch

This is somewhat hacky, but does at least detect failure.

Comment 4

[Simon Montagu :smontagu]

Filed bug 669274 on the failure.

Comment 5

[(no longer active)]

Comment on attachment 543679 [details] [diff] [review]
Alternative patch

I think this is too hacky, but I agree that we should protect against this, once and for all.

How about you register an onload handler which performs the check in the first version of the patch, so that we are guaranteed to run that check even if endTest is not called for some reason?

Comment 6

[(no longer active)]

Thinking about this, I was wondering if there is actually some way for the test framework to catch the case of a test which calls waitForExplicitFinish and then navigates to another test which calls finish for it.  I have a patch which does that, and successfully catches this problem.

Comment 7

[(no longer active)]

Attachment: Patch (v1)

Comment 8

[(not currently active) Ted Mielczarek]

Comment on attachment 554317 [details] [diff] [review]
Patch (v1)

Review of attachment 554317 [details] [diff] [review]:
-----------------------------------------------------------------

The failure mode here is that a test calls waitForExplicitFinish(), then does something that causes us to navigate away from it, and then the page it navigates to calls finish(), so we continue on our merry way?

Should we instead be watching navigation on the iframe, and erroring if it ever changes to a URL we weren't expecting? (This patch seems fine, just wondering if that isn't a more thorough solution.)

::: testing/mochitest/tests/SimpleTest/TestRunner.js
@@ +205,5 @@
> + * Returns the current test URL
> + */
> +TestRunner._getCurrentTestURL = function () {
> +    return $('testframe').contentWindow.location.pathname;
> +};

This seems a little confusing since we already have .currentTestURL. Can we call this getLoadedTestURL or something to distinguish? Maybe also a comment to indicate that we use this to tell if the test has navigated to a new test without being finished?

Comment 9

[(no longer active)]

(In reply to Ted Mielczarek [:ted, :luser] from comment #8)
> Comment on attachment 554317 [details] [diff] [review]
> Patch (v1)
> 
> Review of attachment 554317 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The failure mode here is that a test calls waitForExplicitFinish(), then
> does something that causes us to navigate away from it, and then the page it
> navigates to calls finish(), so we continue on our merry way?
> 
> Should we instead be watching navigation on the iframe, and erroring if it
> ever changes to a URL we weren't expecting? (This patch seems fine, just
> wondering if that isn't a more thorough solution.)

I tried that, but it fails in some legitimate tests.  I don't remember any more details right now, but I can dig in if you want me to.

> ::: testing/mochitest/tests/SimpleTest/TestRunner.js
> @@ +205,5 @@
> > + * Returns the current test URL
> > + */
> > +TestRunner._getCurrentTestURL = function () {
> > +    return $('testframe').contentWindow.location.pathname;
> > +};
> 
> This seems a little confusing since we already have .currentTestURL. Can we
> call this getLoadedTestURL or something to distinguish? Maybe also a comment
> to indicate that we use this to tell if the test has navigated to a new test
> without being finished?

Will do before landing the patch.

Comment 10

[(no longer active)]

So, the failures caught by this patch do not show up as Tinderbox failures (see for example https://tbpl.mozilla.org/php/getParsedLog.php?id=6318847&full=1 which is green here: https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&tree=Try&rev=312dd640909e).

Ted: do you have any idea why?

Comment 11

[(not currently active) Ted Mielczarek]

Ah. Because buildbot simply parses the pass/fail output at the end:
12526 INFO Passed: 9904
12527 INFO Failed: 0
12528 INFO Todo:   337

which is generated from here:
http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/TestRunner.js#391

Comment 12

[(no longer active)]

Attachment: Patch (v2)

Oh, sorry for missing that.  This patch pushes a failure on the tests array, like other places in this file which issue UNEXPECTED-FAIL messages.

Comment 13

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 559367 [details] [diff] [review]
Patch (v2)

This patch looks fine.  Can we add some unittests for this?  Also did you test in chrome and mochitest-plain?

Comment 14

[(no longer active)]

(In reply to Joel Maher (:jmaher) from comment #13)
> Comment on attachment 559367 [details] [diff] [review]
> Patch (v2)
> 
> This patch looks fine.  Can we add some unittests for this?

As far as I know, not directly.  Testing this means writing a test which expects a failure condition to pass, which our testing frameworks don't really support  But the good news is that our existing unit tests make this fail (see bug 685782 and bug 685788 for example, where I have patches to fix the tests), so we'll be testing this indirectly by fixing those tests to comply with the invariant that this test proposes, and once I land this patch, any test which breaks this invariant will turn the tree orange (yay!!!).

> Also did you test in chrome and mochitest-plain?

I assume s/plain/browser-chrome/.  I have pushed a new try server run <http://tbpl.mozilla.org/?tree=Try&rev=cfa7e6405dd9> which does this.  I will file and fix any other bugs in the existing tests that I find before I land this patch, to guarantee a green tree when this lands.

Please let me know if this doesn't make sense to you.

Comment 15

[(no longer active)]

Attachment: Patch (v3)

Actually I needed to make one more adjustment for the insanity of chrome URIs.

Comment 16

[Neil Deakin]

Ehsan, does this cover the same as bug 683143? And do the dependencies cover all the tests listed in that bug?

Comment 17

[(no longer active)]

(In reply to Neil Deakin from comment #16)
> Ehsan, does this cover the same as bug 683143? And do the dependencies cover
> all the tests listed in that bug?

No, bug 683143 is a subset of what goes wrong here.  Also, I believe that some of the tests in that bug will be fixed by the dependencies here.

Comment 18

[(no longer active)]

Finally!

https://hg.mozilla.org/integration/mozilla-inbound/rev/d2ff5d57658d

Comment 19

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d2ff5d57658d