[PATCH] D56424: [clang-tidy] Add check for underscores in googletest names.

[Kar Epker via Phabricator via cfe-commits]

karepker added a comment.

In D56424#1357484 <https://reviews.llvm.org/D56424#1357484>, @MyDeveloperDay wrote:

> In D56424#1357481 <https://reviews.llvm.org/D56424#1357481>, @lebedev.ri wrote:
>
> > In D56424#1357471 <https://reviews.llvm.org/D56424#1357471>, @MyDeveloperDay wrote:
> >
> > > In D56424#1356959 <https://reviews.llvm.org/D56424#1356959>, @karepker wrote:
> > >
> > > > Hi all, ping on this patch. I've addressed all comments to the best of my ability. Is there anything outstanding that needs to be changed?
> > >
> > >
> > > Round about this time of a review we normally hear @JonasToth asking if you've run this on a large C++ code base like LLVM (with fix-its), and seen if the project still builds afterwards..might be worth doing that ahead of time if you haven't done so already
> >
> >
> > From docs: `This check does not propose any fixes.`.
>
>
> Thats a great suggestion for the future then....     transform
>
>   TEST(TestCase_Name, Test_Name) {} 
>
>
> into
>
>   TEST(TestCaseName, TestName) {}
>


I considered doing this for the check, but decided against it based on the cases in which I've seen underscores in use. I've seen a few cases in the style of this:

SuccessfullyWrites_InfiniteDeadline
SuccessfullyWrites_DefaultDeadline

changing these to:

SuccessfullyWritesInfiniteDeadline
SuccessfullyWritesDefaultDeadline

has a subtle difference to the reader. In the first case, underscore functions like "with", whereas in the fix it sounds like the test is for writing the deadline.

While removing the underscore does seem to work for a large portion of the cases, based on the cases like that above, I didn't think it was appropriate to make these modifications.

Please let me know what you think.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56424