Page MenuHomePhabricator

Add the 'googlemock' component of Google Test to LLVM's unittest libraries.
ClosedPublic

Authored by chandlerc on Dec 29 2016, 4:14 AM.

Details

Summary

I have two immediate motivations for adding this:

  1. It makes writing expectations in tests *dramatically* easier. A quick example that is a taste of what is possible:

    std::vector<int> v = ...; EXPECT_THAT(v, UnorderedElementsAre(1, 2, 3));

    This checks that v contains '1', '2', and '3' in some order. There are a wealth of other helpful matchers like this. They tend to be highly generic and STL-friendly so they will in almost all cases work out of the box even on custom LLVM data structures.

    I actually find the matcher syntax substantially easier to read even for simple assertions:

    EXPECT_THAT(a, Eq(b)); EXPECT_THAT(b, Ne(c));

    Both of these make it clear what is being *tested* and what is being *expected*. With EXPECT_EQ this is implicit (the LHS is expected, the RHS is tested) and often confusing. With EXPECT_NE it is just not clear. Even the failure error messages are superior with the matcher based expectations.
  1. When testing any kind of generic code, you are continually defining dummy types with interfaces and then trying to check that the interfaces are manipulated in a particular way. This is actually what mocks are *good* for -- testing *interface interactions*. With generic code, there is often no "fake" or other object that can be used.

    For a concrete example of where this is currently causing significant pain, look at the pass manager unittests which are riddled with counters incremented when methods are called. All of these could be replaced with mocks. The result would be more effective at testing the code by having tighter constraints. It would be substantially more readable and maintainable when updating the code. And the error messages on failure would have substantially more information as mocks automatically record stack traces and other information *when the API is misused* instead of trying to diagnose it after the fact.

I expect that #1 will be the overwhelming majority of the uses of gmock,
but I think that is sufficient to justify having it. I would actually
like to update the coding standards to encourage the use of matchers
rather than any other form of EXPECT_... macros as they are IMO
a strict superset in terms of functionality and readability.

I think that #2 is relatively rarely useful, but there *are* cases where
it is useful. Historically, I think misuse of actual mocking as
described in #2 has led to resistance towards this framework. I am
actually sympathetic to this -- mocking can easily be overused. However
I think this is not a significant concern in LLVM. First and foremost,
LLVM has very careful and rare exposure of abstract interfaces or
dependency injection, which are the most prone to abuse with mocks. So
there are few opportunities to abuse them. Second, a large fraction of
LLVM's unittests are testing *generic code* where mocks actually make
tremendous sense. And gmock is well suited to building interfaces that
exercise generic libraries. Finally, I still think we should be willing
to have testing utilities in tree even if they should be used rarely. We
can use code review to help guide the usage here.

Thoughts?

Depends on D28154.

Diff Detail

Repository
rL LLVM

Event Timeline

chandlerc updated this revision to Diff 82662.Dec 29 2016, 4:14 AM
chandlerc retitled this revision from to Add the 'googlemock' component of Google Test to LLVM's unittest libraries..
chandlerc updated this object.
chandlerc added a subscriber: llvm-commits.

Adding some folks explicitly that I suspect have opinions here.

chandlerc updated this revision to Diff 82664.Dec 29 2016, 4:22 AM

Update to remove the gmock_main.cc file and add gmock initialization to our
test main.

It makes writing expectations in tests *dramatically* easier. A quick example that is a taste of what is possible:

These matcher are cool, but why are they part of gmock and not gtests?

For a concrete example of where this is currently causing significant pain, look at the pass manager unittests which are riddled with counters incremented when methods are called. All of these could be replaced with mocks.

Looks cool, but can you submit another patch with the PM unittests updated with using gmock so we can have a better idea of what you mean here?

silvas added a subscriber: silvas.Dec 29 2016, 11:38 AM

I think that #2 is relatively rarely useful, but there *are* cases where
it is useful. Historically, I think misuse of actual mocking as
described in #2 has led to resistance towards this framework. I am
actually sympathetic to this -- mocking can easily be overused. However
I think this is not a significant concern in LLVM. First and foremost,
LLVM has very careful and rare exposure of abstract interfaces or
dependency injection, which are the most prone to abuse with mocks. So
there are few opportunities to abuse them. Second, a large fraction of
LLVM's unittests are testing *generic code* where mocks actually make
tremendous sense. And gmock is well suited to building interfaces that
exercise generic libraries. Finally, I still think we should be willing
to have testing utilities in tree even if they should be used rarely. We
can use code review to help guide the usage here.

