Closed Bug 542249 Opened 13 years ago Closed 13 years ago

Mozmill: Random orange with switching tabs but not waiting for something to be displayed


(Thunderbird :: Testing Infrastructure, defect)

Not set


(thunderbird3.0 .5-fixed)

Thunderbird 3.1b1
Tracking Status
thunderbird3.0 --- .5-fixed


(Reporter: rain1, Assigned: rain1)




(1 file, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
e.g. <>

I've also clarified what switch_tab does, and allowed plan_for_message_display to take a reference to a tab as an argument.
Attachment #423534 - Flags: review?(bugmail)
Assignee: nobody → sid.bugzilla
Comment on attachment 423534 [details] [diff] [review]

This patch seems to cause timeouts far too often. I need to look into this -- withdrawing review for now.
Attachment #423534 - Flags: review?(bugmail)
Blocks: 540110
asuth, what do we need to do to make sure that a message summary is displayed? test-summarization.js doesn't seem to do anything beyond the customary wait_for_message_display_completion(). Does the test at <> cover summaries too? If so, how do you propose we handle messageLoaded? I think it might be useful to have messageLoaded reflect the status of both messages and summaries.
I think the logic to make sure a message summary is correct is:

assert_displayed/assert_selected_and_displayed via _internal_assert_displayed

calls assert_messages_summarized.

I don't think I ever cared whether the summarization completed.  Just that it got set-up correctly.  Of course, that call was made before the tests started being brittle.

I don't see any harm in having messageLoaded serve as an indicator that what we're displaying in the message page is loaded rather than just message bodies.  Likewise, there's no real benefit of having the mozmill tests run faster than the summarization can keep up.  (Unless we want to test such a case explicitly).

The most end-to-end choice for messageLoaded would just have the summarization logic reach out and notify the MessageDisplay when it finishes what it is doing.  If the logic explodes we may never get such a notification, but that is probably correct.

I forget how the summarization logic is finding out what the set of messages to summarize is right now, but I would have the messageDisplay instance passed/retrieved at the same time and with a specifically defined method for the summarization to call.  (I don't think there's a way for there to be multiple summarizations active at the same time for a single message display, but if there somehow is, we probably want a generation token so we can verify it's the current summarization completing.)
Depends on: 545886
Attached patch patch, v2Splinter Review
This seems to not cause any timeouts (tested with bug 540110) once the patch in bug 545886 is applied.
Attachment #423534 - Attachment is obsolete: true
Attachment #426702 - Flags: review?(bugmail)
Attachment #426702 - Flags: review?(bugmail) → review+
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.1b1
Comment on attachment 426702 [details] [diff] [review]
patch, v2

While this particular patch is npotb, this will cause timeouts unless all the patches it depends on lands, and some of those patches _are_ part of the build, and potentially risky. I think we should take this and suffer the timeouts on branch.
Attachment #426702 - Flags: approval-thunderbird3.0.3?
Depends on: 545674
Attachment #426702 - Flags: approval-thunderbird3.0.3? → approval-thunderbird3.0.4?
Comment on attachment 426702 [details] [diff] [review]
patch, v2

I think I need more convincing on why we should take this and suffer more random oranges on the 3.0.x branch.
Attachment #426702 - Flags: approval-thunderbird3.0.4? → approval-thunderbird3.0.5?
Comment on attachment 426702 [details] [diff] [review]
patch, v2

Sid told me over irc that the timeouts in this case are just waitForxxx timeouts reaching their limit not timeouts as in random orange timeouts. Therefore a=Standard8 for 3.0 branch.
Attachment #426702 - Flags: approval-thunderbird3.0.5? → approval-thunderbird3.0.5+
Verifying test-only patch, I haven't seen this recently and the tests are generally green.
You need to log in before you can comment on or make changes to this bug.