Extend GoogleTest EXPECT_NE() to allow immediate NULLs
chan...@gmail.com
Message:
Currently GoogleTest allows the following highly convenient syntax:
TEST(Foo, test) {
EXPECT_EQ(NULL, SomeNullReturningFunc());
}
However it does not allow the opposing expectation to be made:
TEST(Foo, test) {
EXPECT_NE(NULL, SomeNonNullReturningFunc());
}
This patch adds the latter to the expectation system in precisely the
same way
as the _EQ variant. That said, this patch is extremely sub-optimal, as
it
completely duplicates the EqHelper as NotEqHelper with the only
substantive
change calling CmpHelperNE instead of CmpHelperEQ. This can be trivially
accomplished by passing a function pointer, but that seems undue
run-time
overhead. I attempted to unify them through an added template parameter
to
provide the CmpHelper* method to be called, but that was non-trivial
without
using TR, C++0x, or Boost related helpers. Perhaps you can find a
cleaner way
Zhanyong?
This is also untested. I can certainly add the tests, but I didn't see a
prime
place for them, as EXPECT_EQ is never tested anywher. Thoughts?
Finally, the EXPECT_EQ() form has special (and IMO nicer) logging
surrounding
the idea of "expected" vs. "actual". I would personally enjoy extending
this to
EXPECT_NE() by using the idea of "unexpected" vs. "actual" terminology.
I have
already used those as more descriptive internal variables, but the
logging
refactoring would shuffle more around than I wanted in this change. I'd
appreciate feedback on whether this is even desirable.
Description:
Currently GoogleTest allows the following highly convenient syntax:
TEST(Foo, test) {
EXPECT_EQ(NULL, SomeNullReturningFunc());
}
However it does not allow the opposing expectation to be made:
TEST(Foo, test) {
EXPECT_NE(NULL, SomeNonNullReturningFunc());
}
This patch adds the latter to the expectation system in precisely the
same way as the _EQ variant. That said, this patch is extremely
sub-optimal, as it completely duplicates the EqHelper as NotEqHelper
with the only substantive change calling CmpHelperNE instead of
CmpHelperEQ. This can be trivially accomplished by passing a function
pointer, but that seems undue run-time overhead. I attempted to unify
them through an added template parameter to provide the CmpHelper*
method to be called, but that was non-trivial without using TR, C++0x,
or Boost related helpers. Perhaps you can find a cleaner way Zhanyong?
This is also untested. I can certainly add the tests, but I didn't see a
prime place for them, as EXPECT_EQ is never tested anywher. Thoughts?
Finally, the EXPECT_EQ() form has special (and IMO nicer) logging
surrounding the idea of "expected" vs. "actual". I would personally
enjoy extending this to EXPECT_NE() by using the idea of "unexpected"
vs. "actual" terminology. I have already used those as more descriptive
internal variables, but the logging refactoring would shuffle more
around than I wanted in this change. I'd appreciate feedback on whether
this is even desirable.
Please review this at http://codereview.appspot.com/7049
Affected files:
M include/gtest/gtest.h
Zhanyong Wan (λx.x x)
The reasons we didn't support EXPECT_NE(NULL, actual) are:
- the implementation is rather involved,
- unlike *_EQ, which has a convention that the first argument is the
expected value, *_NE doesn't have such a convention. My experience is
that most people write *_NE(actual, unexpected), not the other way
around. Hence it may be more useful to support *_NE(actual, NULL).
Or we should allow either argument to be NULL, which will make the
implementation even more involved.
- if *_NE(NULL, actual) fails, we know that actual has to be NULL.
Hence EXPECT_TRUE(actual != NULL) can give us the same amount of
information, making it less important to support *_NE(NULL, actual).
Before we review the patch, we need to decide whether to support this,
and if yes, how.
We can discuss how to improve the implementation once we decide this
is indeed what we want to do.
> This is also untested. I can certainly add the tests, but I didn't see a
> prime
> place for them, as EXPECT_EQ is never tested anywher. Thoughts?
I believe EXPECT_EQ is tested by gtest_output_test, which verifies
that a failed EXPECT_EQ() generates the expected failure message. I
don't remember if we also test it using EXPECT_NONFATAL_FAILURE in
gtest-spi.h. If not, it's a good idea to add some tests to do that
(we chose the golden-file-based approach to test EXPECT_EQ as
EXPECT_NONFATAL_FAILURE wasn't available in the early days).
>
> Finally, the EXPECT_EQ() form has special (and IMO nicer) logging
> surrounding
> the idea of "expected" vs. "actual". I would personally enjoy extending
> this to
> EXPECT_NE() by using the idea of "unexpected" vs. "actual" terminology.
> I have
> already used those as more descriptive internal variables, but the
> logging
> refactoring would shuffle more around than I wanted in this change. I'd
> appreciate feedback on whether this is even desirable.
I agree such a distinction would be helpful. However, our effort to
enforce this order for *_EQ met significant resistance, even though it
was a standard xUnit practice. Given that there's no widely adopted
convention for the order of arguments for *_NE in xUnit-based
frameworks, I suspect that this will meet even larger resistance. In
any case, we shouldn't make the decision without getting the users'
buy-in first.
If we decide to establish a convention, we still need to answer the
question whether the first or the second argument should be the actual
value being validated. My experience is that most people put the
actual value first, contrary to what you suggested, although your
suggestion is consistent with *_EQ.
I think it's probably better to separate the two issues:
1. whether to allow the arguments to be NULL, and
2. whether to make the two arguments asymmetric.
We can postpone #2 to a future date, as I expect it to generate a
lengthy discussion and we may never reach a consensus.
For #1, I believe it does improve the user experience, but I'm not
sure if it's worthy of the added complexity given that *_TRUE(actual
!= NULL) achieves the same effect for free (yes, there's some
cognitive burden for the user to learn that *_NE(NULL, actual) doesn't
work, but I'm not sure how big the burden really is).
Another reason to favor *_TRUE(actual != NULL) is uniformity: it is
desirable that all not-NULL assertions have the same look. Having
both *_TRUE(actual != NULL) and *_NE(NULL, actual) in the tests makes
the style inconsistent.
So, what do you and others think now? :-)
--
Zhanyong
Chandler Carruth
> Before we review the patch, we need to decide whether to support this,
> and if yes, how.
Agreed. I provided the patch because I wrote about 20 tests today for
my OSS project using gtest, and wanted them. The real goal was to
figuer out the best way.
> I believe EXPECT_EQ is tested by gtest_output_test, which verifies
> that a failed EXPECT_EQ() generates the expected failure message. I
> don't remember if we also test it using EXPECT_NONFATAL_FAILURE in
> gtest-spi.h. If not, it's a good idea to add some tests to do that
> (we chose the golden-file-based approach to test EXPECT_EQ as
> EXPECT_NONFATAL_FAILURE wasn't available in the early days).
I'll try and add both if and when its useful.
> I think it's probably better to separate the two issues:
>
> 1. whether to allow the arguments to be NULL, and
> 2. whether to make the two arguments asymmetric.
>
> We can postpone #2 to a future date, as I expect it to generate a
> lengthy discussion and we may never reach a consensus.
I can see the issues with this. I consider it a bikeshed, and one
should be picked and we can move on, but I understand getting the
buy-in necessary may be hard. Personally, I would arbitrarily dictate
that consistency trumps, and so NE follows EQ, both in asymmetry and
the ordering. I've already had to learn that, so there is no real
curve for me.
> For #1, I believe it does improve the user experience, but I'm not
> sure if it's worthy of the added complexity given that *_TRUE(actual
> != NULL) achieves the same effect for free (yes, there's some
> cognitive burden for the user to learn that *_NE(NULL, actual) doesn't
> work, but I'm not sure how big the burden really is).
>
> Another reason to favor *_TRUE(actual != NULL) is uniformity: it is
> desirable that all not-NULL assertions have the same look. Having
> both *_TRUE(actual != NULL) and *_NE(NULL, actual) in the tests makes
> the style inconsistent.
I disagree about the latter one. Currently we have an inconsistent
state because there is a mixture of _NE and _TRUE(... != ...) used.
The former claims to implement the latter with better logging, but
doesn't actually support some very common cases. I think that one or
the other should be used rather than both. Certainly the former is
easier to type and offers interesting options for better logging, so
my preference leans toward getting rid of any of the latter by
supporting their needs. That will yield the most consistent set of
expectations.
This in turns ties into the first question, the cognitive burden isn't
to learn that NULL isn't allowed, but to flip-flop between constructs
throughout the test code, and change typing / testing patterns
depending on what the inputs are even though the logic has remained
the same.
-Chandler
Zhanyong Wan (λx.x x)
> On Sat, Oct 4, 2008 at 5:43 PM, Zhanyong Wan (λx.x x) <w...@google.com> wrote:
>> I think it's probably better to separate the two issues:
>>
>> 1. whether to allow the arguments to be NULL, and
>> 2. whether to make the two arguments asymmetric.
>>
>> We can postpone #2 to a future date, as I expect it to generate a
>> lengthy discussion and we may never reach a consensus.
>
> I can see the issues with this. I consider it a bikeshed, and one
> should be picked and we can move on, but I understand getting the
> buy-in necessary may be hard. Personally, I would arbitrarily dictate
> that consistency trumps, and so NE follows EQ, both in asymmetry and
> the ordering. I've already had to learn that, so there is no real
> curve for me.
It's hard to be really consistent as there are also LE, LT, and etc.
It doesn't make much sense to enforce *_LE(unexpected, actual).
Instead of changing the behavior of existing macros and kinda breaking
existing user code, I like introducing new macros and deprecating
*_NE. In fact the new, better solution already exists - we just need
to finish open-sourcing our mocking framework, and then people can
write:
EXPECT_THAT(actual, Ne(foo))
EXPECT_THAT(actual, Eq(expected))
and many more. The advantage of EXPECT_THAT is that it reads pretty
like English, and it's very clear which is the value we are
validating. Since it's much easier to write new matchers like Ne(...)
than to define new macros, EXPECT_THAT has the potential to become the
one macro to rule them all. :-)
>> Another reason to favor *_TRUE(actual != NULL) is uniformity: it is
>> desirable that all not-NULL assertions have the same look. Having
>> both *_TRUE(actual != NULL) and *_NE(NULL, actual) in the tests makes
>> the style inconsistent.
>
> I disagree about the latter one. Currently we have an inconsistent
> state because there is a mixture of _NE and _TRUE(... != ...) used.
> The former claims to implement the latter with better logging, but
> doesn't actually support some very common cases. I think that one or
> the other should be used rather than both. Certainly the former is
> easier to type and offers interesting options for better logging, so
> my preference leans toward getting rid of any of the latter by
> supporting their needs. That will yield the most consistent set of
> expectations.
What you said makes sense. I'd do it if we were starting from
scratch. My point is that there are already many *_TRUE(actual !=
NULL), and none *_NE(NULL, actual), in existing code, and thus if we
have to pick one style without changing all the existing cases, we
need to pick the former. It's always hard to propose a style if it
immediately makes existing code non-conforming. :-)
--
Zhanyong