Attachment events in compose window need to be tested

RESOLVED FIXED in Thunderbird 12.0

Status

Thunderbird
Message Compose Window
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mconley, Assigned: mconley)

Tracking

Trunk
Thunderbird 12.0
x86
All

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

We'd like some Mozmill tests to ensure that the events introduced in bug 708982 actually do what we want them to do.
Depends on: 708982
Created attachment 586482 [details] [diff] [review]
Patch v1
Assignee: nobody → mconley
Status: NEW → ASSIGNED
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
Attachment #586482 - Flags: feedback?(squibblyflabbetydoo)
Duplicate of this bug: 715490

Comment 4

5 years ago
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
Created attachment 587021 [details] [diff] [review]
Patch v2

Thanks for the feedback squib!  Very useful - I've been able to simplify my patch immensely.
Attachment #586482 - Attachment is obsolete: true
Attachment #586482 - Flags: feedback?(squibblyflabbetydoo)
Created attachment 587344 [details] [diff] [review]
Patch v3

Whoops - forgot a pretty critical file.

http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=33bdc74bedb3
Attachment #587021 - Attachment is obsolete: true
Comment on attachment 587344 [details] [diff] [review]
Patch v3

Hey Sid - have any cycles to check this out?
Attachment #587344 - Flags: review?(sagarwal)
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

5 years ago
(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.
Created attachment 589959 [details] [diff] [review]
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
Attachment #587344 - Attachment is obsolete: true
Attachment #587344 - Flags: review?(sagarwal)

Comment 11

5 years ago
(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.
Created attachment 590734 [details] [diff] [review]
Patch v5

Ok, I've also added Sid's suggestion of notifying observers of a renamed attachments original name.
Attachment #589959 - Attachment is obsolete: true
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.
Attachment #590734 - Flags: review?(sagarwal)
Comment on attachment 590734 [details] [diff] [review]
Patch v5

Thanks.
Attachment #590734 - Flags: review?(sagarwal) → review+
Committed to comm-central as http://hg.mozilla.org/comm-central/rev/99470a5f7a8b
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 12.0
You need to log in before you can comment on or make changes to this bug.