725926 - Cleanup test_attachment.js, test_detectAttachmentCharset.js and test_saveDraft.js

Status: RESOLVED FIXED

(MailNews Core :: Testing Infrastructure, defect)

Description

[Hiroyuki Ikezoe (:hiro)]

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)]

Attachment: 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) (:⚙️)]

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)]

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) (:⚙️)]

Comment on attachment 595959 [details] [diff] [review]
Proposed fix

Appending to my review queue

Comment 5

[Mike Conley (:mconley) (:⚙️)]

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)]

Attachment: 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)]

I just pushed this patch on try server:

http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=c37cadc25a29

Comment 8

[Mike Conley (:mconley) (:⚙️)]

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) (:⚙️)]

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]

so this is ready for checkin, right?

Comment 11

[Ryan VanderMeulen [:RyanVM]]

http://hg.mozilla.org/comm-central/rev/88f607d47225