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

Status: RESOLVED FIXED

(Thunderbird :: Testing Infrastructure, defect)

Description

[Rain [:rain1] (inactive)]

Attachment: patch

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.

Comment 1

[Rain [:rain1] (inactive)]

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.

Comment 2

[Rain [:rain1] (inactive)]

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.

Comment 3

[Andrew Sutherland [:asuth] (he/him)]

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

Comment 4

[Rain [:rain1] (inactive)]

Attachment: patch, v2

This seems to not cause any timeouts (tested with bug 540110) once the patch in bug 545886 is applied.

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 426702 [details] [diff] [review]
patch, v2

Danke!

Comment 6

[Rain [:rain1] (inactive)]

http://hg.mozilla.org/comm-central/rev/0785be069955

Comment 7

[Rain [:rain1] (inactive)]

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.

Comment 8

[Mark Banner (:standard8)]

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.

Comment 9

[Mark Banner (:standard8)]

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.

Comment 10

[Rain [:rain1] (inactive)]

http://hg.mozilla.org/releases/comm-1.9.1/rev/4a2ba671820c

Comment 11

[Mark Banner (:standard8)]

Verifying test-only patch, I haven't seen this recently and the tests are generally green.