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

[karepker]

Summary

Adds a clang-tidy warning for underscores in googletest names.

[karepker]

Clean up comments in test file.

[lebedev.ri]

In D56424#1349218, @karepker wrote:

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]

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

clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp

72 ↗

(On Diff #180603)

nit: using Test.contains

clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.h

10 ↗

(On Diff #180603)

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 ↗

(On Diff #180603)

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

7 ↗

(On Diff #180603)

maybe list all test macros that the check detects?

[MyDeveloperDay]

clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp

1 ↗

(On Diff #180603)

nit: space after tidy and before ---

clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.h

1 ↗

(On Diff #180603)

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]

clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp

23 ↗

(On Diff #180603)

Please use llvm::StringLiteral

45 ↗

(On Diff #180603)

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 ↗

(On Diff #180603)

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

50 ↗

(On Diff #180603)

You can ellide the braces and merge these two ifs.

58 ↗

(On Diff #180603)

you can ellide the braces

62 ↗

(On Diff #180603)

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 ↗

(On Diff #180603)

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

ellide braces

69 ↗

(On Diff #180603)

Please use camel-case.

[Eugene.Zelenko]

clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp

46 ↗

(On Diff #180603)

Please elide braces.

[MyDeveloperDay]

clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp

27 ↗

(On Diff #180603)

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]

Address comments.

[karepker]

In D56424#1349336, @lebedev.ri wrote:

In D56424#1349218, @karepker wrote:

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

1 ↗

(On Diff #180603)

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 ↗

(On Diff #180603)

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 ↗

(On Diff #180603)

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]

docs/ReleaseNotes.rst

161 ↗

(On Diff #180716)

Please synchronize with first statement in documentation.

[karepker]

Update release notes documentation to match check documentation more.

[karepker]

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.

[karepker]

clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp

27 ↗

(On Diff #180603)

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]

clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp

27 ↗

(On Diff #180603)

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

64 ↗

(On Diff #180603)

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]

clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp

64 ↗

(On Diff #180603)

minor correction. to my comment

diag(Loc,"avoid using \"_\" in %s() ",testMacro)

[hokein]

clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp

27 ↗

(On Diff #180603)

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

[karepker]

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?

[MyDeveloperDay]

In D56424#1356959, @karepker wrote:

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

[lebedev.ri]

In D56424#1357471, @MyDeveloperDay wrote:

In D56424#1356959, @karepker wrote:

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]

In D56424#1357481, @lebedev.ri wrote:

In D56424#1357471, @MyDeveloperDay wrote:

In D56424#1356959, @karepker wrote:

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

[karepker]

In D56424#1357484, @MyDeveloperDay wrote:

In D56424#1357481, @lebedev.ri wrote:

In D56424#1357471, @MyDeveloperDay wrote:

In D56424#1356959, @karepker wrote:

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.

[MyDeveloperDay]

In D56424#1359227, @karepker wrote:

In D56424#1357484, @MyDeveloperDay wrote:

In D56424#1357481, @lebedev.ri wrote:

In D56424#1357471, @MyDeveloperDay wrote:

In D56424#1356959, @karepker wrote:

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?

[karepker]

In D56424#1360318, @MyDeveloperDay wrote:

In D56424#1359227, @karepker wrote:

In D56424#1357484, @MyDeveloperDay wrote:

In D56424#1357481, @lebedev.ri wrote:

In D56424#1357471, @MyDeveloperDay wrote:

In D56424#1356959, @karepker wrote:

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]

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

[karepker]

Rebasing against master.

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

[hokein]

In D56424#1370461, @karepker wrote:

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

[karepker]

In D56424#1370908, @hokein wrote:

In D56424#1370461, @karepker wrote:

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.

clang-tools-extra/trunk/clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.h

+//===--- AvoidUnderscoreInGoogletestNameCheck.h - clang-tidy ----*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_GOOGLE_AVOIDUNDERSCOREINGOOGLETESTNAMECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_GOOGLE_AVOIDUNDERSCOREINGOOGLETESTNAMECHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace google {
+namespace readability {
+
+// Check for underscores in the names of googletest tests, per
+// https://github.com/google/googletest/blob/master/googletest/docs/faq.md#why-should-test-suite-names-and-test-names-not-contain-underscore
+class AvoidUnderscoreInGoogletestNameCheck : public ClangTidyCheck {
+public:
+  using ClangTidyCheck::ClangTidyCheck;
+
+  void registerPPCallbacks(CompilerInstance &Compiler) override;
+};
+
+} // namespace readability
+} // namespace google
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_GOOGLE_AVOIDUNDERSCOREINGOOGLETESTNAMECHECK_H

clang-tools-extra/trunk/clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp

+//===--- AvoidUnderscoreInGoogletestNameCheck.cpp - clang-tidy --*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include <string>
+
+#include "AvoidUnderscoreInGoogletestNameCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Lex/MacroArgs.h"
+
+namespace clang {
+namespace tidy {
+namespace google {
+namespace readability {
+
+constexpr llvm::StringLiteral kDisabledTestPrefix = "DISABLED_";
+
+// Determines whether the macro is a Googletest test macro.
+static bool isGoogletestTestMacro(StringRef MacroName) {
+  static const llvm::StringSet<> MacroNames = {"TEST", "TEST_F", "TEST_P",
+                                               "TYPED_TEST", "TYPED_TEST_P"};
+  return MacroNames.find(MacroName) != MacroNames.end();
+}
+
+namespace {
+
+class AvoidUnderscoreInGoogletestNameCallback : public PPCallbacks {
+public:
+  AvoidUnderscoreInGoogletestNameCallback(
+      Preprocessor *PP, AvoidUnderscoreInGoogletestNameCheck *Check)
+      : PP(PP), Check(Check) {}
+
+  // Detects expansions of the TEST, TEST_F, TEST_P, TYPED_TEST, TYPED_TEST_P
+  // macros and checks that their arguments do not have any underscores.
+  void MacroExpands(const Token &MacroNameToken,
+                    const MacroDefinition &MacroDefinition, SourceRange Range,
+                    const MacroArgs *Args) override {
+    IdentifierInfo *NameIdentifierInfo = MacroNameToken.getIdentifierInfo();
+    if (!NameIdentifierInfo)
+      return;
+    StringRef MacroName = NameIdentifierInfo->getName();
+    if (!isGoogletestTestMacro(MacroName) || !Args ||
+        Args->getNumMacroArguments() < 2)
+      return;
+    const Token *TestCaseNameToken = Args->getUnexpArgument(0);
+    const Token *TestNameToken = Args->getUnexpArgument(1);
+    if (!TestCaseNameToken || !TestNameToken)
+      return;
+    std::string TestCaseName = PP->getSpelling(*TestCaseNameToken);
+    if (TestCaseName.find('_') != std::string::npos)
+      Check->diag(TestCaseNameToken->getLocation(),
+                  "avoid using \"_\" in test case name \"%0\" according to "
+                  "Googletest FAQ")
+          << TestCaseName;
+
+    std::string TestNameMaybeDisabled = PP->getSpelling(*TestNameToken);
+    StringRef TestName = TestNameMaybeDisabled;
+    TestName.consume_front(kDisabledTestPrefix);
+    if (TestName.contains('_'))
+      Check->diag(TestNameToken->getLocation(),
+                  "avoid using \"_\" in test name \"%0\" according to "
+                  "Googletest FAQ")
+          << TestName;
+  }
+
+private:
+  Preprocessor *PP;
+  AvoidUnderscoreInGoogletestNameCheck *Check;
+};
+
+} // namespace
+
+void AvoidUnderscoreInGoogletestNameCheck::registerPPCallbacks(
+    CompilerInstance &Compiler) {
+  Compiler.getPreprocessor().addPPCallbacks(
+      llvm::make_unique<AvoidUnderscoreInGoogletestNameCallback>(
+          &Compiler.getPreprocessor(), this));
+}
+
+} // namespace readability
+} // namespace google
+} // namespace tidy
+} // namespace clang

clang-tools-extra/trunk/clang-tidy/google/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyGoogleModule
   AvoidCStyleCastsCheck.cpp
   AvoidThrowingObjCExceptionCheck.cpp
+  AvoidUnderscoreInGoogletestNameCheck.cpp
   DefaultArgumentsCheck.cpp
   ExplicitConstructorCheck.cpp
   ExplicitMakePairCheck.cpp
   FunctionNamingCheck.cpp
   GlobalNamesInHeadersCheck.cpp
   GlobalVariableDeclarationCheck.cpp
   GoogleTidyModule.cpp
   IntegerTypesCheck.cpp

clang-tools-extra/trunk/clang-tidy/google/GoogleTidyModule.cpp

 //===--- GoogleTidyModule.cpp - clang-tidy --------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "../readability/BracesAroundStatementsCheck.h"
 #include "../readability/FunctionSizeCheck.h"
 #include "../readability/NamespaceCommentCheck.h"
 #include "AvoidCStyleCastsCheck.h"
 #include "AvoidThrowingObjCExceptionCheck.h"
+#include "AvoidUnderscoreInGoogletestNameCheck.h"
 #include "DefaultArgumentsCheck.h"
 #include "ExplicitConstructorCheck.h"
 #include "ExplicitMakePairCheck.h"
 #include "FunctionNamingCheck.h"
 #include "GlobalNamesInHeadersCheck.h"
 #include "GlobalVariableDeclarationCheck.h"
 #include "IntegerTypesCheck.h"
 #include "NonConstReferences.h"
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<objc::GlobalVariableDeclarationCheck>(
         "google-objc-global-variable-declaration");
     CheckFactories.registerCheck<runtime::IntegerTypesCheck>(
         "google-runtime-int");
     CheckFactories.registerCheck<runtime::OverloadedUnaryAndCheck>(
         "google-runtime-operator");
     CheckFactories.registerCheck<runtime::NonConstReferences>(
         "google-runtime-references");
+    CheckFactories
+        .registerCheck<readability::AvoidUnderscoreInGoogletestNameCheck>(
+            "google-readability-avoid-underscore-in-googletest-name");
     CheckFactories.registerCheck<readability::AvoidCStyleCastsCheck>(
         "google-readability-casting");
     CheckFactories.registerCheck<readability::TodoCommentCheck>(
         "google-readability-todo");
     CheckFactories
         .registerCheck<clang::tidy::readability::BracesAroundStatementsCheck>(
             "google-readability-braces-around-statements");
     CheckFactories.registerCheck<clang::tidy::readability::FunctionSizeCheck>(

clang-tools-extra/trunk/docs/ReleaseNotes.rst

 --------------------------
 
 - New :doc:`abseil-duration-conversion-cast
   <clang-tidy/checks/abseil-duration-conversion-cast>` check.
 
   Checks for casts of ``absl::Duration`` conversion functions, and recommends
   the right conversion function instead.
 
+- New :doc:`google-readability-avoid-underscore-in-googletest-name
+  <clang-tidy/checks/google-readability-avoid-underscore-in-googletest-name>`
+  check.
+
+  Checks whether there are underscores in googletest test and test case names in
+  test macros, which is prohibited by the Googletest FAQ.
+
+
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 
 Improvements to modularize
 --------------------------
 
 The improvements are...

clang-tools-extra/trunk/docs/clang-tidy/checks/google-readability-avoid-underscore-in-googletest-name.rst

+.. title:: clang-tidy - google-readability-avoid-underscore-in-googletest-name
+
+google-readability-avoid-underscore-in-googletest-name
+======================================================
+
+Checks whether there are underscores in googletest test and test case names in
+test macros:
+
+- ``TEST``
+- ``TEST_F``
+- ``TEST_P``
+- ``TYPED_TEST``
+- ``TYPED_TEST_P``
+
+The ``FRIEND_TEST`` macro is not included.
+
+For example:
+
+.. code-block:: c++
+
+  TEST(TestCaseName, Illegal_TestName) {}
+  TEST(Illegal_TestCaseName, TestName) {}
+
+would trigger the check. `Underscores are not allowed`_ in test names nor test
+case names.
+
+The ``DISABLED_`` prefix, which may be used to `disable individual tests`_, is
+ignored when checking test names, but the rest of the rest of the test name is
+still checked.
+
+This check does not propose any fixes.
+
+.. _Underscores are not allowed: https://github.com/google/googletest/blob/master/googletest/docs/faq.md#why-should-test-suite-names-and-test-names-not-contain-underscore
+.. _disable individual tests: https://github.com/google/googletest/blob/master/googletest/docs/faq.md#why-should-test-suite-names-and-test-names-not-contain-underscore

clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst

.. toctree::
    google-build-namespaces
    google-build-using-namespace
    google-default-arguments
    google-explicit-constructor
    google-global-names-in-headers
    google-objc-avoid-throwing-exception
    google-objc-function-naming
    google-objc-global-variable-declaration
+   google-readability-avoid-underscore-in-googletest-name
    google-readability-braces-around-statements (redirects to readability-braces-around-statements) <google-readability-braces-around-statements>
    google-readability-casting
    google-readability-function-size (redirects to readability-function-size) <google-readability-function-size>
    google-readability-namespace-comments (redirects to llvm-namespace-comment) <google-readability-namespace-comments>
    google-readability-todo
    google-runtime-int
    google-runtime-operator
    google-runtime-references

clang-tools-extra/trunk/test/clang-tidy/readability-avoid-underscore-in-googletest-name.cpp

+// RUN: %check_clang_tidy %s google-readability-avoid-underscore-in-googletest-name %t
+
+#define TEST(test_case_name, test_name) void test_case_name##test_name()
+#define TEST_F(test_case_name, test_name) void test_case_name##test_name()
+#define TEST_P(test_case_name, test_name) void test_case_name##test_name()
+#define TYPED_TEST(test_case_name, test_name) void test_case_name##test_name()
+#define TYPED_TEST_P(test_case_name, test_name) void test_case_name##test_name()
+#define FRIEND_TEST(test_case_name, test_name) void test_case_name##test_name()
+
+TEST(TestCaseName, Illegal_TestName) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: avoid using "_" in test name "Illegal_TestName" according to Googletest FAQ [google-readability-avoid-underscore-in-googletest-name]
+
+TEST(TestCaseName, DISABLED_Illegal_TestName) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: avoid using "_" in test name "Illegal_TestName" according to Googletest FAQ [google-readability-avoid-underscore-in-googletest-name]
+TEST(TestCaseName, Illegal_Test_Name) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: avoid using "_" in test name "Illegal_Test_Name" according to Googletest FAQ [google-readability-avoid-underscore-in-googletest-name]
+TEST(Illegal_TestCaseName, TestName) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: avoid using "_" in test case name "Illegal_TestCaseName" according to Googletest FAQ [google-readability-avoid-underscore-in-googletest-name]
+TEST(Illegal_Test_CaseName, TestName) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: avoid using "_" in test case name "Illegal_Test_CaseName" according to Googletest FAQ [google-readability-avoid-underscore-in-googletest-name]
+TEST(Illegal_TestCaseName, Illegal_TestName) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: avoid using "_" in test case name "Illegal_TestCaseName" according to Googletest FAQ [google-readability-avoid-underscore-in-googletest-name]
+// CHECK-MESSAGES: :[[@LINE-2]]:28: warning: avoid using "_" in test name "Illegal_TestName" according to Googletest FAQ [google-readability-avoid-underscore-in-googletest-name]
+
+TEST_F(TestCaseFixtureName, Illegal_TestName) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:29: warning: avoid using "_" in test name "Illegal_TestName" according to Googletest FAQ [google-readability-avoid-underscore-in-googletest-name]
+TEST_F(TestCaseFixtureName, DISABLED_Illegal_Test_Name) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:29: warning: avoid using "_" in test name "Illegal_Test_Name" according to Googletest FAQ [google-readability-avoid-underscore-in-googletest-name]
+TEST_F(TestCaseFixtureName, Illegal_Test_Name) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:29: warning: avoid using "_" in test name "Illegal_Test_Name" according to Googletest FAQ [google-readability-avoid-underscore-in-googletest-name]
+
+TEST_F(Illegal_TestCaseFixtureName, TestName) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: avoid using "_" in test case name "Illegal_TestCaseFixtureName" according to Googletest FAQ [google-readability-avoid-underscore-in-googletest-name]
+TEST_F(Illegal_TestCaseFixtureName, Illegal_TestName) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: avoid using "_" in test case name "Illegal_TestCaseFixtureName" according to Googletest FAQ [google-readability-avoid-underscore-in-googletest-name]
+// CHECK-MESSAGES: :[[@LINE-2]]:37: warning: avoid using "_" in test name "Illegal_TestName" according to Googletest FAQ [google-readability-avoid-underscore-in-googletest-name]
+
+TEST_F(Illegal_Test_CaseFixtureName, TestName) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: avoid using "_" in test case name "Illegal_Test_CaseFixtureName" according to Googletest FAQ [google-readability-avoid-underscore-in-googletest-name]
+
+TEST_P(ParameterizedTestCaseFixtureName, Illegal_TestName) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:42: warning: avoid using "_" in test name "Illegal_TestName" according to Googletest FAQ [google-readability-avoid-underscore-in-googletest-name]
+TEST_P(ParameterizedTestCaseFixtureName, DISABLED_Illegal_TestName) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:42: warning: avoid using "_" in test name "Illegal_TestName" according to Googletest FAQ [google-readability-avoid-underscore-in-googletest-name]
+TEST_P(ParameterizedTestCaseFixtureName, Illegal_Test_Name) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:42: warning: avoid using "_" in test name "Illegal_Test_Name" according to Googletest FAQ [google-readability-avoid-underscore-in-googletest-name]
+
+TEST_P(Illegal_ParameterizedTestCaseFixtureName, TestName) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: avoid using "_" in test case name "Illegal_ParameterizedTestCaseFixtureName" according to Googletest FAQ [google-readability-avoid-underscore-in-googletest-name]
+TEST_P(Illegal_ParameterizedTestCaseFixtureName, Illegal_TestName) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: avoid using "_" in test case name "Illegal_ParameterizedTestCaseFixtureName" according to Googletest FAQ [google-readability-avoid-underscore-in-googletest-name]
+// CHECK-MESSAGES: :[[@LINE-2]]:50: warning: avoid using "_" in test name "Illegal_TestName" according to Googletest FAQ [google-readability-avoid-underscore-in-googletest-name]
+
+TEST_P(Illegal_Parameterized_TestCaseFixtureName, TestName) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: avoid using "_" in test case name "Illegal_Parameterized_TestCaseFixtureName" according to Googletest FAQ [google-readability-avoid-underscore-in-googletest-name]
+
+TYPED_TEST(TypedTestCaseName, Illegal_TestName) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:31: warning: avoid using "_" in test name "Illegal_TestName" according to Googletest FAQ [google-readability-avoid-underscore-in-googletest-name]
+TYPED_TEST(TypedTestCaseName, DISABLED_Illegal_TestName) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:31: warning: avoid using "_" in test name "Illegal_TestName" according to Googletest FAQ [google-readability-avoid-underscore-in-googletest-name]
+TYPED_TEST(TypedTestCaseName, Illegal_Test_Name) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:31: warning: avoid using "_" in test name "Illegal_Test_Name" according to Googletest FAQ [google-readability-avoid-underscore-in-googletest-name]
+
+TYPED_TEST(Illegal_TypedTestCaseName, TestName) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: avoid using "_" in test case name "Illegal_TypedTestCaseName" according to Googletest FAQ [google-readability-avoid-underscore-in-googletest-name]
+TYPED_TEST(Illegal_TypedTestCaseName, Illegal_TestName) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: avoid using "_" in test case name "Illegal_TypedTestCaseName" according to Googletest FAQ [google-readability-avoid-underscore-in-googletest-name]
+// CHECK-MESSAGES: :[[@LINE-2]]:39: warning: avoid using "_" in test name "Illegal_TestName" according to Googletest FAQ [google-readability-avoid-underscore-in-googletest-name]
+
+TYPED_TEST(Illegal_Typed_TestCaseName, TestName) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: avoid using "_" in test case name "Illegal_Typed_TestCaseName" according to Googletest FAQ [google-readability-avoid-underscore-in-googletest-name]
+
+TYPED_TEST_P(TypeParameterizedTestCaseName, Illegal_TestName) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:45: warning: avoid using "_" in test name "Illegal_TestName" according to Googletest FAQ [google-readability-avoid-underscore-in-googletest-name]
+TYPED_TEST_P(TypeParameterizedTestCaseName, DISABLED_Illegal_TestName) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:45: warning: avoid using "_" in test name "Illegal_TestName" according to Googletest FAQ [google-readability-avoid-underscore-in-googletest-name]
+TYPED_TEST_P(TypeParameterizedTestCaseName, Illegal_Test_Name) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:45: warning: avoid using "_" in test name "Illegal_Test_Name" according to Googletest FAQ [google-readability-avoid-underscore-in-googletest-name]
+
+TYPED_TEST_P(Illegal_TypeParameterizedTestCaseName, TestName) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: avoid using "_" in test case name "Illegal_TypeParameterizedTestCaseName" according to Googletest FAQ [google-readability-avoid-underscore-in-googletest-name]
+TYPED_TEST_P(Illegal_TypeParameterizedTestCaseName, Illegal_TestName) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: avoid using "_" in test case name "Illegal_TypeParameterizedTestCaseName" according to Googletest FAQ [google-readability-avoid-underscore-in-googletest-name]
+// CHECK-MESSAGES: :[[@LINE-2]]:53: warning: avoid using "_" in test name "Illegal_TestName" according to Googletest FAQ [google-readability-avoid-underscore-in-googletest-name]
+
+TYPED_TEST_P(Illegal_Type_ParameterizedTestCaseName, TestName) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: avoid using "_" in test case name "Illegal_Type_ParameterizedTestCaseName" according to Googletest FAQ [google-readability-avoid-underscore-in-googletest-name]
+
+// Underscores are allowed to disable a test with the DISABLED_ prefix.
+// https://github.com/google/googletest/blob/master/googletest/docs/faq.md#why-should-test-suite-names-and-test-names-not-contain-underscore
+TEST(TestCaseName, TestName) {}
+TEST(TestCaseName, DISABLED_TestName) {}
+
+TEST_F(TestCaseFixtureName, TestName) {}
+TEST_F(TestCaseFixtureName, DISABLED_TestName) {}
+
+TEST_P(ParameterizedTestCaseFixtureName, TestName) {}
+TEST_P(ParameterizedTestCaseFixtureName, DISABLED_TestName) {}
+
+TYPED_TEST(TypedTestName, TestName) {}
+TYPED_TEST(TypedTestName, DISABLED_TestName) {}
+
+TYPED_TEST_P(TypeParameterizedTestName, TestName) {}
+TYPED_TEST_P(TypeParameterizedTestName, DISABLED_TestName) {}
+
+FRIEND_TEST(FriendTest, Is_NotChecked) {}
+FRIEND_TEST(Friend_Test, IsNotChecked) {}
+FRIEND_TEST(Friend_Test, Is_NotChecked) {}