GTest & GMock

[Ryan Sleevi]

During some recent reviews, it was pointed out by the submitter that Google Test and Google Mock (GTest and GMock, colloquially), have now merged into a single repository - https://github.com/google/googletest

As part of this merger, people writing tests are now encouraged to use GMock-style matchers, rather than GTest-style macros - see https://github.com/google/googletest/blob/master/googletest/docs/FAQ.md ("We are encouraging people to use the unified EXPECT_THAT(value, matcher) syntax more often in tests")

That is, where previously you might have written

EXPECT_EQ(7, Frob());

EXPECT_TRUE(rv > 7 || rv < 13);

EXPECT_EQ(net::ERR_IO_PENDING, rv);

The new form would be

EXPECT_THAT(Frob(), Eq(7));

EXPECT_THAT(rv, AnyOf(Gt(7), Lt(13)));

EXPECT_THAT(rv, net::IsError(net::ERR_IO_PENDING));

There have been plenty of discussions of GMock throughout the Chromium-years - from proposals to remove it ( https://groups.google.com/a/chromium.org/d/msg/chromium-dev/-KH_IP0rIWQ/HynALJ3rsk0J ) - to proposals to use more of its functionality ( https://groups.google.com/a/chromium.org/d/msg/chromium-dev/1zktlTqFb6o/g5-Llb7LCgAJ ) 

While the above examples are convoluted, they do (hopefully) demonstrate the natural application of this guidance. The above //net case, for example, gives pretty-printing of the string constant for what the value of rv is when it's not net::ERR_IO_PENDING. If it's -12, GTest prints "Actual Value: -12", while a GMock-matcher could give "Actual Value: -12, net::ERR_INSUFFICIENT_RESOURCES", since the GMock-matcher could use net::ErrorToString. There are benefits and downsides to this style, hence the broader, Chromium-wide discussion.

I'm curious where sentiments lie on the use of GMock in general, and in the use of GMock's matchers in particular, and to see how that applies to our coding style and our testing style. That is, do we hold that GoogleTest's upstream guidance is on the same level, of say, Google Coding Style guidance? Do we want that to be reflected in Chromium or diverge from it? As reviewers, should we encourage people to use EXPECT_THAT? Or, as reviewers, should we discourage people from using MATCHER_P?

My own inclination is that MATCHER_P and GMock encourage more anti-patterns than desirable patterns, the documentation is too sparse, the compile-time non-ideal (has to be fully inlined if you follow the docs), and the many ways to solve the problem (MATCHER_P, MatcherInterface, MakePolymorphicMatcher) add to danger, but if the sentiment is that this is the Good New Way, I'm happy to hop on board and stomach the unease.

[Peter Kasting]

On Tue, Sep 13, 2016 at 1:26 PM, Ryan Sleevi <rsl...@chromium.org> wrote:

I'm curious where sentiments lie on the use of GMock in general, and in the use of GMock's matchers in particular, and to see how that applies to our coding style and our testing style. That is, do we hold that GoogleTest's upstream guidance is on the same level, of say, Google Coding Style guidance? Do we want that to be reflected in Chromium or diverge from it? As reviewers, should we encourage people to use EXPECT_THAT? Or, as reviewers, should we discourage people from using MATCHER_P?

Does this guidance from the googletest folks match how Google internally is now guided and is shifting use?  I would say following Google recommended practice is probably more important than following the googletest recommended practice, if the two differ.

The FAQ does not say in detail why there's a recommendation for matchers over EXPECT_XX macros, other than that matchers can be combined (e.g. AnyOf() in your example).  It would be nice to know what other pros and cons they would cite.

I think while there may be knee-jerk reluctance to use GMock because GMock, in this case there aren't many problems; EXPECT_THAT() is a bit more readable-as-English to me (only familiarity with the other macros makes them "readable-at-a-glance"), and the benefit of nicer error messages in certain cases (as you cite) would be good.  I'd be inclined to say either are allowed and not provide explicit guidance ("up to the reviewer").

PK

[Ryan Sleevi]

On Tue, Sep 13, 2016 at 1:35 PM, Peter Kasting <pkas...@chromium.org> wrote:

Does this guidance from the googletest folks match how Google internally is now guided and is shifting use?  I would say following Google recommended practice is probably more important than following the googletest recommended practice, if the two differ.

This is an area where it's not readily clear that there is any Google advice other than the GoogleTest/GTest advice.

 

The FAQ does not say in detail why there's a recommendation for matchers over EXPECT_XX macros, other than that matchers can be combined (e.g. AnyOf() in your example).  It would be nice to know what other pros and cons they would cite.

Agreed. Of course, it'd also be useful if anyone on chromium-dev@ would cite examples too :)

I think while there may be knee-jerk reluctance to use GMock because GMock, in this case there aren't many problems; 

I would disagree when you consider what the implementation of a matcher needs to look like.

If we use the MATCHER_P syntax, my convoluted example of net::IsError becomes

MATCHER_P(IsError, expected, std::string(negation ? "not " : "") + net::ErrorToString(expected)) {

  if (arg <= 0)

    *result_listener << net::ErrorToString(arg);

  return arg == expected;

}

As a reader, I'm not sure if this code is clear - |arg| and |*result_listener| being hidden variables are loosely documented in https://github.com/google/googletest/blob/master/googlemock/docs/CookBook.md#writing-new-matchers-quickly (and slightly more thoroughly in https://github.com/google/googletest/blob/d225acc90bc3a8c420a9bcd1f033033c1ccd7fe0/googlemock/include/gmock/gmock-generated-matchers.h#L1166 ). However, note how for matchers, the code needs to be all inlined.

Of course, using MATCHER_P is actively discouraged, per https://github.com/google/googletest/blob/master/googlemock/docs/CookBook.md#writing-new-parameterized-matchers-quickly , with the advice "While it's tempting to always use the MATCHER* macros when defining a new matcher, you should also consider implementing MatcherInterface or using MakePolymorphicMatcher() instead (see the recipes that follow), especially if you need to use the matcher a lot. While these approaches require more work,"

For the above 'simple' case I gave, if you wanted to handle both int and net::Error results (since we pass net::Errors as ints if they can also be sizes), you'd have to implement a polymorphic matcher to handle the type coercions, including printing the output (GTest/GMock's pretty-printer is in the internal namespace, with strong admonitions not to use)

