Closed Bug 1259149 Opened 4 years ago Closed 4 years ago

Add additional tests for the nsIPK11* and nsIPKCS11* implementations

Categories

(Core :: Security: PSM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: Cykesiopka, Assigned: Cykesiopka)

References

Details

Attachments

(1 file)

The nsIPK11* and nsIPKCS11* implementations currently aren't tested very thoroughly in terms of API coverage. We should add more tests to increase coverage so that chances of breakage are lower.

This will also be useful for work in Bug 1251801 and Bug 1252722 to help prevent regressions.
After these additions, the vast majority of the API surface should be covered.

Review commit: https://reviewboard.mozilla.org/r/42063/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42063/
Attachment #8734034 - Flags: review?(dkeeler)
Comment on attachment 8734034 [details]
MozReview Request: Bug 1259149 - Add additional tests for the nsIPK11* and nsIPKCS11* implementations. r=keeler

https://reviewboard.mozilla.org/r/42063/#review38859

Great - LGTM.

::: security/manager/ssl/tests/unit/test_pkcs11_slot.js:40
(Diff revision 1)
> +        "Actual and expected hardware version should match");
> +  equal(testSlot.FWVersion, "0.0",
> +        "Actual and expected firmware version should match");
> +  // Note: testSlot.status is not tested because the implementation calls
> +  //       PK11_IsPresent(), which checks whether the test token is present.
> +  //       The test module inserts and removes the test token in a tight loop,

In another patch (that probably won't get merged since I went with another approach) I explored making this setup a bit easier to work with. I think we could essentially control whether or not the test module pretends the token has been inserted/removed via an environment variable. That way, the tests that aren't interested in this aspect don't have to deal with it. This doesn't have to be dealt with here, though.
Attachment #8734034 - Flags: review?(dkeeler) → review+
https://reviewboard.mozilla.org/r/42063/#review38859

Thanks for the review!

> In another patch (that probably won't get merged since I went with another approach) I explored making this setup a bit easier to work with. I think we could essentially control whether or not the test module pretends the token has been inserted/removed via an environment variable. That way, the tests that aren't interested in this aspect don't have to deal with it. This doesn't have to be dealt with here, though.

Cool, better idea than what I had in mind. I'll file a follow up bug later and look into it when I have time.
Comment on attachment 8734034 [details]
MozReview Request: Bug 1259149 - Add additional tests for the nsIPK11* and nsIPKCS11* implementations. r=keeler

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42063/diff/1-2/
Attachment #8734034 - Attachment description: MozReview Request: Bug 1259149 - Add additional tests for the nsIPK11* and nsIPKCS11* implementations. → MozReview Request: Bug 1259149 - Add additional tests for the nsIPK11* and nsIPKCS11* implementations. r=keeler
https://hg.mozilla.org/mozilla-central/rev/cdcc256acea1
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.