Closed Bug 683143 Opened 8 years ago Closed 7 years ago

Some tests call SimpleTest.finish() twice

Categories

(Testing :: Mochitest, defect)

x86
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla18

People

(Reporter: enndeakin, Assigned: vlad)

References

Details

Attachments

(2 files)

The following tests (at least) are calling SimpleTest.finish() two or more times. This causes the tests following it to not run.

/tests/content/canvas/test/test_mozGetAsFile.html (3 times)
/tests/content/html/content/test/test_bug430392.html (3 times)
/tests/content/media/test/test_audio1.html (3 times)
/tests/toolkit/components/url-classifier/tests/mochitest/test_classifier_worker.html (2 times)

chrome://mochitests/content/chrome/dom/workers/test/test_file.xul (4 times)
chrome://mochitests/content/chrome/dom/workers/test/test_filePosting.xul (2 times)
chrome://mochitests/content/chrome/dom/workers/test/test_fileReaderSync.xul (9 times)
chrome://mochitests/content/browser/browser/base/content/test/browser_tab_dragdrop2_frame1.xul (2 times)

These should be fixed, but finish() could be changed to warn for this situation.
Attachment #556820 - Flags: review?(jmaher)
Comment on attachment 556820 [details] [diff] [review]
simple patch to do this

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

This patch looks like we are just warning and not protecting against it being called twice.  Is the intention of this patch to warn and then fix later?
Attachment #556820 - Flags: review?(jmaher) → review+
The test will go orange if it calls finish() twice. Is there something more that you mean?

Note that we don't want to use this patch until the tests themselves are fixed.
I just assumed these tests were not going orange since they exist and are not perma orange tests.  I recall fixing browser_tab_dragdrop2_frame1.xul in my patch queue.
Depends on: 683457
Depends on: 686026
Duplicate of this bug: 792462
This patch is old and I believe we have changed the way we count finish in the last year.
My patch in bug 792462 is largely the same as this (except it returns to try to avoid the test breaking other things).  In running it through the try server, there were only 2 tests that broke, and Ehsan and I have fixes for both -- I'll update soon.
Assignee: nobody → vladimir
We'd want a combination of both patches: Vlad's has it returning early, and mine includes the document location in the error message to more clearly show which test has the problem.
Attached patch updated patchSplinter Review
Updated patch, combining mine and Enn's patches (note that the test name would have been printed as part of the ok fail, but the docoument.location is handy for debugging too).

Also two test fixes that were the only two that tripped this on a full tryserver run.
Attachment #665469 - Flags: review?(jmaher)
Attachment #665469 - Flags: review?(ehsan)
Comment on attachment 665469 [details] [diff] [review]
updated patch

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

r=me with the below comment addressed.

::: testing/mochitest/tests/SimpleTest/SimpleTest.js
@@ +664,5 @@
>  **/
>  SimpleTest.finish = function () {
> +    if (SimpleTest._alreadyFinished) {
> +        SimpleTest.ok(false, "[SimpleTest.finish()] test " + document.location + " already called finish!");
> +        return;

The return here seems like a mistake -- we should still let finish() make progress.
Attachment #665469 - Flags: review?(ehsan) → review+
(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #8)

> note that the test name would have been printed as part of the ok fail

If I remember correctly, I think that displays the currently running test name, not the test that called SimpleTest.finish(), which can be different when asynchronous callbacks fire after the test finishes. It might be worth fixing or logging that problem as well.
Comment on attachment 665469 [details] [diff] [review]
updated patch

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

I agree with ehsan about the early return, otherwise this is nice and simple.
Attachment #665469 - Flags: review?(jmaher) → review+
https://hg.mozilla.org/mozilla-central/rev/0b1528740981
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.