1174942 - Tests for capture doorhanger password editing

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect)

Description

[Bernardo Rittmeyer [:rittme]]

We need to write some automated tests for the capture doorhanger password editing feature.

Comment 1

[Bernardo Rittmeyer [:rittme]]

Attachment: MozReview Request: Bug 1174942 - Tests the password editing field at the password manager capture doorhanger. r=MattN

Bug 1174942 - Tests the password editing field at the password manager capture doorhanger

Comment 2

[Matthew N. [:MattN]]

Comment on attachment 8626824 [details]
MozReview Request: Bug 1174942 - Tests the password editing field at the password manager capture doorhanger. r=MattN

https://reviewboard.mozilla.org/r/12147/#review10659

You should probably do a try push for this in case there are failures on other platforms.

::: toolkit/components/passwordmgr/test/browser/browser_notifications.js:204
(Diff revision 1)
> + * We check for new logins password edits, existing logins password edits and
> + * for empty username password edits.

I think this part of the comment could be written in a clearer way.

::: toolkit/components/passwordmgr/test/browser/browser_notifications.js:275
(Diff revision 1)
> +      let promiseShown = BrowserTestUtils.waitForEvent(PopupNotifications.panel,
> +                                                       "popupshown");

Can you file a bug about the "Shown" event happening before "popupshown" in Toolkit::Notifications…

::: toolkit/components/passwordmgr/test/browser/browser_notifications.js:401
(Diff revision 1)
> +      PopupNotifications.panel.setAttribute("noautohide", "true");

I think this is leftover from debugging and should be removed so it doesn't interfere with tests following this one.

::: toolkit/components/passwordmgr/test/browser/browser_notifications.js:424
(Diff revision 1)
> +      EventUtils.synthesizeMouse(notificationElement, 20, 20, {}, browser.window);

To avoid the magic numbers you could synthesize a click on one of the other elements e.g. the description with synthesizeMouseAtCenter.

Comment 3

[Bernardo Rittmeyer [:rittme]]

https://reviewboard.mozilla.org/r/12147/#review10659

> Can you file a bug about the "Shown" event happening before "popupshown" in Toolkit::Notifications…

Filed it here: https://bugzilla.mozilla.org/show_bug.cgi?id=1178420

Comment 4

[Bernardo Rittmeyer [:rittme]]

Comment on attachment 8626824 [details]
MozReview Request: Bug 1174942 - Tests the password editing field at the password manager capture doorhanger. r=MattN

Bug 1174942 - Tests the password editing field at the password manager capture doorhanger

Comment 5

[Matthew N. [:MattN]]

Comment on attachment 8626824 [details]
MozReview Request: Bug 1174942 - Tests the password editing field at the password manager capture doorhanger. r=MattN

https://reviewboard.mozilla.org/r/12147/#review10737

Ship It!

Comment 6

