Closed Bug 1508931 Opened 5 years ago Closed 5 years ago

add mbox -> maildir -> mbox roundtrip test

Categories

(Thunderbird :: General, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 67.0

People

(Reporter: mkmelin, Assigned: benc)

Details

Attachments

(1 file, 2 obsolete files)

It would be good to have a test that ensures we can successfully roundtrip the mbox -> maildir -> mbox conversion.
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.
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).
Attached patch mailstore_roundtrip_test_1.patch (obsolete) — Splinter Review
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
Assignee: nobody → benc
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 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]);
Attachment #9032336 - Flags: feedback+
Attached patch mailstore_roundtrip_test_2.patch (obsolete) — Splinter Review
Thanks Magnus. And you'll be pleased to know that I finally bit the bullet and looked up how to run eslint :-)
Attachment #9032336 - Attachment is obsolete: true
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.
Attachment #9032554 - Flags: review?(mkmelin+mozilla)
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
Attachment #9032554 - Flags: review?(mkmelin+mozilla) → review+

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
Attachment #9032554 - Attachment is obsolete: true
Attachment #9039460 - Flags: review+

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

Keywords: checkin-needed

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

Status: NEW → RESOLVED
Closed: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 67.0
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: