1028548 - Fix nsMsgMaildirStore::RenameFolder() and make test_nsIMsgLocalMailFolder.js pass with maildir mailbox storage format

Status: RESOLVED FIXED

(MailNews Core :: Testing Infrastructure, defect)

Comment 1

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v1

Comment 2

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v1.5

Okay, now this tests both mbox and maildir.
Also, I have used Assert.jsm as mdn docs say that
do_check_* is deprecated.

Thanks.

Comment 3

[Kent James (:rkent)]

Comment on attachment 8444048 [details] [diff] [review]
Patch v1.5

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

You have added all of these changes associated with changing from do_check to assert.jsm which makes it hard to figure out what was really needed for maildir, and what is being changed to update to assert.

I don't think now is a good time to be changing all of these tests from do_check to assert. I would really reserve assert.jsm for new tests for now.

If you want to update this test to use assert.jsm, please do that in a separate patch so that the review for maildir is not confused by that issue.

Same issue with var versus let, do not do these changes unless needed. They just make the review harder.

I will do a quick look at this, but I will not really do a review until you have done that separation.

::: mailnews/base/util/nsMsgDBFolder.cpp
@@ +4087,5 @@
>    {
>      rv = parentFolder->AddSubfolder(aNewName, getter_AddRefs(newFolder));
>      if (newFolder)
>      {
> +      newFolder->SetPrettyName(EmptyString());

Why is this needed?

::: mailnews/local/src/nsMsgMaildirStore.cpp
@@ +408,5 @@
>    }
>  
>    // rename summary
> +  nsAutoString summaryName(safeName);
> +  summaryName += NS_LITERAL_STRING(SUMMARY_SUFFIX);

Why is this change needed? Did the previous code fail in some way?

::: mailnews/local/test/unit/test_nsIMsgLocalMailFolder.js
@@ +27,5 @@
>    // Delete "folder" into Trash.
>    let folderArray = toXPCOMArray([folder], Ci.nsIMutableArray);
>    root.deleteSubFolders(folderArray, null);
> +  assert.equal(path.exists(), false, "folder path doesn't exist");
> +  assert.equal(trash.numSubFolders, 1, "Number of folders in trash is 1");

Looking at other users of assert, I don't see all that many comments. It seems to me that if you are going to add a comment, it needs to add some information that is not obvious from the test. "Number of folders in trash is 1" is just repeating what the code says. Better to leave it blank than to add a comment that adds no value, at least that is my initial reaction. Have other encouraged you to always add comments here?

@@ +86,5 @@
>  function subtest_folder_operations(root) {
>    // Test - num/hasSubFolders
>  
>    // Get the current number of folders
> +  let numSubFolders = root.numSubFolders;

Same issue here with var versus let - please do not churn the code doing these changes unless they are needed. There is nothing wrong with var, and changing to let can change the behavior which adds to the review load.

Comment 4

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v1.8

Made the suggested changes.

(In reply to Kent James (:rkent) from comment #3)
> ::: mailnews/base/util/nsMsgDBFolder.cpp
> @@ +4087,5 @@
> > +      newFolder->SetPrettyName(EmptyString());
> 
> Why is this needed?
I read it here so thought this is necessary:
http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsLocalMailFolder.cpp#911

> ::: mailnews/local/src/nsMsgMaildirStore.cpp
> @@ +408,5 @@
> >    // rename summary
> > +  nsAutoString summaryName(safeName);
> > +  summaryName += NS_LITERAL_STRING(SUMMARY_SUFFIX);
> 
> Why is this change needed? Did the previous code fail in some way?
Ya, this is why the code was failing.
Previously, it was modifying safeName and then was adding folder with
that name:
http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsMsgMaildirStore.cpp#418
so, the folder added was being named "folder.msf" instead of "folder".

Thanks.

Comment 5

[Kent James (:rkent)]

Comment on attachment 8444982 [details] [diff] [review]
Patch v1.8

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

This looks good, thanks! Just address that one nit with a comment or delete (or fix) the test.

::: mailnews/local/test/unit/test_nsIMsgLocalMailFolder.js
@@ +259,4 @@
>  
> +  do_check_true(folder1.containsChildNamed(folder2.name));
> +  if (folder2.filePath.exists())
> +    do_check_true(folder2.filePath.exists());

What is the point of this test? if (true) -> do_check_true(true) is what I see.

If you have some legitimate purpose for this, then give it a comment to explain what that purpose is.

Comment 6

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v2

Thanks.

Comment 7

[Kent James (:rkent)]

Checked in https://hg.mozilla.org/comm-central/rev/e2f581a3f07d