Closed Bug 1714277 Opened 2 years ago Closed 2 years ago

mailnews/base/test/unit/test_copyToInvalidDB.js fails when default store is maildir

Categories

(MailNews Core :: Backend, defect)

defect

Tracking

(thunderbird_esr78 wontfix)

RESOLVED FIXED
91 Branch
Tracking Status
thunderbird_esr78 --- wontfix

People

(Reporter: benc, Assigned: benc)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

To replicate:

  1. set maildir as the default store. Eg, in mailnews/mailnews.js:
pref("mail.serverDefaultStoreContractID", "@mozilla.org/msgstore/maildirstore;1");
  1. run the unit test:
./mach xpcshell-test comm/mailnews/base/test/unit/test_copyToInvalidDB.js

Dies with a generator explosion.
Works fine when mbox is the default store type.

Geoff, a question: do try/catch blocks screw up async test stuff implemented with yield and generator functions?
It's this block here:
https://searchfox.org/comm-central/source/mailnews/base/test/unit/test_copyToInvalidDB.js#74
The yield a few lines down doesn't seem to be waiting for the asyncUrlListener to trigger.
Been poking at this all day and now I've got a big JS headache :-)

Bug-specific details:

I think the bug is actually the test falling over rather than the actual maildir code.

The test uses asyncUrlListener to wait for nsIMsgLocalFolder.getDatabaseWithReparse() to complete.

The core of the reparse is handled by nsIMsgPluggableStore.rebuildIndex().
The mbox implementation seems to complete before getDatabaseWithReparse() returns. But the maildir version is driven off a timer callback, so I don't think it even starts before getDatabaseWithReparse() returns. This should be OK - it'll just return NS_ERROR_NOT_INITIALIZED to let us know the database isn't ready yet. The test checks for this error... but I think the try/catch block is screwing things up.
If the exception is thrown, the next yield doesn't seem to wait correctly for asyncListener.

The attached patch adds some dump() calls in to show the asyncUrlListener callbacks, and to show what the test is doing. I can see that the yield returns before the asyncUrlListener.OnStopRunningUrl() is invoked.

Assignee: nobody → benc
Flags: needinfo?(geoff)

Sorry this is delayed and probably not the answer you were looking for, but I think the best solution would be to convert the test to use real async JS instead of the pseudo-async mess it is.

Potential problems I can see are add_sets_to_folders (but it should be synchronous in this test given it returns true), and asyncCopyListener (which you can replace with your own listener).

Flags: needinfo?(geoff)

(In reply to Geoff Lankow (:darktrojan) from comment #2)

Sorry this is delayed and probably not the answer you were looking for, but I think the best solution would be to convert the test to use real async JS instead of the pseudo-async mess it is.

No, that answer lines up with my initial instinct! Anything that makes tests easier to read is a good thing.
(I kind of think that unit tests should be simple enough to do double duty as documentation/examples. We're not quite there, but it's something to aspire to :- )

Potential problems I can see are add_sets_to_folders (but it should be synchronous in this test given it returns true), and asyncCopyListener (which you can replace with your own listener).

Cool - just off the top of my head, I'll be looking at PromiseTestUtils.jsm for asyncifying listeners. Let me know if that's a terrible idea!

Attachment #9227274 - Attachment description: Bug 1714277 - Modernise test_copyToInvalidDB.js to get it working with maildir store. r?darktrojan → Bug 1714277 - Modernise test_copyToInvalidDB.js to get it working with maildir store. r=darktrojan

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/17da893272c5
Modernise test_copyToInvalidDB.js to get it working with maildir store. r=darktrojan

Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 91 Branch
You need to log in before you can comment on or make changes to this bug.