726737 - convert mailnews/addrbook/test to Services.jsm and MailServices.js

Status: RESOLVED FIXED

(MailNews Core :: Address Book, defect)

Description

[:aceman]

Followup to bug 722187. Convert the Addressbook remaining test files.

Comment 1

[:aceman]

Attachment: patch

Comment 2

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

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

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

Hey aceman,

Another great patch.  This looks really good - just a few minor points.

Also, can I assume these tests are passing locally for you?

-Mike

::: mailnews/addrbook/test/unit/test_basic_nsIAbDirectory.js
@@ +26,5 @@
>  
>  // Main function for the this test so we can check both personal and
>  // collected books work correctly in an easy manner.
>  function check_ab(abConfig) {
> +  let gPref = Services.prefs;

Why do we need gPref as an alias for Services.prefs?  If there's no good reason, then we should probably just stick with Services.prefs.

::: mailnews/addrbook/test/unit/test_collection_2.js
@@ +44,5 @@
>    do_check_eq(card.displayName, "Other Book");
>    do_check_eq(card.primaryEmail, "[e-mail]");
>  
>    // Check the CAB has no cards.
> +  var CAB = MailServices.ab.getDirectory(kCABData.URI);

We prefer let over var now.

::: mailnews/addrbook/test/unit/test_db_enumerator.js
@@ +2,5 @@
>   * This test verifies that we don't crash if we have an enumerator on an
>   * addr database and delete the underlying directory, which forces the ab
>   * closed.
>   */
> +var abm             = MailServices.ab;

We prefer let over var now.  Also, why do we need the abm alias now?  We can just use MailServices.ab.

::: mailnews/addrbook/test/unit/test_nsAbAutoCompleteSearch2.js
@@ +6,5 @@
>   * We run this test without address books, constructing manually ourselves,
>   * so that we can ensure that we're not getting the data out of the address
>   * books.
>   */
>  

Why were XPCOMUtils taken out?  Was it superfluous?

::: mailnews/addrbook/test/unit/test_nsAbManager1.js
@@ +6,4 @@
>  const nsIAbListener = Components.interfaces.nsIAbListener;
>  const numListenerOptions = 4;
>  
> +var gAbManager = MailServices.ab;

Why the alias?

::: mailnews/addrbook/test/unit/test_nsAbManager2.js
@@ +7,5 @@
>  const nsIAbDirectory = Components.interfaces.nsIAbDirectory;
>  const nsIAbListener = Components.interfaces.nsIAbListener;
>  const numListenerOptions = 4;
>  
> +var gAbManager = MailServices.ab;

Why the alias?

Comment 3

[:aceman]

(In reply to Mike Conley (:mconley) from comment #2)
> ::: mailnews/addrbook/test/unit/test_nsAbAutoCompleteSearch2.js
> @@ +6,5 @@
> >   * We run this test without address books, constructing manually ourselves,
> >   * so that we can ensure that we're not getting the data out of the address
> >   * books.
> >   */
> >  
> 
> Why were XPCOMUtils taken out?  Was it superfluous?
I think so, as it is already included in mailnews/test/resources/mailDirService.js .
Am I wrong? I think I tested the tests still work.

Comment 4

[:aceman]

Attachment: patch v2

Ok, fixed the nits. Also updated one prefs call in abSetup.js and removed abmgr in test_uuid.js.

These xpcshell tests pass for me locally.

Comment 5

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

Comment on attachment 602614 [details] [diff] [review]
patch v2

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

This looks good - thanks aceman!

Just a few tiny nits.

Assuming all of these tests continue to pass, and if the nits I found are fixed, r=me.

::: mailnews/addrbook/test/unit/test_collection.js
@@ +256,3 @@
>    // XXX Getting all directories ensures we create all ABs because the
>    // address collecter can't currently create ABs itself (bug 314448).
> +  let temp = MailServices.ab.directories;

we don't need let temp for this - just MailServices.ab.directories will do the job.

::: mailnews/addrbook/test/unit/test_collection_2.js
@@ +27,5 @@
>    testAB.copyTo(gProfileDir, kPABData.fileName);
>  
>    // XXX Getting all directories ensures we create all ABs because the
>    // address collecter can't currently create ABs itself (bug 314448).
> +  let temp = MailServices.ab.directories;

we don't need let temp for this - just MailServices.ab.directories will do the job.

::: mailnews/addrbook/test/unit/test_mailList1.js
@@ +42,3 @@
>    // XXX Getting all directories ensures we create all ABs because mailing
>    // lists need help initialising themselves
> +  let temp = MailServices.ab.directories;

we don't need let temp for this - just MailServices.ab.directories will do the job.

::: mailnews/addrbook/test/unit/test_uuid.js
@@ +42,5 @@
>  
>    // Question 3: Do cards returned via searches return UUIDs correctly?
>    var uri = directory.URI;
>    uri += "?(or(DisplayName,=,a)(DisplayName,!=,a))";
> +  var search = MailServices.ab.getDirectory(uri);

let instead of var

Comment 6

[:aceman]

(In reply to Mike Conley (:mconley) from comment #5)
> ::: mailnews/addrbook/test/unit/test_collection.js
> @@ +256,3 @@
> >    // XXX Getting all directories ensures we create all ABs because the
> >    // address collecter can't currently create ABs itself (bug 314448).
> > +  let temp = MailServices.ab.directories;
You mean just calling it without assigning the result into temp?

Comment 7

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

(In reply to :aceman from comment #6)
> (In reply to Mike Conley (:mconley) from comment #5)
> > ::: mailnews/addrbook/test/unit/test_collection.js
> > @@ +256,3 @@
> > >    // XXX Getting all directories ensures we create all ABs because the
> > >    // address collecter can't currently create ABs itself (bug 314448).
> > > +  let temp = MailServices.ab.directories;
> You mean just calling it without assigning the result into temp?

Correct - this happens a few times in the tests.  It's a hack to get the abManager to "build" the address books.  Assigning the value to temp is misleading, since it implies that temp will be used in the future (which is not the case, as far as I can tell).

Comment 8

[:aceman]

Attachment: patch v3, fix nits.

The tests still pass for me. Carrying over r=mconley.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

http://hg.mozilla.org/comm-central/rev/dca487087cf1