Closed Bug 939700 Opened 11 years ago Closed 10 years ago

add some mozmill tests for the manual attachment reminder in compose window

Categories

(Thunderbird :: Message Compose Window, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 29.0

People

(Reporter: aceman, Assigned: aceman)

References

Details

Attachments

(1 file, 6 obsolete files)

Add some mozmill tests for the manual attachment reminder added in bug 521158.

This will also be useful to do before the potential refactoring of the bar in bug 938829.
No longer blocks: 521158
Depends on: 521158, 938759
Summary: add some mozmill tests for the manual attachment reminder → add some mozmill tests for the manual attachment reminder in compose window
Attached patch patch (obsolete) — Splinter Review
Here it is.
Flags: needinfo?(mkmelin+mozilla)
Status: NEW → ASSIGNED
Flags: needinfo?(mkmelin+mozilla)
Comment on attachment 8334055 [details] [diff] [review]
patch

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

I tried to cover the use cases discussed in bug 521158 and bug 938759. If you guys can think up some more edge cases, I can add them.
Attachment #8334055 - Flags: review?(mkmelin+mozilla)
Attachment #8334055 - Flags: feedback?(syshagarwal)
Attachment #8334055 - Flags: feedback?(bugzilla2007)
Comment on attachment 8334055 [details] [diff] [review]
patch

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

Overall this is great! And, though not necessary, can we still add a use case when the "Remind Me later" button is clicked from the notification bar?
And one when, firstly checking the reminder, and then un-checking it again and see if we get the alert or not? And last one when we click on the "Reminder Me later" button on the notification bar and look for its "checked" state on the "Remind Me Later" menu option?

Otherwise this it good. :)
Thanks.

::: mail/test/mozmill/composition/test-attachment-reminder.js
@@ +56,3 @@
>  
>    setupComposeWin(cwc, "test@example.org", "testing attachment reminder!",
>                    "Hjello! ");

Though this isn't related with this patch, can/should we correct this typo here?
Attachment #8334055 - Flags: feedback?(syshagarwal) → feedback+
(In reply to Suyash Agarwal (:sshagarwal) (unfortunately away till 1st Dec) from comment #3)
> Overall this is great! And, though not necessary, can we still add a use
> case when the "Remind Me later" button is clicked from the notification bar?
> And one when, firstly checking the reminder, and then un-checking it again
> and see if we get the alert or not? And last one when we click on the
> "Reminder Me later" button on the notification bar and look for its
> "checked" state on the "Remind Me Later" menu option?
There are already some preexisting tests operating on the automatic reminder bar so I do not cover those. But yes, I will check which use cases are new behaviour introduced in bug 521158 and add those tests.

> >    setupComposeWin(cwc, "test@example.org", "testing attachment reminder!",
> >                    "Hjello! ");
> Though this isn't related with this patch, can/should we correct this typo
> here?
Yes, assuming it is a typo and not a part of the test or the author's joke :)

Thanks!
Attachment #8334055 - Flags: review?(mkmelin+mozilla)
Attached patch patch v2 (obsolete) — Splinter Review
OK, I think I covered all the proposed use cases.
Attachment #8334055 - Attachment is obsolete: true
Attachment #8334055 - Flags: feedback?(bugzilla2007)
Attachment #8334876 - Flags: review?(mkmelin+mozilla)
Attachment #8334876 - Flags: feedback?(syshagarwal)
Attachment #8334876 - Flags: feedback?(bugzilla2007)
Comment on attachment 8334876 [details] [diff] [review]
patch v2

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

Great patch! :)

::: mail/test/mozmill/composition/test-attachment-reminder.js
@@ +308,5 @@
> +  // Now disable the manual reminder.
> +  click_manual_reminder(cwc, false);
> +  // Give the notification time to appear. It doesn't.
> +  // TODO: this behaviour is questionable. There is a keyword already in the
> +  // text so maybe the notification should come up immediately.

I like the questionable behavior :)
But I think this should be acceptable as the user manually chose not be to reminded about the attachments.

That said, if we further extend your use-case and let the notification bar reappear, the user chooses "Remind me later" from the notification bar, and then un-checks the "Remind Me later" from the menu, should we show the bar again? That will get us in a funny game :)

Rest, whatever you say will be done.
Thanks.
Attachment #8334876 - Flags: feedback?(syshagarwal) → feedback+
Comment on attachment 8334876 [details] [diff] [review]
patch v2

