527550 - mozmill tests for message header pane

Status: RESOLVED FIXED

(Thunderbird :: Message Reader UI, enhancement)

Comment 1

[Ben Bucksch (:BenB)]

Attachment: Test for bug 525302 with long subject and many recipients, clicking "more". From bwinton

Carrying over tests from bug 525302. It's written by Blake.

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: test patch, v2 (updated, still has issues)

This patch had bit-rotted, so I've updated it so that it applies cleanly now, and fixed an errant reference to msgc to point to mc.  It still has two problems, however:

a) it doesn't work, because baseFolder is no longer defined during setup.  I could just put it back, I suppose, but it's not at all clear to me what the point of baseFolder was in the original test.  Blake, do you recall what the intent of creating baseFolder in the first place was?

b) we're reaching into folder-display-helper to get the message generator, which seems a bit icky.

Comment 3

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

a) the baseFolder was just to give us somewhere to switch to, so that we could switch back, so that the more link would appear again.  If there's a better way to de-more-ify the headers, I'ld be happy to use it instead.

b1) test-junk-helpers.js and test-search-window-helpers.js grab folder-display-helper's mc, so maybe it's not that bad.

b2) On the other hand, perhaps, since we already call "fdh.installInto(module);", we can just use "msgGen.makeNamesAndAddresses", and avoid the ickyness?

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

a) putting baseFolder back seems fine, but please include a comment describing why it's there

Let's do b2.

Comment 5

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

Attachment: A patch with options a-ish and 2b.

So, the more should collapse when we switch to a different message, so I've gone that way instead, with comments.

But, this test still fails.  And if we change mail/themes/pinstripe/mail/messageHeader.css to add "background-color: red;" to #expandedHeaderView[show_header_mode="all"], we can see that, in fact, the show_header_mode="all" does persist.  But it doesn't when I'm running Shredder, and click on a different message.

asuth, any ideas what's going wrong here?  It's like AdjustHeaderView isn't getting called by updateExpandedView, which isn't getting called by UpdateExpandedMessageHeaders because document.getElementById('msgHeaderViewDeck').selectedIndex isn't 0 or gBuiltExpandedView is false/null…  (In mail/base/content/msgHdrViewOverlay.js, line 430.)

Thanks,
Blake.

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

Does it fail on both trunk and branch, or just one of the two?  In my recent experience, trunk still Has Issues (TM).

Comment 7

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

Fails on branch.

Update: The problem seems to be that I call getIdentityForHeader(msgHdr) in http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.js#870 and it returns [nsIMsgIdentity: id2].
In http://mxr.mozilla.org/comm-central/source/mail/test/mozmill/runtest.py#111, we define id1, but not id2.

I'm tracing into it more.

Later,
Blake.

Comment 8

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

Attachment: A patch which fixes the test.

Andrew, is there a reason account 1 didn't have any identities, or was it just a typo?  (If there's a reason, then I think I should probably create the messages in account 2.)

Thanks,
Blake.

Comment 9

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

Comment on attachment 413500 [details] [diff] [review]
A patch which fixes the test.

I doubt it was intentional.  I assume I just copied and pasted the lines from somewhere else and they either came that way or I botched the copy and paste.

Changing review for the mozmill-change to Standard8 since he really knows more about those things (and their multiplicity requirements) and it was probably his work that I copied and pasted from.

Adding dmose for review since the actual test needs review and I don't see a + in the history.

Comment 10

[Mark Banner (:standard8)]

Comment on attachment 413500 [details] [diff] [review]
A patch which fixes the test.

r=Standard8 for the runtest.py change (assuming this doesn't affect any other mozmill tests).

Please also ensure that we do a patch to update mailnews/test/performance/common/mailnewsTestPrefs.js for the bloat tests in a similar manner.

Comment 11

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

Attachment: The previous patch, with Standard8's suggestion.

Good catch.  Fixed.

Comment 12

[rsx11m]

That patch has been obsoleted by bug 550487 and its follow-up patches.
Anything else left to do here, or do you want to close this bug?

Comment 13

[rsx11m]

Sorry, this appears to be a follow-up on the subject-wrap bug 489609 rather, I may have mixed up the context seeing the more-button handling in this patch...

Comment 14

[Mark Banner (:standard8)]

Comment on attachment 415172 [details] [diff] [review]
The previous patch, with Standard8's suggestion.

Blake, this is bitrotted, any chance you could unbitrot and then re-request review (request from me if you want).

Comment 15

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

Attachment: The previous patch, de-bit-rotted.

(In reply to comment #14)
> Blake, this is bitrotted, any chance you could unbitrot and then re-request
> review (request from me if you want).

Yeah, sure.  Here you go.

I've requested review from you, but if dmose has the time, I think he was poking around in the code under test, so it might make sense for him to review it as well.

Thanks,
Blake.

Comment 16

[Dan Mosedale (:dmosedale, :dmose)]

I don't expect to have time for this soon, unfortunately.

Comment 17

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

Attachment: re-de-bit-rotting.

Carrying forward r=Standard8.

Comment 18

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/1328faef900b

Comment 19

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

And backed out: http://hg.mozilla.org/comm-central/rev/addb24b470d6

I'm recompiling (again) to try and update the affected tests, so that we can check in a new, working, patch.

Comment 20

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

Since the patch for bug 543701 seems to fix the tests that this breaks, I'm going to depend on it to check this in.  (On the plus side, Jim, that means that I'm _way_ more likely to quickly review any patch you post on that bug.  ;)

Thanks,
Blake.

Comment 21

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

Pushed as http://hg.mozilla.org/comm-central/rev/8356c5aebd6c