Closed Bug 1063031 Opened 6 years ago Closed 6 years ago

Remove mozilla::pkix::test::NSSTest

Categories

(Core :: Security: PSM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: briansmith, Assigned: briansmith)

References

Details

Attachments

(1 file)

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.
Attachment #8484356 - Flags: review?(dkeeler)
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
Attachment #8484356 - Flags: review?(dkeeler) → review+
https://hg.mozilla.org/mozilla-central/rev/5dcb5987e6f0
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.