Closed
Bug 1243193
Opened 8 years ago
Closed 8 years ago
Use Assert.throws() more in PSM tests
Categories
(Core :: Security: PSM, defect)
Core
Security: PSM
Tracking
()
RESOLVED
FIXED
mozilla47
Tracking | Status | |
---|---|---|
firefox47 | --- | fixed |
People
(Reporter: Cykesiopka, Assigned: Cykesiopka)
Details
Attachments
(1 file, 1 obsolete file)
11.80 KB,
patch
|
Cykesiopka
:
review+
|
Details | Diff | Splinter Review |
This general pattern is occasionally seen in PSM tests: > let exceptionCaught = false; > try { > thisWillThrow(); > ok(false, "Should have thrown an exception"); > } catch (e) { > equal(e.result, Components.results.NS_ERROR_FAILURE) > exceptionCaught = true; > } > ok(exceptionCaught, "Should have thrown an exception"); This works fine, but can be expressed more compactly like this: > throws(() => thisWillThrow(), /NS_ERROR_FAILURE/, > "thisWillThrow() should throw an exception");
Assignee | ||
Comment 1•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/34159/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/34159/
Attachment #8717367 -
Flags: review?(dkeeler)
Comment on attachment 8717367 [details] MozReview Request: Bug 1243193 - Use Assert.throws() more in PSM tests. https://reviewboard.mozilla.org/r/34159/#review30993 Looks great! I'm only concerned about the cases where this uses undefined. ::: security/manager/ssl/tests/unit/test_cert_dbKey.js:56 (Diff revision 1) > - let exceptionCaught = false; > + throws(() => certDB.findCertByDBKey(dbKey), undefined, I feel like we should use something other than undefined here. The implementation throws NS_ERROR_ILLEGAL_INPUT, right? ::: security/manager/ssl/tests/unit/test_pinning_dynamic.js:155 (Diff revision 1) > - ok(false, "Attempting to set an invalid pin should have failed"); > + }, undefined, "Attempting to set an invalid pin should fail"); Same here - not sure undefined is the best. ::: security/manager/ssl/tests/unit/test_pinning_header_parsing.js:31 (Diff revision 1) > - ok(false, "Invalid pin should have been rejected"); > + }, undefined, "Invalid pin should be rejected"); undefined ::: security/manager/ssl/tests/unit/test_pkcs11_safe_mode.js:50 (Diff revision 1) > - try { > + undefined, "addModule should throw an exception when in safe mode"); undefined
Attachment #8717367 -
Flags: review?(dkeeler) → review+
Assignee | ||
Comment 3•8 years ago
|
||
https://reviewboard.mozilla.org/r/34159/#review30993 Thanks for the review! > I feel like we should use something other than undefined here. The implementation throws NS_ERROR_ILLEGAL_INPUT, right? Sure. Fixed here and everywhere else.
Assignee | ||
Comment 4•8 years ago
|
||
+ Check for specific exception everywhere + Make minor tweaks to some expected result messages
Attachment #8717367 -
Attachment is obsolete: true
Attachment #8718219 -
Flags: review+
Assignee | ||
Comment 5•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a03d9ab33bf2
Keywords: checkin-needed
Comment 7•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3c2e9b348b4b
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in
before you can comment on or make changes to this bug.
Description
•