Closed Bug 1296619 Opened 8 years ago Closed 8 years ago

Add test for PK11PasswordPromptRunnable

Categories

(Core :: Security: PSM, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: Cykesiopka, Assigned: keeler)

Details

(Whiteboard: [psm-assigned])

Attachments

(1 file)

PK11PasswordPromptRunnable currently doesn't have any test coverage. It probably should.
Here's a try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1092ef734f51 (the eslint issue is fixed in the revision I have up for review)
Assignee: nobody → dkeeler
Priority: P3 → P1
Whiteboard: [psm-backlog] → [psm-assigned]
Comment on attachment 8783602 [details]
bug 1296619 - add a test to ensure that prompting for the master password probably works

https://reviewboard.mozilla.org/r/73354/#review71388

::: security/manager/ssl/tests/unit/test_password_prompt.js:16
(Diff revision 1)
> +
> +var gMockPrompter = {
> +  passwordToTry: null,
> +  numPrompts: 0,
> +
> +  promptPassword: (dialogTitle, text, password, checkMsg, checkState) => {

The nsIPromptService.idl version of this method uses ```checkState``` as the param name, but the nsIPrompt.idl version uses ```checkValue``` for some reason.

We're mocking out the nsIPrompt version, so maybe s/checkState/checkValue/, but this is very minor, so up to you.

::: security/manager/ssl/tests/unit/test_password_prompt.js:17
(Diff revision 1)
> +    // Note that because of how xpcom wraps things, |this| is a wrapper on a C++
> +    // object that is a wrapper on a JS object, so |this != gMockPrompter|.
> +    // More importantly, properties on |gMockPrompter| aren't necessarily
> +    // exposed on |this| (hence we use |gMockPrompter| instead of |this|).

Hmm, it looks like this is a non-issue if we use shorthand function syntax instead of an arrow function.

::: security/manager/ssl/tests/unit/test_password_prompt.js:58
(Diff revision 1)
> +
> +  // Set an initial password.
> +  let tokenDB = Cc["@mozilla.org/security/pk11tokendb;1"]
> +                  .getService(Ci.nsIPK11TokenDB);
> +  let token = tokenDB.getInternalKeyToken();
> +  token.initPassword("hunter2");

TIL new meme :D
Comment on attachment 8783602 [details]
bug 1296619 - add a test to ensure that prompting for the master password probably works

https://reviewboard.mozilla.org/r/73354/#review71390

Oops. Looks good!
Attachment #8783602 - Flags: review?(cykesiopka.bmo) → review+
Comment on attachment 8783602 [details]
bug 1296619 - add a test to ensure that prompting for the master password probably works

https://reviewboard.mozilla.org/r/73354/#review71388

Thanks!

> The nsIPromptService.idl version of this method uses ```checkState``` as the param name, but the nsIPrompt.idl version uses ```checkValue``` for some reason.
> 
> We're mocking out the nsIPrompt version, so maybe s/checkState/checkValue/, but this is very minor, so up to you.

Sounds good.

> Hmm, it looks like this is a non-issue if we use shorthand function syntax instead of an arrow function.

Interesting - I wonder if this is a result of some difference between the two or if it's a bug...

> TIL new meme :D

:)
Pushed by dkeeler@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f96a181ce623
add a test to ensure that prompting for the master password probably works r=Cykesiopka
https://hg.mozilla.org/mozilla-central/rev/f96a181ce623
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: