Page MenuHomePhabricator

Create an shared include directory for gtest helper functions
ClosedPublic

Authored by zturner on May 10 2017, 12:59 PM.

Details

Summary

Often different tests in different test executables need to do the same thing. Historically, we have always just copied large blocks of code between tests to solve this. For example, if a function returns an llvm::Error, then we would like to have some set of EXPECT / ASSERT macros that can handle them in the canonical gtest fashion, and these should be shared among all tests. Currently we have no such place to put code of this nature.

This patch adds such a location in llvm/Support/gtest. For now it's header only, but in the future it could be extended to have a library component as well that is force linked into all unittest targets.

Additional use cases of this could include adding asserts and expect macros for std::error_code, or adding various mock objects to be used alongside gmock (think: ASSERT_THAT(ElementExists(Vec, 7)).

In this patch, I take two instances of the same copied file that contains macros for expecting and asserting on llvm::Errors, and move them to this shared directory, then include the shared file.

Diff Detail

Repository
rL LLVM

Event Timeline

zturner created this revision.May 10 2017, 12:59 PM
labath added a subscriber: labath.May 16 2017, 3:26 AM
chandlerc edited edge metadata.May 17 2017, 4:57 PM

I definitely want this. First question is where?

I don't think this should be under include/llvm because it isn't a public header of LLVM. Nothing in lib/... should use it, etc.

How about utils/unittest/Helpers/Foo.h? We can even add .../Foo.cpp if/when we need it? Then we can just make these headers available similarly to how we make the google test headers available.

chandlerc added inline comments.May 17 2017, 5:01 PM
llvm/include/llvm/gtest/ErrorChecking.h
37 ↗(On Diff #98507)

I'm fine moving the existing helpers, but before we expand this, i'd like to think about whether its time to write up the doc about good usage of gmock matchers, get folks OK with that, and write this as a library of matchers that work with EXPECT_THAT instead.

zturner updated this revision to Diff 99387.May 17 2017, 8:25 PM

Moved the folder. I really would like to put it in utils\unittest\googletest\include\gtest\Support, because then you could write

#include "gtest/gtest.h"
#include "gtest/Support/ErrorChecking.h"

but I'm not sure if sticking our own directory inside of a directory of third party code would be weird.

I plan to look into googlemock more in depth in the coming days, and I'll probably start writing a few common matchers (including replacing these macros) along with that effort. But since this code is already here, I might as well get it in first and do that as an incremental improvement.

Moved the folder.

I don't particularly like the new name. ;]

Because we're not going to install anything, I don't think we need the 'include' directory component or to separate the headers from the source files.

If your concern is that #include "Helpers/Foo.h" isn't clear because Helpers is too generic, how about TestHelpers or UnittestHelpers? I'd still keep a single directory, and use more typical LLVM NamingConvention for it. =]

I really would like to put it in utils\unittest\googletest\include\gtest\Support, because then you could write

#include "gtest/gtest.h"
#include "gtest/Support/ErrorChecking.h"

Yeah this would be nice, but...

but I'm not sure if sticking our own directory inside of a directory of third party code would be weird.

I also think i would be weird. I'd like us to keep it separate.

but I'm not sure if sticking our own directory inside of a directory of third party code would be weird.

I also think i would be weird. I'd like us to keep it separate.

Bear with me here. Would it be more or less weird to have a directory of the same name but in a different location?

e.g.

utils
  googlemock
    include
      gmock
  googletest
    include
      gtest
  Support
    include
      gtest
        Support

This way you could still write #include "gtest/Support/ErrorChecking.h" but we wouldn't be stepping on third party code.

but I'm not sure if sticking our own directory inside of a directory of third party code would be weird.

I also think i would be weird. I'd like us to keep it separate.

Bear with me here. Would it be more or less weird to have a directory of the same name but in a different location?

e.g.

utils
  googlemock
    include
      gmock
  googletest
    include
      gtest
  Support
    include
      gtest
        Support

This way you could still write #include "gtest/Support/ErrorChecking.h" but we wouldn't be stepping on third party code.

While I admire your cleverness here, I think a simpler solution would be better. ;]

zturner updated this revision to Diff 101343.Jun 3 2017, 10:21 PM

Trying this again.

Note that this time I went all the way and added matchers instead of the hand rolled macros. that delegate to EXPECT_TRUE etc. Since gmock doesn't support matchers that modify their arguments, the solution here is to make a new EXPECT / ASSERT macro pair that consumes the Error, converting it into a format that can easily be copied around and passed through the mock infrastructure.

There's still some uses of hand rolled macros related to std::error_code in various tests, but I think it's too much for one patch. Maybe in a followup

zturner updated this revision to Diff 101347.Jun 3 2017, 10:54 PM

A few improvements to error message printouts.

zturner updated this revision to Diff 101353.Jun 4 2017, 8:21 AM
  1. Moved the code from UnitTestHelpers to UnitTestHelpers/Support. If this is going to be a general purpose location for sharing code amongst unit tests, then it can't be specific to Support. For example, you might have need for specific matchers related to CodeGen or debug info that nothing outside of those unit tests would ever need to use, but that should still be usable by anything that depends on those. For example, you could write a matcher related to a SelectionDAG and it could be used in a general purpose CodeGen unit test or a target specific one. On the other hand, such a shared matcher doesn't belong next to a Support matcher because then anything that used Support would have to link against CodeGen, and obviously we don't want SupportTests and ADTTests linking CodeGen. We don't have this scenario yet, but I think we need to get the layout right the first time so it's ready to use immediately.
  1. Got rid of StringRef.h. This was only there to have a formatter for StringRef, but due to the design of gmock, all printers really need to be declared in one master file. You don't want someone to get different output depending on whether they remember to include a specific header or not, and when you've got something like trying to print Expected<T>, the printer for Expected<T> needs to be able to find the printer for T or it will print a really ugly version of it, and the only way to remove this burden from the user is to make sure all custom printers are in one file.

ping, PTAL?

FYI, I chatted with Zach in person to understand why the directory layout was as complicated as it was. I really wanted to simplify it. However, my initial attempts broke cross-project usage as Richard Smith pointed out, and this is *exactly* the kind of thing that we will really want to share across LLVM projects: Both LLD and potentially Clang will want access to things like the error matching utilities for their unittests.

With that constraint in mind, I talked for a long time with Richard about the tradeoffs here. Our suggested approach is to be less inventive: testing libraries are, fundamentally, *just libraries*. We probably don't want to install them and it should be clear that they are for testing (and so shouldn't be used elsewhere and can carry more dependencies than would be acceptable elsewhere). But they're still libraries. We suggest the file layout:

