421050 - Need unit tests for applemail import

Status: NEW

(MailNews Core :: Import, defect)

Description

[Håkan Waara]

We need a way to test the mailnews importers, so we can safely rearchitecture that code as needed.

This bug probably involves:

1) Creating some kind of testing harness, or JS import component.
2) Finding suitable test data (see bug 334402)

Comment 1

[Håkan Waara]

OK, I'm gonna morph this bug to be a little more specific: providing a testing system for mailnews/import.

I already have some WIP that I hope to get working, that would drive the import from any importer, of course the first one for me to get working would be the applemail importer.

Comment 2

[Håkan Waara]

Attachment: Fixes needed to get further, v1

Here are two fixes that are needed to even get to the BeginImport() stage of my new test_import.js unit test. 

1. Squash a bogus warning that we get in sMsgDBService::OpenFolderDB() for each new local folder:

WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80550006: file /Users/hakan/Programmering/mozilla/hg/applemail/mozilla/mailnews/db/msgdb/src/nsMsgDatabase.cpp, line 153

We shouldn't use ENSURE_SUCCESS (which warns) here, because there are two error values that are "expected" (and handled by the caller) at that point in the flow and those are NS_MSG_ERROR_FOLDER_SUMMARY_MISSING and NS_MSG_ERROR_FOLDER_SUMMARY_OUT_OF_DATE.

2. The second fix is that while creating subfolders - in this case the local folders "Sent Messages", "Thrash", etc - we are iterating a local directory while with nsIDirectoryEnumerator while changing its contents. The behaviour for that is undefined! 

In my case, it consistently found "Sent Messages" twice and bailed out because of that, causing the creation of the "Apple Mail Import" folder to fail, and by extension the whole import test.

The fix is to first store away the contents of the directory in an array, and then iterate through the array while creating the subfolders.

Comment 3

[Mark Banner (:standard8)]

Comment on attachment 315269 [details] [diff] [review]
Fixes needed to get further, v1

Good work, just needs a couple of tweaks:

>diff -r ef03b9ca5a97 mailnews/db/msgdb/src/nsMsgDatabase.cpp

>@@ -150,7 +150,8 @@ NS_IMETHODIMP nsMsgDBService::OpenFolderDB
>         msgDatabase->SyncCounts();
>     }
>   }
>-  NS_ENSURE_SUCCESS(rv, rv);
>+  if (NS_FAILED(rv))
>+    return rv;
>   for (PRInt32 listenerIndex = 0; listenerIndex < m_foldersPendingListeners.Count(); listenerIndex++)

Given that the if (NS_SUCCEEDED(rv)... doesn't modify rv, and won't be executed if rv has failed, lets move this before that section.

Additionally, how about we do:

if (rv == NS_MSG_ERROR_FOLDER_SUMMARY_MISSING ||
    rv == NS_MSG_ERROR_FOLDER_SUMMARY_OUT_OF_DATE)
  return rv;

NS_ENSURE_SUCCESS(rv, rv);

Its a couple of checks slower, but it'll still be explicit and we'll at least know that we are intending that those are valid error codes there (though I'm open to debate on this).

Whilst you're in the area, would you mind expanding the comment in nsIMsgDatabase.idl to say that these are possible valid exceptions?

>diff -r ef03b9ca5a97 mailnews/local/src/nsLocalMailFolder.cpp

>   PRBool hasMore;
>-  directoryEnumerator->HasMoreElements(&hasMore);
>-  while (hasMore && NS_SUCCEEDED(rv))
>-  {
>+  while (NS_SUCCEEDED(directoryEnumerator->HasMoreElements(&hasMore)) && hasMore) {
>     nsCOMPtr<nsISupports> aSupport;
>     rv = directoryEnumerator->GetNext(getter_AddRefs(aSupport));

Given that you're not checking rv, no point in assigning it here.

>-    nsCOMPtr<nsIFile> currentFolderPath(do_QueryInterface(aSupport, &rv));
>+    nsCOMPtr<nsIFile> currentFile(do_QueryInterface(aSupport, &rv));

Either check rv here, or check that currentFile isn't null so that we will know that when you do this:

>+    currentDirEntries.AppendObject(currentFile);

we don't get null pointers later on (and hence we won't crash).

>+  // add the folders
>+  for (int i=0; i<currentDirEntries.Count(); ++i) {

nit: I think the following format is generally preferred:

for (int i = 0; i < count; ++i)

and the brackets in this file, appear to be in the other format:

if (...)
{
...
}

Comment 4

[Mark Banner (:standard8)]

Attachment: unit test for .subFolders call on local accounts/folders

This is the unit test to go with attachment 315269 [details] [diff] [review]. It breaks without that attachment. Its a tidied up form of what jminta and I have been trying to get working for a while. I definitely want this in as I don't want it to regress.

Note that the subFolders part (that attachment 315269 [details] [diff] [review] fixes) may actually only happen on Mac and not on Linux

Comment 5

[Mark Banner (:standard8)]

(In reply to comment #4)
> Note that the subFolders part (that attachment 315269 [details] [diff] [review] fixes) may actually only
> happen on Mac and not on Linux
> 
I've just confirmed, this doesn't happen on Linux (at least on my setup). Guess we need to be running unit tests on mac...

Comment 6

[Håkan Waara]

Attachment: test_import.js WIP

Just posting what I have right now. This is the beginning of a mail import test. I use my own apple mailbox (which I won't post) as test data right now.

Right now, my problem is that the asynchronous calling of importing won't work properly. The test stops prematurely, even though I use do_test_pending()...

Comment 7

[Mark Banner (:standard8)]

Attachment: unit test for .subFolders call on local accounts/folders v2

Slightly revised unit test incorporating correct shutdown (of account manager) and tidy up of the mail folders.

Comment 8

[Mark Banner (:standard8)]

Attachment: unit test for .subFolders call on local accounts/folders v3

This one includes the missing Makefile.in...

Comment 9

[Mark Banner (:standard8)]

(In reply to comment #3)
> Additionally, how about we do:
> 
> if (rv == NS_MSG_ERROR_FOLDER_SUMMARY_MISSING ||
>     rv == NS_MSG_ERROR_FOLDER_SUMMARY_OUT_OF_DATE)
>   return rv;
> 
> NS_ENSURE_SUCCESS(rv, rv);

Actually something like:

if (NS_FAILED(rv))
{
#ifdef _DEBUG
  // Warn if the error isn't one of the expected two
  if (rv != NS_MSG_ERROR_FOLDER_SUMMARY_MISSING ||
      rv != NS_MSG_ERROR_FOLDER_SUMMARY_OUT_OF_DATE)
    NS_WARNING("....");
#endif
  return rv;
}

may be more efficient overall.

Comment 10

[Mark Banner (:standard8)]

Comment on attachment 315269 [details] [diff] [review]
Fixes needed to get further, v1

I've split updating this into bug 434708 (for easier tracking wrt regressions if any).

Comment 11

[Mark Banner (:standard8)]

We've got unit tests for most of the others, morphing this into applemail as that's what the draft patch here is for.