581922 - Random orange: TEST-UNEXPECTED-FAIL | test-message-header.js | test_show_all_header_mode

Status: VERIFIED FIXED

(Thunderbird :: Message Reader UI, defect)

Description

[Mark Banner (:standard8)]

Although bug 579305 basically fixed the permanent orange, we're now seeing a frequent random orange remaining on Windows:

NEXT ERROR TEST-UNEXPECTED-FAIL | e:\buildbot\win32-comm-central-check\build\mail\test\mozmill\message-header\test-message-header.js | test_show_all_header_mode
  EXCEPTION: more node should be collapsed in all header lines mode
    at: test-message-header.js line 464
       Error("more node should be collapsed in all header lines mode")  0
       subtest_change_to_all_header_mode([object XULElement]) test-message-header.js 464
       test_show_all_header_mode() test-message-header.js 342
            frame.js 474
            frame.js 530
            frame.js 573
            frame.js 417
            frame.js 579
            server.js 164
            server.js 168
NEXT ERROR TEST-UNEXPECTED-FAIL | e:\buildbot\win32-comm-central-check\build\mail\test\mozmill\message-header\test-message-header.js | test_more_widget_with_maxlines_of_3
  EXCEPTION: more node was collapsed when it should have been visible
    at: test-message-header.js line 424
       Error("more node was collapsed when it should have been visible")  0
       subtest_more_widget_display([object XULElement]) test-message-header.js 424
       test_more_widget() test-message-header.js 314
       test_more_widget_with_maxlines_of_3() test-message-header.js 507
            frame.js 474
            frame.js 530
            frame.js 573
            frame.js 417
            frame.js 579
            server.js 164
            server.js 168

http://tinderbox.mozilla.org/showlog.cgi?log=Thunderbird/1280135146.1280138265.12397.gz

Comment 1

[Blake Winton (:bwinton) (:☕️)]

I was wondering (after http://hg.mozilla.org/comm-central/rev/2ba810a84fa6 was committed) whether it would have made sense to put a call to "mc.sleep(0);" after each call mc.click on a menu, to give the menu a chance to open.

I didn't add it then, because the tests worked for me, but if we're seeing random oranges, adding the calls might help.  What do you think?

Thanks,
Blake.

Comment 2

[rsx11m]

It is weird that test_more_widget_with_maxlines_of_3 now fails as well in the cases where test_show_all_header_mode fails. This looks to me as if the View > Headers > All proceeded but removing the "more" button in the first test is still pending. Consequently, that test fails without resetting the "Headers" pref back to normal. Apparently the prefs carry over to the next test, thus n=3 fails now as well as it shows all addresses and doesn't have a "more" button. Makes sense?

Comment 3

[Mark Banner (:standard8)]

(In reply to comment #1)
> I was wondering (after http://hg.mozilla.org/comm-central/rev/2ba810a84fa6 was
> committed) whether it would have made sense to put a call to "mc.sleep(0);"
> after each call mc.click on a menu, to give the menu a chance to open.
> 
> I didn't add it then, because the tests worked for me, but if we're seeing
> random oranges, adding the calls might help.  What do you think?

I think it is worth a try. I have pushed a changeset to add those in:

http://hg.mozilla.org/comm-central/rev/860cdad7425a

(I'm agreeing with the unwritten (I believe) m-c policy that minor test fixes for random oranges don't need a review).

Comment 4

[Mark Banner (:standard8)]

Unfortunately adding the sleeps didn't fix the issue.

Comment 5

[rsx11m]

There is also a single (at least recent) instance where it failed on Mac OSX,
http://tinderbox.mozilla.org/showlog.cgi?log=Thunderbird/1280457632.1280462746.6854.gz#err7
but without test_more_widget_with_maxlines_of_3 failing as well. Do you think another wait_for_message_display_completion(mc); might help to ensure that the message is fully redrawn after switching to all-header mode? (comment #2)

Comment 6

[Mark Banner (:standard8)]

So from what I can tell, when we swap between all & normal headers, we're not reloading the message. Therefore wait_for_message_display_completion isn't going to do anything for us.

Looking at the test, I'm thinking that the issue is that we're taking a while to redraw the message header, and this happens to be longer than, or more events than mc.sleep(0) allows.

However, I'm also not sure how we could detect the end, unless we did something like a waitForEval on moreNode.collapsed and timed out (and failed) if it didn't get there after a couple of seconds.

If anyone else has ideas, that would be useful as well.

Comment 7

[Mark Banner (:standard8)]

I've just checked in another attempted fix to kill this orange:

http://hg.mozilla.org/comm-central/rev/63166c8c1e86

The basic rationale behind this was that the menus weren't being closed after a click on the element with the last version of the test, and because they were open, they were causing the fixed test-reply-to-list-from-address-selection.js to fail.

This seems to be related to bug 474486 which I've seen as pointed to in some of the Firefox tests wrt check menuitems, which is what we're interacting with here (I don't want to pick that code up just yet as we've just increased our mozmill requirement anyway).

Therefore the changes call the functions behind the menuitems direct, avoiding the issue of MozMill, and added some XXX comments.

Comment 8

[Mark Banner (:standard8)]

Ok, I'm pretty sure (despite other regressions) that we've at least nailed the very-frequent failure case on the head. So therefore I'm marking this as fixed and we can file other bugs if we need to.

Comment 9

[rsx11m]

There was another occurrence after that patch has been checked in:

WINNT 5.2 comm-central check on 2010/08/06 20:41:23
http://tinderbox.mozilla.org/showlog.cgi?log=Thunderbird/1281152483.1281158246.23595.gz#err0

That was just before clobbering.

Comment 10

[Mark Banner (:standard8)]

Like I said, let's deal with further issues in follow-up bugs dependent on this one.

Comment 11

[rsx11m]

Never mind, I haven't seen it since, thus marking as verified.