The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Thunderbird 15.0

Status

MailNews Core
Import
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: hiro, Assigned: hiro)

Tracking

(Depends on: 1 bug)

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

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

6 years ago
We need an import module to test import implementation in mailnews core to avoid regression such as bug 698987.
(Assignee)

Comment 1

6 years ago
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.
Attachment #573079 - Flags: review?(dbienvenu)
(Assignee)

Comment 2

6 years ago
Created attachment 573080 [details] [diff] [review]
Part2 - the import module

This is it.
Attachment #573080 - Flags: review?
(Assignee)

Comment 3

6 years ago
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.
Attachment #573081 - Flags: review?(dbienvenu)
(Assignee)

Updated

6 years ago
Attachment #573080 - Flags: review? → review?(dbienvenu)

Updated

6 years ago
Assignee: nobody → hiikezoe
Blocks: 684455

Comment 4

6 years ago
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

6 years ago
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...
Attachment #573079 - Flags: review?(dbienvenu) → review?(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.
Attachment #573079 - Flags: review?(mconley) → review+
(Assignee)

Comment 7

5 years ago
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
Attachment #573079 - Attachment is obsolete: true
Attachment #575002 - Flags: review+
(Assignee)

Updated

5 years ago
Blocks: 703175
(Assignee)

Comment 8

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

Updated

5 years ago
Blocks: 124486
(Assignee)

Updated

5 years ago
Depends on: 729676
(Assignee)

Updated

5 years ago
Attachment #573080 - Flags: review?(dbienvenu)
(Assignee)

Updated

5 years ago
Attachment #573081 - Flags: review?(dbienvenu)
(Assignee)

Comment 9

5 years ago
Created attachment 600566 [details] [diff] [review]
Rewrite with new ImportMailbox interface
Attachment #600566 - Flags: review?
(Assignee)

Comment 10

5 years ago
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.
Depends on: 730498
Attachment #600566 - Flags: review? → review?(dbienvenu)

Comment 11

5 years ago
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?
Attachment #600566 - Flags: review?(dbienvenu) → review+
(Assignee)

Comment 12

5 years ago
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.
Attachment #600566 - Attachment is obsolete: true
Attachment #604204 - Flags: review+

Comment 13

5 years ago
the two revised patches are ready for landing, I believe. Hiro, if that's not correct, let us know, thx!
Keywords: checkin-needed
Attachment #573080 - Attachment is obsolete: true
Attachment #573081 - Attachment is obsolete: true
https://hg.mozilla.org/comm-central/rev/8a729f2c3243
https://hg.mozilla.org/comm-central/rev/4078e23e6057
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 15.0
You need to log in before you can comment on or make changes to this bug.