include/llvm/Testing/Support/Error.h
lib/Testing/Support/Error.cpp

And then you could use it from unittests/Support/FooTest.cpp in the obvious way:

#include "llvm/Testing/Support/Error.h"

This may be a bit controversial so it is worth making sure others are on board with this direction. The big thing that makes all of the other alternatives hard is ending up with a way for a Clang or LLD unittest to include a header for the llvm/Support or llvm/CodeGen testing utilities as well as their own clang/Basic or clang/CodeGen (see what I did there?) without confusion or in the latter case, a flat ambiguity.

An alternative if people are strongly opposed to just treating this like any other collection of LLVM libraries would have the file layout like:

utils/unittest/llvm-testing/Support/Error.cpp
utils/unittest/llvm-testing/Support/SupportHelpers.h

And then usage from unittests/Support/FooTest.cpp would look like:

#include "llvm-testing/Support/Error.h"
MatzeB added a subscriber: MatzeB.Jun 12 2017, 6:55 PM

about the directories: 'meh'. I'm fine with whatever works.

  • Why is a patch that claims to create a shared include directory actually changing so many unit tests?
  • EXPECT_THAT_ERROR(x, Succeeded()) seems bad. SHouldn't this rather be EXPECT_THAT(x, succeeded()) to look more natural?

about the directories: 'meh'. I'm fine with whatever works.

  • Why is a patch that claims to create a shared include directory actually changing so many unit tests?

I felt like we needed a motivating example of why this effort is useful. The error macros were copy/pasted amongst many different files, so it was the perfect candidate.

  • EXPECT_THAT_ERROR(x, Succeeded()) seems bad. SHouldn't this rather be EXPECT_THAT(x, succeeded()) to look more natural?

