1027255 - Add ASSERT_/EXPECT_ helper macros for mozilla::pkix::Result

Status: RESOLVED FIXED

(Core :: Security: PSM, enhancement)

Description

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

Attachment: add-ASSERT_Success.patch

This makes tests more readable.

I'm not going to rewrite all the existing tests to use these though. We can do that in a follow-up, if people thing it is worthwhile.

Comment 1

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8442312 [details] [diff] [review]
add-ASSERT_Success.patch

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

::: security/pkix/test/gtest/pkixgtest.cpp
@@ +21,5 @@
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#include "pkixgtest.h"

This spelling looks like some sort of vowel shortage (http://www.theonion.com/articles/wheel-of-fortune-contestants-hit-hard-as-vowel-pri,6159/). Maybe pkix_gtest.h?

@@ +23,5 @@
> + */
> +
> +#include "pkixgtest.h"
> +
> +namespace mozilla { namespace pkix { namespace test {

I sense this is going to be a losing battle, but is there a reason for using namespace mozilla? I mean, isn't everything except third party basically mozilla?

::: security/pkix/test/gtest/pkixgtest.h
@@ +41,5 @@
> +    , mErrorCode(errorCode)
> +  {
> +  }
> +
> +  ResultWithPRErrorCode(Result rv)

explicit

@@ +59,5 @@
> +
> +  friend std::ostream& operator<<(std::ostream& os,
> +                                  const ResultWithPRErrorCode & value);
> +
> +  void operator=(const ResultWithPRErrorCode&) /*delete*/;

This comment seems wrong.

Probably a good idea to disallow copy constructor, too.

@@ +65,5 @@
> +
> +::std::ostream& operator<<(::std::ostream&, const ResultWithPRErrorCode &);
> +
> +#define ASSERT_Success(rv) \
> +  ASSERT_EQ(::mozilla::pkix::test::ResultWithPRErrorCode( \

No need to namespace-prefix in the same namespace, is there? Callers are going to be in ::mozilla::pkix::test anyway.

Comment 2

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

Comment on attachment 8442312 [details] [diff] [review]
add-ASSERT_Success.patch

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

Thanks for the quick review!

https://hg.mozilla.org/integration/mozilla-inbound/rev/4574a8f24e71

::: security/pkix/test/gtest/pkixgtest.cpp
@@ +21,5 @@
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#include "pkixgtest.h"

I didn't make this change because the original name is following the convention that the rest of the filenames are using. Happy to discuss changing the overall naming convention via email if you are still interested though.

@@ +23,5 @@
> + */
> +
> +#include "pkixgtest.h"
> +
> +namespace mozilla { namespace pkix { namespace test {

I don't quite understand what you mean here, but again I am just following the convention we've been using, and I'm happy to discuss changing the convention via email.

@@ +44,5 @@
> +      os << "[Invalid Result: " << static_cast<int64_t>(value.mRv) << ']';
> +      break;
> +  }
> +
> +  if (value.mRv != SECSuccess) {

s/SECSuccess/Success/

::: security/pkix/test/gtest/pkixgtest.h
@@ +41,5 @@
> +    , mErrorCode(errorCode)
> +  {
> +  }
> +
> +  ResultWithPRErrorCode(Result rv)

Thanks! I changed SECStatusWithPRErrorCode's constructor to be explicit too.

@@ +43,5 @@
> +  }
> +
> +  ResultWithPRErrorCode(Result rv)
> +    : mRv(rv)
> +    , mErrorCode(rv == SECSuccess ? 0 : PR_GetError())

s/SECSuccess/Success/

@@ +59,5 @@
> +
> +  friend std::ostream& operator<<(std::ostream& os,
> +                                  const ResultWithPRErrorCode & value);
> +
> +  void operator=(const ResultWithPRErrorCode&) /*delete*/;

The copy constructor is actually used. Not sure why I disabled operator= in the first place. I changed the comment to /*= delete*/ to match the other code.

@@ +65,5 @@
> +
> +::std::ostream& operator<<(::std::ostream&, const ResultWithPRErrorCode &);
> +
> +#define ASSERT_Success(rv) \
> +  ASSERT_EQ(::mozilla::pkix::test::ResultWithPRErrorCode( \

I think not all the tests are in the mozilla::pkix::test namespace. Also, I think it is better to follow the convention that all namespaced names are fully qualified in header files w/o having too many special cases.

Comment 3

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4574a8f24e71

Comment 4

[[:mmc] Monica Chew (no longer reading bugmail)]

> > +namespace mozilla { namespace pkix { namespace test {
> 
> I don't quite understand what you mean here, but again I am just following
> the convention we've been using, and I'm happy to discuss changing the
> convention via email.

This is not that important, but if everything (or almost everything) is namespaced to mozilla, then the top-level namespace doesn't actually disambiguate anything and makes everyone type mozilla:: a bunch of times when they shouldn't have to. pkix::test should be good enough, but I understand that that's not the convention.

Comment 5

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

Uplifted as part of my work to fix bug 1031542, since the test requires these utilities:

https://hg.mozilla.org/releases/mozilla-aurora/rev/4da7fe44c04c
https://hg.mozilla.org/releases/mozilla-beta/rev/06db3c6b9ada