That's perhaps a longer explanation over the concern. Having looked at GMock matchers several times, I'm not sure if they help readability/maintainability. I'd be especially curious if there were things that GTest made harder than GMock, since I've generally seen "Have to dig in for 30 minutes in GMock docs & headers & look at how Google is using it to make sense of how this code being submitted is using it, and still not be sure if it's the Right Way"

[John Abd-El-Malek]

On Tue, Sep 13, 2016 at 2:09 PM, Ryan Sleevi <rsl...@chromium.org> wrote:

On Tue, Sep 13, 2016 at 1:35 PM, Peter Kasting <pkas...@chromium.org> wrote:

Does this guidance from the googletest folks match how Google internally is now guided and is shifting use?  I would say following Google recommended practice is probably more important than following the googletest recommended practice, if the two differ.

This is an area where it's not readily clear that there is any Google advice other than the GoogleTest/GTest advice.

I think it's not surprising that the gtest repo is encouraging more usage of another repo it merged with.

IMO unless Google style guide changes, we don't need to follow this encouragement.

 

The FAQ does not say in detail why there's a recommendation for matchers over EXPECT_XX macros, other than that matchers can be combined (e.g. AnyOf() in your example).  It would be nice to know what other pros and cons they would cite.

Agreed. Of course, it'd also be useful if anyone on chromium-dev@ would cite examples too :)

I think while there may be knee-jerk reluctance to use GMock because GMock, in this case there aren't many problems; 

I would disagree when you consider what the implementation of a matcher needs to look like.

If we use the MATCHER_P syntax, my convoluted example of net::IsError becomes

MATCHER_P(IsError, expected, std::string(negation ? "not " : "") + net::ErrorToString(expected)) {

  if (arg <= 0)

    *result_listener << net::ErrorToString(arg);

  return arg == expected;

}

