Closed Bug 1174389 Opened 4 years ago Closed 4 years ago

Add result strings to PSM OCSP xpcshell tests

Categories

(Core :: Security: PSM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: Cykesiopka, Assigned: Cykesiopka)

Details

Attachments

(1 file, 1 obsolete file)

To make debugging easier, result strings should be added to the PSM test_ocsp* xpcshell tests.
Bug 1174389 - Add result strings to PSM OCSP xpcshell tests.
Attachment #8627071 - Flags: review?(dkeeler)
https://reviewboard.mozilla.org/r/12185/#review10647

Apologies ahead of time if some of the strings turn out to be incorrect - I had a harder time writing strings for this batch of tests for some reason.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cf3e4e136fff

::: security/manager/ssl/tests/unit/test_ocsp_caching.js:162
(Diff revision 1)
> -  add_test(function() { do_check_eq(gFetchCount, 1); run_next_test(); });
> +  add_test(function() { 

I'll fix this before check-in.
Comment on attachment 8627071 [details]
MozReview Request: Bug 1174389 - Add result strings to PSM OCSP xpcshell tests.

https://reviewboard.mozilla.org/r/12187/#review10763

Looks great. I just had some suggestions for wording.

::: security/manager/ssl/tests/unit/test_ocsp_caching.js:7
(Diff revision 1)
> +// Checks various aspects of the OCSP cache, mainly to ensure it does not

Maybe something more like "to ensure we do not fetch responses more than necessary".

::: security/manager/ssl/tests/unit/test_ocsp_caching.js:97
(Diff revision 1)
> +          "Cached Unknown case: fetch count should be 0 after getting a" +

Maybe "Should not have attempted to fetch OCSP given a stapled Unknown response" or something.

::: security/manager/ssl/tests/unit/test_ocsp_caching.js:109
(Diff revision 1)
> +          "Cached Unknown case: fetch count should be 1 after not getting a" +

Maybe "no stapled OCSP response, so a fetch should have been attempted"

::: security/manager/ssl/tests/unit/test_ocsp_caching.js:135
(Diff revision 1)
> +          "Responder override case: fetch count should be 2 after not getting" +

Maybe "another fetch should have been attempted" or something.

::: security/manager/ssl/tests/unit/test_ocsp_caching.js:147
(Diff revision 1)
> +          "Responder override case: fetch count should still be 2 after not" +

How about "cached Good response should have prevented attempting to fetch another OCSP response"

::: security/manager/ssl/tests/unit/test_ocsp_caching.js:164
(Diff revision 1)
> +          "Error entry case: fetch count should be 1 after not getting a" +

Maybe "no stapled response -> should have attempted to fetch OCSP"

::: security/manager/ssl/tests/unit/test_ocsp_caching.js:174
(Diff revision 1)
> +          "Error entry case: fetch count should still be 1 after not getting" +

"Noted server failure -> should not have attempted to fetch another response"

::: security/manager/ssl/tests/unit/test_ocsp_caching.js:186
(Diff revision 1)
> +          "Error entry case: fetch count should still be 1 after getting a" +

"Stapled Revoked response -> should not have attempted to fetch another response"

::: security/manager/ssl/tests/unit/test_ocsp_no_hsts_upgrade.js:50
(Diff revision 1)
> +     "Domain for the OCSP AIA URI should be considered a HSTS host");

This is more of a consistency check to ensure we're testing what we think we're testing.

::: security/manager/ssl/tests/unit/test_ocsp_stapling_with_intermediate.js:42
(Diff revision 1)
> +          "Should have 0 OCSP requests for an intermediate that signed a cert" +

This should probably be "no OCSP requests should have been made"

::: security/manager/ssl/tests/unit/test_ocsp_timeout.js:74
(Diff revision 1)
> +         "Hard-fail time difference (with fuzz) should be greater than 10s");

Maybe "automatic OCSP timeout should be about 10s for hard-fail"

::: security/manager/ssl/tests/unit/test_ocsp_timeout.js:77
(Diff revision 1)
> +         "Soft-fail time difference (with fuzz) should be greater than 2s");

(similarly here)

::: security/manager/ssl/tests/unit/test_ocsp_timeout.js:83
(Diff revision 1)
> -    do_check_true(timeDifference < 60000);
> +    ok(timeDifference < 60000, "Time difference should be less than 60s");

Maybe "automatic OCSP timeout shouldn't be more than 60 seconds"
Attachment #8627071 - Flags: review?(dkeeler) → review+
+ Make suggested wording changes, with modifications
Attachment #8627071 - Attachment is obsolete: true
Attachment #8628679 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1918e65eeb04
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.