Closed Bug 1243193 Opened 4 years ago Closed 4 years ago

Use Assert.throws() more in PSM tests

Categories

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

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: Cykesiopka, Assigned: Cykesiopka)

Details

Attachments

(1 file, 1 obsolete file)

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");
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+
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.
+ Check for specific exception everywhere
+ Make minor tweaks to some expected result messages
Attachment #8717367 - Attachment is obsolete: true
Attachment #8718219 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/3c2e9b348b4b
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.