The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Thunderbird 14.0

Status

MailNews Core
Address Book
--
trivial
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: aceman, Assigned: aceman)

Tracking

(Blocks: 2 bugs)

Trunk
Thunderbird 14.0
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

51.85 KB, patch
aceman
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
Followup to bug 722187. Convert the Addressbook remaining test files.
(Assignee)

Comment 1

5 years ago
Created attachment 598002 [details] [diff] [review]
patch
Attachment #598002 - Flags: review?(mconley)
(Assignee)

Updated

5 years ago
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-
(Assignee)

Comment 3

5 years ago
(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.
(Assignee)

Comment 4

5 years ago
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.
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+
(Assignee)

Comment 6

5 years ago
(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).
(Assignee)

Comment 8

5 years ago
Created attachment 607275 [details] [diff] [review]
patch v3, fix nits.

The tests still pass for me. Carrying over r=mconley.
Attachment #602614 - Attachment is obsolete: true
Attachment #607275 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
http://hg.mozilla.org/comm-central/rev/dca487087cf1
Status: ASSIGNED → RESOLVED
Last Resolved: 5 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.