Closed Bug 998067 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: Security: PSM, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: briansmith, Assigned: briansmith)

References

Details

Attachments

(1 file)

Attached patch nssgtest.patchSplinter Review
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.
Attachment #8408586 - Flags: review?(dkeeler)
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.
Attachment #8408586 - Flags: review?(dkeeler) → review+
(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
https://hg.mozilla.org/mozilla-central/rev/64ce640f432e
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: mozilla31 → mozilla32
You need to log in before you can comment on or make changes to this bug.