Thoughts?

You brought this up a couple years ago in the llvmdev thread "[LLVMdev] Any objections to my importing GoogleMock to go with GoogleTest in LLVM?" and was rejected (or at least stalled out) at the time. The context was also the same (wanting to unittest the new PM).

I think that if the discussion of whether integrate gmock was important enough at the time to bring to llvmdev, that discussion probably still needs to be done on llvmdev.

One thing that was missing last time was a compelling patch showing how gmock can really simplify one of the tests. If you come into a fresh discussion with such an example, I think it is going to be a much clearer decision this time.

And FWIW, based on my experience working on the new PM, I do agree that the new PM unit tests are currently harder to understand and debug than they need to be (for reasons that you outlined well in that LLVMdev thread years ago and in the description of this patch). Especially getting more useful error reports out when the tests fail would be fantastic, so I'd really like to see that aspect of gmock! When you post a patch updating some new PM tests to use gmock, definitely include some example error output from when the test fails.

I think that #2 is relatively rarely useful, but there *are* cases where
it is useful. Historically, I think misuse of actual mocking as
described in #2 has led to resistance towards this framework. I am
actually sympathetic to this -- mocking can easily be overused. However
I think this is not a significant concern in LLVM. First and foremost,
LLVM has very careful and rare exposure of abstract interfaces or
dependency injection, which are the most prone to abuse with mocks. So
there are few opportunities to abuse them. Second, a large fraction of
LLVM's unittests are testing *generic code* where mocks actually make
tremendous sense. And gmock is well suited to building interfaces that
exercise generic libraries. Finally, I still think we should be willing
to have testing utilities in tree even if they should be used rarely. We
can use code review to help guide the usage here.

Thoughts?

You brought this up a couple years ago in the llvmdev thread "[LLVMdev] Any objections to my importing GoogleMock to go with GoogleTest in LLVM?" and was rejected (or at least stalled out) at the time. The context was also the same (wanting to unittest the new PM).

Wow, I had completely forgotten about this. I think that makes this the third time I've tried...

I think that if the discussion of whether integrate gmock was important enough at the time to bring to llvmdev, that discussion probably still needs to be done on llvmdev.

Sure, makes total sense. I'll keep the patches CC-ed to llvm-commits but once I have some examples to point to I'll rejuvenate the discussion on llvm-dev.

One thing that was missing last time was a compelling patch showing how gmock can really simplify one of the tests. If you come into a fresh discussion with such an example, I think it is going to be a much clearer decision this time.

Yep. Especially as now the tests are of a complexity to really show things.

chandlerc updated this revision to Diff 83034.Jan 4 2017, 5:08 AM

Rebase and pick up some fixes.

No need to review just yet. I'm just preparing some patches as the basis for
a discussion on llvm-dev.

You brought this up a couple years ago in the llvmdev thread "[LLVMdev] Any objections to my importing GoogleMock to go with GoogleTest in LLVM?" and was rejected (or at least stalled out) at the time. The context was also the same (wanting to unittest the new PM).

Wow, I had completely forgotten about this. I think that makes this the third time I've tried...

I think that if the discussion of whether integrate gmock was important enough at the time to bring to llvmdev, that discussion probably still needs to be done on llvmdev.

Sure, makes total sense. I'll keep the patches CC-ed to llvm-commits but once I have some examples to point to I'll rejuvenate the discussion on llvm-dev.

Thread started here: http://lists.llvm.org/pipermail/llvm-dev/2017-January/108672.html

I tried to give a more thorough discussion of what is proposed and links to example patches.

chandlerc updated this revision to Diff 83513.Jan 6 2017, 10:18 PM

Rebase and ping now that the discussion seems in favor of at least trying this
and seeing how it goes.

chandlerc updated this revision to Diff 83663.Jan 9 2017, 12:10 PM

Rebase and ping. This should be a super simple review now that the llvm-dev
discussion has died down.

dblaikie accepted this revision.Jan 9 2017, 12:39 PM
dblaikie added a reviewer: dblaikie.
This revision is now accepted and ready to land.Jan 9 2017, 12:39 PM
This revision was automatically updated to reflect the committed changes.