Make test_quarantineFilterMove.js work with maildir

RESOLVED WONTFIX

Status

MailNews Core
Testing Infrastructure
RESOLVED WONTFIX
3 years ago
2 years ago

People

(Reporter: sshagarwal, Assigned: sshagarwal)

Tracking

(Blocks: 1 bug)

unspecified
Thunderbird 34.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [patchlove])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
We intend to make test_quarantineFilterMove.js to work with both
mbox and maildir mailbox storage formats.
(Assignee)

Updated

3 years ago
Blocks: 1011399
(Assignee)

Comment 1

3 years ago
Created attachment 8463010 [details] [diff] [review]
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
Attachment #8463010 - Flags: review?(kent)
(Assignee)

Comment 2

3 years ago
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

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

Comment 4

3 years ago
Created attachment 8463772 [details] [diff] [review]
Patch for check-in

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

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/c6cea1a53ccc
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 34.0
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 7

2 years ago
Does bug 1144999  make this bug wontfix?
Blocks: 845952
Flags: needinfo?(acelists)
Whiteboard: [patchlove]

Comment 8

2 years ago
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.
Flags: needinfo?(acelists) → needinfo?(syshagarwal)
(Assignee)

Comment 9

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

Thanks.
Flags: needinfo?(syshagarwal)
(Assignee)

Comment 10

2 years ago
Since, this test doesn't make sense for maildir (ref comment 8 and comment 9), can we close this as wontfix?
Flags: needinfo?(rkent)

Comment 11

2 years ago
OK
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago2 years ago
Flags: needinfo?(rkent)
Resolution: --- → WONTFIX

Comment 12

2 years ago
Well shouldn't you make the test just bail out for maildir?
You need to log in before you can comment on or make changes to this bug.