Last Comment Bug 726737 - convert mailnews/addrbook/test to Services.jsm and MailServices.js
: convert mailnews/addrbook/test to Services.jsm and MailServices.js
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Address Book (show other bugs)
: Trunk
: All All
: -- trivial (vote)
: Thunderbird 14.0
Assigned To: :aceman
:
Mentors:
Depends on: 722187
Blocks: 720356 720358
  Show dependency treegraph
 
Reported: 2012-02-13 12:27 PST by :aceman
Modified: 2012-03-19 15:30 PDT (History)
2 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (36.35 KB, patch)
2012-02-16 14:38 PST, :aceman
mconley: review-
Details | Diff | Splinter Review
patch v2 (51.88 KB, patch)
2012-03-03 07:35 PST, :aceman
mconley: review+
Details | Diff | Splinter Review
patch v3, fix nits. (51.85 KB, patch)
2012-03-19 13:20 PDT, :aceman
acelists: review+
Details | Diff | Splinter Review

Description :aceman 2012-02-13 12:27:35 PST
Followup to bug 722187. Convert the Addressbook remaining test files.
Comment 1 :aceman 2012-02-16 14:38:51 PST
Created attachment 598002 [details] [diff] [review]
patch
Comment 2 Mike Conley (:mconley) - (Needinfo me!) 2012-03-02 13:16:42 PST
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, "other@book.invalid");
>  
>    // 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 2012-03-02 13:30:04 PST
(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 2012-03-03 07:35:57 PST
Created attachment 602614 [details] [diff] [review]
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) - (Needinfo me!) 2012-03-19 08:46:49 PDT
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 2012-03-19 08:50:11 PDT
(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) - (Needinfo me!) 2012-03-19 08:51:30 PDT
(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 2012-03-19 13:20:11 PDT
Created attachment 607275 [details] [diff] [review]
patch v3, fix nits.

The tests still pass for me. Carrying over r=mconley.
Comment 9 Ryan VanderMeulen [:RyanVM] 2012-03-19 15:30:29 PDT
http://hg.mozilla.org/comm-central/rev/dca487087cf1

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