Closed Bug 662960 Opened 10 years ago Closed 9 years ago

Investigate how to provide rich error messages on waitForEval timeouts with MozMill 1.5

Categories

(Thunderbird :: Testing Infrastructure, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 9.0

People

(Reporter: rain1, Assigned: rain1)

References

Details

Attachments

(1 file)

With our current version of MozMill (1.4.2b1), controller.waitForEval returns true on happening and false on timing out. This allows us to detect the timeout and throw a potentially richer and more detailed error with mark_failure, e.g. at <https://mxr.mozilla.org/comm-central/source/mail/test/mozmill/shared-modules/test-folder-display-helpers.js#1644>.

With MozMill 1.5, waitForEval throws a canned exception on timing out. While this is great for detecting bugs, it is somewhat less so for seeing exactly what went wrong. The alternative is to use waitFor and pass in a string, but that doesn't directly allow printing the state of something after it's timed out, just an effectively static string.

One way to fix this seems be to wrap the data in an object which when toString() is called does whatever mark_failure does, and pass this object in as the second argument to waitFor. We're essentially tricking mozmill into calling mark_failure whenever an error happens.
Bug 637941 will allow us to filter by TimeoutError instead of Error, so that we don't have to rely on hackery. (I'm honestly not comfortable with toString having side-effects.)
Depends on: 637941
The patch needs <https://github.com/mozautomation/mozmill/commit/fb58f704c38481626c07e34a285d701e1c9c4906>, which isn't in MozMill 1.5.4b2 but should make it to 1.5.4 final.

All uses of waitForEval are replaced by utils.waitFor. Within tests, I've used mc.waitFor etc (which just forwards to utils.waitFor).

Blake, it'd be nice if you could review the changes to the migration tests. I know Mark's pretty busy, so I'm asking bienvenu to review everything else. It's fairly straightforward I think.
Assignee: nobody → sid.bugzilla
Status: NEW → ASSIGNED
Attachment #538442 - Flags: review?(dbienvenu)
Attachment #538442 - Flags: review?(bwinton)
Attachment #538442 - Flags: feedback?(bugmail)
Oh, and asuth: I know you're not fulltime Tb any more, but you know this better than anyone else, so it'd be awesome if you could give it the once over.
Comment on attachment 538442 [details] [diff] [review]
replace all uses of waitForEval, v1

whew, that's a large patch. Tests pass on windows, once I get all the pre-reqs done, except for test_bad_password_uses_old_settings, which is a known failure, as I understand it.
Attachment #538442 - Flags: review?(dbienvenu) → review+
Comment on attachment 538442 [details] [diff] [review]
replace all uses of waitForEval, v1

Bonus points for cleaning up a bunch of the tests to use the content helper! :)

Good call on the TimeoutError; it is nice to be able to provide those extra failures, but we don't need to be using mark_failure unless we have rich objects we are trying to serialize so that they show up in ArbPL land.

aside: This is definitely a case where reviewboard still beats splinter since it highlights the actual changes in modified lines where feasible.
Attachment #538442 - Flags: feedback?(bugmail) → feedback+
(In reply to comment #5)
> Good call on the TimeoutError; it is nice to be able to provide those extra
> failures, but we don't need to be using mark_failure unless we have rich
> objects we are trying to serialize so that they show up in ArbPL land.

This came across a bit more incoherent than it should have.  Second try:

Good call on tracking down TimeoutError to allow us to provide more specific failure messages.  There is definitely no reason to prefer mark_failure over more straightforward exception mechanisms as long as we aren't putting rich objects in the message in the hope they show up in ArbPL.
(In reply to comment #5)
> Comment on attachment 538442 [details] [diff] [review] [review]
> replace all uses of waitForEval, v1
> 
> Bonus points for cleaning up a bunch of the tests to use the content helper!
> :)

It was between that and manually calling waitFor and doing all the other checks. This seemed to need fewer keystrokes :)
Comment on attachment 538442 [details] [diff] [review]
replace all uses of waitForEval, v1

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

Yeah, this seems good to me.  I've pointed out a couple of tiny things, but they don't stop me from saying r=me.  :)

::: mail/test/mozmill/account/test-mail-account-setup-wizard.js
@@ -137,4 +138,4 @@
> >  }
> >  
> >  function subtest_verify_account(amc) {
> > -  amc.waitForEval("subject.currentAccount != null", 6000, 600, amc.window);
> > +  amc.waitFor(function () amc.window.currentAccount != null,

I like brackets around the condition (like on line 218 below).

::: mail/test/mozmill/account/test-retest-config.js
@@ -110,4 +112,4 @@
> >  
> >      // Wait for the "continue" button to be back, which means we're back to the
> >      // original state.
> > -    awc.waitForEval("subject.hidden == false", 20000, 600,
> > +    awc.waitFor(function () this.hidden == false,

Brackets here, too...
Attachment #538442 - Flags: review?(bwinton) → review+
https://hg.mozilla.org/comm-central/rev/c0fa78af189d
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 9.0
You need to log in before you can comment on or make changes to this bug.