Last Comment Bug 700920 - Need an import module to test import implementation in mailnews core.
: Need an import module to test import implementation in mailnews core.
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Import (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 15.0
Assigned To: Hiroyuki Ikezoe (:hiro)
:
Mentors:
Depends on: 730498 729676
Blocks: 124486 684455 703175
  Show dependency treegraph
 
Reported: 2011-11-08 20:25 PST by Hiroyuki Ikezoe (:hiro)
Modified: 2012-05-24 15:31 PDT (History)
7 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Part 1 - preparation of the import module . Create MailImportHelper similar to AbImportHelper (21.79 KB, patch)
2011-11-08 21:44 PST, Hiroyuki Ikezoe (:hiro)
mconley: review+
Details | Diff | Review
Part2 - the import module (5.38 KB, patch)
2011-11-08 21:45 PST, Hiroyuki Ikezoe (:hiro)
no flags Details | Diff | Review
The test for bug 698987 (1.76 KB, patch)
2011-11-08 21:47 PST, Hiroyuki Ikezoe (:hiro)
no flags Details | Diff | Review
Revised part 1 patch (21.54 KB, patch)
2011-11-16 14:24 PST, Hiroyuki Ikezoe (:hiro)
hiikezoe: review+
Details | Diff | Review
Rewrite with new ImportMailbox interface (5.62 KB, patch)
2012-02-24 16:38 PST, Hiroyuki Ikezoe (:hiro)
mozilla: review+
Details | Diff | Review
Revised patch (5.62 KB, patch)
2012-03-08 14:36 PST, Hiroyuki Ikezoe (:hiro)
hiikezoe: review+
Details | Diff | Review

Description Hiroyuki Ikezoe (:hiro) 2011-11-08 20:25:47 PST
We need an import module to test import implementation in mailnews core to avoid regression such as bug 698987.
Comment 1 Hiroyuki Ikezoe (:hiro) 2011-11-08 21:44:43 PST
Created attachment 573079 [details] [diff] [review]
Part 1 - preparation of the import module . Create MailImportHelper similar to AbImportHelper

This patch modifies the AbImportHelper.

- Create GenericImportHelper class which is parent of AbImportHelper and MailImportHelper.
- Remove 'aType' argument from constructor of AbImportHelper. The type is defined by the file extension(i.e. '.csv' or '.ldif').
- To avoid confusable, change module name to search for. '.csv' -> 'Text file' 

MailImportHelper compares imported mail folders to the directory which is pointed by the third argument of the constructor of MailImportHelper. Currently it compares just directory structures not files in the directory. I mean, it compares tree structure of the mail folders and the directory for test of bug 698987.
Comment 2 Hiroyuki Ikezoe (:hiro) 2011-11-08 21:45:19 PST
Created attachment 573080 [details] [diff] [review]
Part2 - the import module

This is it.
Comment 3 Hiroyuki Ikezoe (:hiro) 2011-11-08 21:47:20 PST
Created attachment 573081 [details] [diff] [review]
The test for bug 698987

I just want this!

Mercurial does not allow empty directory, so I just add some empty file in directories which are used in the test code.
Comment 4 David :Bienvenu 2011-11-11 15:01:23 PST
thx very much for the patches. They look reasonable, but they crash when run in debug mode, because the import code causes a lot of thread-safety assertions, and assertions make xpcshell tests fail. I'm not sure if there's a way for a given xpcshell test to turn off assertions. Fixing the assertions in the import code is ultimately the way to go, but that would require some significant re-structuring of the code, I think.

I also didn't see this test run when I did make xpcshell-tests from the mailnews directory. Not sure what that is, because it looks like you've done the right thing with the makefiles and xpcshell.ini files.
Comment 5 David :Bienvenu 2011-11-11 15:10:44 PST
Comment on attachment 573079 [details] [diff] [review]
Part 1 - preparation of the import module . Create MailImportHelper similar to AbImportHelper

Thx again for the patch.

This comment is no longer right, since it refers to addressbook:

+GenericImportHelper.prototype =
+{
+  /**
+   * GenericImportHelper.beginImport
+   * Imports the given address book export and checks the imported address book
+   * with the array in addressbook.json if aAbName and aJsonName were supplied
+   * to the constructor.
+   */

Other than that, it looks OK, but I'm going to pass the review to mconley, since he's more up to speed on AB things...
Comment 6 Mike Conley (:mconley) - (Needinfo me!) 2011-11-14 11:38:45 PST
Comment on attachment 573079 [details] [diff] [review]
Part 1 - preparation of the import module . Create MailImportHelper similar to AbImportHelper

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

So this looks OK to me.  I'm not a huge fan of the explicit checks for the type of the module ("mail", "addressbook").  To me, that's just screaming for a factory - but I think, now looking at your code, that that might be some overengineering - since you need those "mail", "addressbook" strings to get the import interface.

Also, I found an erroneous comment for GenericImportHelper.

Other than that, it looks pretty good. r=me, modulo the comment change, and a try build passing all of the tests.

::: mailnews/import/test/unit/resources/import_helper.js
@@ +4,5 @@
>  // used by checkProgress to periodically check the progress of the import
> +var gGenericImportHelper;
> +/**
> + * GenericImportHelper
> + * A helper for Address Book imports. To use, supply at least the file and type.

Hrm - I don't think this comment is true anymore, since (as far as I understand), GenericImportHelper is a generalized import helper for both address books and mail.
Comment 7 Hiroyuki Ikezoe (:hiro) 2011-11-16 14:24:54 PST
Created attachment 575002 [details] [diff] [review]
Revised part 1 patch

Carrying over review+

Addressing review comments.

I pushed a try, but unfortunately there is an issue to run xpcshell tests. It seems to be caused by missing md5 module of python.

http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=30246285b538
Comment 8 Hiroyuki Ikezoe (:hiro) 2011-11-25 00:18:23 PST
I re-tried two tries.

http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=f08f98025c52

Unfortunately this time xpcshell tests in toolkit/places failed on WinXP. So I tried again on WinXP.

http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=cab9de82424b

This time all xpcshell tests passed.
Comment 9 Hiroyuki Ikezoe (:hiro) 2012-02-24 16:38:51 PST
Created attachment 600566 [details] [diff] [review]
Rewrite with new ImportMailbox interface
Comment 10 Hiroyuki Ikezoe (:hiro) 2012-02-24 16:43:16 PST
The test in comment #3 causes the following assertion:

###!!! ASSERTION: nsCOMArrayEnumerator not thread-safe: '_mOwningThread.GetThread() == PR_GetCurrentThread()', file /home/zoe/hg/comm-central/objdir-thunderbird/mozilla/xpcom/build/nsArrayEnumerator.cpp, line 161

I've filed bug 730498 for the assertion.
Comment 11 David :Bienvenu 2012-03-05 16:51:40 PST
Comment on attachment 600566 [details] [diff] [review]
Rewrite with new ImportMailbox interface

looks good, one nit,

+    var name = Cc["@mozilla.org/supports-string;1"]
+               .createInstance(Ci.nsISupportsString);

can you use let instead of var here?
Comment 12 Hiroyuki Ikezoe (:hiro) 2012-03-08 14:36:55 PST
Created attachment 604204 [details] [diff] [review]
Revised patch

> +    var name = Cc["@mozilla.org/supports-string;1"]
> +               .createInstance(Ci.nsISupportsString);
> 
> can you use let instead of var here?

Done.
Comment 13 David :Bienvenu 2012-05-24 10:12:26 PDT
the two revised patches are ready for landing, I believe. Hiro, if that's not correct, let us know, thx!

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