(In reply to Suyash Agarwal (:sshagarwal) (unfortunately away till 1st Dec) from comment #6)
> Comment on attachment 8334876 [details] [diff] [review]
> patch v2
> Review of attachment 8334876 [details] [diff] [review]:
> -----------------------------------------------------------------
> Great patch! :)

+1! These tests are really impressive. I'm still a newbie to tests, so I just scanned this over but I haven't checked all the details, except where they hit me ;)

> ::: mail/test/mozmill/composition/test-attachment-reminder.js
> @@ +308,5 @@
> > +  // Now disable the manual reminder.
> > +  click_manual_reminder(cwc, false);
> > +  // Give the notification time to appear. It doesn't.
> > +  // TODO: this behaviour is questionable. There is a keyword already in the
> > +  // text so maybe the notification should come up immediately.
> 
> I like the questionable behavior :)
> But I think this should be acceptable as the user manually chose not be to
> reminded about the attachments.

+1. Unchecking "Remind me later" means "Do NOT remind me later".
That's deliberate opt-out after which there is no point showing notification whose main purpose is to remind user and offer him to be hard-reminded later.
So the remaining use of the notification would be only "Add Attachment..." button which (without the just-rejected reminder function) is now just a duplication of button on toolbar, and it's unlikely user wants that.

> That said, if we further extend your use-case and let the notification bar
> reappear, the user chooses "Remind me later" from the notification bar, and
> then un-checks the "Remind Me later" from the menu, should we show the bar
> again? That will get us in a funny game :)

+1...lol! We could turn that into a shooting game where users have to shoot the reminder whereever they see it pop up again... ;)


