1011402 - Make test_localFolder.js pass with maildir mailbox storage format

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Suyash Agarwal (:sshagarwal)]

mailnews/local/test/unit/test_localFolder.js
passes with mbox as the storage format but fails with maildir.

Possible cause seems to be the create_sub_folders() helper function
which seems to make folders specifically based on the mbox format.
So, we need to make it generic to use pluggable stores' functions
to create sub-folders.

Comment 1

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v1

This patch makes the test pass for me with both, mbox and maildir mailbox formats.

Thanks.

Comment 2

[Kent James (:rkent)]

Comment on attachment 8424071 [details] [diff] [review]
Patch v1

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

1) As we discussed on IRC, the plan is supposed to be this. 1) use mailnews methods (not file methods) to create a directory structure. 2) delete those mailnews objects. 3) create new mailnews objects, pointing to the previously-created directories. 4) test folder discovery on the new objects. Only then can you be sure that folder discover is actually "discovering" and not just remembering what you just created as mailnews objects.

2) You need to improve the documentation some. That is, state what the test is trying to accomplish, and use either better naming or comments to clarify what items are. "folder" is always ambiguous, try "filefolder" or "msgfolder" in naming to clarify. Otherwise the code is really hard to interpret when you are unsure of the type of everything.

Comment 3

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v2

I have tried to make the suggested changes.
Also, this test now loops over both the mailbox formats
so that both the formats (mbox and maildir) can be tested.
Please let me know if this doesn't work as intended.

Thanks.

Comment 4

[:aceman]

Great how you add documentation of the functions!

Comment 5

[Kent James (:rkent)]

Comment on attachment 8424163 [details] [diff] [review]
Patch v2

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

