[PATCH] D44764: [clangd] Use a header for common inclusions for tests

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In https://reviews.llvm.org/D44764#1045992, @malaperle wrote:

> In https://reviews.llvm.org/D44764#1045682, @ilya-biryukov wrote:
>
> > In https://reviews.llvm.org/D44764#1045648, @simark wrote:
> >
> > > We could create a file `ClangdTesting.h" which includes everything tested related (gtest, gmock, printers, syncapi, etc). The convention would be that test files would just include that.
> >
> >
> > Yeah, sounds good. I would exclude syncapi from the list, though. Not all tests should necessarily depend on `ClangdServer`.
> >  @sammccall , @ioeric , @hokein, @bkramer are you on board with the idea to have an umbrella header for gtest+gmock+printers?
>
>
> Sounds good to me too. I can update the patch once there is an agreement.


(Sorry, have been OOO)
I understand the template instantiations here are a mess and we need to solve the problem. However I'm not sure this is the right direction:

- it violates IWYU, and we expect people consistently to get gmock transitively via this header. This goes against common practice, is inconsistent with the rest of LLVM, and confuses tools (including clangd itself!). It would need to be carefully documented, but I don't think this is enough.
- it's fragile - as soon as one test includes gmock directly we have this problem again
- `printers.h` takes a horizontal slice of the types we have - it seems to suffer from the same problem as "not all tests should depend on clangdserver".

I'd suggest instead:

- where there's a fairly nice general-purpose debugging representation of a type, we should name that `operator<<` and declare it in the header alongside the type. That way it's clear that it's available, there's no risk of ODR violation, and it can be used for debugging as well as by gtest.
  - where there isn't one (or it's insufficiently detailed), we should use something more primitive like `EXPECT_THAT(foo, ...) << dump(foo)` with `dump` defined locally in the cpp file. We can't share these, but these aren't really sharable anyway. This isn't ideal (nested matcher messages) but should be good enough in practice.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44764