540676 - fake IMAP server doesn't support BODYSTRUCTURE

Status: RESOLVED FIXED

(MailNews Core :: Testing Infrastructure, defect)

Description

[Phil Lacy]

Need to test various messages for 'download as links' bugs.

Comment 1

[Phil Lacy]

There are some open source GNU GPL license imap servers if that would even be practical.

Comment 2

[Phil Lacy]

Returning a fake bodystructure would be easy enough, but how would we inspect the download on demand problem?

Comment 3

[David :Bienvenu]

As I understand it, we can't use fakeserver with mozmill yet, but if we could, we could just use the dom of the message pane to figure out what is displayed. 

I believe that when we reply to a message with this problem, we actually get the "this body part will be downloaded" as part of the reply quoted text, so if you run the same kind of message fetch request as the compose reply quoted code, you could look at the data that your stream listener gets sent and look for the bad text.  See http://mxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgQuote.cpp#149 and its caller to see how to set up that kind of stream.

Comment 4

[Phil Lacy]

quoteMessage with quoteHeaders=false (headers=quotebody) returns 18 bytes and no body:
</body>

</html>

if it's set to true it returns the normal html format for a quoted message but it doesn't send a bodystructure fetch.

may be our fakeserver not handling the header=quotebody but doesn't appear so.

Comment 5

[Phil Lacy]

no I don't think the header=quotebody has anything to do with the fakeserver. That's handled by client side I believe.

Comment 6

[Phil Lacy]

Attachment: WIP- preliminary to get a basic check test working

uses quoteMessage, but has problems.

Comment 7

[Phil Lacy]

I tried the imapservice.DisplayMessage but it returns the whole message.

also I notice this http://mxr.mozilla.org/comm-central/source/mailnews/imap/src/nsImapService.cpp#469

I'm not sure how this works but is it possible it may be 
"?type=application/x-message-display" or "&type=application/x-message-display"

Comment 8

[Phil Lacy]

tried streamMessage also, but it doesn't try a bodystructure

Comment 9

[Phil Lacy]

so... I spliced in a BODYSTRUCTURE response but I see we also don't support getting subparts, BODY[0] etc.

I'll guess we could fake those too but it'll be awhile before I can get enough time to figure this out.

Comment 10

[Phil Lacy]

Attachment: WIP fetch BODYSTRUCTURE and BODY[partnum]

code to fetche partnum.MIME and partnum.TEXT. I guess partnum.HEADER would be same as .MIME.

Still needs incorporating into the server code and needs actual tests.

Comment 11

[Phil Lacy]

Attachment: this is a patch to reverse changeset 02beb7d68456

With this reverse patch applied one can test the bodystructure tests.

Comment 12

[Phil Lacy]

Attachment: add Body[parts] and BODYSTRUCTURE to imap fakeserver

works, I need to upload to tryserver sometime to make sure it doesn't break anything. I will upload a log of the test details.

Comment 13

[Phil Lacy]

Attachment: test for one file

need to find a way to check more then one file per test.
This needs eml files that have a keyword and we test to see if it is in the output of message quote.

Comment 14

[Phil Lacy]

Attachment: test results

3 known problem emails.  I inserted the key text and tested with display-as in html and plain text and noted the structures. Also noted the BODYSTRUCTURE compared with gmail.

Noted is with the applied fix patch and then with the patch backed out.

Comment 15

[Phil Lacy]

I can open a channel and then add the query ?header=quotebody. That way it would be synchronous and I can do multiple files in the one test and change the pref from html to text etc. Similiar to attachment id=426942.

Comment 16

[Phil Lacy]

Comment on attachment 428103 [details] [diff] [review]
test for one file

working on tests for multiple files.
If you have any suggestions then ok. But currently I am doing a nsIChannel.open that works so I just need to find the best way to loop through files so we can add test case files later as needed. Or more importantly if we already have cases we need to add to the tests.

Comment 17

[Phil Lacy]

Comment on attachment 428102 [details] [diff] [review]
add Body[parts] and BODYSTRUCTURE to imap fakeserver

