In bug 735805 comment 11, I found two tests that called waitForFocus() without calling waitForExplicitFinish(). This meant that they finished before ever getting focus, which caused them to run no tests. The patch on bug 735805 will catch this case, but tests that run at least one test and then waitForFocus() will still pass, skipping all the other tests. finish() should check if there are any waitForFocus()es that haven't yet run their callbacks, and fail the test if so.

This is a first shot. Let's see if it catches any more bad tests than bug 735805 did.

Autoland Patchset: Patches: 607188 Branch: mozilla-central => try Patch 607188 could not be applied to mozilla-central. patching file testing/mochitest/tests/SimpleTest/SimpleTest.js Hunk #4 FAILED at 674 1 out of 4 hunks FAILED -- saving rejects to file testing/mochitest/tests/SimpleTest/SimpleTest.js.rej patch failed, unable to continue (try -v) patch failed, rejects left in working dir Patch 607188 could not be applied to mozilla-central. patching file testing/mochitest/tests/SimpleTest/SimpleTest.js Hunk #1 FAILED at 456 Hunk #2 FAILED at 476 Hunk #3 FAILED at 497 Hunk #4 FAILED at 671 4 out of 4 hunks FAILED -- saving rejects to file testing/mochitest/tests/SimpleTest/SimpleTest.js.rej patch failed, unable to continue (try -v) patch failed, rejects left in working dir Patchset could not be applied and pushed.

Bah, I need to pay more attention. I had written this on top of my patch for bug 735805, so it doesn't apply cleanly.

Autoland Patchset: Patches: 607202 Branch: mozilla-central => try Destination: http://hg.mozilla.org/try/pushloghtml?changeset=3b80500b6ef0 Try run started, revision 3b80500b6ef0. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=3b80500b6ef0

Try run for 3b80500b6ef0 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=3b80500b6ef0 Results (out of 115 total builds): success: 94 warnings: 21 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-3b80500b6ef0

It looks like the failures will be fixed by the patches to bug 735805. I'll wait till those lands and do another try run. On the other hand, maybe this isn't so useful if it catches no extra failures.

Autoland Patchset: Patches: 607202 Branch: mozilla-central => try Destination: http://hg.mozilla.org/try/pushloghtml?changeset=e1347cd5f645 Try run started, revision e1347cd5f645. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=e1347cd5f645

Comment on attachment 607202 [details] [diff] [review] Patch v0.1, no test changes, but actually applies So this caught a couple of the same broken tests that bug 735805 did. The new try run will probably report no new broken tests. Do you think this is worth having as an extra safeguard? With bug 735805 landed, it will catch any cases that use waitForFocus() incorrectly but also run some other tests beforehand.

Comment on attachment 607202 [details] [diff] [review] Patch v0.1, no test changes, but actually applies Review of attachment 607202 [details] [diff] [review]: ----------------------------------------------------------------- the only caution here is in waitForFocus sometimes we call this from the parent or child window. I could see there being a scenario where we call the child instance of SimpleTest.waitForFocus(), it doesn't finish, then the parent calls simpleTest.finish() successfully.

Yeah, it's not totally foolproof. Better than nothing, though. Thanks for the review!

Try run for e1347cd5f645 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=e1347cd5f645 Results (out of 825 total builds): success: 815 warnings: 10 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-e1347cd5f645