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

[Haojian Wu via Phabricator via cfe-commits]

hokein 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"};
----------------
MyDeveloperDay wrote:
> 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");
> ```
> 
> 
> 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.

I agree that we should reuse the code as much as possible. As @karepker mentioned, we don't know the details about other testing frameworks (they may use the same principle), it is unsafe to make this assumption.

Note that the original motivation of this check is for google test framework, I think we should focus on this scope in this patch, rather than trying to make this check fit everything. And we could always find a way to reuse the code (either by making this check more general, configurable or by creating a base class) if in the future we want to support other test frameworks.


> 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..

As a user not familiar with gtest internal implementation, I found the diagnostic "avoid using \"_\" in test case name \"%0\"" a bit confusing, I don't know why `_` is not allowed, adding `according to Googletest FAQ` words would make it clearer to users, at least given them a clue ;)




Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56424