>+bodypart.prototype = {
>+  set msg(x){
>+    if(!x || x == ""){
>+      this.index = null;
>+      this.mime = "";
>+      this.text = "";
>+      this._msg = "";
>+      return;
>+    }
>+    this._msg = x;
>+    this.init();
>+  },

I don't see a use or need for setting the msg except on new construction
I'll remove this


>+  init  : function() {
>+  this.BndryAttrRE.lastIndex = 0;
>+    for each (var x in this._pn) {

array interation not according to hoyle.

Comment 18

[Phil Lacy]

Comment on attachment 428102 [details] [diff] [review]
add Body[parts] and BODYSTRUCTURE to imap fakeserver

>+    var information = "";
>+    var bp = new bodypart(message.getText());
>+    if (partNum == "") {
>+      information = message.getPart(partNum, false);
>+      bp.pn = 0;
>+    }

bp.pn=[0]



>+    this._pn = x;
>+    this.init();
>+  },
>+  init  : function() {
>+  this.BndryAttrRE.lastIndex = 0;
>+    for each (var x in this._pn) {
>+      var bndryAttrArray = this.BndryAttrRE(this._msg);

bombs our other tests without multipart need to add
if (!bndryAttrArray)
 return

Comment 19

[Phil Lacy]

Attachment: adds BODYSTRUCTURE and fetch by body[part]

I'm done with it... ran through the imap xpcshell tests with all passing.
then applied my test cases and passed that test, then backed out the fix for pass bugs and tests failed appropriately.

Comment 20

[Phil Lacy]

Attachment: test cases for and 246415 and future test cases

Test for messages. Others can be added to database and they will be picked up by test.
Test has 2 tries, one for  prefer plain text and one for not

Comment 21

[Phil Lacy]

Comment on attachment 429927 [details] [diff] [review]
test cases for  and 246415 and future test cases

>+  prefBranch.setIntPref ("mail.imap.mime_parts_on_demand_max_depth", 1);
>+

This shouldn't be set.  It doesn't seem to be used anywhere in our code nevertheless if anything it should be a large value not small. 15 is default.

Comment 22

[David :Bienvenu]

Comment on attachment 429926 [details] [diff] [review]
adds BODYSTRUCTURE and fetch by body[part]

Phil, thx for the patch; sorry for the delay. this has bit-rotted, though it's easily de-bitrotted.

please use let instead of var, and change the name "bp" to bodyPart:

+    var information = "";
+    var bp = new bodypart(message.getText());
+    if (partNum == "") {
+      information = message.getPart(partNum, false);
+      bp.pn = [0];
+    }
+    else {
+      bp.pn = partNum.split(/\./);
+    }

similarly,

+  set pn(x){

please use partNum - it's a lot more readable, and the code will be read a lot more often than typed.

please put a space after "if" and spaces around the "==":

+    if(!x || x.length==0)

+    for each (var x in this._pn) {

use let instead of var.

+    var textRE;
+
+    // we have a mime part and it is a multipart so find terminator
+    if (xStr && /boundary=/.test(xStr[0])) {
+      textRE= new RegExp("([\\s\\S]*?)\\r\\n" + bndryTag + "--", "mg");
+    } else {
+      textRE = new RegExp("([\\s\\S]*?)\\r\\n" + bndryTag , "mg");
+    }

don't need the braces. And what does xStr as a variable name mean? A more meaningful var name might be helpful.

+  if(!msg || msg == "")

space after "if"

+  var _bs = function (str, abndryTag)

please use a better var name than _bs - bodyStructure, perhaps.

space after "if":

+    if (!str || str == "")

lose the commented out line:

+    // mime==[ ,mime]

ct should be contentType. In general, 2 char variable names aren't very helpful.

Comment 23

[David :Bienvenu]

Comment on attachment 429927 [details] [diff] [review]
test cases for  and 246415 and future test cases

thx very much for the tests!

+  // fetched inlieu of parts. There may be conditions that bypass bodypart fetch.

in lieu should be two words

this isn't used:

+//      yRe = /htmltextshouldnotbefetchedtopasstest/;

need better var names here:

+      let ch = ioS.newChannelFromURI(uri.value);
+      let inp = ch.open();
+      let sc = Cc["@mozilla.org/scriptableinputstream;1"]
+                 .createInstance(Ci.nsIScriptableInputStream);
+      sc.init(inp);
+      let av;

not used:
+        do_check_true(xRe.test(buf));
+//        do_check_false(yRe.test(buf));

this needs to be do_timeout_function for the test to work:

+  do_timeout(700, endTest);

However, once I fixed that, the test does work!

+---------toppart--
\ No newline at end of file

is that intentional? If not, please fix.

Comment 24

[Phil Lacy]

(In reply to comment #23)
> this needs to be do_timeout_function for the test to work:
> 
> +  do_timeout(700, endTest);

Worked for me but I'll change it and recheck with my setup. 

(In reply to comment #22)
 
> don't need the braces. And what does xStr as a variable name mean? A more
> meaningful var name might be helpful.

I forgot.. ;). I'll fix the cryptic variables. 

The whole thing became an exercise in regex. After I was done it looked like string functions may have been clearer but I'm not changing it now. It isn't very robust and will take some hand holding in making up the test files to suit.
But it is a start and can check our problems in 'download on demand'. If other purposes need to fit, it can be adapted then.

This isn't right either
+      mime[1] = mime[1].replace(/\r\n\s*/," ") // folding

believe it needs to be like previous folding line plus both need a /g
+      mime[1] = mime[1].replace(/\r\n\s+/g," ") // folding

Comment 25

[Phil Lacy]

Attachment: adds BODYSTRUCTURE and fetch by body[part] per r-

feel free to edit my variable naming. Easier to spot by the editor than author.

Comment 26

[Phil Lacy]

Attachment: test cases for and 246415 and future test cases per r-

again, read over my variable naming.

Comment 27

[Phil Lacy]

(In reply to comment #23)
> +---------toppart--
> \ No newline at end of file
> 
> is that intentional? If not, please fix.

forgot to fix this but it is a sample message which is not required to end in newline so it should be ok.

Comment 28

[Phil Lacy]

Comment on attachment 436869 [details] [diff] [review]
test cases for  and 246415 and future test cases per r-

>+ * To add messages to the test, place the 'marker' text used for testing in the

'markerRe' text

>+    let xRe;
>+    if (isPlain)
>+      xRe = /thisplaintextneedstodisplaytopasstest/;
>+    else
>+      xRe = /thishtmltextneedstodisplaytopasstest/;

this is the 'marker' text referred to in the description so the name should be changed from xRe to markerRe

Comment 29

[Phil Lacy]

Attachment: ready for checkin or sr-- r+=bienvenu

Comment 30

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/1664be66815d

Comment 31

[Mark Banner (:standard8)]

I just backed this out due to unit test failures: http://hg.mozilla.org/comm-central/rev/1b490fdf5e8a

http://tinderbox.mozilla.org/showlog.cgi?log=Thunderbird3.1/1270761513.1270762664.14275.gz

(I was seeing them locally on mac as well, but couldn't figure out the issue at a quick glance).

Comment 32

[Phil Lacy]

reminder to check in both attachments, one is the actual patch to imapd.js but the one checked in is the test for the patch.

Don't check in attachment 428077 [details]

Comment 33

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/b973e83b1e4e

Sorry for the delay in getting this back in again.