1044456 - Make test_quarantineFilterMove.js work with maildir

Status: RESOLVED WONTFIX

(MailNews Core :: Testing Infrastructure, defect)

Description

[Suyash Agarwal (:sshagarwal)]

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

Comment 1

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v1

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

Comment 2

[Suyash Agarwal (:sshagarwal)]

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 3

[Kent James (:rkent)]

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) {

Comment 4

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch for check-in

Thanks.

Comment 5

[Mark Banner (:standard8)]

https://hg.mozilla.org/comm-central/rev/c6cea1a53ccc

Comment 6

[Mark Banner (:standard8)]

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

Comment 7

[Wayne Mery (:wsmwk)]

Does bug 1144999  make this bug wontfix?

Comment 8

[:aceman]

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.

Comment 9

[Suyash Agarwal (:sshagarwal)]

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

Thanks.

Comment 10

[Suyash Agarwal (:sshagarwal)]

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

Comment 11

[Kent James (:rkent)]

OK

Comment 12

[Magnus Melin [:mkmelin]]

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