># HG changeset patch
># Parent ffe30ba8cd1bcc73b49e9b5bbcf8e47c7d7199de
># User aceman <acelists@atlas.sk>
>Bug 939700 - Tests for manual attachment reminder from bug 521158. r?mkmelin
>
>diff --git a/mail/test/mozmill/composition/test-attachment-reminder.js b/mail/test/mozmill/composition/test-attachment-reminder.js
>--- a/mail/test/mozmill/composition/test-attachment-reminder.js
>+++ b/mail/test/mozmill/composition/test-attachment-reminder.js
>@@ -10,116 +10,162 @@ const MODULE_NAME = "test-attachment-rem
> 
> const RELATIVE_ROOT = "../shared-modules";
> const MODULE_REQUIRES = ["folder-display-helpers",
>                          "compose-helpers",
>                          "window-helpers",
>                          "notificationbox-helpers"];
> 
> Cu.import("resource://gre/modules/Services.jsm");
>+Cu.import("resource:///modules/mailServices.js");
> 
> const kBoxId = "attachmentNotificationBox";
> const kNotificationId = "attachmentReminder";
>+const notificationSlackTime = 1100;
> 
> function setupModule(module) {
>-  collector.getModule("folder-display-helpers").installInto(module);
>-  collector.getModule("compose-helpers").installInto(module);
>-  collector.getModule("window-helpers").installInto(module);
>-  collector.getModule("notificationbox-helpers").installInto(module);
>+  for (let lib of MODULE_REQUIRES) {
>+    collector.getModule(lib).installInto(module);
>+  }
> };
> 
> function setupComposeWin(aCwc, toAddr, subj, body) {
>   aCwc.type(null, toAddr);
>   aCwc.type(aCwc.eid("msgSubject"), subj)
>   aCwc.type(aCwc.eid("content-frame"), body);
> }
> 
> /**
>- * Test that the attachment works, in general.
>+ * Check if the attachment reminder bar is in the wished state.
>+ *
>+ * @param aCwc    A compose window controller.
>+ * @param aShown  True for expecting the bar to be shown, false otherwise.
>+ *
>+ * @return        If the bar is shown, return the notification object.
>+ */
>+function assert_reminder_state(aCwc, aShown) {

Nice wrap for better readability!
Perhaps the name of this function could include a hint to the *notification bar* to avoid being mistaken for the manual-reminder state (when I hear "Reminder" only, I really think of the reminder alert...)

something like:
assert_reminderbar_shown (my favourite)
assert_reminderbar_state
assert_reminder_notification_state

>+  return assert_notification_displayed(aCwc, kBoxId, kNotificationId, aShown);
>+}
>+
>+/**
>+ * Check whether the manual reminder is in the proper state.
>+ *
>+ * @param aCwc      A compose window controller.
>+ * @param aChecked  Whether the reminder should be enabled.
>+ */
>+function check_manual_reminder_state(aCwc, aChecked) {

is the difference between assert_... and check_... in function names deliberate?
Perhaps better use assert_ for both?

>+  // Check the reminder is really enabled.
>+  let attachment_menu = aCwc.click_menus_in_sequence(aCwc.e("menu_FilePopup"),
>+                                                     [ {id: "menu_Attach"} ], true);
>+  let checkedValue = aChecked ? "true" : "false";
>+  assert_equals(aCwc.e("menu_Attach_RemindLaterItem").getAttribute("checked"),
>+                checkedValue);
>+  aCwc.close_popup_sequence(attachment_menu);
>+
>+  assert_equals(aCwc.e("cmd_remindLater").getAttribute("checked"), checkedValue);
>+}
>+
>+/**
>+ * Test that the attachment reminder works, in general.
>  */
> function test_attachment_reminder_appears_properly() {
>   let cwc = open_compose_new_mail();
>   let notificationBox = cwc.e(kBoxId);
> 
[snip]

>+  // Now enable the manual reminder.
>+  click_manual_reminder(cwc, true);
>+  // The attachment notification should disappear.
>+  wait_for_notification_to_stop(cwc, kBoxId, kNotificationId);
>+
>+  // Add some more text with another keyword so the automatic notification
>+  // could potentially show up.
>+  setupComposeWin(cwc, "", "", " and find it attached!");
>+  // Give the notification time to appear. It shouldn't.
>+  cwc.sleep(notificationSlackTime);
>+  assert_reminder_state(cwc, false);
>+
>+  // Now disable the manual reminder.
>+  click_manual_reminder(cwc, false);
>+  // Give the notification time to appear. It doesn't.

"It shouldn't.". See top of this comment.

>+  // TODO: this behaviour is questionable. There is a keyword already in the
>+  // text so maybe the notification should come up immediately.

dito, no.

>+  cwc.sleep(notificationSlackTime);
>+  assert_reminder_state(cwc, false);
>+
>+  // Add some more text without any new keyword, but the automatic notification
>+  // could potentially show up.
>+  setupComposeWin(cwc, "", "", " Did I write anything?");
>+  // Give the notification time to appear. It does now.
>+  cwc.sleep(notificationSlackTime);
>+  assert_reminder_state(cwc, true);

That looks wrong: Bar is hidden, type text /without/ keyword, why should bar appear?
I think this needs "It shouldn't" and assert_reminder_state(cwc, false);

[snip]
Attachment #8334876 - Flags: feedback?(bugzilla2007)
(In reply to Thomas D. from comment #7)
> Comment on attachment 8334876 [details] [diff] [review]
> patch v2
> 
> (In reply to Suyash Agarwal (:sshagarwal) (unfortunately away till 1st Dec)
> from comment #6)
> > Comment on attachment 8334876 [details] [diff] [review]
> > patch v2
> > Review of attachment 8334876 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > Great patch! :)
> 
> +1! These tests are really impressive. I'm still a newbie to tests, so I
> just scanned this over but I haven't checked all the details, except where
> they hit me ;)

So that is why I add so many comments :) Everybody can see what the test does and what results it expects. You can properly comment if that result is expected. And you did, thanks :)

