Add macros for asserting/expecting equality for ObjC objects. (issue1743056)

[Robert Sesek]

Reviewers: ,

Message:
Requesting review for this patch.  Thanks.

Description:
This adds adds four macros when compiling using GTEST_OS_MAC:
{ASSERT,EXPECT}_NS{NE,EQ}.  These test ObjC objects using |-isEqual:|
and
prints failures using the |-description| selector.  This allows for
better
debugging output with ObjC++ test cases.

This also makes changes to the xcconfig files because linking to the
Foundation
framework on 10.4 while supporting 10.5 and i386/x64 requires
special-casing.

BUG=http://code.google.com/p/googletest/issues/detail?id=303
TEST=Covered by unit tests.

Please review this at http://codereview.appspot.com/1743056/show

Affected files:
 A     include/gtest/gtest-mac.h
 A     src/gtest-mac.mm
 A     test/gtest_mac_test.mm
 M     xcode/Config/General.xcconfig
 M     xcode/gtest.xcodeproj/project.pbxproj

[Mark Mentovai]

http://codereview.appspot.com/1743056/diff/5001/6002
File include/gtest/gtest-mac.h (right):

http://codereview.appspot.com/1743056/diff/5001/6002#newcode71
include/gtest/gtest-mac.h:71:
Extra blank line.

http://codereview.appspot.com/1743056/diff/5001/6003
File src/gtest-mac.mm (right):

