Closed
Bug 1028548
Opened 10 years ago
Closed 10 years ago
Fix nsMsgMaildirStore::RenameFolder() and make test_nsIMsgLocalMailFolder.js pass with maildir mailbox storage format
Categories
(MailNews Core :: Testing Infrastructure, defect)
MailNews Core
Testing Infrastructure
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 33.0
People
(Reporter: sshagarwal, Assigned: sshagarwal)
References
Details
Attachments
(1 file, 3 obsolete files)
6.77 KB,
patch
|
sshagarwal
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Updated•10 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•10 years ago
|
||
Attachment #8443932 -
Flags: review?(kent)
Assignee | ||
Comment 2•10 years ago
|
||
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•10 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•10 years ago
|
||
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•10 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•10 years ago
|
||
Thanks.
Assignee: nobody → syshagarwal
Attachment #8444982 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8445360 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 7•10 years ago
|
||
Checked in https://hg.mozilla.org/comm-central/rev/e2f581a3f07d
Status: ASSIGNED → RESOLVED
Closed: 10 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.
Description
•