> >+function assert_reminder_state(aCwc, aShown) {
> Nice wrap for better readability!
> Perhaps the name of this function could include a hint to the *notification
> bar* to avoid being mistaken for the manual-reminder state (when I hear
> "Reminder" only, I really think of the reminder alert...)
> something like:
> assert_reminderbar_shown (my favourite)
> assert_reminderbar_state
> assert_reminder_notification_state
> >+function check_manual_reminder_state(aCwc, aChecked) {
> is the difference between assert_... and check_... in function names
> deliberate?
> Perhaps better use assert_ for both?
Yeah, I will sort it out.

> >+  // Add some more text without any new keyword, but the automatic notification
> >+  // could potentially show up.
> >+  setupComposeWin(cwc, "", "", " Did I write anything?");
> >+  // Give the notification time to appear. It does now.
> >+  cwc.sleep(notificationSlackTime);
> >+  assert_reminder_state(cwc, true);
> 
> That looks wrong: Bar is hidden, type text /without/ keyword, why should bar
> appear?
> I think this needs "It shouldn't" and assert_reminder_state(cwc, false);
That is simply the current behaviour. If the manual reminder is off, the automatic one does its job when you type anything. It finds the old keyword so it shows the bar. There is no remembering that you dismissed the bar sometime in the past.

The tests just store the current behaviour. They serve to catch unintentional changes in behaviour. If anybody changes behaviour intentionally (but in a new bug), then he updates the test.
Comment on attachment 8334876 [details] [diff] [review]
patch v2

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

Looks good to me. r=mkmelin
Attachment #8334876 - Flags: review?(mkmelin+mozilla) → review+
Attached patch patch v3 (obsolete) — Splinter Review
Cleanup of the code by Thomas D. No functional changes. Carrying over r=mkmelin.
Attachment #8334876 - Attachment is obsolete: true
Attachment #8340073 - Flags: review+
Keywords: checkin-needed
Priority: P2 → --
https://hg.mozilla.org/comm-central/rev/8192e8b529d2
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 28.0
Backed out for OSX failures.
https://hg.mozilla.org/comm-central/rev/aeeb4b2ee209

https://tbpl.mozilla.org/php/getParsedLog.php?id=31318963&tree=Thunderbird-Trunk
Status: RESOLVED → REOPENED
Flags: in-testsuite+
Resolution: FIXED → ---
Target Milestone: Thunderbird 28.0 → ---
Yeah, OS X always needs to be different!

The popup menu_FilePopup should be there on Mac (is not ifdefed out), so what is different there?
Status: REOPENED → ASSIGNED
Flags: needinfo?(mconley)
Flags: needinfo?(bwinton)
It's there, but not available to mozmill. I had similar experiences in bug 179033 comment 46.
I don't think there is an appmenu in the compose window. But yes, the compose window menu can also be detached somewhere. I can try using the toolbar button.
Attached patch patch v4 (obsolete) — Splinter Review
Ok, this version uses the toolbar button (which includes the Remind me later item in the popup menu).

Aryx, if you could make me a try run with this patch, all platforms, mozmill only.
Attachment #8340073 - Attachment is obsolete: true
Flags: needinfo?(mconley)
Flags: needinfo?(bwinton)
Flags: needinfo?(archaeopteryx)
So the results on OS X 10.8 are inconclusive. The test does fail, but i think it is before my code gets to run. It's like the compose window can't be opened. And then there is the "Disconnect Error: Application unexpectedly closed".
That is possible. But the patch was backed out and changed exactly due to failing on OS X, it needs to prove it works on OS X now. I need to wait until this other breakage is fixed.
Aryx, can you try to push this again?
Flags: needinfo?(archaeopteryx)
Comment on attachment 8342564 [details] [diff] [review]
patch v4

OK, I see no failures of test-attachment-reminder.js in https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=14994ceaa2f5 .
Attachment #8342564 - Flags: review?(mkmelin+mozilla)
Comment on attachment 8342564 [details] [diff] [review]
patch v4

Somehow, there are suddenly failures on OS X. They were not there when I looked an hour ago on another machine. That is weird...
Anyway, the failures in this test are there even without my patch. So we need to wait again.
Attachment #8342564 - Flags: review?(mkmelin+mozilla)
Flags: needinfo?(archaeopteryx)
Aryx, can you please push this again to TB-try? OS X seems green right now.
Flags: needinfo?(archaeopteryx)
Is there a way to retrigger a try build without this patch so we have a reference for comparison?
If it is always necessary to push at least one patch, try the one in bug 946669.
Flags: needinfo?(archaeopteryx)
It seems this new patch did it:
https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=535555b00bab

Credits go to Aryx for debugging the problem and finding the suggestion to remove the draft message after the test!
Keywords: checkin-needed
Attachment #8342564 - Attachment is obsolete: true
https://hg.mozilla.org/comm-central/rev/88fdc4a97ae7
Status: ASSIGNED → RESOLVED
Closed: 10 years ago10 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 29.0
Backed out for failures on OSX.
https://hg.mozilla.org/comm-central/rev/bdb58bfe1041

https://tbpl.mozilla.org/php/getParsedLog.php?id=33399721&tree=Thunderbird-Trunk
https://tbpl.mozilla.org/php/getParsedLog.php?id=33399811&tree=Thunderbird-Trunk

TEST-START | /builds/slave/talos-slave/test/build/mozmill/composition/test-attachment-reminder.js | test_attachment_reminder_dismissal
Step Pass: {"function": "Controller.keypress()"}
addOverrideStyleSheet in MsgComposeCommands.js threw an exception, hopefully due to a missing stylesheet
Step Pass: {"function": "Controller.type()"}
Step Pass: {"function": "Controller.type()"}
Step Pass: {"function": "Controller.type()"}
Step Pass: {"function": "controller.waitFor()"}
Step Pass: {"function": "controller.click()"}
Step Pass: {"function": "controller.click()"}
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIMsgMailNewsUrl.server]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: resource:///modules/activity/alertHook.js :: alertHook.onAlert :: line 48"  data: no]
************************************************************
TEST-PASS | /builds/slave/talos-slave/test/build/mozmill/composition/test-attachment-reminder.js | test-attachment-reminder.js::test_attachment_reminder_dismissal
TEST-START | /builds/slave/talos-slave/test/build/mozmill/composition/test-attachment-reminder.js | test_attachment_reminder_aggressive_pref
Step Pass: {"function": "Controller.keypress()"}
addOverrideStyleSheet in MsgComposeCommands.js threw an exception, hopefully due to a missing stylesheet
Step Pass: {"function": "Controller.type()"}
Step Pass: {"function": "Controller.type()"}
Step Pass: {"function": "Controller.type()"}
Step Pass: {"function": "controller.waitFor()"}
Step Pass: {"function": "controller.click()"}
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIMsgMailNewsUrl.server]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: resource:///modules/activity/alertHook.js :: alertHook.onAlert :: line 48"  data: no]
************************************************************
TEST-PASS | /builds/slave/talos-slave/test/build/mozmill/composition/test-attachment-reminder.js | test-attachment-reminder.js::test_attachment_reminder_aggressive_pref
TEST-START | /builds/slave/talos-slave/test/build/mozmill/composition/test-attachment-reminder.js | test_no_send_now_sends
Step Pass: {"function": "Controller.keypress()"}
addOverrideStyleSheet in MsgComposeCommands.js threw an exception, hopefully due to a missing stylesheet
Step Pass: {"function": "Controller.type()"}
Step Pass: {"function": "Controller.type()"}
Step Pass: {"function": "Controller.type()"}
Step Pass: {"function": "controller.waitFor()"}
Timeout: bridge.execFunction("a004a446-8380-11e3-97fe-c82a140bc4f1", bridge.registry["{0e54541b-5893-f64f-a84d-230b012511f3}"]["runTestDirectory"], ["/builds/slave/talos-slave/test/build/mozmill/composition"])

