1508931 - add mbox -> maildir -> mbox roundtrip test

Status: RESOLVED FIXED

(Thunderbird :: General, enhancement)

Description

[Magnus Melin [:mkmelin]]

It would be good to have a test that ensures we can successfully roundtrip the mbox -> maildir -> mbox conversion.

Comment 1

[:aceman]

Yes, and also the main code could have checks that e.g. it can compare the number of original messages to the number in the converted folder. If they do not match, the user could be notified.
In bug 1491228 the conversion totally failed, but if it wouldn't hit the 256MB limit, e.g. on a smaller folder, the resulting maildir folder would have 0 or 1 messages (as the delimiters weren't properly found), which would be incorrect.

Comment 2

[Magnus Melin [:mkmelin]]

There's some caveats there though. IIRC some messages we do not copy over, or rather, were the count's wouldn't add up in the converter code compared to what the folder tells you. There are special cases for imap messages where we don't have the full body, or just the header (perhaps NTTP/POP too, can't recall).

Comment 3

[Ben Campbell]

Attachment: mailstore_roundtrip_test_1.patch

Here's the roundtrip test I implemented to test out my mailstore-converter revamp over in Bug 1491228. It requires the mailstore-converter revamp patch `mbox_convert_revamp_2.patch` from that bug.

To run:

./mach xpcshell-test comm/mailnews/base/test/unit/test_mailstoreConverter.js

Comment 4

[Ben Campbell]

This test brings up a possible pre-existing encoding issue with the mailstore conversion - I've added it as  Bug 1515254, and there's a `XXX TODO` comment in the patch (the offending emails are excluded from the test set for now).

Comment 5

[Magnus Melin [:mkmelin]]

Comment on attachment 9032336 [details] [diff] [review]
mailstore_roundtrip_test_1.patch

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

Apart from some style nits this looks good

::: mailnews/base/test/unit/test_mailstoreConverter.js
@@ +130,5 @@
> +      await OS.File.makeDir(fullPath, {ignoreExisting: false});
> +      return fullPath;
> +    } catch(e) {
> +      // If directory already exists, try another name. Else bail out.
> +      if(!(e instanceof OS.File.Error && e.becauseExists)) {

space after if

@@ +196,5 @@
> +  // compare results - use checksums, because filenames will differ.
> +  await recursiveMaildirCompare(initialRoot, finalRoot);
> +}
> +
> +

nit: extra empty line

@@ +197,5 @@
> +  await recursiveMaildirCompare(initialRoot, finalRoot);
> +}
> +
> +
> +/** Helper to adapt the callbacks from converterWorker into a promise.

nit: add line

/**
 *  Helper ....

@@ +208,5 @@
> +function doConvert(srcType, srcRoot, destType, destRoot) {
> +  return new Promise(function(resolve,reject) {
> +    let worker = new ChromeWorker("resource:///modules/converterWorker.js");
> +    worker.addEventListener("message", function(ev) {
> +      if (ev.data.msg == 'success') {

nit: use double quotes

@@ +223,5 @@
> +    });
> +  });
> +}
> +
> +

nit: extra empty line

@@ +264,5 @@
> +  "../../../data/29-(HTML+embedded-image)+attachment.eml",
> +  "../../../data/30-plaintext+(HTML+embedded-image)+attachment.eml",  // using windows-1252
> +];
> +
> +

nit: extra empty line

@@ +271,5 @@
> + * and "tmp" subdirs if required.
> + *
> + * @param {String} maildir   - Path to the maildir directory.
> + * @param {Array} emailFiles - list of files to install.
> + */

lists of paths to file to install perhaps?

and @param {Array<String>}

@@ +303,5 @@
> +  // Get a list of all the email files.
> +  let files = [];
> +  let it = new OS.File.DirectoryIterator(cur);
> +  await it.forEach( function(ent) {
> +    files.push(ent.path);

nit: no space before function

@@ +309,5 @@
> +
> +  // Calculate checksums for them all.
> +  let checksums = [];
> +  for( let f of files) {
> +    let md5 = Cc["@mozilla.org/security/hash;1"]

nit: no space before let

@@ +350,5 @@
> +    let checksumsA = await scanMaildir(OS.Path.join(rootA,name));
> +    let checksumsB = await scanMaildir(OS.Path.join(rootB,name));
> +
> +    let match = (checksumsA.length == checksumsB.length);
> +    for (let i=0; match && i<checksumsA.length; i++) {

nit, add spaces like

for (let i = 0; match && i < checksumsA.length; i++) {

@@ +353,5 @@
> +    let match = (checksumsA.length == checksumsB.length);
> +    for (let i=0; match && i<checksumsA.length; i++) {
> +      if (checksumsA[i] !=checksumsB[i]) {
> +        match = false;
> +      }

since yu're checkin match,

match = (checksumsA[i] != checksumsB[i]);

Comment 6

[Ben Campbell]

Attachment: mailstore_roundtrip_test_2.patch

Thanks Magnus. And you'll be pleased to know that I finally bit the bullet and looked up how to run eslint :-)

Comment 7

[Ben Campbell]

Comment on attachment 9032554 [details] [diff] [review]
mailstore_roundtrip_test_2.patch

Now that bug 1491228 is fixed we can look at landing this one.
No changes beyond the nit-fixes highlighted in the first revision of this patch.

Comment 8

[Magnus Melin [:mkmelin]]

Comment on attachment 9032554 [details] [diff] [review]
mailstore_roundtrip_test_2.patch

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

Looks good to me, just some minor updates

::: mailnews/base/test/unit/test_mailstoreConverter.js
@@ +218,5 @@
> +    });
> +  });
> +}
> +
> +let testEmails = [

maybe move this before all the other functions?

@@ +231,5 @@
> +  "../../../data/08-plaintext+HTML+attachment.eml",
> +  "../../../data/09-(HTML+embedded-image)+attachment.eml",
> +  "../../../data/10-plaintext+(HTML+embedded-image)+attachment.eml",
> +
> +  // XXX TODO: 12 and 20 get screwed up! Investigate!

can you file bugs for these (and reference them)?

@@ +272,5 @@
> +  await OS.File.makeDir(OS.Path.join(maildir, "tmp"));
> +
> +  let ident = Date.now();
> +  for (let src of emailFiles) {
> +    // XXX TODO: When Bug 1259040 lands, add .eml extension here.

Bug 1259040 landed

Comment 9

[Ben Campbell]

Attachment: mailstore_roundtrip_test_3.patch

tweaks:

• Moved test data array above functions
• added .eml to maildir test message filenames
• mentioned Bug 1515254 in comment by the two disabled test messages

Comment 10

[Magnus Melin [:mkmelin]]

I suppose it's ready to land then? If so, add checkin-needed

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/5e2e2c63c2cd
Add unit test to check roundtrip maildir->mbox->maildir conversion. r=mkmelin