This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Add check for underscores in googletest names.
ClosedPublic

Authored by karepker on Jan 7 2019, 6:33 PM.

Event Timeline

karepker created this revision.Jan 7 2019, 6:33 PM
karepker updated this revision to Diff 180603.Jan 7 2019, 6:56 PM

Clean up comments in test file.

lebedev.ri retitled this revision from Add check for underscores in googletest names. to [clang-tidy] Add check for underscores in googletest names..Jan 7 2019, 11:33 PM
lebedev.ri added a project: Restricted Project.

Clean up comments in test file.

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.

hokein added a comment.Jan 8 2019, 1:30 AM

Thanks, mostly good to me. just a few nits.

clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp
73

nit: using Test.contains

clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.h
11

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
4

nit: also mention the new check in /docs/ReleaseNotes.rst

8

maybe list all test macros that the check detects?

MyDeveloperDay added inline comments.
clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp
2

nit: space after tidy and before ---

clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.h
2

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

JonasToth added inline comments.
clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp
24

Please use llvm::StringLiteral

46

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.

50

no const in this case, as its a value and those are not const'ed in LLVM.

51

You can ellide the braces and merge these two ifs.

59

you can ellide the braces

63

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.

65

Duplicated message text, you can store it in a local variable/StringRef/StringLiteral,

ellide braces

70

Please use camel-case.

Eugene.Zelenko added inline comments.
clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp
47

Please elide braces.

MyDeveloperDay added inline comments.Jan 8 2019, 10:39 AM
clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp
28

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

karepker updated this revision to Diff 180716.Jan 8 2019, 12:32 PM
karepker marked 15 inline comments as done.

Address comments.

Clean up comments in test file.

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.

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
2

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.

63

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.

65

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.

Eugene.Zelenko added inline comments.Jan 8 2019, 12:51 PM
docs/ReleaseNotes.rst
161

Please synchronize with first statement in documentation.

karepker updated this revision to Diff 180727.Jan 8 2019, 1:33 PM
karepker marked 4 inline comments as done.

Update release notes documentation to match check documentation more.

karepker added inline comments.Jan 8 2019, 1:34 PM
docs/ReleaseNotes.rst
161

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.

karepker marked 2 inline comments as done and an inline comment as not done.Jan 8 2019, 1:37 PM
karepker added inline comments.
clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp
28

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.

MyDeveloperDay added inline comments.Jan 9 2019, 1:38 AM
clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp
28

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");
65

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.

MyDeveloperDay added inline comments.Jan 9 2019, 1:49 AM
clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp
65

minor correction. to my comment

diag(Loc,"avoid using \"_\" in %s() ",testMacro)
hokein added inline comments.Jan 9 2019, 3:17 AM
clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp
28

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 ;)

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?

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

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

MyDeveloperDay added a comment.EditedJan 15 2019, 12:15 AM

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) {}

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.

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.

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?

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.

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.

hokein accepted this revision.Jan 21 2019, 1:19 AM

Sorry for the delay, I was OOO last week. The check looks good.

This revision is now accepted and ready to land.Jan 21 2019, 1:19 AM
karepker updated this revision to Diff 183437.Jan 24 2019, 4:26 PM

Rebasing against master.

Not sure if re-arc diffing this is necessary, but I hope it doesn't hurt.

This revision was automatically updated to reflect the committed changes.

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

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.