Closed Bug 1142110 Opened 6 years ago Closed 6 years ago

Add a simple browser-chrome test for the popup notifications

Categories

(Toolkit :: Password Manager, defect)

defect
Not set
normal
Points:
2

Tracking

()

RESOLVED FIXED
mozilla39
Iteration:
39.2 - 23 Mar
Tracking Status
firefox39 --- fixed

People

(Reporter: Paolo, Assigned: Paolo)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

Attached patch The patch (obsolete) — Splinter Review
This is a non-intermittent and e10s compatible test for popup notifications.

For the moment I've kept the test very straightforward, but as we add more cases I'll move the code to shared functions as needed. I'm not doing this in advance because just copying the notifications_common.js helpers might not end up with the best structure.

I've already filed bug 1142108 to make TestUtils.topicObserved serve our use case as well, instead of writing our local helper.
Attachment #8576065 - Flags: review?(MattN+bmo)
I'll note that the observer notification I added to the PopupNotification code is fired when everything in the panel is ready, after the consumer-specified callbacks. This means we'll be able to check for the actual contents of the editable fields when we add them later.
Flags: qe-verify-
Flags: firefox-backlog+
Comment on attachment 8576065 [details] [diff] [review]
The patch

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

::: toolkit/components/passwordmgr/test/browser/browser.ini
@@ +1,4 @@
>  [DEFAULT]
>  support-files =
>    authenticate.sjs
> +  form_basic.html

I kinda prefer support files in a data/ subdirectory but I guess you aren't the first to add something here.

