1063031 - Remove mozilla::pkix::test::NSSTest

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

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

Attachment: remove-NSSTest.patch

The way tests initialize NSS through the NSSTest base class is error-prone. In particular, it is easy to forget to derive your test class from NSSTest and it is easy to forget to call SetUpTestCase. This patch changes NSS so that it is implicitly initialized when needed.

This effectively removes the last direct dependencies on NSS from the test suite, other than pkixtestcrypto.cpp and pkixbuild_tests.cpp. pkixbuild_tests.cpp will get redone based on future work so it doesn't depend on NSS. Doing that requires changes to mozilla::pkix proper.

Comment 1

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

Comment on attachment 8484356 [details] [diff] [review]
remove-NSSTest.patch

Review of attachment 8484356 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.

::: security/pkix/test/gtest/pkixbuild_tests.cpp
@@ +205,5 @@
>  {
>  public:
>    static void SetUpTestCase()
>    {
> +    // XXX: We have to initialize NSS explicitly for these tests, unlike other

nit: 'XXX' comments to me mean 'TODO: BUG (hasn't been filed yet)' - is this a bug that we're going to fix later or just something we're going to live with and should be aware of? Maybe 'NB:' would be better in this case.

@@ +215,5 @@
>      if (!trustDomain.SetUpCertChainTail()) {
>        abort();
>      }
>    }
> +protected:

nit: looks like a blank line got removed from above this to below it - I prefer how it was previously

::: security/pkix/test/gtest/pkixder_pki_types_tests.cpp
@@ +30,5 @@
>  #include "pkixder.h"
>  
>  using namespace mozilla::pkix;
>  using namespace mozilla::pkix::der;
> +//using namespace mozilla::pkix::test;

nit: remove commented-out line

Comment 2

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5dcb5987e6f0