::: mailnews/local/test/unit/test_localFolder.js
@@ +43,5 @@
>  {
>    let mailbox = setup_mailbox(type, create_temporary_directory());
>  
>    check_sub_folders(expected, mailbox.subFolders);
> +  mailbox.Delete();

This is not working for me (on Windows). What happens is that you are setting up a server, which then creates the default folders Trash and Unsent Messages. But when you try to delete, the .msf files for those folders are open, and the delete fails. So my test of this fails at this point.

@@ +51,5 @@
> + * A helper method to add the folders in aFolderArray
> + * to the aParentFolder as subfolders.
> + *
> + * @param aFolderArray   array of folders and subfolders
> + * @param aParentFolder  folder to with the folders and

do you mean "to which"?

@@ +86,5 @@
> +}
> +
> +function run_all_tests() {
> +  test_default_mailbox([{ name: "Trash" }, { name: "Outbox" }], "none");
> +  test_default_mailbox([{ name: "Inbox" }, { name: "Trash" }], "pop3");

I don't really understand what these two test are actually supposed to be checking. Do you?

@@ +92,5 @@
> +  // Assuming that the order of the folders returned from the actual folder
> +  // discovery is independent and is un-important for this test.
> +  test_mailbox([
> +    {
> +      name: "Trash"

If I comment out the mailbox.Delete() so that the test will run for me, this test fails since the first folder it see is Inbox and not Trash.

I thought that we agreed that you are not going to rely on the folder order, but were going to lookup the folder by name, them also make sure that there are no extra folders?

Comment 6

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v3

(In reply to Kent James (:rkent) from comment #5)
> Review of attachment 8424163 [details] [diff] [review]:
> ::: mailnews/local/test/unit/test_localFolder.js
> @@ +43,5 @@
> > +  mailbox.Delete();
> 
> This is not working for me (on Windows). What happens is that you are
> setting up a server, which then creates the default folders Trash and Unsent
> Messages. But when you try to delete, the .msf files for those folders are
> open, and the delete fails. So my test of this fails at this point.
Okay, so I have tried to remove the incoming server along with its files, maybe it works for you now.
Also, it works for me without deleting the folders.

> @@ +86,5 @@
> > +  test_default_mailbox([{ name: "Trash" }, { name: "Outbox" }], "none");
> > +  test_default_mailbox([{ name: "Inbox" }, { name: "Trash" }], "pop3");
> I don't really understand what these two test are actually supposed to be
> checking. Do you?
Maybe just the mailfolders without any subfolders... Not sure though.
We can also get rid of this extra (and possibly redundant) method by calling
its more generic test_mailbox() with the two test cases used in the calls to
test_default_mailbox().

> @@ +92,5 @@
> If I comment out the mailbox.Delete() so that the test will run for me, this
> test fails since the first folder it see is Inbox and not Trash.
> 
> I thought that we agreed that you are not going to rely on the folder order,
> but were going to lookup the folder by name, them also make sure that there
> are no extra folders?
I have tried to run the test irrespective of the order of the folders.
For better testing of the approach, I have also included another test case.
It passes for me locally.
Please let me know if this still doesn't work.

Thanks.

Comment 7

[Suyash Agarwal (:sshagarwal)]

https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=e921c7a7016f

It seems that the test passes on try for all platforms.
And test failures are due to other tests.

Comment 8

[Kent James (:rkent)]

Comment on attachment 8425647 [details] [diff] [review]
Patch v3

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

As you will learn in this process, many existing tests do a poor job of explaining what they were supposed to test, which makes it very hard to know what to do when they break. I'm trying to make sure that we don't make the same mistake.

Overall though this test seems fine, but incorporate my changes and let's do another try run.

::: mailnews/local/test/unit/test_localFolder.js
@@ +40,5 @@
> +    do_check_eq(!!expected[index].subFolders, actualFolder.hasSubFolders);
> +    if (actualFolder.hasSubFolders) {
> +      check_sub_folders(expected[index].subFolders, actualFolder.subFolders);
> +      // We can safely splice the element as we don't need it anymore.
> +      expected.splice(index, 1);

Why do you need to do this? I tested without and it ran fine, and modifying an input like this is unexpected.

Also, please also test that the number of subfolders in expected and actual is equal.

@@ +55,5 @@
>  
>    check_sub_folders(expected, mailbox.subFolders);
> +
> +  // Remove the server and associated files from the profile.
> +  MailServices.accounts.removeIncomingServer(mailbox.server, true);

Why do you need this? It makes it harder to debug, as the folders are gone when you check later. The profile creates extra folders like mailbox-1 when there is a duplicate.

@@ +66,5 @@
> + * @param aFolderArray   array of folders and subfolders
> + * @param aParentFolder  folder to which the folders and
> + *                       subfolders from aFolderArray are
> + *                       to be added.
> + */

You are using the term "folder" here to mean both a custom js object with a name and subfolders elements, as well as an nsIMsgFolder XPCOM object. This is the source of constant confusion in mailnews js code. Please somehow make it clear in the description that these are different types. It does not hurt to state that aParentFolder is an nsIMsgFolder while aFolderArray is an array of js objects.

@@ +83,5 @@
> +/**
> + * Test to show that discoverSubFolders works.
> + * Add folders and subfolders to the root folder and
> + * reiterate over the folder tree to see if the values
> + * are as expected.

I would word this:

Create a server with folders and subfolders from the "expected" structure, then create a new server with the same filePath, and test that we can discover these folders based on that filePath.

@@ +93,5 @@
> +  let actualFolder = setup_mailbox(type, mailboxRootFolder.filePath);
> +  check_sub_folders(expected, actualFolder.subFolders);
> +
> +  // Remove the server and associated files from the profile.
> +  MailServices.accounts.removeIncomingServer(actualFolder.server, true);

Why is this needed?

Comment 9

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v4

Made the suggested changes.
The test passes locally for me.
Try builds are failing as of now, due to some unrelated failures.

Thanks.

Comment 10

[Kent James (:rkent)]

Comment on attachment 8427508 [details] [diff] [review]
Patch v4

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

Looks good, and the try run seems OK (https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=1895369f4884)

r=me

Comment 11

[Suyash Agarwal (:sshagarwal)]

Thanks.
Going for check-in then.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/6d68e92d1403