1113275 - Make tests dependent on imapPump work with maildir

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Kent James (:rkent)]

An initial attempt to run test_downloadOffline.js with maildir enabled gives the following:

{"action":"log","time":1418931106286,"thread":null,"pid":null,"source":"xpcshell/head.js","level":"INFO","message":"(xpcshell/head.js) | test download
AllForOffline finished (2)"}

{"action":"log","time":1418931106296,"thread":null,"pid":null,"source":"xpcshell/head.js","level":"INFO","message":"mailnews/imap/test/unit/test_offli
neCopy.js | Starting copyMessagesToInbox"}

{"action":"log","time":1418931106306,"thread":null,"pid":null,"source":"xpcshell/head.js","level":"INFO","message":"(xpcshell/head.js) | test copyMess
agesToInbox pending (2)"}

{"action":"log","time":1418931106321,"thread":null,"pid":null,"source":"xpcshell/head.js","level":"INFO","message":"(xpcshell/head.js) | test run_next
_test 3 finished (2)"}
[4440] ###!!! ASSERTION: FinishNewMessage - oops! file does not exist!: 'Error', file c:/tb/4-central/src/mailnews/local/src/nsMsgMaildirStore.cpp, li
ne 738

Comment 1

[Kent James (:rkent)]

Attachment: Fixes to let imap pump run with maildir

Suyash, would you look this over? Thanks.

Comment 2

[Kent James (:rkent)]

Attachment: Part 2: test_searchChaining.js

I think what I will do is work on each IMAPPump test in this bug.

Comment 3

[Suyash Agarwal (:sshagarwal)]

Comment on attachment 8540253 [details] [diff] [review]
Fixes to let imap pump run with maildir

Review of attachment 8540253 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for taking so long.
All the tests pass for me perfectly for both mbox and maildir.

Thanks :)

::: mailnews/imap/test/unit/test_downloadOffline.js
@@ +42,5 @@
>    
>    // ...and download for offline use.
> +  let promiseUrlListener = new PromiseTestUtils.PromiseUrlListener();
> +  IMAPPump.inbox.downloadAllForOffline(promiseUrlListener, null);
> +  yield promiseUrlListener.promise;

I think this function (setup) should be a generator (setup*)
because of this promise yield.

@@ +48,5 @@
>  
>  function downloadAllForOffline() {
> +  let promiseUrlListener = new PromiseTestUtils.PromiseUrlListener();
> +  IMAPPump.inbox.downloadAllForOffline(promiseUrlListener, null);
> +  yield promiseUrlListener.promise;

Same here.

@@ +76,5 @@
> +      });
> +    tests.forEach(add_task);
> +  });
> +
> +  tests.forEach(add_task);

This line repeats the tests for the last msgStore.
So, I think we can remove this.

::: mailnews/imap/test/unit/test_offlineCopy.js
@@ +119,5 @@
>             '>\n');
>        // This fails for file copies in bug 790912. Without  this, messages that
>        //  are copied are not visible in pre-pluggableStores versions of TB (pre TB 12)
> +      if (IMAPPump.inbox.msgStore.storeType == "mbox")
> +        Assert.equal(message.messageOffset, parseInt(message.getStringProperty("storeToken")));

Similarly, shouldn't we check if storeToken holds the filename in
case of maildir?