As a reader, I'm not sure if this code is clear - |arg| and |*result_listener| being hidden variables are loosely documented in https://github.com/google/googletest/blob/master/googlemock/docs/CookBook.md#writing-new-matchers-quickly (and slightly more thoroughly in https://github.com/google/googletest/blob/d225acc90bc3a8c420a9bcd1f033033c1ccd7fe0/googlemock/include/gmock/gmock-generated-matchers.h#L1166 ). However, note how for matchers, the code needs to be all inlined.

Of course, using MATCHER_P is actively discouraged, per https://github.com/google/googletest/blob/master/googlemock/docs/CookBook.md#writing-new-parameterized-matchers-quickly , with the advice "While it's tempting to always use the MATCHER* macros when defining a new matcher, you should also consider implementing MatcherInterface or using MakePolymorphicMatcher() instead (see the recipes that follow), especially if you need to use the matcher a lot. While these approaches require more work,"

For the above 'simple' case I gave, if you wanted to handle both int and net::Error results (since we pass net::Errors as ints if they can also be sizes), you'd have to implement a polymorphic matcher to handle the type coercions, including printing the output (GTest/GMock's pretty-printer is in the internal namespace, with strong admonitions not to use)

That's perhaps a longer explanation over the concern. Having looked at GMock matchers several times, I'm not sure if they help readability/maintainability. I'd be especially curious if there were things that GTest made harder than GMock, since I've generally seen "Have to dig in for 30 minutes in GMock docs & headers & look at how Google is using it to make sense of how this code being submitted is using it, and still not be sure if it's the Right Way"

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[Matthew]

I was working in google3 up until this past December. Gmock was definitely becoming The Way, at least in the corner of ads I was working in. Here's my experience, if it's helpful to any one. I don't have strong opinions either way.

For me, GMock was most useful in the way that it easily scaled up to testing complicated data: expects on things like protos or flume PObjects looked the same as basic data types, and the various AnyOf, etc, made working with vectors or sets very uniform whether the underlying data type was basic or complex.

The fairly severe disadvantage is if you do something wrong, you get a spew of incomprehensible error messages. Given the standards of documentation, extending any matcher means trying something new and wading through those messages to figure out what trivial type signature error you made. The composability of everything matters here too. I don't have any specific examples off the top of my head, but the type of situation would be like looking at a repeated field in a proto. It's almost like a vector, but not all of the vector matchers work exactly as you might think and you have to experiment/look around for code to copy/paste.

So tests end up being much more readable than writable.

[Rachel Blum]

tl;dr: I like it, but more as a recommendation than a style guide?

I'd be especially curious if there were things that GTest made harder than GMock,

Trivial, but still pet peeve:

EXPECT_LT(a, b);  // OK, what's less that again?

EXPECT_THAT(a, Lt(b));  // Something I can easily parse.

That doesn't justify making this the recommended style Chrome-wide, but I think a stance of "EXPECT_THAT is OK, please be careful around MATCHER_P" would make sense.

Also, I'd expect MATCHER_P to be used much less frequently than EXPECT_THAT. I'd think it's akin to things we currently collect in test_util files. Maybe I underestimate people's desire for pretty code, though :)

 

So tests end up being much more readable than writable.

That's a good thing. We read them way more often than we write them. 

[Nico Weber]

Does anyone have numbers on build time impact? Last I checked, including gmock headers made your file compile even more slowly than with just gtest.h.

--

[Ryan Sleevi]

That seems like a hard to answer question, but your experience matches mine.

Thoughts on how to measure:

1) Convert a single file to use EXPECT_THAT(Foo, Eq(bar)) instead of EXPECT_EQ(Bar, Foo) for its comparisons, and sample that compile time across a variety of platforms

2) Add the GMock header wherever GTest is included (all files) and then sample compile times

3) ... ? Something else?

I am curious about this problem as well, because it seems like it could be compile-time-death-by-a-thousand-cuts if we don't measure.

At the risk of optimizing too much, I am curious whether the 'recommended' pattern of

using ::testing::Eq;

using ::testing::IsNull;

using ::testing::Not;

using ::testing::NotNull;

Will also have any impact in aggregate, and if so, whether EXPECT_THAT(Foo, ::testing::Not(::testing::Eq(7))) will still offer the readability benefits (admittedly, a convoluted and contrived example)