We intend to make test_quarantineFilterMove.js to work with both mbox and maildir mailbox storage formats.

There was a subtle difference between MoveIncorporatedMessage() and MoveNewlyDownloadedMessage() related to filters. I have tried to cover it up. I saw that previously we were using "DeleteorMoveMsgCompleted" atom for copyMessages() but [1] suggests that this atom event isn't notified for copying. So I have removed it. Thanks. [1]http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsLocalMailFolder.cpp#1487

Okay, there's another place where we get this same atom for copymessages too. http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsMailboxProtocol.cpp#297 But for maildir, m_mailboxAction has value 1 whereas the condition demands 2 here: http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsMailboxProtocol.cpp#282 But actually we shouldn't notify this atom for copy messages but rest is what you decide. Thanks.

Comment on attachment 8463010 [details] [diff] [review] Patch v1 Review of attachment 8463010 [details] [diff] [review]: ----------------------------------------------------------------- There are several changes needed here, but I don't think I need to see it again. r=me with the changes. ::: mailnews/base/test/unit/test_quarantineFilterMove.js @@ +14,5 @@ > load("../../../resources/POP3pump.js"); > > const gFiles = ["../../../data/bugmail1", "../../../data/bugmail10"]; > > +var gPluggableStores = [ Use the list in localAccountUtils. @@ +76,5 @@ > let promiseCopyListener = new PromiseTestUtils.PromiseCopyListener(); > MailServices.copy.CopyMessages(gMoveFolder, messages, gMoveFolder2, false, > promiseCopyListener, null, false); > + > + yield promiseCopyListener.promise; I'm OK with this. @@ +136,2 @@ > > + for (let store of gPluggableStores) { This should be localAccountUtils.pluggableStores ::: mailnews/local/src/nsParseMailbox.cpp @@ +1844,5 @@ > + > + if (NS_SUCCEEDED(rv) && msgMoved) { > + if (!m_filterTargetFolders.Contains(trash)) > + m_filterTargetFolders.AppendObject(trash); > + } else if (NS_SUCCEEDED(rv) && !msgMoved) { m_filterTargetFolders is used to run spam processing, which is never run on trash, so there is no need to add the trash folder here. So remove the code that adds trash, and just use if (NS_SUCCEEDED(rv) && !msgMoved) {

Thanks.

Backed out due to causing test failures: https://hg.mozilla.org/comm-central/rev/a355f2afcc44 02:27:33 INFO - TEST-INFO | /builds/slave/test/build/tests/xpcshell/tests/mailnews/base/test/unit/test_quarantineFilterMove.js | Starting getLocalMessages1 02:27:33 INFO - TEST-INFO | (xpcshell/head.js) | test getLocalMessages1 pending (2) 02:27:33 INFO - TEST-INFO | (xpcshell/head.js) | test pending (3) 02:27:33 INFO - failed opening offline store for mailbox://nobody@Local%20Folders/Inbox 02:27:33 WARNING - TEST-UNEXPECTED-FAIL | ../../../resources/POP3pump.js | 2147500037 == 0 - See following stack: 02:27:33 INFO - ../../../resources/POP3pump.js:OnStopRunningUrl:70 02:27:33 INFO - resource://testing-common/mailnews/maild.js:nsMailServer.prototype.performTest:246 02:27:33 INFO - ../../../resources/POP3pump.js:_testNext:201 02:27:33 INFO - ../../../resources/POP3pump.js:run:239 02:27:33 INFO - /builds/slave/test/build/tests/xpcshell/tests/mailnews/base/test/unit/test_quarantineFilterMove.js:getLocalMessages1:46 02:27:33 INFO - _run_next_test@/builds/slave/test/build/tests/xpcshell/head.js:1308:9 02:27:33 INFO - do_execute_soon/<.run@/builds/slave/test/build/tests/xpcshell/head.js:570:9 02:27:33 INFO - _do_main@/builds/slave/test/build/tests/xpcshell/head.js:191:5 02:27:33 INFO - _execute_test@/builds/slave/test/build/tests/xpcshell/head.js:405:5 02:27:33 INFO - @-e:1:1 02:27:33 INFO - TEST-INFO | (xpcshell/head.js) | exiting test

Does bug 1144999 make this bug wontfix?

If quaratine option has no effect in maildir and the sole purpose of the test is to test the effect, then the test could be skipped for maildir.

Hi, yes I guess in that case, there's no point in running this test. Thanks.

Since, this test doesn't make sense for maildir (ref comment 8 and comment 9), can we close this as wontfix?

OK

Well shouldn't you make the test just bail out for maildir?