Closed Bug 736529 Opened 9 years ago Closed 9 years ago

Calling waitForFocus() and then finishing before you get focus should cause the test to fail

Categories

(Testing :: Mochitest, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla14

People

(Reporter: ayg, Assigned: ayg)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Attached patch Patch v0, no test changes (obsolete) — Splinter Review
This is a first shot.  Let's see if it catches any more bad tests than bug 735805 did.
Assignee: nobody → ayg
Status: NEW → ASSIGNED
Whiteboard: [autoland-try:-u mochitests]
Whiteboard: [autoland-try:-u mochitests] → [autoland-in-queue]
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.
Whiteboard: [autoland-in-queue]
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.
Attachment #607188 - Attachment is obsolete: true
Whiteboard: [autoland-try:-u mochitests]
Whiteboard: [autoland-try:-u mochitests] → [autoland-in-queue]
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
Whiteboard: [autoland-in-queue]
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.
Whiteboard: [autoland-try:-u mochitests]
Whiteboard: [autoland-try:-u mochitests] → [autoland-in-queue]
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.
Attachment #607202 - Flags: review?(jmaher)
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.
Attachment #607202 - Flags: review?(jmaher) → review+
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
Whiteboard: [autoland-in-queue]
https://hg.mozilla.org/integration/mozilla-inbound/rev/54cb23113c6f
Flags: in-testsuite?
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/54cb23113c6f
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.