700920 - Need an import module to test import implementation in mailnews core.

Status: RESOLVED FIXED

(MailNews Core :: Import, defect)

Description

[Hiroyuki Ikezoe (:hiro)]

We need an import module to test import implementation in mailnews core to avoid regression such as bug 698987.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Attachment: 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)]

Attachment: Part2 - the import module

This is it.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Attachment: 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]

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]

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) (:⚙️)]

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)]

Attachment: 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)]

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)]

Attachment: Rewrite with new ImportMailbox interface

Comment 10

[Hiroyuki Ikezoe (:hiro)]

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]

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)]

Attachment: 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]

the two revised patches are ready for landing, I believe. Hiro, if that's not correct, let us know, thx!

Comment 14

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/comm-central/rev/8a729f2c3243
https://hg.mozilla.org/comm-central/rev/4078e23e6057