If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in Thunderbird 33.0

Status

MailNews Core
Testing Infrastructure
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: sshagarwal, Assigned: sshagarwal)

Tracking

unspecified
Thunderbird 33.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Comment hidden (empty)
(Assignee)

Updated

3 years ago
Summary: Make test_nsIMsgLocalMailFolder.js pass with maildir mailbox storage format → Fix nsMsgMaildirStore::RenameFolder() and make test_nsIMsgLocalMailFolder.js pass with maildir mailbox storage format
(Assignee)

Comment 1

3 years ago
Created attachment 8443932 [details] [diff] [review]
Patch v1
Attachment #8443932 - Flags: review?(kent)
(Assignee)

Comment 2

3 years ago
Created attachment 8444048 [details] [diff] [review]
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.
Attachment #8443932 - Attachment is obsolete: true
Attachment #8443932 - Flags: review?(kent)
Attachment #8444048 - Flags: review?(kent)

Comment 3

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

Comment 4

3 years ago
Created attachment 8444982 [details] [diff] [review]
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.
Attachment #8444048 - Attachment is obsolete: true
Attachment #8444048 - Flags: review?(kent)
Attachment #8444982 - Flags: review?(kent)

Comment 5

3 years ago
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.
Attachment #8444982 - Flags: review?(kent) → review+
(Assignee)

Comment 6

3 years ago
Created attachment 8445360 [details] [diff] [review]
Patch v2

Thanks.
Assignee: nobody → syshagarwal
Attachment #8444982 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8445360 - Flags: review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed

Comment 7

3 years ago
Checked in https://hg.mozilla.org/comm-central/rev/e2f581a3f07d
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 33.0
You need to log in before you can comment on or make changes to this bug.