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

[Kar Epker via Phabricator via cfe-commits]

karepker added a comment.

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.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56424



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

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


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56424



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Haojian Wu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL352183: [clang-tidy] Add check for underscores in googletest 
names. (authored by hokein, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D56424?vs=183437=183497#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D56424

Files:
  
clang-tools-extra/trunk/clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp
  
clang-tools-extra/trunk/clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.h
  clang-tools-extra/trunk/clang-tidy/google/CMakeLists.txt
  clang-tools-extra/trunk/clang-tidy/google/GoogleTidyModule.cpp
  clang-tools-extra/trunk/docs/ReleaseNotes.rst
  
clang-tools-extra/trunk/docs/clang-tidy/checks/google-readability-avoid-underscore-in-googletest-name.rst
  clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/trunk/test/clang-tidy/readability-avoid-underscore-in-googletest-name.cpp

Index: clang-tools-extra/trunk/clang-tidy/google/CMakeLists.txt
===
--- clang-tools-extra/trunk/clang-tidy/google/CMakeLists.txt
+++ clang-tools-extra/trunk/clang-tidy/google/CMakeLists.txt
@@ -3,6 +3,7 @@
 add_clang_library(clangTidyGoogleModule
   AvoidCStyleCastsCheck.cpp
   AvoidThrowingObjCExceptionCheck.cpp
+  AvoidUnderscoreInGoogletestNameCheck.cpp
   DefaultArgumentsCheck.cpp
   ExplicitConstructorCheck.cpp
   ExplicitMakePairCheck.cpp
Index: clang-tools-extra/trunk/clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp
@@ -0,0 +1,89 @@
+//===--- 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 
+
+#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 ,
+const 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.getPreprocessor().addPPCallbacks(

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

[Kar Epker via Phabricator via cfe-commits]

karepker updated this revision to Diff 183437.
karepker added a comment.

Rebasing against master.

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


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56424

Files:
  clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp
  clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.h
  clang-tidy/google/CMakeLists.txt
  clang-tidy/google/GoogleTidyModule.cpp
  docs/ReleaseNotes.rst
  
docs/clang-tidy/checks/google-readability-avoid-underscore-in-googletest-name.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/readability-avoid-underscore-in-googletest-name.cpp

Index: test/clang-tidy/readability-avoid-underscore-in-googletest-name.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-avoid-underscore-in-googletest-name.cpp
@@ -0,0 +1,108 @@
+// 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, 

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

[Haojian Wu via Phabricator via cfe-commits]

hokein accepted this revision.
hokein added a comment.
This revision is now accepted and ready to land.

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


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

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

[Kar Epker via Phabricator via cfe-commits]

karepker added a comment.

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.


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

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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

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?


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

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

[Kar Epker via Phabricator via cfe-commits]

karepker added a comment.

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.


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

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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

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(TestCase_Name, TestName) {}


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

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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

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


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

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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

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


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

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

[Kar Epker via Phabricator via cfe-commits]

karepker added a comment.

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?


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

[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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp:64
+  Check->diag(TestCaseNameToken->getLocation(),
+  "avoid using \"_\" in test case name \"%0\" according to "
+  "Googletest FAQ")

MyDeveloperDay wrote:
> 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.
> 
> 
> 
> 
minor correction. to my comment

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


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

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

[MyDeveloperDay via Phabricator via cfe-commits]

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

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

[Kar Epker via Phabricator via cfe-commits]

karepker marked 2 inline comments as done and an inline comment as not done.
karepker 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:
> 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.


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

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

[Kar Epker via Phabricator via cfe-commits]

karepker updated this revision to Diff 180727.
karepker marked 4 inline comments as done.
karepker added a comment.

Update release notes documentation to match check documentation more.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56424

Files:
  clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp
  clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.h
  clang-tidy/google/CMakeLists.txt
  clang-tidy/google/GoogleTidyModule.cpp
  docs/ReleaseNotes.rst
  
docs/clang-tidy/checks/google-readability-avoid-underscore-in-googletest-name.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/readability-avoid-underscore-in-googletest-name.cpp

Index: test/clang-tidy/readability-avoid-underscore-in-googletest-name.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-avoid-underscore-in-googletest-name.cpp
@@ -0,0 +1,108 @@
+// 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]

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

[Kar Epker via Phabricator via cfe-commits]

karepker added inline comments.



Comment at: docs/ReleaseNotes.rst:161
+
+  Checks that Googletest test and test case names do not contain an underscore,
+  which is prohibited by the Googletest FAQ.

Eugene.Zelenko wrote:
> Please synchronize with first statement in documentation.
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.


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

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

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/ReleaseNotes.rst:161
+
+  Checks that Googletest test and test case names do not contain an underscore,
+  which is prohibited by the Googletest FAQ.

Please synchronize with first statement in documentation.


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

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

[Kar Epker via Phabricator via cfe-commits]

karepker added a comment.

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.




Comment at: clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp:1
+//===--- AvoidUnderscoreInGoogletestNameCheck.cpp - 
clang-tidy-===//
+//

MyDeveloperDay wrote:
> nit: space after tidy and before ---
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.



Comment at: clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp:62
+std::string TestCaseName = PP->getSpelling(*TestCaseNameToken);
+if (TestCaseName.find('_') != std::string::npos) {
+  Check->diag(TestCaseNameToken->getLocation(),

JonasToth wrote:
> 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.
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.



Comment at: clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp:64
+  Check->diag(TestCaseNameToken->getLocation(),
+  "avoid using \"_\" in test case name \"%0\" according to "
+  "Googletest FAQ")

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.


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

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

[Kar Epker via Phabricator via cfe-commits]

karepker updated this revision to Diff 180716.
karepker marked 15 inline comments as done.
karepker added a comment.

Address comments.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56424

Files:
  clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp
  clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.h
  clang-tidy/google/CMakeLists.txt
  clang-tidy/google/GoogleTidyModule.cpp
  docs/ReleaseNotes.rst
  
docs/clang-tidy/checks/google-readability-avoid-underscore-in-googletest-name.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/readability-avoid-underscore-in-googletest-name.cpp

Index: test/clang-tidy/readability-avoid-underscore-in-googletest-name.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-avoid-underscore-in-googletest-name.cpp
@@ -0,0 +1,108 @@
+// 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) {}
+// 

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

[MyDeveloperDay via Phabricator via cfe-commits]

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

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



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

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

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp:46
+auto IdentifierInfo = MacroNameToken.getIdentifierInfo();
+if (!IdentifierInfo) {
+  return;

Please elide braces.


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

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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp:23
+
+constexpr char kDisabledTestPrefix[] = "DISABLED_";
+

Please use `llvm::StringLiteral`



Comment at: clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp:45
+const MacroArgs *Args) override {
+auto IdentifierInfo = MacroNameToken.getIdentifierInfo();
+if (!IdentifierInfo) {

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.



Comment at: clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp:49
+}
+const StringRef MacroName = IdentifierInfo->getName();
+if (!isGoogletestTestMacro(MacroName)) {

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



Comment at: clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp:50
+const StringRef MacroName = IdentifierInfo->getName();
+if (!isGoogletestTestMacro(MacroName)) {
+  return;

You can ellide the braces and merge these two ifs.



Comment at: clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp:58
+const Token *TestNameToken = Args->getUnexpArgument(1);
+if (!TestCaseNameToken || !TestNameToken) {
+  return;

you can ellide the braces



Comment at: clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp:62
+std::string TestCaseName = PP->getSpelling(*TestCaseNameToken);
+if (TestCaseName.find('_') != std::string::npos) {
+  Check->diag(TestCaseNameToken->getLocation(),

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.



Comment at: clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp:64
+  Check->diag(TestCaseNameToken->getLocation(),
+  "avoid using \"_\" in test case name \"%0\" according to "
+  "Googletest FAQ")

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

ellide braces



Comment at: clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp:69
+
+std::string TestName_maybe_disabled = PP->getSpelling(*TestNameToken);
+StringRef TestName = TestName_maybe_disabled;

Please use camel-case.


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

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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp:1
+//===--- AvoidUnderscoreInGoogletestNameCheck.cpp - 
clang-tidy-===//
+//

nit: space after tidy and before ---



Comment at: clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.h:1
+//===--- AvoidUnderscoreInGoogletestNameCheck.h - 
clang-tidy---===//
+//

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


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

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

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

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




Comment at: clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp:72
+TestName.consume_front(kDisabledTestPrefix);
+if (TestName.find('_') != std::string::npos) {
+  Check->diag(TestNameToken->getLocation(),

nit: using `Test.contains`



Comment at: clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.h:10
+
+#ifndef 
LLVM_TOOLS_CLANG_TOOLS_EXTRA_CLANG_TIDY_CLANG_TIDY_READABILITY_UNDERSCORE_IN_GOOGLETEST_TEST_MACRO_H_
+#define 
LLVM_TOOLS_CLANG_TOOLS_EXTRA_CLANG_TIDY_CLANG_TIDY_READABILITY_UNDERSCORE_IN_GOOGLETEST_TEST_MACRO_H_

nit: the header guard should be 
`LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_GOOGLE_AVOIDUNDERSCOREINGOOGLETESTNAMECHECK_H`



Comment at: 
docs/clang-tidy/checks/google-readability-avoid-underscore-in-googletest-name.rst:3
+
+google-readability-avoid-underscore-in-googletest-name
+==

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



Comment at: 
docs/clang-tidy/checks/google-readability-avoid-underscore-in-googletest-name.rst:7
+Checks whether there are underscores in googletest test and test case names in
+test macros, not including the ``FRIEND_TEST`` macro.
+

maybe list all test macros that the check detects?


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

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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

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.


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