You're right, it is bad. But unfortunately there is not a good alternative. Error is a move only type, and gmock a) can't handle move only types, and b) can't exhibits undefined behavior if you change the state of an object you're matching inside of a matcher. Because of that, we have to convert the Error into something else by basically consuming its failure/success status and error messge and storing them in a different type, and then matching against that type. So we need the macro to convert the Error / Expected into the type we actually match against.

It's not ideal, but it seems like the best we can do for this one specific class.

about the directories: 'meh'. I'm fine with whatever works.

  • Why is a patch that claims to create a shared include directory actually changing so many unit tests?

I felt like we needed a motivating example of why this effort is useful. The error macros were copy/pasted amongst many different files, so it was the perfect candidate.

So this doesn't really seem to be a "motivating" example that shows how gmock makes life simpler. But again I'm 'meh' here, I think we already decided to use gmock and I assume you guys have examples where it actually improves the experience.

about the directories: 'meh'. I'm fine with whatever works.

  • Why is a patch that claims to create a shared include directory actually changing so many unit tests?

I felt like we needed a motivating example of why this effort is useful. The error macros were copy/pasted amongst many different files, so it was the perfect candidate.

So this doesn't really seem to be a "motivating" example that shows how gmock makes life simpler. But again I'm 'meh' here, I think we already decided to use gmock and I assume you guys have examples where it actually improves the experience.

I'm not talking about gmock, I'm talking about a motivating example of why having a shared include directory is useful. That aside, I definitely think gmock improves the experience here. If you write EXPECT_THAT_ERROR(foo(), succeeded()) and it doesn't succeed, it will show you the error message that it failed with. Same thing with the Expected case.

zturner added a comment.EditedJun 12 2017, 7:31 PM

To motivate the gmock specific portion, here are some examples of what happens when you get a test failure now using the new gmock matchers as implemented in this patch.

Error fails when you expect it to succeed:

E:\src\llvm-mono\llvm\unittests\DebugInfo\PDB\MappedBlockStreamTest.cpp(189): error: Value of: llvm::detail::TakeError(R.readFixedString(Str, 11))
Expected: succeeded
  Actual: failed  (Stream Error: The stream is too short to perform the requested operation.)

Expected fails when you expected it to succeed

E:\src\llvm-mono\llvm\unittests\DebugInfo\PDB\MSFBuilderTest.cpp(250): error: Value of: llvm::detail::TakeExpected(Msf.addStream(6144, Blocks))
Expected: succeeded
  Actual: failed  (MSF Error: Attempt to re-use an already allocated block)

Expected<T> operation succeeded but doesn't have the expected value.

E:\src\llvm-mono\llvm\unittests\DebugInfo\PDB\StringTableBuilderTest.cpp(51): error: Value of: llvm::detail::TakeExpected(Table.getStringForID(3))
Expected: succeeded with value "baz"
  Actual: succeeded with value "{ 'o' (111, 0x6F) }", but "{ 'o' (111, 0x6F) }" != "baz"

Expected<T> operation failed when you expected it to succeed with a certain value.

E:\src\llvm-mono\llvm\unittests\DebugInfo\PDB\StringTableBuilderTest.cpp(52): error: Value of: llvm::detail::TakeExpected(Table.getIDForString("abc"))
Expected: succeeded with value 1
  Actual: failed  (Native PDB Error: The entry does not exist.), operation failed

about the directories: 'meh'. I'm fine with whatever works.

  • Why is a patch that claims to create a shared include directory actually changing so many unit tests?

I felt like we needed a motivating example of why this effort is useful. The error macros were copy/pasted amongst many different files, so it was the perfect candidate.

  • EXPECT_THAT_ERROR(x, Succeeded()) seems bad. SHouldn't this rather be EXPECT_THAT(x, succeeded()) to look more natural?

You're right, it is bad. But unfortunately there is not a good alternative. Error is a move only type, and gmock a) can't handle move only types, and b) can't exhibits undefined behavior if you change the state of an object you're matching inside of a matcher. Because of that, we have to convert the Error into something else by basically consuming its failure/success status and error messge and storing them in a different type, and then matching against that type. So we need the macro to convert the Error / Expected into the type we actually match against.

It's not ideal, but it seems like the best we can do for this one specific class.

