Closed Bug 540676 Opened 10 years ago Closed 10 years ago

fake IMAP server doesn't support BODYSTRUCTURE

Categories

(MailNews Core :: Testing Infrastructure, defect)

defect
Not set

Tracking

(thunderbird3.1 beta2-fixed)

RESOLVED FIXED
Thunderbird 3.1b2
Tracking Status
thunderbird3.1 --- beta2-fixed

People

(Reporter: philbaseless-firefox, Assigned: philbaseless-firefox)

Details

Attachments

(3 files, 8 obsolete files)

Need to test various messages for 'download as links' bugs.
There are some open source GNU GPL license imap servers if that would even be practical.
Assignee: nobody → philbaseless-firefox
Returning a fake bodystructure would be easy enough, but how would we inspect the download on demand problem?
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.
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.
no I don't think the header=quotebody has anything to do with the fakeserver. That's handled by client side I believe.
uses quoteMessage, but has problems.
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"
tried streamMessage also, but it doesn't try a bodystructure
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.
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.
Attachment #424515 - Attachment is obsolete: true
With this reverse patch applied one can test the bodystructure tests.
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.
Attachment #426942 - Attachment is obsolete: true
Attachment #428102 - Flags: review?(bienvenu)
Attached patch test for one file (obsolete) — Splinter Review
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.
Attachment #428103 - Flags: review?(bienvenu)
Attached file test results (obsolete) —
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.
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.
Attachment #428103 - Flags: review?(bienvenu)
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 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 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
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.
Attachment #428102 - Attachment is obsolete: true
Attachment #429926 - Flags: review?(bienvenu)
Attachment #428102 - Flags: review?(bienvenu)
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
Attachment #428103 - Attachment is obsolete: true
Attachment #428104 - Attachment is obsolete: true
Attachment #429927 - Flags: review?(bienvenu)
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.
Status: NEW → ASSIGNED
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.
Attachment #429926 - Flags: review?(bienvenu) → review-
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.
Attachment #429927 - Attachment description: test cases for bugs 244741 and 246415 and future test cases → test cases for and 246415 and future test cases
Attachment #429927 - Flags: review?(bienvenu) → review-
(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
feel free to edit my variable naming. Easier to spot by the editor than author.
Attachment #429926 - Attachment is obsolete: true
Attachment #436868 - Flags: review?(bienvenu)
again, read over my variable naming.
Attachment #429927 - Attachment is obsolete: true
Attachment #436869 - Flags: review?(bienvenu)
(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 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
Attachment #436868 - Flags: review?(bienvenu) → review+
Attachment #436869 - Flags: review?(bienvenu) → review+
Attachment #436869 - Attachment is obsolete: true
Attachment #437489 - Flags: review+
Keywords: checkin-needed
Checked in: http://hg.mozilla.org/comm-central/rev/1664be66815d
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.1b2
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).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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]
Status: REOPENED → ASSIGNED
Checked in: http://hg.mozilla.org/comm-central/rev/b973e83b1e4e

Sorry for the delay in getting this back in again.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.