Closed Bug 726737 Opened 10 years ago Closed 10 years ago

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

Categories

(MailNews Core :: Address Book, defect)

defect
Not set
trivial

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 14.0

People

(Reporter: aceman, Assigned: aceman)

References

Details

Attachments

(1 file, 2 obsolete files)

Followup to bug 722187. Convert the Addressbook remaining test files.
Attached patch patch (obsolete) — Splinter Review
Attachment #598002 - Flags: review?(mconley)
Status: NEW → ASSIGNED
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?
Attachment #598002 - Flags: review?(mconley) → review-
(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.
Attached patch patch v2 (obsolete) — Splinter Review
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.
Attachment #598002 - Attachment is obsolete: true
Attachment #602614 - Flags: review?(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
Attachment #602614 - Flags: review?(mconley) → review+
(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?
(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).
The tests still pass for me. Carrying over r=mconley.
Attachment #602614 - Attachment is obsolete: true
Attachment #607275 - Flags: review+
Keywords: checkin-needed
http://hg.mozilla.org/comm-central/rev/dca487087cf1
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 14.0
You need to log in before you can comment on or make changes to this bug.