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

RESOLVED FIXED in Firefox 31

Status

()

enhancement
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: briansmith, Assigned: briansmith)

Tracking

Trunk
mozilla33
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox31 fixed, firefox32 fixed, firefox33 fixed)

Details

Attachments

(1 attachment)

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