TEST-UNEXPECTED-FAIL | Disconnect Error: Application unexpectedly closed
Status: RESOLVED → REOPENED
Flags: in-testsuite+
Resolution: FIXED → ---
Target Milestone: Thunderbird 29.0 → ---
Hey, there were no failures of OS X on TB-try :(
And the retriggers were green too :\. Don't you "love" when this happens? :)
So what can I do now? Is there any difference between the servers? Is it a timing issue as the TB-try servers seems to be a lot slower?
Flags: needinfo?(mbanner)
Yes, so you retriggered the tests in the original push and they succeeded.
Also when you backed out my patch, the same offending test test_no_send_now_sends still failed:
https://tbpl.mozilla.org/php/getParsedLog.php?id=33405948&tree=Thunderbird-Trunk

So maybe there is no problem in my patch. But I still try to update a new one as there is questionable handling of the dialogs in that test.
Comment on attachment 8363929 [details] [diff] [review]
patch v6 (tweak clicking on dialogs in test_no_send_now_sends)

Looks like this was successful on try again:
https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=224df3574ae9

mkmelin, can you please check the change in click_send_and_handle_send_error and can we try to push this to trunk again?
Attachment #8363929 - Flags: review?(mkmelin+mozilla)
Comment on attachment 8363929 [details] [diff] [review]
patch v6 (tweak clicking on dialogs in test_no_send_now_sends)

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

::: mail/test/mozmill/composition/test-attachment-reminder.js
@@ +188,5 @@
>    wait_for_modal_dialog("commonDialog");
>  
> +  click_send_and_handle_send_error(cwc, true);
> +
> +  close_compose_window(cwc);

you could add a comment that "we're now back in the compose window, let's close it then"
Attachment #8363929 - Flags: review?(mkmelin+mozilla) → review+
Attached patch patch v7Splinter Review
Thanks. Let's try to push it to trunk again.
Attachment #8363090 - Attachment is obsolete: true
Attachment #8363929 - Attachment is obsolete: true
Attachment #8365666 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/7bbe14c99c91
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 29.0
Wow, did it work this time?! https://tbpl.mozilla.org/?tree=Thunderbird-Trunk&rev=c1f8875ca4cd

I think this second idea was from Aryx too so more credits to him! :)
Flags: needinfo?(mbanner)
Depends on: 964377
You need to log in before you can comment on or make changes to this bug.