[Bernardo Rittmeyer [:rittme]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=62cbd6d982b0

Comment 7

[Bernardo Rittmeyer [:rittme]]

Comment on attachment 8626824 [details]
MozReview Request: Bug 1174942 - Tests the password editing field at the password manager capture doorhanger. r=MattN

Bug 1174942 - Tests the password editing field at the password manager capture doorhanger. r=MattN

Comment 8

[Matthew N. [:MattN]]

Comment on attachment 8626824 [details]
MozReview Request: Bug 1174942 - Tests the password editing field at the password manager capture doorhanger. r=MattN

https://reviewboard.mozilla.org/r/12147/#review11313

::: toolkit/components/passwordmgr/test/browser/browser_notifications.js:349
(Diff revisions 2 - 3)
> + * We skip Linux for now because focusing elements on the doorhanger
> + * doesn't work as expected

Please include a bug number to fix the issue.

::: toolkit/components/passwordmgr/test/browser/browser_notifications.js:402
(Diff revisions 2 - 3)
> + * We skip Linux for now because focusing elements on the doorhanger
> + * doesn't work as expected
>   */
> +if(Services.appinfo.OS != "Linux") {

Ditto

::: toolkit/components/passwordmgr/test/browser/browser_notifications.js:352
(Diff revisions 2 - 3)
> +if(Services.appinfo.OS != "Linux") {

Nit: missing space after `if`.

To avoid having to change blame later when this is fixed on Linux, do an early return inside the task functions instead. You may need to put an `ok(true, …)` call if the harness complains.

Comment 9

[Bernardo Rittmeyer [:rittme]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=24f49df7b15a

Comment 10

[Bernardo Rittmeyer [:rittme]]

Comment on attachment 8626824 [details]
MozReview Request: Bug 1174942 - Tests the password editing field at the password manager capture doorhanger. r=MattN

Bug 1174942 - Tests the password editing field at the password manager capture doorhanger. r=MattN

Comment 11

[Bernardo Rittmeyer [:rittme]]

I wasn't satisfied with skipping Linux on this tests, so I spent some more time trying to figure out what was going on. 

Apparently in Linux we can't rely on the `Show` or the `showpopup` events. The test tried to focus the textbox that was not yet available. 

I used `yield ContentTaskUtils.waitForCondition(() => PopupNotifications.isPanelOpen);` to make sure the doorhanger is really open before continuing the test.
 
Local tests on OSX and Linux worked as expected, I'm waiting for the try to confirm it's working everywhere.

Comment 12

[Matthew N. [:MattN]]

Comment on attachment 8626824 [details]
MozReview Request: Bug 1174942 - Tests the password editing field at the password manager capture doorhanger. r=MattN

https://reviewboard.mozilla.org/r/12147/#review11411

If the try results are good then you can post the try link in the bug and add the checkin-needed keyword.

Comment 13

[Bernardo Rittmeyer [:rittme]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc41915b7b0e

Comment 14

[Bernardo Rittmeyer [:rittme]]

Comment on attachment 8626824 [details]
MozReview Request: Bug 1174942 - Tests the password editing field at the password manager capture doorhanger. r=MattN

Bug 1174942 - Tests the password editing field at the password manager capture doorhanger. r=MattN

Comment 15

[Matthew N. [:MattN]]

Comment on attachment 8626824 [details]
MozReview Request: Bug 1174942 - Tests the password editing field at the password manager capture doorhanger. r=MattN

https://reviewboard.mozilla.org/r/12147/#review11571

::: toolkit/components/passwordmgr/test/browser/browser_notifications.js:393
(Diff revisions 4 - 5)
> +      yield ContentTaskUtils.waitForCondition(() => mainActionButton.disabled);
>        Assert.ok(mainActionButton.disabled, "Main action button is disabled");

These 2 lines can be combined into one:
yield ContentTaskUtils.waitForCondition(() => mainActionButton.disabled, "Main action button is disabled");

::: toolkit/components/passwordmgr/test/browser/browser_notifications.js:392
(Diff revisions 4 - 5)
> +      // Wait for main button to get deactivated

Nit: s/deactivated/disabled/

Comment 16

[Matthew N. [:MattN]]

Comment on attachment 8626824 [details]
MozReview Request: Bug 1174942 - Tests the password editing field at the password manager capture doorhanger. r=MattN

https://reviewboard.mozilla.org/r/12147/#review11573

Ship It!

Comment 17

[Bernardo Rittmeyer [:rittme]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9eefe0ece4d1

Comment 18

[Bernardo Rittmeyer [:rittme]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a0dd36c286d

Comment 19

[Bernardo Rittmeyer [:rittme]]

Comment on attachment 8626824 [details]
MozReview Request: Bug 1174942 - Tests the password editing field at the password manager capture doorhanger. r=MattN

Bug 1174942 - Tests the password editing field at the password manager capture doorhanger. r=MattN

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/3e0ef2cb3dc5

Comment 21

[Matthew N. [:MattN]]

Comment on attachment 8626824 [details]
MozReview Request: Bug 1174942 - Tests the password editing field at the password manager capture doorhanger. r=MattN

https://reviewboard.mozilla.org/r/12147/#review12203

::: toolkit/components/passwordmgr/test/browser/browser_notifications.js:449
(Diff revision 6)
> +      yield EventUtils.synthesizeMouseAtCenter(notificationIcon, {}, browser.window);

browser.window? I don't know what that's referring to but I think you can omit that argument since the default value is `window` which I believe is correct in this case.

Comment 22

[Matthew N. [:MattN]]

Comment on attachment 8626824 [details]
MozReview Request: Bug 1174942 - Tests the password editing field at the password manager capture doorhanger. r=MattN

https://reviewboard.mozilla.org/r/12147/#review12209

Ship It!

Comment 23

[Legacy TBPL/Treeherder Robot]

log: https://treeherder.mozilla.org/logviewer.html#?repo=fx-team&job_id=3832158
repository: fx-team
start_time: 2015-07-19T21:15:45
who: mnoorenberghe[at]mozilla[dot]com
machine: tst-linux32-spot-658
buildname: Ubuntu VM 12.04 fx-team debug test mochitest-e10s-browser-chrome-2
revision: 202e9233d130

417 INFO TEST-UNEXPECTED-FAIL | toolkit/components/passwordmgr/test/browser/browser_notifications.js | This test exceeded the timeout threshold. It should be rewritten or split up. If that's not possible, use requestLongerTimeout(N), but only as a last resort. - expected PASS

Comment 24

[Legacy TBPL/Treeherder Robot]

log: https://treeherder.mozilla.org/logviewer.html#?repo=fx-team&job_id=3831650
repository: fx-team
start_time: 2015-07-19T20:32:10
who: mnoorenberghe[at]mozilla[dot]com
machine: t-snow-r4-0066
buildname: Rev4 MacOSX Snow Leopard 10.6 fx-team debug test mochitest-browser-chrome-1
revision: 202e9233d130

574 INFO TEST-UNEXPECTED-FAIL | toolkit/components/passwordmgr/test/browser/browser_notifications.js | Uncaught exception - Main action button is disabled - timed out after 50 tries.
577 INFO TEST-UNEXPECTED-FAIL | toolkit/components/passwordmgr/test/browser/browser_notifications.js | Uncaught exception - Password textbox changed type back to password - timed out after 50 tries.
580 INFO TEST-UNEXPECTED-FAIL | toolkit/components/passwordmgr/test/browser/browser_notifications.js | Found an unexpected tab at the end of test run: https://example.com/browser/toolkit/components/passwordmgr/test/browser/form_basic.html?username=username&password=password - expected PASS
581 INFO TEST-UNEXPECTED-FAIL | toolkit/components/passwordmgr/test/browser/browser_notifications.js | Found an unexpected tab at the end of test run: https://example.com/browser/toolkit/components/passwordmgr/test/browser/form_basic.html - expected PASS

Comment 25

[Legacy TBPL/Treeherder Robot]

log: https://treeherder.mozilla.org/logviewer.html#?repo=fx-team&job_id=3831980
repository: fx-team
start_time: 2015-07-19T21:01:23
who: mnoorenberghe[at]mozilla[dot]com
machine: t-yosemite-r5-0019
buildname: Rev5 MacOSX Yosemite 10.10 fx-team debug test mochitest-browser-chrome-2
revision: 202e9233d130

1207 INFO TEST-UNEXPECTED-FAIL | toolkit/components/passwordmgr/test/browser/browser_notifications.js | Uncaught exception - Main action button is disabled - timed out after 50 tries.
1212 INFO TEST-UNEXPECTED-FAIL | toolkit/components/passwordmgr/test/browser/browser_notifications.js | Found an unexpected tab at the end of test run: https://example.com/browser/toolkit/components/passwordmgr/test/browser/form_basic.html - expected PASS

Comment 26

[Matthew N. [:MattN]]

Bernardo, the test is still intermittently failing, even on your try push. See the three comments above.

Comment 27

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/0a4e761bf352

Comment 28

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/3e0ef2cb3dc5

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0a4e761bf352