998067 - Add utility code for making it easier to create GTests based on NSS

Status: RESOLVED FIXED

(Core :: Security: PSM, enhancement)

Description

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

Attachment: nssgtest.patch

Currently in GTest we can write:

   ASSERT_EQ(SECSuccess, SomeFunctionThatReturnsSECStatus(...));

However, when the assertion fails, it is a PITA to debug without knowing the value of PR_GetError(). Thus, I created ASSERT_SECSuccess() which, when the assertion fails, automatically prints out the value of PR_GetError() in the error message.

Similarly, we can already write:

   ASSERT_EQ(SECFailure, SomeFunctionThatReturnSECStatus(...));
   ASSERT_EQ(SEC_SOME_ERROR, PR_GetError());

However, it is more convenient to write and read:

   ASSERT_SECFailure(SEC_SOME_ERROR, SomeFunctionThatReturnsSECStatus(...));

Also, arguably this is more likely to result in correct tests because when we expect SECFailure we usually expect a specific error code; with two separate ASSERT_EQ calls, it is easy to forget the second one that tests the result of PR_GetError().

The patch to integrate this into the build system will be a separate patch.

Comment 1

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

Comment on attachment 8408586 [details] [diff] [review]
nssgtest.patch

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

r=me with nits addressed. I assume nssgtest.cpp needs to be added to a moz.build file as well.

::: security/pkix/test/gtest/nssgtest.cpp
@@ +59,5 @@
> +Pred_SECFailure(const char* expectedExpr, const char* actualExpr,
> +                PRErrorCode expectedErrorCode, SECStatus actual)
> +{
> +  if (SECFailure == actual && expectedErrorCode == PR_GetError())
> +    return AssertionSuccess();

nit: braces around conditional body

::: security/pkix/test/gtest/nssgtest.h
@@ +19,5 @@
> +#define mozilla_pkix__nssgtest_h
> +
> +#ifdef _MSC_VER
> +#pragma warning(push)
> +#pragma warning(disable: 4275)

What is warning 4275? A comment would be appreciated.

Comment 2

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

(In reply to David Keeler (:keeler) from comment #1)
> I assume nssgtest.cpp needs to be added to a
> moz.build file as well.

Yes, done.

> nit: braces around conditional body

Done.

> What is warning 4275? A comment would be appreciated.

I removed this #pragma warning stuff. It is only needed when compiling with -W4 which we don't do (yet).

Thanks for the review.

https://hg.mozilla.org/integration/mozilla-inbound/rev/64ce640f432e

Comment 3

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/64ce640f432e