[PATCH] D83651: [FileCheck] Report captured variables

[Thomas Preud'homme via Phabricator via llvm-commits]

thopre accepted this revision.
thopre added a comment.
This revision is now accepted and ready to land.

In D83651#2150645 <https://reviews.llvm.org/D83651#2150645>, @jdenny wrote:

> Removed an unused parameter from `printVariableDefs`.
>
> Made unit testing of `getStringValue` stricter.
>
> Added unit test for `printVariableDefs`.  This is just a basic API test and doesn't check every aspect, such as sorting.  More thorough testing appears in the FileCheck utility tests.  Should we replicate all that here too?  What's the general rule?


Unfortunately there's no rule right now and there's already overlap. My personal feeling is:

- Every new code should have unit test with a focus on code coverage
- User visible behavior of a code should be tested in llvm/test (it does not need to aim at full coverage since there is unittest)
- Try to avoid duplication as much as possible
- Aim for readability

In this specific example the test in llvm/test shows the definition diagnostic alongside other diagnostic. The unit test would cover all possible case (e.g. loops taken/not taken). Unfortunately this already leads to some duplication so perhaps keep the sorting for the llvm/test which is much easier to read in llvm/test since output with several variable definition spans several lines.

I think the general rule should be discussed separately and I'm happy to approve the patch as it is.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83651/new/

https://reviews.llvm.org/D83651