Last Comment Bug 725926 - Cleanup test_attachment.js, test_detectAttachmentCharset.js and test_saveDraft.js
: Cleanup test_attachment.js, test_detectAttachmentCharset.js and test_saveDraf...
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Testing Infrastructure (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 15.0
Assigned To: Hiroyuki Ikezoe (:hiro)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-09 21:08 PST by Hiroyuki Ikezoe (:hiro)
Modified: 2012-05-08 15:41 PDT (History)
4 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Proposed fix (22.36 KB, patch)
2012-02-09 21:19 PST, Hiroyuki Ikezoe (:hiro)
mconley: review-
Details | Diff | Review
Revised patch (26.71 KB, patch)
2012-04-10 03:27 PDT, Hiroyuki Ikezoe (:hiro)
mconley: review+
Details | Diff | Review

Description Hiroyuki Ikezoe (:hiro) 2012-02-09 21:08:20 PST
test_attachment.js, test_detectAttachmentCharset.js and test_saveDraft.js have duplicate codes (e.g. progressListener, createMessage, doTest, etc, etc.), it should be unified.
Comment 1 Hiroyuki Ikezoe (:hiro) 2012-02-09 21:19:48 PST
Created attachment 595959 [details] [diff] [review]
Proposed fix

* Move duplicate codes into head_compose.js
* Add utility function (getAttachmentFromContent) to get the string of the first attachment data.
* Rewrite all tests using asyncTestUtils

I just noticed the asyncTestUtils.js today, so please correct me if I am doing wrong about asyncTestUtils.
Comment 2 Mike Conley (:mconley) - (Away until June 29th) 2012-02-13 09:00:40 PST
Comment on attachment 595959 [details] [diff] [review]
Proposed fix

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

Hey Hiro,

This looks really good!  Just a few nits. 

Do you have a try build for this patch, showing the tests passing?

-Mike

::: mailnews/compose/test/unit/head_compose.js
@@ -91,0 +92,25 @@
> > +
> > +var progressListener = {
> > +  onStateChange: function(aWebProgress, aRequest, aStateFlags, aStatus) {
> > +    if (aStateFlags & Ci.nsIWebProgressListener.STATE_STOP)
NaN more ...

Replace all of the "var" here in this function with "let"

@@ -91,0 +92,47 @@
> > +
> > +var progressListener = {
> > +  onStateChange: function(aWebProgress, aRequest, aStateFlags, aStatus) {
> > +    if (aStateFlags & Ci.nsIWebProgressListener.STATE_STOP)
NaN more ...

nit: space after //

@@ -91,0 +92,67 @@
> > +
> > +var progressListener = {
> > +  onStateChange: function(aWebProgress, aRequest, aStateFlags, aStatus) {
> > +    if (aStateFlags & Ci.nsIWebProgressListener.STATE_STOP)
NaN more ...

I can't see much use for this yield here, *unless* it's required by async_run...is it?

::: mailnews/compose/test/unit/test_attachment.js
@@ -169,3 +104,3 @@
> >  
> > -const gTestArray =
> > +function testInput0() {
> > -[
> > +  for (let folding = 0; folding <= 2; folding++) {

Where did this magic number 2 come from? Maybe this should be a const.

@@ +112,3 @@
>  
> +function testInput1() {
> +  for (let folding = 0; folding <= 2; folding++) {

Same as above, re magic number
Comment 3 Hiroyuki Ikezoe (:hiro) 2012-02-13 21:57:45 PST
Mike, Thank you for your review!
But unfortunately, Splinter broke some parts of your review.... so I couldn't understand whole of your comments. Please review again.
Comment 4 Mike Conley (:mconley) - (Away until June 29th) 2012-02-14 10:32:31 PST
Comment on attachment 595959 [details] [diff] [review]
Proposed fix

Appending to my review queue
Comment 5 Mike Conley (:mconley) - (Away until June 29th) 2012-02-17 06:49:30 PST
Comment on attachment 595959 [details] [diff] [review]
Proposed fix

+};
+
+function createMessage(aAttachment) {
+  var fields = Cc["@mozilla.org/messengercompose/composefields;1"]

Replace all of the "var" in this function with "let".

+  //Set attachment
+  if (aAttachment) {
+    var attachment = Cc["@mozilla.org/messengercompose/attachment;1"]
+                       .createInstance(Ci.nsIMsgAttachment);

Nit - space after //


+                   .createInstance(Ci.nsIMsgProgress);
+  progress.registerListener(progressListener);
+  msgCompose.SendMsg(Ci.nsIMsgSend.nsMsgSaveAsDraft, identity, "", null,
+                     progress);
+  yield false;
+}

I can't see much use for this yield here, *unless* it's required by async_run...is it?

::: mailnews/compose/test/unit/test_attachment.js
@@ -169,3 +104,3 @@
> >  
> > -const gTestArray =
> > +function testInput0() {
> > -[
> > +  for (let folding = 0; folding <= 2; folding++) {

Where did this magic number 2 come from? Maybe this should be a const.

@@ +112,3 @@
>  
> +function testInput1() {
> +  for (let folding = 0; folding <= 2; folding++) {

Same as above, re magic number


Also - we've been oranging a lot on comm-central, and I'd like to see some try build results for these tests.

Thanks!

-Mike
Comment 6 Hiroyuki Ikezoe (:hiro) 2012-04-10 03:27:45 PDT
Created attachment 613536 [details] [diff] [review]
Revised patch

(In reply to Mike Conley (:mconley) from comment #5)

> +
> +function createMessage(aAttachment) {
> +  var fields = Cc["@mozilla.org/messengercompose/composefields;1"]
> 
> Replace all of the "var" in this function with "let".

Fixed.
 
> +  //Set attachment
> +  if (aAttachment) {
> +    var attachment = Cc["@mozilla.org/messengercompose/attachment;1"]
> +                       .createInstance(Ci.nsIMsgAttachment);
> 
> Nit - space after //

Fixed.

> +                   .createInstance(Ci.nsIMsgProgress);
> +  progress.registerListener(progressListener);
> +  msgCompose.SendMsg(Ci.nsIMsgSend.nsMsgSaveAsDraft, identity, "", null,
> +                     progress);
> +  yield false;
> +}
> 
> I can't see much use for this yield here, *unless* it's required by
> async_run...is it?

You are right. This was my mistake.
 
> ::: mailnews/compose/test/unit/test_attachment.js
> @@ -169,3 +104,3 @@
> > >  
> > > -const gTestArray =
> > > +function testInput0() {
> > > -[
> > > +  for (let folding = 0; folding <= 2; folding++) {
> 
> Where did this magic number 2 come from? Maybe this should be a const.
> 
> @@ +112,3 @@
> >  
> > +function testInput1() {
> > +  for (let folding = 0; folding <= 2; folding++) {
> 
> Same as above, re magic number

This magic number originally come from preference values. But right, it is hard to understand.

In this patch, some new objects are introduced to hide this magic numbers like this:

const ParamFoldingPref = {
  RFC2047: 0,
  RFC2047WithCRLF: 1,
  RFC2231: 2
}

const expectedCTList0 = {
  RFC2047:         'Content-Type: ...',
  RFC2047WithCRLF: 'Content-Type: ...',
  RFC2231:         'Content-Type: ...'
}
.
.
.
for (let folding in ParamFoldingPref) {
  Services.prefs.setIntPref("mail.strictly_mime.parm_folding", ParamFoldingPref[folding]);
  checkAttachment(expectedCD0, expectedCTList0[folding]);
}

I have no better idea other than this.
Comment 7 Hiroyuki Ikezoe (:hiro) 2012-04-10 03:28:58 PDT
I just pushed this patch on try server:

http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=c37cadc25a29
Comment 8 Mike Conley (:mconley) - (Away until June 29th) 2012-04-16 14:02:44 PDT
Comment on attachment 613536 [details] [diff] [review]
Revised patch

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

A few more nits - also, I've asked bienvenu to take a look at this as well, for the attachment extraction bit.

::: mailnews/compose/test/unit/head_compose.js
@@ +96,5 @@
> +      async_driver();
> +  },
> +
> +  onProgressChange: function(aWebProgress, aRequest, aCurSelfProgress,
> +    aMaxSelfProgress, aCurTotalProgress, aMaxTotalProgress) {},

Since we're still in parentheses, I think this should be indented such that the first "a" in aMaxSelfProgress lines up with the first "a" in aWebProgress on the line above.  If that makes the line go over 80 chars, you'll need to break it again.

@@ +101,5 @@
> +  onLocationChange: function(aWebProgress, aRequest, aLocation, aFlags) {},
> +  onStatusChange: function(aWebProgress, aRequest, aStatus, aMessage) {},
> +  onSecurityChange: function(aWebProgress, aRequest, state) {},
> +
> +  QueryInterface : function(iid) {

If you import resource://gre/modules/XPCOMUtils.jsm, you can just do:

QueryInterface: XPCOMUtils.generateQI([Ci.nsIWebProgressListener,
                                       Ci.nsISupportsWeakReference]),

@@ +165,5 @@
> +    return enumerator.getNext().QueryInterface(Ci.nsIMsgDBHdr);
> +  return null;
> +}
> +
> +function getAttachmentFromContent(aContent) {

Not sure about this function here.  It strikes me that it's likely we've got some built-in stuff that could / should do this better than using regex.

I'll ping bienvenu and see what he thinks.

::: mailnews/compose/test/unit/test_attachment.js
@@ +43,3 @@
>  
> +const expectedCTList0 = {
> +  RFC2047: 'Content-Type: text/plain; charset=US-ASCII;\r\n'+

All of the pluses in these long strings should have a space before them to distinguish them from the strings themselves.

So:

'=?ISO-8859-1?Q?efghijklmnopqrstuvwxyz=7B=7C=7D=7E=A0=A1=A2=A3=A4=A5=A6=A7?=' +
'=?ISO-8859-1?Q?=CE=CF=D0=D1=D2=D3=D4=D5=D6=D7=D8=D9=DA=DB=DC=DD=DE=DF=E0?=' +

etc.  Applies to all below.
Comment 9 Mike Conley (:mconley) - (Away until June 29th) 2012-04-16 14:11:06 PDT
Comment on attachment 613536 [details] [diff] [review]
Revised patch

bienvenu seems to think that getAttachmentFromContent is fine for checking the charset.

So, with the nits I found fixed, I'm cool giving this an r+.
Comment 10 David :Bienvenu 2012-05-07 13:55:59 PDT
so this is ready for checkin, right?
Comment 11 Ryan VanderMeulen [:RyanVM] 2012-05-08 15:41:15 PDT
http://hg.mozilla.org/comm-central/rev/88f607d47225

Note You need to log in before you can comment on or make changes to this bug.