715961 - Attachment events in compose window need to be tested

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

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

We'd like some Mozmill tests to ensure that the events introduced in bug 708982 actually do what we want them to do.

Comment 1

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

Attachment: Patch v1

Comment 2

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

Comment on attachment 586482 [details] [diff] [review]
Patch v1

Hey squib,

I know your swamped, so I'm not asking for a full review from you, but could you give my tests a quick peek to make sure I'm poking at the right things?

Also, if you can think of more interesting tests I should try, let me know.

Thanks,

-Mike

Comment 4

[Jim Porter (:squib)]

I haven't had a chance to look at all of this, but for simplicity, you don't need to attach a real file. It should be possible to add a fake attachment via add_attachment[1]. You also don't need to synthesize mouse events to select attachments in the list; all the XUL listbox methods, like selectItemRange(), are available.

[1] http://mxr.mozilla.org/comm-central/source/mail/test/mozmill/shared-modules/test-compose-helpers.js#256

Comment 5

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

Attachment: Patch v2

Thanks for the feedback squib!  Very useful - I've been able to simplify my patch immensely.

Comment 6

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

Attachment: Patch v3

Whoops - forgot a pretty critical file.

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

Comment 7

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

Comment on attachment 587344 [details] [diff] [review]
Patch v3

Hey Sid - have any cycles to check this out?

Comment 8

[Rain [:rain1] (inactive)]

Comment on attachment 587344 [details] [diff] [review]
Patch v3

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

::: mail/test/mozmill/attachment/test-attachment-events.js
@@ +1,2 @@
> +/* ***** BEGIN LICENSE BLOCK *****
> + * Version: MPL 1.1/GPL 2.0/LGPL 2.1

We use MPL 2.0 now, which means much simpler headers! http://www.mozilla.org/MPL/headers/

@@ +282,5 @@
> +  let removedAttachmentItems = select_attachments(cw, 0, 2);
> +
> +  let removedAttachmentUrls = removedAttachmentItems.map(function(aAttachment) {
> +    return aAttachment.attachment.url;
> +  });

function (aAttachment) aAttachment.attachment.url

@@ +374,5 @@
> +  assert_true(obs.didSee(kAttachmentRenamed));
> +  // Ensure that the event mentions the right attachment
> +  let renamedAttachment1 = obs.subject[kAttachmentRenamed][0];
> +  assert_true(renamedAttachment1 instanceof Ci.nsIMsgAttachment);
> +  assert_equals(kRenameTo1, renamedAttachment1.name);

Does mail:attachmentRenamed not tell you what the original name was? That seems like an oversight.

@@ +379,5 @@
> +  assert_true(renamedAttachment1.url.indexOf("http://www.example.com/1") != -1);
> +
> +  // Ok, let's try renaming the same attachment.
> +  gMockPromptService.reset();
> +  gMockPromptService.inoutValue = kRenameTo2

;

@@ +393,5 @@
> +  assert_true(renamedAttachment2.url.indexOf("http://www.example.com/1") != -1);
> +
> +  // Ok, let's rename another attachment
> +  gMockPromptService.reset();
> +  gMockPromptService.inoutValue = kRenameTo3

;

@@ +415,5 @@
> +}
> +
> +/**
> + * Test that the mail:attachmentsRenamed event is not fired if we set the
> + * filename to be blank.

Why does this behaviour exist?

::: mail/test/mozmill/shared-modules/test-observer-helpers.js
@@ +1,2 @@
> +/* ***** BEGIN LICENSE BLOCK *****
> + * Version: MPL 1.1/GPL 2.0/LGPL 2.1

MPL 2.0

@@ +34,5 @@
> + * the terms of any one of the MPL, the GPL or the LGPL.
> + *
> + * ***** END LICENSE BLOCK ***** */
> +
> +const MODULE_NAME = "observer-helpers";

This is going to be really, really useful. Thanks for doing this.

@@ +50,5 @@
> +function ObservationRecorder() {
> +  this._topics = [];
> +  this.data = {};
> +  this.subject = {};
> +  this.saw = {};

I'd suggest calling reset instead.

@@ +57,5 @@
> +ObservationRecorder.prototype = {
> +  /**
> +   * Called by the Observer Service when an event is fired.
> +   */
> +  observe: function(aSubject, aTopic, aData) {

Please name this and all the other functions.

@@ +79,5 @@
> +
> +  /**
> +   * Puts the observer back into its starting state.
> +   */
> +  reset: function() {

Might a resetTopic function make sense?

@@ +90,5 @@
> +  /**
> +   * Gets the AttachmentObserver ready to observe events.  Must be called
> +   * before any recording can be done.
> +   *
> +   * @param aTopics A string representing the topic that the AttachmentObserver

that the ObservationRecorder

@@ +107,5 @@
> +
> +  /**
> +   * Returns true of a particular topic was observed at least once.
> +   *
> +   * @param aTopic the topic to check if the AttachmentObserver saw.

the ObservationRecorder saw

Comment 9

[Jim Porter (:squib)]

(In reply to Siddharth Agarwal [:sid0] from comment #8)
> @@ +415,5 @@
> > +}
> > +
> > +/**
> > + * Test that the mail:attachmentsRenamed event is not fired if we set the
> > + * filename to be blank.
> 
> Why does this behaviour exist?

If you try to rename an attachment to the empty string, it will bail out and refuse to actually rename it. Thus, no event gets fired.

Comment 10

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

Attachment: Patch v4

Thanks Sid - I've made the corrections you've suggested.

Awesome how short the MPL 2 header is.

> Does mail:attachmentRenamed not tell you what the original name was? That
> seems like an oversight.

squib:  what's your take on that?

-Mike

Comment 11

[Jim Porter (:squib)]

(In reply to Mike Conley (:mconley) from comment #10)
> > Does mail:attachmentRenamed not tell you what the original name was? That
> > seems like an oversight.
> 
> squib:  what's your take on that?

Seems reasonable. There's a wstring argument to notifyObservers that we could use.

Comment 12

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

Attachment: Patch v5

Ok, I've also added Sid's suggestion of notifying observers of a renamed attachments original name.

Comment 13

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

Comment on attachment 590734 [details] [diff] [review]
Patch v5

Ok, we're notifying observers about the original names of renamed attachments now, as per your suggestion.

Comment 14

[Rain [:rain1] (inactive)]

Comment on attachment 590734 [details] [diff] [review]
Patch v5

Thanks.

Comment 15

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

Committed to comm-central as http://hg.mozilla.org/comm-central/rev/99470a5f7a8b