I think it is possible to avoid the explosion of EXPECT/ASSERT_THAT_<FOO> macros (I can't think of any right now, but I'm sure something will crop up eventually, which will require specific handling). For example, we could just define a single expect macro, and hide all of the type-specific stuff in the helper function. Then overload resolution should take care of selecting the right behavior.

I'm thinking of something like:

#define EXPECT(Obj, Matcher) EXPECT_THAT(::llvm::detail::gmock_helper(Obj), Matcher)
#define ASSERT(Obj, Matcher) ASSERT_THAT(::llvm::detail::gmock_helper(Obj), Matcher)

template<typename T>
T gmock_helper(T Obj) { return Obj; }

template<typename T>
ExpectedHolder<T> gmock_helper(Expected<T> Obj) { ... }

This way, we could just write ASSERT(foo, bar) always (names are just a suggestion, but I would prefer something shorter than ASSERT_THAT_EXPECTED), and if a type needed special handling, we would just specialize gmock_helper. What do you think?

(Disclaimer: I haven't tried to actually implement this, but I don't see a reason why it wouldn't work)

I think it is possible to avoid the explosion of EXPECT/ASSERT_THAT_<FOO> macros (I can't think of any right now, but I'm sure something will crop up eventually, which will require specific handling).

There are a lot of things we can investigate to make this better. However, I don't think they belong in this patch. This patch is *mostly* about consolidating where things live to a common home. It does so. It updates some custom macro expectations to be slightly different custom macro expectations with specific benefits Zach outlined.

Because these were already custom macros, I consider this "no harm done". I wouldn't want to go farther than that in this patch though, I'd prefer to get this (or something like it) in place to prove the concept of building more robust libraries that make testing easy and effective. Once we have that, we can write a much more focused patch that *just* tries to improve these particular cases to not require custom macros.

I'm refraining from commenting on any particulars of the approach you suggest or alternatives for that future time / place / discussion.

I think it is possible to avoid the explosion of EXPECT/ASSERT_THAT_<FOO> macros (I can't think of any right now, but I'm sure something will crop up eventually, which will require specific handling).

There are a lot of things we can investigate to make this better. However, I don't think they belong in this patch. This patch is *mostly* about consolidating where things live to a common home. It does so. It updates some custom macro expectations to be slightly different custom macro expectations with specific benefits Zach outlined.

Because these were already custom macros, I consider this "no harm done". I wouldn't want to go farther than that in this patch though, I'd prefer to get this (or something like it) in place to prove the concept of building more robust libraries that make testing easy and effective. Once we have that, we can write a much more focused patch that *just* tries to improve these particular cases to not require custom macros.

I'm refraining from commenting on any particulars of the approach you suggest or alternatives for that future time / place / discussion.

Fair enough. I'll wait until this patch lands to propose that.

zturner updated this revision to Diff 102340.Jun 13 2017, 8:44 AM

Updated with new directory layout suggestions.

chandlerc accepted this revision.Jun 13 2017, 3:05 PM

LGTM. Some comments below about the name of the library and getting the linking and other stuff set up correctly, but I'm happy for you to sort that out and land (and fix bots as necessary) w/o going through more rounds of review. =D

One thing I would suggest is mentioning the fact that the particular interface being used for the EXPECT_THAT_ERROR stuff is likely to have further progress to try and improve it, but that this at least consolidates things and provides a clear place for that development to take place. Hopefully that'll help other folks on the commit list understand the context that came up in this review.

Lastly, you should add some warning text somewhere in this code here (maybe a readme near the headers? maybe just in the headers?) that this is intended for use for testing *only*, and shouldn't be depended on by any non-test code. And you should probably add some discussion of this libarry (and the above warning) to the programmer guide for LLVM. But I'm happy for that to be done in subsequent commits where convenient.

Thansk for working on the infrastructure here!

llvm/lib/Testing/Support/CMakeLists.txt
1 ↗(On Diff #102340)

I would call this 'LLVMTestingSupport' to match the directory name I guess?

8–9 ↗(On Diff #102340)

Do you need to link gtest and gmock as well? I'm guessing without that a shared build will fail...

This revision is now accepted and ready to land.Jun 13 2017, 3:05 PM
This revision was automatically updated to reflect the committed changes.