Pete Cooper via llvm-dev
2017-Jan-04 19:55 UTC
[llvm-dev] RFC: Reconsidering adding gmock to LLVM's unittest utilities
> On Jan 4, 2017, at 9:49 AM, Zachary Turner via llvm-dev <llvm-dev at lists.llvm.org> wrote: > > TL;DR - I want this.For the most part, +1 from me too. A few comments though.> > On Wed, Jan 4, 2017 at 6:11 AM Chandler Carruth via llvm-dev <llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>> wrote: > > ## Matchers > > To start off, it is important to understand that there are two components to what gmock offers. The first has very little to do with "mocks". It is actually a matcher language and system for writing test predicates: > > EXPECT_EQ(expected, actual); > EXPECT_NE(something, something); > > Become instead: > > EXPECT_THAT(actual, Eq(expected)); > EXPECT_THAT(actual, Ne(not-expected));For the cases where you have containers and other non-trivial objects, I completely agree that this is compelling. However, for simple cases like string equality I don't like the change from EXPECT_EQ(a, b) to EXPECT_THAT(a, Eq(b)). Which brings me to what I guess is my main question. Are we going to be able to keep using EXPECT_EQ (and others) via gtest? Or are we going to slowly migrate from gtest to gmock? I don't think you are suggesting phasing out gtest, but at the same time I'm not really sure why we should have both. It may be easier to move completely to gmock if its more powerful, even if the checks are sometimes more verbose for simple cases. Anyway for at least the subset of cases which need the more powerful forms of testing, this seems like a reasonable thing to add. Cheers, Pete> > This pattern moves the *matcher* out of the *macro*, giving it a proper C++ API. With that, we get two huge benefits: extensibility and composability. You can easily write a matcher that summarizes concisely the expectation for custom data types. And you can compose these matchers in powerful ways. I'll give one example here: > > EXPECT_THAT(MyDenseMap, UnorderedElementsAre(Eq(key1, value1), Eq(key2, value2), Eq(key3, value3))); > > Here I'm composing equality matchers inside a matcher that can handle *unordered* container element-wise comparison for generic, arbitrary containers. With a small patch, I've even extended it to support arbitrary iterator ranges! Combine this with custom matchers for the elements, and it becomes a very expressive an declarative way to write expectations in tests. > > I wanted to give a realistic and compelling example so I rewrote an entire test: https://reviews.llvm.org/D28290 <https://reviews.llvm.org/D28290> Note that I moved *every* EXPECT to the new syntax so this is essentially worst-case. It also involves a non-trivial custom matcher. Despite this, the code is shorter, easier to read and easier to maintain. It has fewer unnecessary orderings enforced. And it is much easier to extend. Also, the error messages when it fails are substantially improved because these composed matchers have logic to carefully explain *why* they failed to match. > > I hope folks find this compelling. I think this alone is worth carrying the gmock code in tree -- it is just used by tests and not substantially larger than gtest. Even if we decide we want nothing to do with mocks, I would very much like to have the matchers. > > +1, these look amazing. Often times I find myself writing many EXPECT statements to test a single logical condition. When you want to do this for many different inputs / outputs of an API it turns into a long list of expect statements that the person reading the test can't easily grok and see how they're related. Here's an example from the formatv tests that I wrote: > > Replacements = formatv_object_base::parseFormatString("{0,-3}"); > ASSERT_EQ(1u, Replacements.size()); > EXPECT_EQ(ReplacementType::Format, Replacements[0].Type); > EXPECT_EQ(0u, Replacements[0].Index); > EXPECT_EQ(3u, Replacements[0].Align); > EXPECT_EQ(AlignStyle::Left, Replacements[0].Where); > EXPECT_EQ("", Replacements[0].Options); > > It would be nice if I could write: > > EXPECT_THAT(Replacements, ReplacementsAre(Rep(Format, 0, 3, Left, ""))); > > This isn't a huge win here, but if you have a longer format string where there's multiple replacements, you end up 5 lines per replacement, which starts to become very unwieldy and hard to follow. Now multiply that by the number of different edge cases you want to test, and you end up losing test coverage because you have to balance maintainability of the test's code with test coverage, and adding 100 lines to test one API hurts readability more than it helps test coverage. > > Another thing. Often times I find myself writing a function to test a complex condition, like this: > > EXPECT_TRUE(Value, ComplexTest(Value)); > > But then you lose the error message ability to see why the complex test failed. You say this is handled by the matcher infrastructure although I don't see an example, but I'll take your word for it. If so, these matchers seem like an across the board win and I hope to be able to use them in-tree soon. > > > > > > ## Mocks > > So, now let's consider mocks. First off, what are mocks? I'll give a fairly casual definition here: they are test objects which implement some API and allow the test to explicitly set expectations on how that API is used and how it in turn should behave. For a more detailed vocabulary see [1] and for a more lengthy discussion see [2]. > > As came up in the original discussion, LLVM relatively infrequently has a need to test API interactions in this way. Usually we're in the business of translating things from format A to B (instructions, metadata, whatever) and can write down one format and write checks against the other format for tests. This is a wonderful world to live in with tests. I never want LLVM to *decrease* how much we leverage this. > You're forgetting about that troublesome LLVM subproject that nobody wants to think about which does things completely differently: LLDB. ;-) LLDB *very frequently* has a need to test API interactions in this way, and is *very infrequently* in the business of translating things from format A to format B. > > > Also, I remain very sympathetic to the idea that this kind of testing apparatus should be relatively rarely needed. We shouldn't be writing new complex unit tests for APIs every week. But even a few use cases such as to test ADTs and generic tools like the pass manager seem to justify the cost to me, and I'm happy to help draw up fairly restrictive guidance around mocks for the coding standards. > > In LLDB, I think this will end up being the most useful kind of unit test. There is so little test coverage right now precisely because certain things in an interactive application are hard/impossible to test with a garbage-in garbage-out model. > > Consider me on board. > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20170104/45001030/attachment.html>
Chandler Carruth via llvm-dev
2017-Jan-04 22:25 UTC
[llvm-dev] RFC: Reconsidering adding gmock to LLVM's unittest utilities
On Wed, Jan 4, 2017 at 11:55 AM Pete Cooper via llvm-dev < llvm-dev at lists.llvm.org> wrote:> EXPECT_THAT(actual, Eq(expected)); > EXPECT_THAT(actual, Ne(not-expected)); > > For the cases where you have containers and other non-trivial objects, I > completely agree that this is compelling. However, for simple cases like > string equality I don't like the change from EXPECT_EQ(a, b) to > EXPECT_THAT(a, Eq(b)). >I'd like to understand -- why do you not like it? On one hand, I dislike it because it is longer to type and read. On the other hand, I like it because it is more consistent and explicit what is being *tested* and what the *expectation* is.> > Which brings me to what I guess is my main question. Are we going to be > able to keep using EXPECT_EQ (and others) via gtest? Or are we going to > slowly migrate from gtest to gmock? >Note that gmock is a subset of gtest, relies on it, and can't work without it. And all of the TEST(...) stuff is strictly gtest. It is only the EXPECT_* and ASSERT_* bits that gmock provides an alternative for.> I don't think you are suggesting phasing out gtest, but at the same time > I'm not really sure why we should have both. It may be easier to move > completely to gmock if its more powerful, even if the checks are sometimes > more verbose for simple cases. >This is essentially where I am at. If we didn't need the more expressive expectations in some cases, I would totally stick with EXPECT_EQ and friends for consistency and simplicity. But if the use cases for more rich and powerful matchers in test EXPECT lines is compelling, I have a mild preference for not having two ways of writing EXPECT_* and ASSERT_* lines, even though the simple cases are a bit more typing. The consistency and predictability for me win out, but not by much. =] I don't think its a huge deal either way. I'd also be totally willing to have LLVM-specific aliases of: EXPECT(...) -> EXPECT_THAT(...) ASSERT(...) -> ASSERT_THAT(...) Because these are only inside of tests, and LLVM's projects are reasonably self contained, if saving the 5 characters helps folks, I'm OK with it. Anyways, this is something of a detail to sort out if we want the matcher functionality. -Chandler -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20170104/86858244/attachment.html>
Zachary Turner via llvm-dev
2017-Jan-04 22:35 UTC
[llvm-dev] RFC: Reconsidering adding gmock to LLVM's unittest utilities
Alongside this, can we come up with a place in the codebase to put shared matchers? I almost tried to solve this previously when i wanted to unit test some functions returning llvm::Error and/or llvm::Expected objects. Because if a function returns an error, you have to consume it before you continue or the unit test asserts. So this is an example of where you might to have a shared matcher that anyone can use. It seems like there could be other examples of wanting shared matchers as well, so perhaps it's worth adding a folder somewhere under llvm/Unittests that we can provide some support matchers for commonly used things. On Wed, Jan 4, 2017 at 2:25 PM Chandler Carruth <chandlerc at google.com> wrote:> On Wed, Jan 4, 2017 at 11:55 AM Pete Cooper via llvm-dev < > llvm-dev at lists.llvm.org> wrote: > > EXPECT_THAT(actual, Eq(expected)); > EXPECT_THAT(actual, Ne(not-expected)); > > For the cases where you have containers and other non-trivial objects, I > completely agree that this is compelling. However, for simple cases like > string equality I don't like the change from EXPECT_EQ(a, b) to > EXPECT_THAT(a, Eq(b)). > > > I'd like to understand -- why do you not like it? > > On one hand, I dislike it because it is longer to type and read. > On the other hand, I like it because it is more consistent and explicit > what is being *tested* and what the *expectation* is. > > > > Which brings me to what I guess is my main question. Are we going to be > able to keep using EXPECT_EQ (and others) via gtest? Or are we going to > slowly migrate from gtest to gmock? > > > Note that gmock is a subset of gtest, relies on it, and can't work without > it. And all of the TEST(...) stuff is strictly gtest. > > It is only the EXPECT_* and ASSERT_* bits that gmock provides an > alternative for. > > > I don't think you are suggesting phasing out gtest, but at the same time > I'm not really sure why we should have both. It may be easier to move > completely to gmock if its more powerful, even if the checks are sometimes > more verbose for simple cases. > > > This is essentially where I am at. > > If we didn't need the more expressive expectations in some cases, I would > totally stick with EXPECT_EQ and friends for consistency and simplicity. > But if the use cases for more rich and powerful matchers in test EXPECT > lines is compelling, I have a mild preference for not having two ways of > writing EXPECT_* and ASSERT_* lines, even though the simple cases are a bit > more typing. The consistency and predictability for me win out, but not by > much. =] I don't think its a huge deal either way. > > I'd also be totally willing to have LLVM-specific aliases of: > > EXPECT(...) -> EXPECT_THAT(...) > ASSERT(...) -> ASSERT_THAT(...) > > Because these are only inside of tests, and LLVM's projects are reasonably > self contained, if saving the 5 characters helps folks, I'm OK with it. > > Anyways, this is something of a detail to sort out if we want the matcher > functionality. > > -Chandler >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20170104/f3bda32d/attachment.html>
Justin Bogner via llvm-dev
2017-Jan-04 22:43 UTC
[llvm-dev] RFC: Reconsidering adding gmock to LLVM's unittest utilities
Chandler Carruth via llvm-dev <llvm-dev at lists.llvm.org> writes:> On Wed, Jan 4, 2017 at 11:55 AM Pete Cooper via llvm-dev < > llvm-dev at lists.llvm.org> wrote: > >> EXPECT_THAT(actual, Eq(expected)); >> EXPECT_THAT(actual, Ne(not-expected)); >> >> For the cases where you have containers and other non-trivial objects, I >> completely agree that this is compelling. However, for simple cases like >> string equality I don't like the change from EXPECT_EQ(a, b) to >> EXPECT_THAT(a, Eq(b)). >> > > I'd like to understand -- why do you not like it? > > On one hand, I dislike it because it is longer to type and read. > On the other hand, I like it because it is more consistent and explicit > what is being *tested* and what the *expectation* is.Personally I actually kind of like how the gmock matchers read - "expect that actual equals expected" rather than "expect equal thing other-thing".
Pete Cooper via llvm-dev
2017-Jan-04 23:03 UTC
[llvm-dev] RFC: Reconsidering adding gmock to LLVM's unittest utilities
> On Jan 4, 2017, at 2:25 PM, Chandler Carruth <chandlerc at google.com> wrote: > > On Wed, Jan 4, 2017 at 11:55 AM Pete Cooper via llvm-dev <llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>> wrote: >> EXPECT_THAT(actual, Eq(expected)); >> EXPECT_THAT(actual, Ne(not-expected)); > > For the cases where you have containers and other non-trivial objects, I completely agree that this is compelling. However, for simple cases like string equality I don't like the change from EXPECT_EQ(a, b) to EXPECT_THAT(a, Eq(b)). > > I'd like to understand -- why do you not like it? > > On one hand, I dislike it because it is longer to type and read.Thats all it is TBH. Seems like we should just do "#define EXPECT_EQ(a, b) EXPECT_THAT(a, Eq(b))" or something similar. Or perhaps it just doesn't matter that much.> On the other hand, I like it because it is more consistent and explicit what is being *tested* and what the *expectation* is. > > > Which brings me to what I guess is my main question. Are we going to be able to keep using EXPECT_EQ (and others) via gtest? Or are we going to slowly migrate from gtest to gmock? > > Note that gmock is a subset of gtest, relies on it, and can't work without it. And all of the TEST(...) stuff is strictly gtest. > > It is only the EXPECT_* and ASSERT_* bits that gmock provides an alternative for.Ah ok, thanks for clarifying.> > > I don't think you are suggesting phasing out gtest, but at the same time I'm not really sure why we should have both. It may be easier to move completely to gmock if its more powerful, even if the checks are sometimes more verbose for simple cases. > > This is essentially where I am at. > > If we didn't need the more expressive expectations in some cases, I would totally stick with EXPECT_EQ and friends for consistency and simplicity. But if the use cases for more rich and powerful matchers in test EXPECT lines is compelling, I have a mild preference for not having two ways of writing EXPECT_* and ASSERT_* lines, even though the simple cases are a bit more typing. The consistency and predictability for me win out, but not by much. =] I don't think its a huge deal either way. > > I'd also be totally willing to have LLVM-specific aliases of: > > EXPECT(...) -> EXPECT_THAT(...) > ASSERT(...) -> ASSERT_THAT(...) > > Because these are only inside of tests, and LLVM's projects are reasonably self contained, if saving the 5 characters helps folks, I'm OK with it.Yeah, thats totally fine with me.> > Anyways, this is something of a detail to sort out if we want the matcher functionality.Sounds good. Cheers! Pete> > -Chandler-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20170104/14370ad4/attachment.html>