::: mailnews/local/src/nsMsgBrkMBoxStore.cpp
@@ +1006,5 @@
>  }
>  
> +NS_IMETHODIMP nsMsgBrkMBoxStore::GetStoreType(nsACString& aType)
> +{
> +  aType.Assign("mbox");

AssignLiteral() ?

::: mailnews/local/src/nsMsgMaildirStore.cpp
@@ +1360,5 @@
>  }
>  
> +NS_IMETHODIMP nsMsgMaildirStore::GetStoreType(nsACString& aType)
> +{
> +  aType.Assign("maildir");

Same here.

Comment 4

[Kent James (:rkent)]

I'm morphing this bug to deal with a variety of issues that were discovered working with existing tests that use imapPump, and were failing with maildir. Rather than do separate patches for each test, adding issues that were discovered per test, I'll have a patch the fixes all of the core issues, then a patch that demonstrates changes to imapPump, then another that converts various tests to work with maildir.

Comment 5

[Kent James (:rkent)]

Attachment: [CHECKED_IN] Part 1, Fix core imap issues affecting maildir

Fix the core issues affecting maildir in the imapPump tests.

Comment 6

[Kent James (:rkent)]

Attachment: Part 2, Fix imapPump to better reset on teardown

This changes imapPump to do the teardown slightly differently. Most of these changes just reorder things to what (I think) is a more logical order.

The most significant change is to forcibly remove the folders from RDF if they still exist after we tried to delete them. Unfortunately in almost all cases they are still there, which means that RDF will now complain when the actual folder objects are released. But without this, there are cases where the existing folder objects, which get discovered on the restart since they have the same folder URL, causes issues on the second pass. 

This shows that is is quite hard for us to get all of the references to release to folders. Yes this is bad, but I would argue that this has nothing to do with maildir, it is only being exposed by the attempts to restart the app. We should figure out how to better do cleanup, but we do not need to do it now, the priority is to get maildir running (and tested) prior to the next release.

Joshua pointed out to me on IRC that there was an alternative way to handle the maildir tests, namely to have a variant xpcshell.ini that uses a different head file, that could have set a different value of the store than default. That;s a good idea, but unfortunately that ship has largely sailed already, so I don't think we should try to rethink the current maildir testing approach (which is to restart the app in each test) at this point in time.

Comment 7

[Kent James (:rkent)]

Attachment: part 3, get tests related to imapPump working with maildir

Here are the changes for the tests themselves. I asked jcranmer on IRC if he thought it OK if Suyash reviewed these, and he said yes. Feel free to overrule after seeing this, or Suyash you are of course free to decline.

The main controversial item here is that I have made the filter tests both sequential, as well as relying on delays to finish async operations of unknown duration. This is far from optimal, and I wish I did not have to do that, but there are just too many cases where operations do not properly terminate with any known notification, so trying to find the exact combination of events that truly marks an end of all operations is difficult to impossible. Yes we should fix this so that there is a clean way to detect end-of-operation, but that is beyond the scope of this bug.

Comment 8

[Kent James (:rkent)]

Oh, and I also believe it is time to not only converts tests to use Promises instead of async_test_utils, but it is also time to start using Assert.jsm. These test changes all reflect that.

Comment 9

[Kent James (:rkent)]

(In reply to Suyash Agarwal (:sshagarwal) from comment #3)
> Comment on attachment 8540253 [details] [diff] [review]
> Fixes to let imap pump run with maildir
> 
> Review of attachment 8540253 [details] [diff] [review]:
> -----------------------------------------------------------------
>

I incorporate all review comments except this one:

> 
> ::: mailnews/imap/test/unit/test_offlineCopy.js
> @@ +119,5 @@
> >             '>\n');
> >        // This fails for file copies in bug 790912. Without  this, messages that
> >        //  are copied are not visible in pre-pluggableStores versions of TB (pre TB 12)
> > +      if (IMAPPump.inbox.msgStore.storeType == "mbox")
> > +        Assert.equal(message.messageOffset, parseInt(message.getStringProperty("storeToken")));
> 
> Similarly, shouldn't we check if storeToken holds the filename in
> case of maildir?

This tests a repair to the message database that was necessary due to an old bug that existed briefly after pluggableStores was first implemented. Nobody was running maildir back then, so there is no fix needed for maildir. I don't recall if the bug even affected maildir or not. It has been a long time since that fix was needed, in fact that fix could probably be removed completely now. It certainly makes no sense to implement it, and test it, for maildir.

Comment 10

[[e-mail]]

Comment on attachment 8543228 [details] [diff] [review]
[CHECKED_IN] Part 1, Fix core imap issues affecting maildir

>+  /**
>+   * Identifies a specific type of store. Please use this only for legacy
>+   * bug fixes, and not as a method to change behavior!
>+   *
>+   * Typical values: "mbox", "maildir"
>+   */
>+  readonly attribute ACString storeType;
[If only we'd thought to use a contract ID of the form
#define NS_STORE_CONTRACTID_PREFIX "@mozilla.org/messenger/store;1?type="
#define NS_BRKMBOXSTORE_CONTRACTID NS_STORE_CONTRACTID_PREFIX "mbox"
#define NS_MAILDIRSTORE_CONTRACTID NS_STORE_CONTRACTID_PREFIX "maildir"
]

>-  else
>-    NS_ERROR("null db in thread");
>+  else // This can happen if db is forced closed
>+    NS_WARNING("null db in thread");
Part of another patch?

>+        // Call FinishNewMessage before setting pending attributes, as in
>+        //   maildir it copies from tmp to cur and may change the storeToken
>+        //   to get a unique filename.
Nit: Please also move this to before you create the nsIArray to set pending attributes.

>+        aListener->SetMessageKey(dstKey);
[The listener doesn't actually care about the key except in the case of copyFileMessage.]

Comment 11

[Kent James (:rkent)]

(In reply to [e-mail] from comment #10)
> Comment on attachment 8543228 [details] [diff] [review]
> Part 1, Fix core imap issues affecting maildir
> 
> >-  else
> >-    NS_ERROR("null db in thread");
> >+  else // This can happen if db is forced closed
> >+    NS_WARNING("null db in thread");
> Part of another patch?

No, this was intentional. Part of the process of resetting the store type is to call nsMsgDatabase::ForceClosed() which calls nsMsgDatabase::ClearThreads() which calls nsMsgThread::Clear(), which nulls m_mdbDB for the thread. We should not be throwing an error on null m_mdbDB (which causes xpcshell tests to fail) when we have methods where we are purposely nulling that. I was hitting those errors in some of the tests. This is probably a result of the difficulties of successfully getting all of the objects to destruct when we reset the store type.

Comment 14

[Kent James (:rkent)]

See this try run that shows tests passing with these three patches loaded:

https://treeherder.mozilla.org/ui/#/jobs?repo=try-comm-central&revision=51e0d1ae35e8

Comment 15

[Kent James (:rkent)]

Comment on attachment 8543228 [details] [diff] [review]
[CHECKED_IN] Part 1, Fix core imap issues affecting maildir

Checked in http://hg.mozilla.org/comm-central/rev/c8113accba21 with review changes.

Comment 16

[Suyash Agarwal (:sshagarwal)]

Comment on attachment 8543230 [details] [diff] [review]
part 3, get tests related to imapPump working with maildir

Review of attachment 8543230 [details] [diff] [review]:
-----------------------------------------------------------------

Apologies for taking so long.
All the tests pass for me perfectly. I just have a few suggestions.

Thanks.

::: mailnews/base/test/unit/test_searchChaining.js
@@ +55,5 @@
> +  yield listener.promise;
> +
> +  // After the search completes, there still seem to be active URLs, so we
> +  //   have to wait before we are done and clear.
> +  yield PromiseTestUtils.promiseDelay(1000);

[1] -> Shouldn't we use Promise.all([listener.promise, PromiseTestUtils.promiseDelay(1000)])?

@@ +93,5 @@
> +      Services.prefs.setCharPref("mail.serverDefaultStoreContractID", storeID);
> +    });
> +    tests.forEach(add_task);
> +  });
> +  run_next_test();