::: toolkit/components/passwordmgr/test/browser/browser_notifications.js
@@ +2,5 @@
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +add_task(function* test_save() {
> +  let tab = gBrowser.addTab("https://example.com/browser/toolkit/components/" +
> +                            "passwordmgr/test/browser/form_basic.html");

Nit: It would be nice if there was a utility to get a URL to a relative path from the directory for a specified domain in BrowserTestUtils. Otherwise making a helper with a hard-coded passwordmgr dir in head.js would be good since browser_passwordmgr_switchtab.js also could use it. Neither of these need to block but they would be nice to have.

@@ +7,5 @@
> +  let browser = tab.linkedBrowser;
> +  yield BrowserTestUtils.browserLoaded(browser);
> +  gBrowser.selectedTab = tab;
> +
> +  let promiseShown = TestUtils.topicObserved("popupnotifications-panel-shown");

If we do keep the observer notification, the subject should be the panel so we can distinguish between different consumers of PopupNotifications. We would then use it instead of `PopupNotifications.panel`

@@ +18,5 @@
> +  let notificationElement = PopupNotifications.panel.childNodes[0];
> +
> +  let promiseLogin = TestUtils.topicObserved("passwordmgr-storage-changed",
> +                                             (_, data) => data == "addLogin");
> +  notificationElement.button.doCommand();

I think using  like triggerMainCommand in [2] is closer to what users do.

[2] https://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/popupNotifications/head.js#277

::: toolkit/components/passwordmgr/test/browser/form_basic.html
@@ +1,3 @@
> +<!DOCTYPE html><html><head><meta charset="utf-8"></head><body>
> +<!-- Any copyright is dedicated to the Public Domain.
> +   - http://creativecommons.org/publicdomain/zero/1.0/ -->

My understanding is that tests are implicitly CC0 now so you could make this even more compact by removing this.

@@ +5,5 @@
> +<!-- Simplest form with username and password fields. -->
> +<form id="form-basic">
> +  <input id="form-basic-username" name="username"></input>
> +  <input id="form-basic-password" name="password" type="password"></input>
> +  <input id="form-basic-submit" type="submit"></input>

<input> elements don't get closing tags.

::: toolkit/modules/PopupNotifications.jsm
@@ +644,5 @@
>        notificationsToShow.forEach(function (n) {
>          this._fireCallback(n, NOTIFICATION_EVENT_SHOWN);
>        }, this);
> +      // Global notification used by tests to know when everything is ready.
> +      Services.obs.notifyObservers(null, "popupnotifications-panel-shown", "");

I think it's preferred to use DOM Events when we have a DOM element (e.g. on the panel) since they are tied to windows and avoid accidentally doing some work in the wrong window and wrong PopupNotification in this case. PopupNotifications are also used in social chat windows (used by Loop) to give permissions[1]. I think the existing "popupshown" event should be sufficient here (at least when the popup isn't already open).

[1] https://mxr.mozilla.org/mozilla-central/source/browser/base/content/chatWindow.xul#78
Comment on attachment 8576065 [details] [diff] [review]
The patch

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

Apologies about the delay. I blame interviewing…

(In reply to Matthew N. [:MattN] from comment #2)
> @@ +18,5 @@
> > +  let notificationElement = PopupNotifications.panel.childNodes[0];
> > +
> > +  let promiseLogin = TestUtils.topicObserved("passwordmgr-storage-changed",
> > +                                             (_, data) => data == "addLogin");
> > +  notificationElement.button.doCommand();
> 
> I think using  like triggerMainCommand in [2] is closer to what users do.

The missing word above was EventUtils.synthesizeMouse[AtCenter]

::: toolkit/components/passwordmgr/test/browser/browser_notifications.js
@@ +26,5 @@
> +  Assert.equal(login.username, "username");
> +  Assert.equal(login.password, "password");
> +
> +  // Cleanup.
> +  Services.logins.removeAllLogins();

Have you considered using registerCleanupFunction for this?
Attachment #8576065 - Flags: review?(MattN+bmo) → feedback+
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Depends on: 1143807
Iteration: --- → 39.2 - 23 Mar
(In reply to Matthew N. [:MattN] from comment #2)
> Nit: It would be nice if there was a utility to get a URL to a relative path
> from the directory for a specified domain in BrowserTestUtils.

I'm even thinking of TestUtils, just filed bug 1143807 for that.

> If we do keep the observer notification, the subject should be the panel so
> we can distinguish between different consumers of PopupNotifications. We
> would then use it instead of `PopupNotifications.panel`

We could, anyways this is designed for testing so I wouldn't be concerned about concurrency. If multiple panels show when we expected only one, there is something wrong. We may probably add an assertion though.

> I think using  like triggerMainCommand in [2] is closer to what users do.

I did think about using a synthesized mouse event, but then opted for a more robust approach since the panel itself is not what we're testing here. However, we might consider moving triggerMainCommand to a test-only module like PopupNotificationTest.jsm and use that from here.

> My understanding is that tests are implicitly CC0 now so you could make this
> even more compact by removing this.

I think an explicit license header is still recommended where it doesn't interfere with the test? For cases where people forget, it's assumed CC0.

> I think the existing "popupshown" event should be
> sufficient here (at least when the popup isn't already open).

It's fired before the final callback though, that may be doing things we want to check like initializing fields, as the "showing" callback is fired too early in the process.
Hi Paolo, can you provide a point value.
Flags: needinfo?(paolo.mozmail)
Points: --- → 2
Flags: needinfo?(paolo.mozmail)
(In reply to Matthew N. [:MattN] from comment #3)
> Have you considered using registerCleanupFunction for this?

That's always tricky because it's executed at the end of all test rather than the individual test, which is what we need here. I'm thinking more about a helper function to execute a "function* (browser) {}" on a tab opened on a given URL.
Attached patch Updated patch (obsolete) — Splinter Review
Attachment #8576065 - Attachment is obsolete: true
Attachment #8578174 - Flags: review?(MattN+bmo)
Depends on: 1143852
No longer depends on: 1143852
Depends on: 1143937
Attachment #8578174 - Attachment is obsolete: true
Attachment #8578174 - Flags: review?(MattN+bmo)
Attachment #8578347 - Flags: review?(MattN+bmo)
Attachment #8578347 - Flags: review?(MattN+bmo) → review+
https://hg.mozilla.org/mozilla-central/rev/62f338d8f747
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Depends on: 1148771
No longer blocks: passwords-2015-UX
You need to log in before you can comment on or make changes to this bug.