1440029 - Add a test for the TrustOverrideUtils.h

Status: RESOLVED FIXED

(Core :: Security: PSM, enhancement, P2)

Description

[J.C. Jones [:jcj] (he/they)]

We have a gtest that we tried to land in Bug 1434300 called TrustOverrideTest which breaks the AllocReplacement gtests [1]. This bug is to land that test separately.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1434300#c69

Comment 1

[J.C. Jones [:jcj] (he/they)]

Attachment: Bug 1440029 - Add a test for TrustOverrideUtils.h

This is the test originally from Bug 1434300 that was pulled due to
Bug 1433015.

Review commit: https://reviewboard.mozilla.org/r/222098/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/222098/

Comment 2

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

Comment on attachment 8952867 [details]
Bug 1440029 - Add a test for TrustOverrideUtils.h

https://reviewboard.mozilla.org/r/222098/#review228042

So what's the plan for not breaking gtests again? Just waiting until bug 1433015 is fixed?

::: security/certverifier/tests/gtest/TrustOverrideTest.cpp:111
(Diff revision 1)
> +
> +static mozilla::UniqueCERTCertificate
> +CertFromString(const char* aPem)
> +{
> +  nsCOMPtr<nsIX509Cert> cert = nsNSSCertificate::ConstructFromDER(
> +                                         const_cast<char*>(aPem), strlen(aPem));

style nit: I think it's common to either indent this line only two more than the last or to break the line after the '=' on the previous line (if the rest will fit on one line)

Comment 3

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8952867 [details]
Bug 1440029 - Add a test for TrustOverrideUtils.h

https://reviewboard.mozilla.org/r/222098/#review228042

Yeah, wait until it's fixed. It sounds like Nathan has a plan. I just didn't want to forget this test / code so I put it up. :D

Thanks!

Comment 4

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8952867 [details]
Bug 1440029 - Add a test for TrustOverrideUtils.h

https://reviewboard.mozilla.org/r/222098/#review228042

> style nit: I think it's common to either indent this line only two more than the last or to break the line after the '=' on the previous line (if the rest will fit on one line)

Ah. Thanks :)

Comment 5

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8952867 [details]
Bug 1440029 - Add a test for TrustOverrideUtils.h

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/222098/diff/1-2/

Comment 6

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8952867 [details]
Bug 1440029 - Add a test for TrustOverrideUtils.h

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/222098/diff/2-3/

Comment 7

[J.C. Jones [:jcj] (he/they)]

Try run looks good after a rebase: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ed0f9d4de73880e4a1b802ac95e707a35e32df8&selectedJob=164395062

Since bug 1433015 has landed, marking this checkin-needed.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4d345e3cfeb9
Add a test for TrustOverrideUtils.h r=keeler

Comment 9

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/4d345e3cfeb9