975763 - convert mochitest test_certificate_overrides.html to xpcshell test test_cert_override_bits_mismatches.js

Status: RESOLVED FIXED

(Core :: Security: PSM, enhancement)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

See bug 975122 comment 7 for what needs to be done. Whenever possible, we should be using xpcshell instead of mochitest to test networking/certificate functionality. Only when we're testing interaction with the DOM, the browser chrome, or other things should we use mochitest.

Comment 1

[Mike Hoye [:mhoye]]

Doing some good-first-bug auditing, and just about nothing in security is really going to be a good first bug.

Comment 2

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

I'm morphing this bug. test_certificate_overrides.html is no longer necessary. The xpcshell test test_cert_overrides.js does everything it does and more.

Comment 3

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: MozReview Request: bug 975763 - move test_certificate_overrides.html to test_cert_override_bits_mismatches.js r?mgoodwin

bug 975763 - remove unnecessary mochitest test_certificate_overrides.html r?mgoodwin

The xpcshell test test_cert_overrides.js does everything this test did and more.

Comment 4

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8667482 [details]
MozReview Request: bug 975763 - move test_certificate_overrides.html to test_cert_override_bits_mismatches.js r?mgoodwin

https://reviewboard.mozilla.org/r/20765/#review18715

Looks good to me.

Comment 5

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8667482 [details]
MozReview Request: bug 975763 - move test_certificate_overrides.html to test_cert_override_bits_mismatches.js r?mgoodwin

bug 975763 - move unnecessary mochitest test_certificate_overrides.html to xpcshell test test_cert_overrides.js r?mgoodwin

The xpcshell test test_cert_overrides.js now does everything this test did and more.

Comment 6

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Thanks for the review. However, I had a second look at the original test and found an additional aspect of nsICertOverrideService that it tested that I thought would be good to include in test_cert_overrides.js. If you could have another look at the update, that would be great.

Comment 7

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8667482 [details]
MozReview Request: bug 975763 - move test_certificate_overrides.html to test_cert_override_bits_mismatches.js r?mgoodwin

bug 975763 - move test_certificate_overrides.html to test_cert_override_bits_mismatches.js r?mgoodwin

test_certificate_overrides.html didn't need to be a mochitest.

Comment 8

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

It turns out adding those tests to test_cert_overrides.js made it take too long on B2G, so I just moved the new testcases to a separate file.

Comment 9

[Mark Goodwin [:mgoodwin]]

https://reviewboard.mozilla.org/r/20765/#review19261

Looks OK to me.

::: security/manager/ssl/tests/unit/test_cert_override_bits_mismatches.js:21
(Diff revision 3)
> +    certOverrideService.clearValidityOverride(host, 8443);

Do we need this? We're clearing the validity override for each iteration of the loop (in line 29).

Comment 10

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

https://reviewboard.mozilla.org/r/20765/#review19261

> Do we need this? We're clearing the validity override for each iteration of the loop (in line 29).

Oh yeah, good point.

Comment 11

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=914da7537a5c

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/86e01d1960c0

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/86e01d1960c0