Closed
Bug 668728
Opened 13 years ago
Closed 13 years ago
Teach the mochitest harness to verify that the test calling finish is the same one that we expect it to be
Categories
(Testing :: Mochitest, defect)
Testing
Mochitest
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla12
People
(Reporter: smontagu, Assigned: ehsan.akhgari)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 4 obsolete files)
(deleted),
patch
|
ted
:
review+
|
Details | Diff | Splinter Review |
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•13 years ago
|
||
I'm not sure whether this will detect failure or just get bypassed. Checking it out on tryserver now.
Reporter | ||
Comment 2•13 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•13 years ago
|
||
This is somewhat hacky, but does at least detect failure.
Reporter | ||
Comment 4•13 years ago
|
||
Filed bug 669274 on the failure.
Assignee | ||
Comment 5•13 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•13 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•13 years ago
|
||
Attachment #543364 -
Attachment is obsolete: true
Attachment #543679 -
Attachment is obsolete: true
Attachment #554317 -
Flags: review?(ted.mielczarek)
Comment 8•13 years ago
|
||
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•13 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•13 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?
Comment 11•13 years ago
|
||
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•13 years ago
|
||
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 13•13 years ago
|
||
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 | ||
Comment 14•13 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•13 years ago
|
||
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)
Comment 16•13 years ago
|
||
Ehsan, does this cover the same as bug 683143? And do the dependencies cover all the tests listed in that bug?
Assignee | ||
Comment 17•13 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.
Updated•13 years ago
|
Attachment #559543 -
Flags: review?(ted.mielczarek) → review+
Assignee | ||
Updated•13 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 | ||
Comment 18•13 years ago
|
||
Target Milestone: --- → mozilla12
Comment 19•13 years ago
|
||
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•