Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Need unit tests for applemail import

NEW
Unassigned

Status

MailNews Core
Import
10 years ago
3 years ago

People

(Reporter: Håkan Waara, Unassigned)

Tracking

Trunk
x86
Mac OS X
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [patchlove][has draft patch])

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

10 years ago
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)
(Reporter)

Comment 1

9 years ago
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.
Summary: Need unit tests for all the mailnews importers → Need unit tests for all the mailnews/import
(Reporter)

Comment 2

9 years ago
Created attachment 315269 [details] [diff] [review]
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.
Attachment #315269 - Flags: review?(bugzilla)
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 (...)
{
...
}
Attachment #315269 - Flags: review?(bugzilla) → review-
Created attachment 315821 [details] [diff] [review]
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
(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...
(Reporter)

Comment 6

9 years ago
Created attachment 315823 [details] [diff] [review]
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()...
Created attachment 316675 [details] [diff] [review]
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.
Attachment #315821 - Attachment is obsolete: true
Created attachment 318783 [details] [diff] [review]
unit test for .subFolders call on local accounts/folders v3

This one includes the missing Makefile.in...
Attachment #316675 - Attachment is obsolete: true
(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.
Depends on: 434708
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).
Attachment #315269 - Attachment is obsolete: true
Attachment #318783 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Product: Core → MailNews Core
Whiteboard: [patchlove][has draft patch]
We've got unit tests for most of the others, morphing this into applemail as that's what the draft patch here is for.
Summary: Need unit tests for all the mailnews/import → Need unit tests for applemail import
You need to log in before you can comment on or make changes to this bug.