http://codereview.appspot.com/1743056/diff/5001/6003#newcode73
src/gtest-mac.mm:73: msg << "Expected: (" << expected_expression << ")
!= (" << actual_expression
I’m surprised that there’s no NeFailure since there’s an EqFailure.
Want to introduce it?

http://codereview.appspot.com/1743056/diff/5001/6001
File test/gtest_mac_test.mm (right):

http://codereview.appspot.com/1743056/diff/5001/6001#newcode1
test/gtest_mac_test.mm:1: // Copyright 2008, Google Inc.
2010. You got it right in the other files.

http://codereview.appspot.com/1743056/diff/5001/6001#newcode45
test/gtest_mac_test.mm:45: EXPECT_NSEQ(@"a", @"a");
@"a" == @"a" too (pointer-wise, as opposed to -isEqual:), so this
isn’t absolutely the best possible test. You can keep this, but you
should also try it in a case where you have NSString* s1, s2; s1 ≠ s2;
and [s1 isEqual:s2] == YES.

http://codereview.appspot.com/1743056/show

[rse...@google.com]

I've addressed all comments. I added ::testing::internal::NeFailure()
but I have not converted all other sites that use the same type of
message. Since it's not a logical part of this CL, I'll do that in a
followup CL.

http://codereview.appspot.com/1743056/diff/5001/6003
File src/gtest-mac.mm (right):

http://codereview.appspot.com/1743056/diff/5001/6003#newcode73
src/gtest-mac.mm:73: msg << "Expected: (" << expected_expression << ")
!= (" << actual_expression

On 2010/08/04 22:04:09, Mark Mentovai wrote:
> I’m surprised that there’s no NeFailure since there’s an EqFailure.
Want to
> introduce it?

I've added this and use it in gtest-mac.mm, but I did not convert all
the other instances of this type of code to use it. I'll do so in a
follow-up CL.

http://codereview.appspot.com/1743056/diff/5001/6001
File test/gtest_mac_test.mm (right):

http://codereview.appspot.com/1743056/diff/5001/6001#newcode45
test/gtest_mac_test.mm:45: EXPECT_NSEQ(@"a", @"a");

On 2010/08/04 22:04:09, Mark Mentovai wrote:
> @"a" == @"a" too (pointer-wise, as opposed to -isEqual:), so this
isn’t
> absolutely the best possible test. You can keep this, but you should
also try it

> in a case where you have NSString* s1, s2; s1 ≠&nbsp;s2; and [s1
isEqual:s2] == YES.

I've added another set of expectations to this test that takes care of
that. Nice use of Unicode ;).

http://codereview.appspot.com/1743056/show

[ma...@chromium.org]

LGTM!

You should also get buy-in from a gtester. I can’t check this in.


http://codereview.appspot.com/1743056/diff/20001/21004
File include/gtest/internal/gtest-internal.h (right):

http://codereview.appspot.com/1743056/diff/20001/21004#newcode301
include/gtest/internal/gtest-internal.h:301: // where foo is 5 and bar
is 5, we have:
we? Know your reviewer!

http://codereview.appspot.com/1743056/diff/20001/21004#newcode303
include/gtest/internal/gtest-internal.h:303: // Expected: (foo) !=
(bar), actual: 5 vs 5
I wonder if it ever makes sense to print the actual values twice. The
existing stuff does, I think, right?

http://codereview.appspot.com/1743056/diff/20001/21006
File src/gtest-mac.mm (right):

http://codereview.appspot.com/1743056/diff/20001/21006#newcode52
src/gtest-mac.mm:52: if ([expected isEqual:actual]) {
Not enough indent in this function.

http://codereview.appspot.com/1743056/show

[Zhanyong Wan (λx.x x)]

Hi Robert and Mark,

We try hard to avoid adding new macros, as they don't obey namespaces
and can easily clash.

I think this is best done as gmock matchers. The syntax would be

EXPECT_THAT(x, NsEq(y));

Please see http://code.google.com/p/googlemock/wiki/CookBook#Using_Matchers_in_Google_Test_Assertions,
http://code.google.com/p/googlemock/wiki/CookBook#Extending_Google_Mock,
and http://code.google.com/p/googlemock/wiki/CookBook#Teaching_Google_Mock_How_to_Print_Your_Values
for more info.

Also, I'd avoid adding ObjC++ code to gtest, as that adds much
complexity to the project but is only useful to a very small
percentage of gtest's users. Since the proposed functionality can be
implemented entirely in the user space, there's no need to modify
gtest itself. I'd suggest that you keep it in your own project.
Thanks,

--
Zhanyong

[Mark Mentovai]

Zhanyong Wan wrote:
> We try hard to avoid adding new macros, as they don't obey namespaces
> and can easily clash.
>
> I think this is best done as gmock matchers.  The syntax would be
>
>   EXPECT_THAT(x, NsEq(y));
>

> Please seehttp://code.google.com/p/googlemock/wiki/CookBook#Using_Matchers_in_G...,http://code.google.com/p/googlemock/wiki/CookBook#Extending_Google_Mock,
> andhttp://code.google.com/p/googlemock/wiki/CookBook#Teaching_Google_Moc...
> for more info.

I can’t say that I agree. I see this as equivalent to the existing
macros such as the EXPECT_STREQ family. When working in Mac code, this
is a fairly fundamental comparison to need to make. I’ll note that
gtest already provides the very Windows-specific EXPECT_HRESULT_*
family, for a very similar case: a fairly trivial comparison where the
test’s output can be made substantially more useful with better error
reporting provided by HRESULTFailureHelper.

Bringing gmock into a project just to access its matchers seems to add
its own level of complexity.

> Also, I'd avoid adding ObjC++ code to gtest, as that adds much
> complexity to the project but is only useful to a very small
> percentage of gtest's users.

The proposed patch has zero impact on non-Objective-C, non-Mac users
of gtest: the implementation is in a completely separate file, and you
need to include a completely separate header to access the new macros.
The macro namespace won’t be “polluted” for anyone that doesn’t
require this functionality. I certainly can’t see how the change would
be characterized as “much complexity.” Since NSObject equality is so
fundamental, I see this change as making gtest much more usable for a
fraction (however small you may assert it is) of the userbase without
having any impact on other users. Since the gtest home page lists Mac
second among the “variety of platforms” it supports, I’m a little
surprised that you’d pessimize the patch on the basis of Mac developer
“share,” though.

I hope you’ll reconsider your position. This certainly “feels” like
something that belongs more in gtest than as a local addition to our
project, Chromium, and other projects that might wish to use gtest
with Mac code. If you need an owner/maintainer for Mac bits in gtest,
I’d certainly be happy to step up, and I’m sure Robert would too.

Mark

[Zhanyong Wan (λx.x x)]

On Thu, Aug 5, 2010 at 2:04 PM, Mark Mentovai <ma...@chromium.org> wrote:
> Zhanyong Wan wrote:
>> We try hard to avoid adding new macros, as they don't obey namespaces
>> and can easily clash.
>>
>> I think this is best done as gmock matchers.  The syntax would be
>>
>>   EXPECT_THAT(x, NsEq(y));
>>
>> Please seehttp://code.google.com/p/googlemock/wiki/CookBook#Using_Matchers_in_G...,http://code.google.com/p/googlemock/wiki/CookBook#Extending_Google_Mock,
>> andhttp://code.google.com/p/googlemock/wiki/CookBook#Teaching_Google_Moc...
>> for more info.
>
> I can’t say that I agree. I see this as equivalent to the existing
> macros such as the EXPECT_STREQ family. When working in Mac code, this
> is a fairly fundamental comparison to need to make. I’ll note that
> gtest already provides the very Windows-specific EXPECT_HRESULT_*
> family, for a very similar case: a fairly trivial comparison where the
> test’s output can be made substantially more useful with better error
> reporting provided by HRESULTFailureHelper.

EXPECT_STREQ and EXPECT_HRESULT_* are there fore historical reasons.
If the matcher mechanism were available then, they would've been
implemented as matchers.

So, they are grand-fathered and we don't want to add new macros when
matchers will do.

Note that matchers give you much more power than macros, as you can
compose the former, e.g.

// x should be in a_container.
EXPECT_THAT(a_container, Contains(NsEq(x)));
// x should be equal to either y or z.
EXPECT_THAT(x, AnyOf(NsEq(y), NsEq(z)));

> Bringing gmock into a project just to access its matchers seems to add
> its own level of complexity.

Chromium already uses gmock. Also, we have plan to move matchers from
gmock to gtest.

If you still don't want to use matchers, you can define a predicate
s.t. you can write:

EXPECT_TRUE(NsEq(x, y));

which can print the value of x and y on failure.

See http://code.google.com/p/googletest/wiki/GoogleTestAdvancedGuide#Using_a_Function_That_Returns_an_AssertionResult

>> Also, I'd avoid adding ObjC++ code to gtest, as that adds much
>> complexity to the project but is only useful to a very small
>> percentage of gtest's users.
>
> The proposed patch has zero impact on non-Objective-C, non-Mac users
> of gtest: the implementation is in a completely separate file, and you
> need to include a completely separate header to access the new macros.
> The macro namespace won’t be “polluted” for anyone that doesn’t
> require this functionality. I certainly can’t see how the change would
> be characterized as “much complexity.”

The complexity is in the project set-up, not the code itself.

If we host it, we are responsible for testing, packaging, and
maintaining it. We'll need to add the test to the CMake script s.t.
it can be run as part of our continuous build. And the CMake script
needs to be smart enough to not try to build or run the test on
non-Mac platforms. Since gtest never contained .mm files before, we
may need to add a bunch of supporting routines to allow .mm tests to
work with our build system. (The xcode project is just provided for
the user's convenience and not really used for testing gtest itself.)
And we need to document how to set this up. Not insurmountable, but
things add up, and who knows what surprises await us. Given that
gtest and gmock are not staffed (we work on it as volunteers), we have
to focus on issues with higher benefit-cost ratios.

> Since NSObject equality is so
> fundamental, I see this change as making gtest much more usable for a
> fraction (however small you may assert it is) of the userbase without
> having any impact on other users. Since the gtest home page lists Mac
> second among the “variety of platforms” it supports, I’m a little
> surprised that you’d pessimize the patch on the basis of Mac developer
> “share,” though.

We build gtest as a testing framework for C++ code. It's nice to see
gtest used for ObjC++. It came to me as a surprise. :-)

> I hope you’ll reconsider your position. This certainly “feels” like
> something that belongs more in gtest than as a local addition to our
> project, Chromium, and other projects that might wish to use gtest
> with Mac code. If you need an owner/maintainer for Mac bits in gtest,
> I’d certainly be happy to step up, and I’m sure Robert would too.

I agree that the proposed functionality is fundamental for ObjC++
programmers that use gtest for testing (although I still have no idea
how many there are). I also appreciate your offer to step up.

How does it sound if we post your code on the gtest/gmock wiki as a
recipe? When there are enough people wanting it, we can discuss how
to package it better, but until then I'd like to avoid that work.

Thanks,
--
Zhanyong

[Zhanyong Wan (λx.x x)]

FYI

You can define the matcher as something like this (I don't use ObjC++
so the code likely contains syntactic/typing errors):

#include <gmock/gmock.h>
#include <iostream>

inline void PrintTo(id<NSObject> x, ::std::ostream* os) {
*os << [[x description] UTF8String];
}

MATCHER_P(NsEq, rhs,
(negation ? "doesn't equal " : "equals ") + [[rhs description]
UTF8String]) {
return [arg isEqual: rhs];
}

This is simpler than the EXPECT_NSEQ macro implementation.

--
Zhanyong