Last Comment Bug 815283 - create test file to check proper operations of the Account manager account tree
: create test file to check proper operations of the Account manager account tree
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Account Manager (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 21.0
Assigned To: :aceman
:
:
Mentors:
Depends on: 58713 536248 740617
Blocks:
  Show dependency treegraph
 
Reported: 2012-11-26 12:18 PST by :aceman
Modified: 2013-02-04 11:53 PST (History)
3 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (8.07 KB, patch)
2012-12-02 07:39 PST, :aceman
mconley: feedback+
Details | Diff | Splinter Review
patch v2 (7.82 KB, patch)
2013-01-05 13:03 PST, :aceman
mconley: review+
Details | Diff | Splinter Review
patch v3 (7.80 KB, patch)
2013-02-04 09:39 PST, :aceman
acelists: review+
Details | Diff | Splinter Review

Description :aceman 2012-11-26 12:18:16 PST
I can do a mozmill test to check correct operation of code added in bug 58713, bug 536248, bug 740617. Let's call it test-account-tree.js. Other tests can be added to it in the future.
Comment 1 :aceman 2012-12-02 07:39:08 PST
Created attachment 687520 [details] [diff] [review]
patch

Here is the patch.
I doubt it is OK to have a amc.sleep() in a test. But I have it there to prevent the "popup never opened" failures on linux. I still couldn't find the cause of that failure (that is tracked in other bugs). But when a sleep helps it, it looks like we need to wait for something that we currently don't do.

So please review the rest of the test and tell me if we can leave the sleep in, or remove it, as it probably isn't needed on other platforms. And also on linux it only fails randomly.
Comment 2 Ian Neal 2012-12-02 11:19:10 PST
Comment on attachment 687520 [details] [diff] [review]
patch

Sorry, I don't know mozmill
Comment 3 :aceman 2012-12-02 11:28:24 PST
Can't you comment on the rest of the test?
Comment 4 Mike Conley (:mconley) 2012-12-31 11:09:14 PST
Comment on attachment 687520 [details] [diff] [review]
patch

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

Just needs a little clean-up, but otherwise, I think you're on the right track.

::: mail/test/mozmill/account/test-account-tree.js
@@ +4,5 @@
> +
> +/**
> + * This test checks proper operation of the account tree in the Account manager.
> + *
> + * New checks can be added to it as needed.

This comment is superfluous, IMO.

@@ +13,5 @@
> +const RELATIVE_ROOT = "../shared-modules";
> +const MODULE_REQUIRES = ["folder-display-helpers", "window-helpers",
> +                         "account-manager-helpers"];
> +
> +let elib = {};

I don't think you need elib.

@@ +44,5 @@
> +}
> +
> +function teardownModule(module) {
> +  // Remove our test account to leave the profile clean.
> +//  MailServices.accounts.removeAccount(gPopAccount);

Why is this commented out?

@@ +122,5 @@
> +
> +  // We can't read the computed style of the tree cell directly, so at least see
> +  // if the isDefaultServer-true property is set on it. Hopefully the proper style
> +  // is attached to this property.
> +  let propArray = Components.classes["@mozilla.org/supports-array;1"]

You should be able to use Cc and Ci in Mozmill tests. I think you get them for free.

@@ +180,5 @@
> +
> +  // Get position of the current account in the account list.
> +  let accountIndex = accountList.indexOf(gPopAccount);
> +
> +  amc.sleep(1000);

Sleeping in tests is something we try to avoid... and I don't see this is adding anything. It should be removed.
Comment 5 :aceman 2013-01-02 23:49:06 PST
(In reply to Mike Conley (:mconley) from comment #4)
> Review of attachment 687520 [details] [diff] [review]:
> @@ +180,5 @@
> > +
> > +  // Get position of the current account in the account list.
> > +  let accountIndex = accountList.indexOf(gPopAccount);
> > +
> > +  amc.sleep(1000);
> 
> Sleeping in tests is something we try to avoid... and I don't see this is
> adding anything. It should be removed.

The sleep makes the test work on Linux, see comment 1. So what can we do instead?
Comment 6 Mark Banner (:standard8, limited time in Dec) 2013-01-03 05:12:28 PST
Comment on attachment 687520 [details] [diff] [review]
patch

I'm going to pass this review to Mike.

Not sure what we can do about the sleep, at a minimum a comment as to why it is there would be good :-)
Comment 7 :aceman 2013-01-03 05:18:52 PST
Comment on attachment 687520 [details] [diff] [review]
patch

Needs new patch after the given feedback.
Comment 8 :aceman 2013-01-05 13:03:01 PST
Created attachment 698332 [details] [diff] [review]
patch v2

Ok, we can remove the sleep and let it intermittently fail as some of the other tests on Linux. There must be a common problem in all of them.
Comment 9 Mike Conley (:mconley) 2013-02-03 14:51:58 PST
Comment on attachment 698332 [details] [diff] [review]
patch v2

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

Just some style nits, but otherwise this looks great. Thanks aceman!

::: mail/test/mozmill/account/test-account-tree.js
@@ +121,5 @@
> +  let propArray = Cc["@mozilla.org/supports-array;1"]
> +                    .createInstance(Ci.nsISupportsArray);
> +
> +  accountTree.view.getCellProperties(accountRow, accountTree.columns.getColumnAt(0),
> +                                                 propArray);

Nit - please fix funky indentation here.

@@ +124,5 @@
> +  accountTree.view.getCellProperties(accountRow, accountTree.columns.getColumnAt(0),
> +                                                 propArray);
> +  assert_equals(propArray.Count(), 1);
> +  assert_equals(propArray.QueryElementAt(0, Ci.nsIAtom).toString(), "isDefaultServer-true");
> +

Not - please remove the extra newline.

@@ +133,5 @@
> +
> +  cell = accountTree.view.getItemAtIndex(accountRow).firstChild.firstChild;
> +  propArray.Clear();
> +  accountTree.view.getCellProperties(accountRow, accountTree.columns.getColumnAt(0),
> +                                                 propArray);

Nit - same funky indentation as before.

@@ +137,5 @@
> +                                                 propArray);
> +  // There should be no properties set on its tree cell.
> +  assert_equals(propArray.Count(), 0);
> +}
> +/**

Nit - please put a newline before the comment block.
Comment 10 :aceman 2013-02-04 09:39:55 PST
Created attachment 709756 [details] [diff] [review]
patch v3

Thanks!
Comment 11 Ryan VanderMeulen [:RyanVM] 2013-02-04 11:53:33 PST
https://hg.mozilla.org/comm-central/rev/0cf5688a02e9

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