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

RESOLVED FIXED in mozilla12

Status

RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: smontagu, Assigned: Ehsan)

Tracking

(Depends on: 2 bugs, Blocks: 1 bug)

Trunk
mozilla12
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

8 years ago
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.
(Reporter)

Comment 1

8 years ago
Posted patch Tentative patch (obsolete) — Splinter Review
I'm not sure whether this will detect failure or just get bypassed. Checking it out on tryserver now.
(Reporter)

Comment 2

8 years ago
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?
(Reporter)

Comment 3

8 years ago
Posted patch Alternative patch (obsolete) — Splinter Review
This is somewhat hacky, but does at least detect failure.
(Reporter)

Updated

8 years ago
Depends on: 669274
(Reporter)

Comment 4

8 years ago
Filed bug 669274 on the failure.
(Assignee)

Comment 5

8 years ago
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?
(Assignee)

Comment 6

8 years ago
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.
Assignee: nobody → ehsan
Component: Layout → Mochitest
Product: Core → Testing
QA Contact: layout → mochitest
(Assignee)

Comment 7

8 years ago
Posted patch Patch (v1) (obsolete) — Splinter Review
Attachment #543364 - Attachment is obsolete: true
Attachment #543679 - Attachment is obsolete: true
Attachment #554317 - Flags: review?(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?
Attachment #554317 - Flags: review?(ted.mielczarek) → review+
(Assignee)

Comment 9

8 years ago
(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.
(Assignee)

Comment 10

8 years ago
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?
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
(Assignee)

Comment 12

8 years ago
Posted patch Patch (v2) (obsolete) — Splinter Review
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.
Attachment #554317 - Attachment is obsolete: true
Attachment #559367 - Flags: review?(ted.mielczarek)
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?
(Assignee)

Updated

8 years ago
Depends on: 685782
(Assignee)

Updated

8 years ago
Depends on: 685788
(Assignee)

Comment 14

8 years ago
(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.
(Assignee)

Comment 15

8 years ago
Posted patch Patch (v3)Splinter Review
Actually I needed to make one more adjustment for the insanity of chrome URIs.
Attachment #559367 - Attachment is obsolete: true
Attachment #559543 - Flags: review?(ted.mielczarek)
Attachment #559367 - Flags: review?(ted.mielczarek)
(Assignee)

Updated

8 years ago
Depends on: 685995
(Assignee)

Updated

8 years ago
Depends on: 686003
(Assignee)

Updated

8 years ago
Depends on: 686022
(Assignee)

Updated

8 years ago
Depends on: 686026
(Assignee)

Updated

8 years ago
Depends on: 686032
(Assignee)

Updated

8 years ago
No longer depends on: 686022
(Assignee)

Updated

8 years ago
Depends on: 680431
Ehsan, does this cover the same as bug 683143? And do the dependencies cover all the tests listed in that bug?
(Assignee)

Comment 17

8 years ago
(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.
Attachment #559543 - Flags: review?(ted.mielczarek) → review+
(Assignee)

Updated

7 years ago
Summary: Make sure that test_reftests_with_caret.html runs all its subtests → Teach the mochitest harness to verify that the test calling finish is the same one that we expect it to be
(Assignee)

Updated

7 years ago
Depends on: 716145
(Assignee)

Updated

7 years ago
Depends on: 716593
(Assignee)

Updated

7 years ago
Depends on: 716677
(Assignee)

Updated

7 years ago
Depends on: 716867
(Assignee)

Updated

7 years ago
Depends on: 716877
(Assignee)

Comment 18

7 years ago
Finally!

https://hg.mozilla.org/integration/mozilla-inbound/rev/d2ff5d57658d
Target Milestone: --- → mozilla12
Depends on: 717154
Depends on: 717243
https://hg.mozilla.org/mozilla-central/rev/d2ff5d57658d
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Depends on: 717868
Depends on: 717871
Depends on: 719186
Depends on: 781116
You need to log in before you can comment on or make changes to this bug.