-Albert--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
[from the right address]
Personally I'd much rather see more unit tests, which would presumably result in more appropriate uses of GMock. It's the integration tests that are killing us, IMHO.
Gmock is a framework for removing boilerplate out of unittests that are structured around the use of mock objects. It really only works well in this usage. Since we don't follow a purist "mock-based unittest" philosophy, my gut feeling is that Gmock adds more complexity than it saves and can encourage bad testing patterns (eg., characterization test, or partial mocking of objects).
What are people's thoughts? Is my perception wrong? Do find Gmock to be a net benefit for Chromium testing stability and maintenance? If not, should we ban further usage and try to migrate current tests away from it?
-Albert--
-Albert--
+1You've put this in much better terms than I would have. My impression with gmock has been that it's misused in most test. A lot of the places I see it used are calling foo() and ensuring that bar() is called. These tests are not that useful. It seems to me that many pieces of the code that have good interfaces around each component get by with thorough unit tests without a need for gmock. But classes that are heavily intertwined and which pass concrete objects as delegates, instead of interfaces, use gmock instead of adding proper interfaces to separate components.
gmock is also not that readable and adds a maintenance burden. Even if someone who writes a test knows it well, it imposes a cost on everyone else who wants to touch a code that's covered by gmock, since they might have to change a 7 line gmock statement.I'm very supportive of banning gmock from any more tests. If we just say existing usage are fine, then they'll never be converted though. We would need to set a deadline, say a quarter or two, after which these tests would be disabled.
Well, I'd like to study the good uses of gmock to learn more. It sounds like you may have one. Personally, I've only seen bad uses, so my opinion is pretty biased :-/
Hmm, perhaps the problem occurs when folks try to mock concrete classes. It seems like mocking interfaces is a good thing, and gmock simply helps one create mocks. Perhaps this thread just boils down to us needing to be more disciplined about introducing proper interfaces to help separate classes (or groups of classes). Perhaps there are also some gmock anti-patterns to identify.
Hmm.. in that case WebURLLoader is an "input" interface. Perhaps we should look at the differences between mocking output and input interfaces? It seems like in gman's example of wanting to monitor resultant GL calls that what we have is a pure output mock example. Hmm...
Hmm.. in that case WebURLLoader is an "input" interface. Perhaps we should look at the differences between mocking output and input interfaces? It seems like in gman's example of wanting to monitor resultant GL calls that what we have is a pure output mock example. Hmm...
On Jun 27, 2012 7:58 PM, "Andrew Scherkus" <sche...@chromium.org> wrote:My experience with gmock is that they're very awesome tests that make you feel all warm and fuzzy inside (hey look! everything was called correctly!) until the next person comes along and touches the code.I'll offer an example of code I wrote that I absolutely loved at the time when I wrote it but grown to hate:I didn't want to use the "real" implementation of WebURLLoader (AssociatedURLLoader) because doing so required that I set up a WebFrame etc... when all I wanted to do is simulate a few tricky HTTP responses. Seems easy enough task for gmock, right?In reality, this test is an integration test between my code and AssociatedURLLoader (even though there's an interface between the two) and I should have identified some other way to simulate those tricky HTTP responses without using a mocked WebURLLoader. I've been bitten by bugs where AssociatedURLLoader has changed but my mock failed to reflect the new behaviour (not good!).
AndrewOn Wed, Jun 27, 2012 at 7:31 PM, Darin Fisher <da...@chromium.org> wrote:
Hmm, perhaps the problem occurs when folks try to mock concrete classes. It seems like mocking interfaces is a good thing, and gmock simply helps one create mocks.
Perhaps this thread just boils down to us needing to be more disciplined about introducing proper interfaces to help separate classes (or groups of classes). Perhaps there are also some gmock anti-patterns to identify.
On Jun 27, 2012 4:20 PM, "James Robinson" <jam...@google.com> wrote:On Wed, Jun 27, 2012 at 4:04 PM, Darin Fisher <da...@chromium.org> wrote:Well, I'd like to study the good uses of gmock to learn more. It sounds like you may have one. Personally, I've only seen bad uses, so my opinion is pretty biased :-/To use an example from WebKit, I've found gmock to be helpful when writing these tests: http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/tests/WebCompositorInputHandlerImplTest.cpp for this class: http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp. The code is small (few hundred lines of logic) but rather dense and the tests are fairly small.Would folks mind looking over my terrible code and seeing if this is a good or bad use of gmock and, if not, how tests for this should be constructed? I think having something concrete to discuss is more helpful than the abstract. I'd be happy to attempt rewriting this test in some other form if people think it would be an improvement and update the thread with my experience.- JamesI think mocking is a great thing, but the auto-generated mocks and associated macros in gmock are not always so intuitive. Maybe it is just an education thing, but I'd really prefer if our codebase were as intuitive as possible.Maybe we just need good rules for when to use gmock and when not to use it.-DarinOn Wed, Jun 27, 2012 at 3:51 PM, Gregg Tavares (社用) <gm...@google.com> wrote:
So what are we suggesting should be done instead? If you have a use case that exactly matches gmock then why not use it? If people are using it wrong that should be reflected in reviews but outright banning it seems a little extreme....confused...On Wed, Jun 27, 2012 at 3:41 PM, Darin Fisher <da...@chromium.org> wrote:
+1 to banning gmock. Not sure about how the process would go for migrating existing tests. Hmm...On Wed, Jun 27, 2012 at 12:11 PM, John Abd-El-Malek <j...@chromium.org> wrote:
+1You've put this in much better terms than I would have. My impression with gmock has been that it's misused in most test. A lot of the places I see it used are calling foo() and ensuring that bar() is called. These tests are not that useful. It seems to me that many pieces of the code that have good interfaces around each component get by with thorough unit tests without a need for gmock. But classes that are heavily intertwined and which pass concrete objects as delegates, instead of interfaces, use gmock instead of adding proper interfaces to separate components.
gmock is also not that readable and adds a maintenance burden. Even if someone who writes a test knows it well, it imposes a cost on everyone else who wants to touch a code that's covered by gmock, since they might have to change a 7 line gmock statement.I'm very supportive of banning gmock from any more tests. If we just say existing usage are fine, then they'll never be converted though. We would need to set a deadline, say a quarter or two, after which these tests would be disabled.On Wed, Jun 27, 2012 at 11:26 AM, Albert J. Wong (王重傑) <ajw...@chromium.org> wrote:
I added gmock because I saw it being used very effectively inside Google for creating small, surgical, unittests. Unfortunately, I think it hasn't been working nicely within Chromium.My observation has been that many Chromium "unittests" are really integration tests that exercise multiple classes with wide interfaces and complex interactions. As such, it is hard to really mock out the whole interface without adding a bunch of behavior into the mock. Thus, the "mock object" ceases being a mock and becomes a fragile, hard to read, "fake". (definition of terms: http://googletesting.blogspot.com/2008/06/tott-friends-you-can-depend-on.html)Even worse, because it is easy to add behavior into a Gmock object, I've seen people script a mock with a long sequence of calls/results. The test now has moved beyond asserting invariants about API behavior to asserting the specific call sequence the current implementation has. (aka a Characterization Test).Gmock is a framework for removing boilerplate out of unittests that are structured around the use of mock objects. It really only works well in this usage. Since we don't follow a purist "mock-based unittest" philosophy, my gut feeling is that Gmock adds more complexity than it saves and can encourage bad testing patterns (eg., characterization test, or partial mocking of objects).
There are things I don't like about gMock - in particular the need to declare expectations for stuff you don't care about in order to avoid log spam; if I do it restrictively, the tests are brittle; if I do it in a lax way, I need comments to reassure my reviewer that I've got reasons for being lax.
One example I very much like is this: Testing that an observer gets notified at the right times. With gmock, you generate a mock implementation of the observer and set up your expectations. I find that much simpler than implementing a custom mock observer that either (a) needs to be told what it's supposed to expect or (b) records all events, requiring you to look at the state afterwards to determine whether the right events fired.
I agree that gmock-ing concrete classes is a bad idea in many cases as you'll get a weird mix of mocked and non-mocked behavior.
On Thu, Jun 28, 2012 at 5:18 AM, Darin Fisher <da...@google.com> wrote:
Hmm.. in that case WebURLLoader is an "input" interface. Perhaps we should look at the differences between mocking output and input interfaces? It seems like in gman's example of wanting to monitor resultant GL calls that what we have is a pure output mock example. Hmm...
Many interfaces have both input and output parts, what about them?On Jun 27, 2012 7:58 PM, "Andrew Scherkus" <sche...@chromium.org> wrote:My experience with gmock is that they're very awesome tests that make you feel all warm and fuzzy inside (hey look! everything was called correctly!) until the next person comes along and touches the code.I'll offer an example of code I wrote that I absolutely loved at the time when I wrote it but grown to hate:I didn't want to use the "real" implementation of WebURLLoader (AssociatedURLLoader) because doing so required that I set up a WebFrame etc... when all I wanted to do is simulate a few tricky HTTP responses. Seems easy enough task for gmock, right?In reality, this test is an integration test between my code and AssociatedURLLoader (even though there's an interface between the two) and I should have identified some other way to simulate those tricky HTTP responses without using a mocked WebURLLoader. I've been bitten by bugs where AssociatedURLLoader has changed but my mock failed to reflect the new behaviour (not good!).That's true for any mock-based testing. If you change the semantics of the interface that you mock out, you'll have to update the mocks regardless of you whether you use gmock or not.
On Thu, Jun 28, 2012 at 5:08 AM, Mattias Nissler <mnis...@chromium.org> wrote:
On Thu, Jun 28, 2012 at 5:18 AM, Darin Fisher <da...@google.com> wrote:
Hmm.. in that case WebURLLoader is an "input" interface. Perhaps we should look at the differences between mocking output and input interfaces? It seems like in gman's example of wanting to monitor resultant GL calls that what we have is a pure output mock example. Hmm...
Many interfaces have both input and output parts, what about them?On Jun 27, 2012 7:58 PM, "Andrew Scherkus" <sche...@chromium.org> wrote:My experience with gmock is that they're very awesome tests that make you feel all warm and fuzzy inside (hey look! everything was called correctly!) until the next person comes along and touches the code.I'll offer an example of code I wrote that I absolutely loved at the time when I wrote it but grown to hate:I didn't want to use the "real" implementation of WebURLLoader (AssociatedURLLoader) because doing so required that I set up a WebFrame etc... when all I wanted to do is simulate a few tricky HTTP responses. Seems easy enough task for gmock, right?In reality, this test is an integration test between my code and AssociatedURLLoader (even though there's an interface between the two) and I should have identified some other way to simulate those tricky HTTP responses without using a mocked WebURLLoader. I've been bitten by bugs where AssociatedURLLoader has changed but my mock failed to reflect the new behaviour (not good!).That's true for any mock-based testing. If you change the semantics of the interface that you mock out, you'll have to update the mocks regardless of you whether you use gmock or not.I'd say the major drawback to using gmock is there's no natural migration path from building a mock with gmock to building a fake. As a result, people start out using gmock when they are building tests, and as they add more complex tests, they naturally evolve into a situation where they need a fake instead, but at any given point it's always easier to just add a few more call expectations than to stop using gmock and rewrite using a fake. It's like a built-in anti-pattern.I find that when using my own, hand-rolled mock, it's generally easier to migrate this to being a fake. So for that reason, I'd support banning gmock for new tests, even though there's *slightly* more work in cases where you truly only need a mock.
EXPECT_CALL(*mock_pss_, sync_initialized()).WillRepeatedly(Return(false));
EXPECT_CALL(*mock_pss_, sync_initialized()) .WillOnce(Return(false)) .WillRepeatedly(Return(true));
That's not true. You first say:EXPECT_CALL(*mock_pss_, sync_initialized()).WillRepeatedly(Return(false));at the point where your test proceeded to the logged in state, you say:EXPECT_CALL(*mock_pss_, sync_initialized()).WillRepeatedly(Return(true));And that's much better as it allows you to put that statement at exactly the point where you have switched to logged-in state.
Pulling up back up a level, I think it's also uninteresting to debate, in a vacuum, whether or not GMock itself is a learnable/good tool.Google has internally used it for years over a large complex C++ code base in a manner that keeps test maintainable, easy to understand, and fast. In Chromium, though we clearly have some happy users, this has not been the case in a number of situations. This says the issues are not intrinsic to GMock, but in how it interacts with Chromium's coding and testing style.From this thread, the main positives I hear are thata) it gives a consistent, easy, method for creating mocks.b) since it is consistent, it can be easier to read that ad-hoc mock implementationsThe negatives are:a) it has been often misapplied in situations where mocks are inappropriate (eg., integration tests).b) as tests grow, people just add complexity to the mock rather than refactor the API to be testable.c) people who aren't versed in it have to learn a new meta-language.For (a), I think we could probably successfully address this with a set of anti-patterns as Darin suggested. Before creating this set though, I think we should settle issues (b) and (c) because they are harder to solve.For (b), this is more of a question of reviewer and developer discipline with writing tests rather than a GMock issue.Test are effectively a set of assertions (implicit or explicit) about what behavior you want to guarantee in your production code. If your and you reviewer aren't asking "oh, you just added another assumption about behavior into this test...am I okay with that?" we've already got a problem. This is true with or without GMock.There is an argument that GMock makes it easier to add too much behavior, but I'd venture the fundamental issue is not thinking that all code in a test is effectively an assertion about behavior.Creating adding a set of anti-patterns, and having had this uber-thread as negative-reinforcement, probably goes a long way to solving this issue.As for (c), requiring developers to learn the meta-language, this was the kicker that made me think GMock might not be worth its weight. It means that GMock, as a tool, is a burden on users and non-users alike. I think this is actually the core issue to evaluate.Do we want to retain this tax in order to have a consistent mock system inside our "true" unittests?
On Thu, Jun 28, 2012 at 7:38 PM, Fred Akalin <aka...@chromium.org> wrote:
On Thu, Jun 28, 2012 at 10:13 AM, Mattias Nissler <mnis...@google.com> wrote:
That's not true. You first say:EXPECT_CALL(*mock_pss_, sync_initialized()).WillRepeatedly(Return(false));at the point where your test proceeded to the logged in state, you say:EXPECT_CALL(*mock_pss_, sync_initialized()).WillRepeatedly(Return(true));And that's much better as it allows you to put that statement at exactly the point where you have switched to logged-in state.It's not obvious, but you can't do that. From http://code.google.com/p/googlemock/wiki/ForDummies :Important note: Google Mock requires expectations to be set before the mock functions are called, otherwise the behavior is undefined. In particular, you mustn't interleave EXPECT_CALL()s and calls to the mock functions.Aha, I wasn't aware of that advice in the guide. A while back, I actually read the gmock source and figured that it's safe to use this pattern in certain cases. In the general case, I think it's good if you throw in aMock::VerifyAndClearExpectations(mock_pss_);
On Thu, Jun 28, 2012 at 10:56 AM, Mattias Nissler <mnis...@google.com> wrote:
On Thu, Jun 28, 2012 at 7:38 PM, Fred Akalin <aka...@chromium.org> wrote:
On Thu, Jun 28, 2012 at 10:13 AM, Mattias Nissler <mnis...@google.com> wrote:
That's not true. You first say:EXPECT_CALL(*mock_pss_, sync_initialized()).WillRepeatedly(Return(false));at the point where your test proceeded to the logged in state, you say:EXPECT_CALL(*mock_pss_, sync_initialized()).WillRepeatedly(Return(true));And that's much better as it allows you to put that statement at exactly the point where you have switched to logged-in state.It's not obvious, but you can't do that. From http://code.google.com/p/googlemock/wiki/ForDummies :Important note: Google Mock requires expectations to be set before the mock functions are called, otherwise the behavior is undefined. In particular, you mustn't interleave EXPECT_CALL()s and calls to the mock functions.Aha, I wasn't aware of that advice in the guide. A while back, I actually read the gmock source and figured that it's safe to use this pattern in certain cases. In the general case, I think it's good if you throw in aMock::VerifyAndClearExpectations(mock_pss_);And I've done this as well. The problem then being that you have to verify and reset *all* expectations (which is a bummer if you are using common code to setup your expectations for actions taken later in the object's lifecycle, since those expectations won't be satisfied yet). Basically, you can't just verify and reset expectations for a single API function. Again, not a huge issue, but it's clunky, and the *easiest* solution in gmock (just add a single "WillOnce" clause rather than resetting all expectations) is not the *right* one.
Anyhow, I'm fine with whatever we decide to do with gmock. I recall that google3 has its own share of misuses of gmock, so I'm pessimistic that somehow we're going to educate/review our way towards no further misuses in Chromium, but I don't think it's such a critical issue that I feel dogmatic about it.
I think gmock is one of those things which can be very useful but is very easy to abuse.But I have used it in tests, in Chrome and in google3, to great effect. So I would not vote for an outright ban but would support some ways to avoid the ab-use. I totally agree that GMock makes it easy to exercise some corner cases and increase test coverage (e.g. mock url fetcher, set it to respond with 400. I know URL fetcher has good test classes, but this is still much easier to setup).
Several things I have noticed in Chromium code in bad uses of GMock:- Lack of shared mocks for some very common classes
--