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

RESOLVED FIXED in Thunderbird 3.1b1

Status

defect
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: rain1, Assigned: rain1)

Tracking

Trunk
Thunderbird 3.1b1
Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird3.0 .5-fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

10 years ago
Posted patch patch (obsolete) — Splinter Review
e.g. <http://tinderbox.mozilla.org/showlog.cgi?log=ThunderbirdTest/1264470689.1264472559.24279.gz#err0>

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

Updated

10 years ago
Assignee: nobody → sid.bugzilla
Status: NEW → ASSIGNED
Assignee

Comment 1

10 years ago
Comment on attachment 423534 [details] [diff] [review]
patch

This patch seems to cause timeouts far too often. I need to look into this -- withdrawing review for now.
Attachment #423534 - Flags: review?(bugmail)
Assignee

Updated

10 years ago
Blocks: 540110
Assignee

Comment 2

10 years ago
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 <http://mxr.mozilla.org/comm-central/source/mail/test/mozmill/shared-modules/test-folder-display-helpers.js#1226> 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

http://mxr.mozilla.org/comm-central/source/mail/test/mozmill/shared-modules/test-folder-display-helpers.js#1621

calls assert_messages_summarized.

http://mxr.mozilla.org/comm-central/source/mail/test/mozmill/shared-modules/test-folder-display-helpers.js#1685


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.)
Assignee

Updated

10 years ago
Depends on: 545886
Assignee

Comment 4

10 years ago
Posted 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+
Assignee

Comment 6

10 years ago
http://hg.mozilla.org/comm-central/rev/0785be069955
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.1b1
Assignee

Comment 7

10 years ago
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?
Assignee

Updated

10 years ago
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.