Adds a clang-tidy warning for underscores in googletest names.
Details
Diff Detail
- Repository
- rCTE Clang Tools Extra
- Build Status
Buildable 26475 Build 26474: arc lint + arc unit
Event Timeline
For reference, what documentation sources did you read when creating the review itseft?
I thought it was documented that an appropriate category ([clang-tidy]) should be present in the subject.
Thanks, mostly good to me. just a few nits.
clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp | ||
---|---|---|
72 | nit: using Test.contains | |
clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.h | ||
10 | nit: the header guard should be LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_GOOGLE_AVOIDUNDERSCOREINGOOGLETESTNAMECHECK_H | |
docs/clang-tidy/checks/google-readability-avoid-underscore-in-googletest-name.rst | ||
3 | nit: also mention the new check in /docs/ReleaseNotes.rst | |
7 | maybe list all test macros that the check detects? |
clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp | ||
---|---|---|
1 | nit: space after tidy and before --- | |
clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.h | ||
1 | nit: I think you may have used an older version of add_new_check.py because the header looks like this now A gap after the tidy and *- C++ -* //===--- UseNodiscardCheck.h - clang-tidy -----------------------*- C++ -*-===// This could be why the header guard isn't the correct format |
clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp | ||
---|---|---|
23 | Please use llvm::StringLiteral | |
45 | please dont use auto here, subjectiv as IdentifierInfo is the type, but new contributors might not know that, so i would prefer not-auto here. | |
49 | no const in this case, as its a value and those are not const'ed in LLVM. | |
50 | You can ellide the braces and merge these two ifs. | |
58 | you can ellide the braces | |
62 | Maybe the duplicated diag call can be merged, you can store the diagnostic with auto Diag = Check->diag(...) and pass in the right location and arguments. | |
64 | Duplicated message text, you can store it in a local variable/StringRef/StringLiteral, ellide braces | |
69 | Please use camel-case. |
clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp | ||
---|---|---|
46 | Please elide braces. |
clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp | ||
---|---|---|
27 | 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 see now that putting the category in the commit message is recommend by http://llvm.org/docs/DeveloperPolicy.html#commit-messages, though if I were reading that for the first time, it would have been unclear to me what the protocol for determining the category is (e.g. should this be [clang-tidy] vs. [clang-tools-extra])?
When I was preparing this for review, though, I was mostly going by example off this previous patch: https://reviews.llvm.org/D18408.
clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp | ||
---|---|---|
1 | I think I did this. Not sure which "---" you're referring to, but I think this comment is now in line with other clang-tidy comments I've looked at. | |
62 | I don't think I understand the suggestion. Don't I have to pass in the location immediately? Check->diag(...) doesn't seem to have an overload that allows deferring passing in the location until later. | |
64 | 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. |
docs/ReleaseNotes.rst | ||
---|---|---|
161 ↗ | (On Diff #180716) | Please synchronize with first statement in documentation. |
docs/ReleaseNotes.rst | ||
---|---|---|
161 ↗ | (On Diff #180716) | Done, I think. I assume by documentation you mean documentation in the check rst file. The first statement there isn't really a sentence, but I took as much of the wording from there as I could. |
clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp | ||
---|---|---|
27 | 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. |
clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp | ||
---|---|---|
27 | 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
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"); | |
64 | 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. |
clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp | ||
---|---|---|
64 | minor correction. to my comment diag(Loc,"avoid using \"_\" in %s() ",testMacro) |
clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp | ||
---|---|---|
27 |
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.
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 ;) |
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
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.
Did I misunderstand I thought the point of the checker was to say "_" in the name was illegal? surely the fixit is at liberty to resolve that?
That's correct, underscore is illegal, but my understanding is that fixits should only be applied if they are able to keep the semantic meaning of the code consistent. While removing the underscore does seem to work in many cases, in some cases, like the one I pointed out above, it does not. Since it's not easy to slice off which cases the fixit is appropriate vs. which cases it isn't, I've opted not to implement the fixit and rely on the developer to fix the name of the test manually.
Rebasing against master.
Not sure if re-arc diffing this is necessary, but I hope it doesn't hurt.
Didn't notice that you don't have commit access, committed in rL352183 (with a small change of the new license).
I just received commit access yesterday but hadn't quite figured out how to use it by the time your modified patch went in. Thanks for taking care of it.
nit: I think you may have used an older version of add_new_check.py because the header looks like this now
A gap after the tidy and *- C++ -*
This could be why the header guard isn't the correct format