I read somewhere that we shouldn't be adding tasks inside an
add_task(). But since now I don't remember the source, this thing can be useless.

::: mailnews/imap/test/unit/test_imapFilterActions.js
@@ +428,5 @@
> +                            IMAPPump.mailbox.uidnext++, []));
> +    let promiseUrlListener = new PromiseTestUtils.PromiseUrlListener();
> +    IMAPPump.inbox.updateFolderWithListener(null, promiseUrlListener);
> +    yield promiseUrlListener.promise;
> +    yield PromiseTestUtils.promiseDelay(200);

Same here. Shouldn't we be using Promise.all() ?

::: mailnews/imap/test/unit/test_imapMove.js
@@ +33,5 @@
>  
> +  addImapMessage();
> +  let listener = new PromiseTestUtils.PromiseUrlListener();
> +  IMAPPump.inbox.updateFolderWithListener(null, listener);
> +  yield listener.promise;

*startTest() ?
And Promise.all() ? At many other places too.

@@ +38,3 @@
>  }
>  
>  function doMove() {

*doMove() ?

@@ +53,3 @@
>  }
>  
>  function testMove() {

Same here.

::: mailnews/imap/test/unit/test_offlineCopy.js
@@ +100,5 @@
>  
>      let promiseUrlListener = new PromiseTestUtils.PromiseUrlListener();
>      IMAPPump.inbox.updateFolderWithListener(null, promiseUrlListener);
>      yield promiseUrlListener.promise;
>  

Promise.all() ?

::: mailnews/imap/test/unit/test_offlinePlayback.js
@@ +137,5 @@
>  }
>  
>  function teardown() {
>    teardownIMAPPump();
>  }

