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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
