1463584 - Assert.throws for an empty password in browser_notifications_2.js isn't testing the correct error

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect, P1)

Description

[Mark Banner (:standard8)]

In browser_notifications_2.js, there's a bit of code:

      Assert.throws(notificationElement.button.doCommand(),
                    "Can't add a login with a null or empty password.",
                    "Should fail for an empty password");

There's two problems with this:

- Assert.throws expects to be passed a function to be called rather than the result of the called argument.
- The second argument is a string, rather than a regexp.

When I correct the call, I get a missing expected exception. It appears that CustomizableUI is swallowing the doCommand somehow.

This code did get re-arranged in bug 1217134.

I suspect this check needs to be changed to something else, but I'm guessing.

Matt, can you take a look at this please?

Comment 1

[Matthew N. [:MattN]]

We can simply remove that Assert since it's out of place to test it here where the button is disabled and we have thorough tests for empty passwords in https://dxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/test/unit/test_logins_change.js#158-162

Comment 2

[Matthew N. [:MattN]]

Attachment: Bug 1463584 - Remove invalid Assert.throws from test_empty_password.

Review commit: https://reviewboard.mozilla.org/r/245974/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/245974/

Comment 3

[Matthew N. [:MattN]]

Comment on attachment 8979815 [details]
Bug 1463584 - Remove invalid Assert.throws from test_empty_password.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/245974/diff/1-2/

Comment 4

[Matthew N. [:MattN]]

Feel free to autoland for me

Comment 5

[Mark Banner (:standard8)]

Comment on attachment 8979815 [details]
Bug 1463584 - Remove invalid Assert.throws from test_empty_password.

https://reviewboard.mozilla.org/r/245974/#review252032

Great, thank you for investigating.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/eea7f6eebf0f
Remove invalid Assert.throws from test_empty_password. r=standard8

Comment 7

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/eea7f6eebf0f