815283 - create test file to check proper operations of the Account manager account tree

Status: RESOLVED FIXED

(Thunderbird :: Account Manager, defect)

Description

[:aceman]

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]

Attachment: 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]

Comment on attachment 687520 [details] [diff] [review]
patch

Sorry, I don't know mozmill

Comment 3

[:aceman]

Can't you comment on the rest of the test?

Comment 4

[Mike Conley (:mconley) (:⚙️)]

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]

(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)]

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]

Comment on attachment 687520 [details] [diff] [review]
patch

Needs new patch after the given feedback.

Comment 8

[:aceman]

Attachment: 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) (:⚙️)]

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]

Attachment: patch v3

Thanks!

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/0cf5688a02e9