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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.


================
Comment at: clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp:27
+static bool isGoogletestTestMacro(StringRef MacroName) {
+  static const llvm::StringSet<> MacroNames = {"TEST", "TEST_F", "TEST_P",
+                                               "TYPED_TEST", "TYPED_TEST_P"};
----------------
karepker wrote:
> MyDeveloperDay wrote:
> > Is there value in making the list of macros and option?, I've worked with other unit testing packages (some inhouse) that use the same format as google test, but don't use TEST() itself
> > 
> > e.g.  (to name a few)
> > 
> > ```
> > TEST_CASE(testclass, testname)
> > BOOST_TEST(statement, floating_point_comparison_manipulation);
> > BOOST_DATA_TEST_CASE(test_case_name, dataset)
> > BOOST_FIXTURE_TEST_CASE( test_case2, F )
> > BOOST_AUTO_TEST_CASE( test_case3 )
> > 
> > ```
> > 
> > too many for you to capture in the checker...but a nice addition for those who use alternative frameworks and would like to benefit from similar  "no underscore" naming conventions
> > 
> I'm not familiar with, e.g. the boost testing framework, so I don't know how closely it mirrors Googletest, but I think my preference would be to have a separate check for other testing frameworks.
> 
> While the testing frameworks might share some high-level similarities, there could be some different corner cases which would make having a separate check worth it as opposed to making this code more complex by trying to generalize it for several cases. At the very least, a different diagnostic message would be required. Factoring out similar functionality into some utility functions might reduce some of the boilerplate from implementing separate checks.
Maybe, but a shame as the code for a different check would be almost identical and unlikely to be able to support seperate in house systems that work on the same principle, of combining the testcase and testname as a macro and the presense of _ at the start or in the middle generating invalid symbols or ambiguities.

So perhaps even the

> "according to Googletest FAQ"

might be unnecessary text anyway for the warning, its easily something that can go in the documentation for the justification for the checker, I'm not sure everyone needs to see this every time they look at a warning..

most checks just say what to do, not why its a bad idea or where to find the justification.

e.g.


```
diag(Loc, "do not use unnamed namespaces in header files");
```




================
Comment at: clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp:64
+      Check->diag(TestCaseNameToken->getLocation(),
+                  "avoid using \"_\" in test case name \"%0\" according to "
+                  "Googletest FAQ")
----------------
karepker wrote:
> JonasToth wrote:
> > Duplicated message text, you can store it in a local variable/StringRef/StringLiteral,
> > 
> > ellide braces
> The message text is not quite the same. The first says "test case name" and the second says "test name" per the naming conventions for the two name arguments to the test macros in Googletest. I preferred having these as two separate strings instead of trying to add the word "case" conditionally to one, which I thought would be too complex.
> 
> Please let me know if you feel differently or have other suggestions regarding the message.
> 
> Braces have been removed.
I think it would be enough to have one diag message saying 

```
diag(Loc,"don't use "_" in %s() ",testMacro) 
```

simply elide the name of the parameter, the user will be drawn to the line and will see they are using an "_" in either testname or testcase, I don't think it necessary to tell them which parameter is incorrect..

And so whilst I see why you might prefer to generate 2 different messages for either testcase and testname, isn't it also true that these messages will be wrong if your using one of the other macros likes TEST_F() shouldn't the second argument now be testfixture? and if I'm using TEST_P() shouldn't it be pattern or parameterized?

This is why I think the test macros used would be useful as an option, by generalizing the checker by removing the "googletest" specifics makes the checker much more useful.

I know for one, I don't use gtest, but a similar framework (with the same issues), and I'd use your checker if I could customize it.






Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56424