Last Comment Bug 600798 - Implement unit tests for importing vcards and csv files into address book
: Implement unit tests for importing vcards and csv files into address book
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Import (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 17.0
Assigned To: Mark Banner (:standard8)
:
Mentors:
Depends on: 79709 142123
Blocks: 603265
  Show dependency treegraph
 
Reported: 2010-09-30 01:39 PDT by Mark Banner (:standard8)
Modified: 2012-08-20 08:42 PDT (History)
3 users (show)
standard8: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Original Test Cases from bug 79709 (7.95 KB, patch)
2010-09-30 01:39 PDT, Mark Banner (:standard8)
no flags Details | Diff | Review
Updated patch (8.43 KB, patch)
2012-08-13 08:59 PDT, Mark Banner (:standard8)
mconley: review-
Details | Diff | Review
Updated patch v2 (8.73 KB, patch)
2012-08-20 03:03 PDT, Mark Banner (:standard8)
mconley: review+
Details | Diff | Review

Description Mark Banner (:standard8) 2010-09-30 01:39:03 PDT
Created attachment 479722 [details] [diff] [review]
Original Test Cases from bug 79709

In bug 79709 we wrote some unit tests for importing vcards and cvs files. However due to the current threadsafe assertions (bug 142123), these couldn't be committed due to breaking developer's test runs.

This bug should look at getting these test cases landed.
Comment 1 Mark Banner (:standard8) 2012-08-13 08:59:53 PDT
Created attachment 651407 [details] [diff] [review]
Updated patch

This patch just updates the original test cases to the current format, now that the assertion bug is fixed these run and pass fine.
Comment 2 Mike Conley (:mconley) - (needinfo me!) 2012-08-17 13:48:26 PDT
Comment on attachment 651407 [details] [diff] [review]
Updated patch

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

Good work, Mark!

Just a few minor points / questions - nothing major.

Thanks,

-Mike

::: mailnews/import/test/unit/resources/basic_vcard_addressbook.vcf
@@ +1,2 @@
> +BEGIN:VCARD
> +VERSION:3.0

We don't actually support vCard >= 2.1 .... so this might be a bit deceptive.

::: mailnews/import/test/unit/resources/import_helper.js
@@ +161,5 @@
> +    this.setFieldMap(this.getDefaultFieldMap(true));
> +  } else if (endsWith(this.mFile.leafName.toLowerCase(), ".vcf")) {
> +    this.mSupportedAttributes = supportedAttributes;
> +  };
> +  

Trailing whitespace

::: mailnews/import/test/unit/test_csv_import.js
@@ +11,5 @@
> +function run_test()
> +{
> +  // Due to the import code using nsIAbManager off the main thread, we need
> +  // to ensure that it is initialized before we start the main test.
> +  var abMgr = Cc["@mozilla.org/abmanager;1"].getService(Ci.nsIAbManager);

Can't use MailServices.ab?

@@ +13,5 @@
> +  // Due to the import code using nsIAbManager off the main thread, we need
> +  // to ensure that it is initialized before we start the main test.
> +  var abMgr = Cc["@mozilla.org/abmanager;1"].getService(Ci.nsIAbManager);
> +
> +  var file = do_get_file("resources/basic_csv_addressbook.csv");

let over var

::: mailnews/import/test/unit/test_vcard_import.js
@@ +11,5 @@
> +function run_test()
> +{
> +  // Due to the import code using nsIAbManager off the main thread, we need
> +  // to ensure that it is initialized before we start the main test.
> +  var abMgr = Cc["@mozilla.org/abmanager;1"].getService(Ci.nsIAbManager);

let over var. Also, I guess there's no ability to use MailServices.ab?

@@ +15,5 @@
> +  var abMgr = Cc["@mozilla.org/abmanager;1"].getService(Ci.nsIAbManager);
> +  var file;
> +
> +  // test regular import (e.g. from file exported by another mail client)
> +  file = do_get_file("resources/basic_vcard_addressbook.vcf");

Might as well merge this with line 16, and do

let file = do_get_file("resources/basic_vcard_addressbook.vcf");
Comment 3 Mark Banner (:standard8) 2012-08-20 03:03:11 PDT
Created attachment 653326 [details] [diff] [review]
Updated patch v2

The only reason not to use MailServices was it wasn't there at the time the tests were originally written ;-)
Comment 4 Mike Conley (:mconley) - (needinfo me!) 2012-08-20 08:08:01 PDT
Comment on attachment 653326 [details] [diff] [review]
Updated patch v2

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

3 tiny nits - other than that, this looks good. Thanks Mark!

::: mailnews/import/test/unit/resources/basic_vcard_addressbook.vcf
@@ +4,5 @@
> +N:Doe;John;;;
> +EMAIL;TYPE=INTERNET:john.doe@genericemail.com
> +END:VCARD
> +BEGIN:VCARD
> +VERSION:3.0

2.1 here

::: mailnews/import/test/unit/resources/emptylines_vcard_addressbook.vcf
@@ +1,4 @@
> +
> +
> +BEGIN:VCARD
> +VERSION:3.0

2.1 here

@@ +8,5 @@
> +END:VCARD
> +
> +
> +BEGIN:VCARD
> +VERSION:3.0

2.1 here
Comment 5 Mark Banner (:standard8) 2012-08-20 08:42:48 PDT
Checked in: https://hg.mozilla.org/comm-central/rev/15bfe7e9d763

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