Last Comment Bug 738719 - Run mozmill tests with an IM account in the default profile
: Run mozmill tests with an IM account in the default profile
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Instant Messaging (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 18.0
Assigned To: Florian Quèze [:florian] [:flo] (PTO until August 29th)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-23 11:14 PDT by Florian Quèze [:florian] [:flo] (PTO until August 29th)
Modified: 2012-09-14 06:54 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch (3.80 KB, patch)
2012-03-23 11:14 PDT, Florian Quèze [:florian] [:flo] (PTO until August 29th)
mozilla: feedback+
standard8: feedback+
Details | Diff | Splinter Review
Patch v2 (5.10 KB, patch)
2012-09-14 06:22 PDT, Florian Quèze [:florian] [:flo] (PTO until August 29th)
mconley: review+
Details | Diff | Splinter Review

Description Florian Quèze [:florian] [:flo] (PTO until August 29th) 2012-03-23 11:14:06 PDT
Created attachment 608784 [details] [diff] [review]
Patch

(In reply to David :Bienvenu from bug 736486 comment #3)
> I wonder if it would possible to tweak the mozmill tests to define an im
> account in the default profile and see if any of the tests break in
> interesting ways.

I've tried this, here are the results:

- The only test that is consistently broken is mail/test/mozmill/account/test-archive-options.js because it hardcodes the index of accounts in the Account Settings dialog.

- The other failures I saw seem random (although I wouldn't guarantee it, I'm not expert yet at the art of reading these obfuscated test result logs).

- I also noticed some noise in the log that can (and I think should) be fixed (bug 738711).

I'm attaching the changes I made to try this, so that others can try too.

I don't know if we want to integrate this patch, so requesting feedback instead of review.
Comment 1 Florian Quèze [:florian] [:flo] (PTO until August 29th) 2012-03-27 10:20:56 PDT
Comment on attachment 608784 [details] [diff] [review]
Patch

I'm wondering if this is something we will want to push to comm-central or if it's too hackish. I've just asked on IRC, David thinks we should do it but would like to have your opinion too.

I definitely wouldn't land this before looking at try server build results to see if it creates perma-oranges that weren't visible on my machine.
Comment 2 Mark Banner (:standard8) 2012-05-24 14:52:26 PDT
Comment on attachment 608784 [details] [diff] [review]
Patch

I don't think this is too hackish as its based on what we've done before - it would be nice to have tests for chat account creation though when we get that far.
Comment 3 Ludovic Hirlimann [:Usul] 2012-06-11 05:47:45 PDT
florian ?
Comment 4 Florian Quèze [:florian] [:flo] (PTO until August 29th) 2012-09-14 06:22:32 PDT
Created attachment 661196 [details] [diff] [review]
Patch v2

Now that Thunderbird ships with IM on by default, I think we should move forward here.
I agree that "it would be nice to have tests for chat account creation" (comment 1) but I think we can do it in a separate bug.

I pushed attachment 608784 [details] [diff] [review] to try and it failed consistently (https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=118d7e1ebfb8).

The error log is:
SUMMARY-UNEXPECTED-FAIL | test-newmailaccount.js | test-newmailaccount.js::test_get_an_account
  EXCEPTION: account.defaultIdentity is null
    at: test-newmailaccount-helpers.js line 166
       remove_email_account test-newmailaccount-helpers.js 166
       test_get_an_account test-newmailaccount.js 167
       Runner.prototype.wrapper frame.js 582
       Runner.prototype._runTestModule frame.js 652
       Runner.prototype.runTestModule frame.js 698
       Runner.prototype.runTestDirectory frame.js 522
       runTestDirectory frame.js 704
       Bridge.prototype._execFunction server.js 179
       Bridge.prototype.execFunction server.js 183
SUMMARY-UNEXPECTED-FAIL | test-newmailaccount.js | test-newmailaccount.js::test_restored_ap_tab_works
  EXCEPTION: account.defaultIdentity is null
    at: test-newmailaccount-helpers.js line 166
       remove_email_account test-newmailaccount-helpers.js 166
       test_get_an_account test-newmailaccount.js 167
       test_restored_ap_tab_works test-newmailaccount.js 227
       Runner.prototype.wrapper frame.js 582
       Runner.prototype._runTestModule frame.js 652
       Runner.prototype.runTestModule frame.js 698
       Runner.prototype.runTestDirectory frame.js 522
       runTestDirectory frame.js 704
       Bridge.prototype._execFunction server.js 179
       Bridge.prototype.execFunction server.js 183


This new attachment fixes this. I pushed it to try, and things look OK (https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=869169ae802c).
Comment 5 Mike Conley (:mconley) - (Needinfo me!) 2012-09-14 06:29:58 PDT
Comment on attachment 661196 [details] [diff] [review]
Patch v2

Review of attachment 661196 [details] [diff] [review]:
-----------------------------------------------------------------

Drive-by - this mostly looks good to me.

::: mail/test/mozmill/account/test-archive-options.js
@@ +43,5 @@
>    // XXX: This is pretty brittle, and assumes 1) that there are 8 items in each
>    // account's tree, and 2) that the order of the accounts is as we expect.
> +  // The + 1 when index != 0 is for the line used by the IRC account,
> +  // which is at the second position.
> +  click_account_tree_row(amc, index*8 + 2 + (index ? 1 : 0));

Spaces on either side of the *
Comment 6 Florian Quèze [:florian] [:flo] (PTO until August 29th) 2012-09-14 06:38:05 PDT
Comment on attachment 661196 [details] [diff] [review]
Patch v2

Mike has already looked today and can r+ this, so redirecting the review to him.
Comment 7 Mike Conley (:mconley) - (Needinfo me!) 2012-09-14 06:40:27 PDT
Comment on attachment 661196 [details] [diff] [review]
Patch v2

Review of attachment 661196 [details] [diff] [review]:
-----------------------------------------------------------------

Yep, this looks good (and thanks for the try build). With the nit I found fixed, r=me.

Thanks!
Comment 8 Florian Quèze [:florian] [:flo] (PTO until August 29th) 2012-09-14 06:54:20 PDT
https://hg.mozilla.org/comm-central/rev/8bbb9f722e7b

Note You need to log in before you can comment on or make changes to this bug.