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

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

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

[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, DI

[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: Add check for underscores in googletest names.

[Kar Epker via Phabricator via cfe-commits]

karepker created this revision.
karepker added a reviewer: hokein.
Herald added a subscriber: mgorny.

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


Repository:
  rCTE Clang Tools Extra

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/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,107 @@
+// 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()
+
+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-und

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

[Kar Epker via Phabricator via cfe-commits]

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

Clean up comments in test file.


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/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,103 @@
+// 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()
+
+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-avo

[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) {}
+// CHECK-

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

[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]
+TEST_P(ParameterizedTestCaseFi

[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