Closed
Bug 1296619
Opened 8 years ago
Closed 8 years ago
Add test for PK11PasswordPromptRunnable
Categories
(Core :: Security: PSM, defect, P1)
Core
Security: PSM
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.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•8 years ago
|
||
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]
Reporter | ||
Comment 3•8 years ago
|
||
mozreview-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 ::: 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
Reporter | ||
Comment 4•8 years ago
|
||
mozreview-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/#review71390 Oops. Looks good!
Attachment #8783602 -
Flags: review?(cykesiopka.bmo) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•8 years ago
|
||
mozreview-review-reply |
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
Comment 8•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f96a181ce623
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in
before you can comment on or make changes to this bug.
Description
•