This function can be removed now.

::: mailnews/imap/test/unit/test_offlinePlaybackMaildir.js
@@ +142,5 @@
> +}
> +
> +function teardown() {
> +  teardownIMAPPump();
> +}

This function can be removed now.

Comment 17

[Kent James (:rkent)]

Comment on attachment 8543229 [details] [diff] [review]
Part 2, Fix imapPump to better reset on teardown

So I am going to switch instead to using alternate heads to change the default store. This will means fewer changes needed in IMAPPump teardown. I'll do updated patches soon, but this patch will not need review.

Comment 18

[Kent James (:rkent)]

Attachment: Part 2, test changes

jcranmer pointed out an alternate way to run test variants, which is to maintain separate versions of the test manifest using different heads. I tried that approach, and it is definitely easier than what we did previously. So I'll adapt it on IMAP. That changes the tests, so that I do not need to loop through each twice.

I've added in the changes to IMAP pump (since they are now much simpler than before), plus the review changes. I did not however to any additional Promise.All calls. A single Promise.All is not the same as two promises run sequentially, and in all cases I intended the promises to run sequentially.

So please look this over again. Thanks!

Comment 19

[Kent James (:rkent)]

Attachment: Part 3, alternate manifest with modified head

Comment 20

[Suyash Agarwal (:sshagarwal)]

Comment on attachment 8546173 [details] [diff] [review]
Part 2, test changes

Review of attachment 8546173 [details] [diff] [review]:
-----------------------------------------------------------------

Extremely sorry for taking so long.
All the tests pass for me perfectly, both for mbox and maildir formats.

Thanks.

Comment 21

[Kent James (:rkent)]

Comment on attachment 8546174 [details] [diff] [review]
Part 3, alternate manifest with modified head

I want to make sure that you are comfortable with expanding the IMAP testing greatly to encompass maildir. I removed a few tests from maildir, but if the test seemed to touch folders at all I added it to test on maildir as well as mbox.

I want to add UI in TB 38 to enable maildir on new accounts, and I'd like to make sure that we have the testing in place.

BTW thanks for the tip on using an alternate test manifest with a different head!

Comment 22

[Joshua Cranmer [:jcranmer]]

If I apply part 3, I notice that test_offlineCopy.js reliably fails for me. Is there some other patch I'm missing?

Comment 23

[Kent James (:rkent)]

You should not need anything more than Part 2 and Part 3 in this bug. I tried this again, and I am not seeing any issue with test_offlineCopy.js  Could you show me the output when it fails?

Comment 24

[Joshua Cranmer [:jcranmer]]

(In reply to Kent James (:rkent) from comment #23)
> You should not need anything more than Part 2 and Part 3 in this bug. I
> tried this again, and I am not seeing any issue with test_offlineCopy.js 
> Could you show me the output when it fails?

I thought part 2 had been checked in, hence I did not apply it when I tested. Now that I applied it, things look good to me.

Comment 25

[Kent James (:rkent)]

https://hg.mozilla.org/comm-central/rev/f792bb59b8e5
https://hg.mozilla.org/comm-central/rev/1f549a4e9351