[clang-tidy] Ignore DISABLED_ in test suite name in google-avoid-underscore-in-googletest-name

[carlosgalvezp]

Summary

Test suite name can also be disabled with DISABLED_, not just
the test case name.

Fix also broken link in the test that refers to the explanation
as to why underscores may not be used.

[carlosgalvezp]

Rebase

[carlosgalvezp]

Add to release notes.

[carlosgalvezp]

Update commit message

[carlosgalvezp]

Remove excessive newline

[Eugene.Zelenko]

clang-tools-extra/docs/ReleaseNotes.rst

237

Please keep alphabetical order (by check name) in this section.

[PiotrZSL]

clang-tools-extra/test/clang-tidy/checkers/google/avoid-underscore-in-googletest-name.cpp

90

Would be good if this link would exist also in documentation for this check.

[LegalizeAdulthood]

clang-tools-extra/test/clang-tidy/checkers/google/avoid-underscore-in-googletest-name.cpp

90

My experience has been that these external links move around with gtest and are a source of churn.

[carlosgalvezp]

clang-tools-extra/docs/ReleaseNotes.rst

237

I was planning to do that but noticed that the alphabetical order is already broken. It seems to be a source of friction and there's no official documentation that states it should be done like that, so I can understand if it gets broken often. Do you know if this is documented somewhere? If not, do we see value in keeping this convention? I suppose now we would need an NFC patch to fix the order again, causing churn.

clang-tools-extra/test/clang-tidy/checkers/google/avoid-underscore-in-googletest-name.cpp

90

Good point, I can add on a separate NFC patch :) I believe if we use the github.io (instead of github.com) the links can be a bit more stable.

[PiotrZSL]

clang-tools-extra/docs/ReleaseNotes.rst

237

I run into same issue also. I would say, let leave it as it is, and fix it with one commit at the end of release.

[carlosgalvezp]

clang-tools-extra/docs/ReleaseNotes.rst

237

Good idea, let's do that!

[Eugene.Zelenko]

clang-tools-extra/docs/ReleaseNotes.rst

237

Often it's also broken after rebases which may be automatic.

[Eugene.Zelenko]

clang-tools-extra/docs/ReleaseNotes.rst

237

Anyway, some kind of order is much better than disorder.

[carlosgalvezp]

clang-tools-extra/docs/ReleaseNotes.rst

237

Definitely. Could we stick to some simple convention? For example always append or prepend to the list of modifications to checks. Then before release we put up a patch for reordering.

[Eugene.Zelenko]

clang-tools-extra/docs/ReleaseNotes.rst

237

I think it will be harder to reader. Sorting by check name is much better in this respect. And this was used in many releases.

[carlosgalvezp]

clang-tools-extra/docs/ReleaseNotes.rst

237

To clarify, what I mean is:

• Apply a simple convention (e.g. append or prepend to the list) while developing.
• Right before creating a release, put up a patch to sort alphabetically. Then it will be easy to read for users when it's released.

Or do you mean that the list shall be alphabetically sorted at all times?

[Eugene.Zelenko]

clang-tools-extra/docs/ReleaseNotes.rst

237

It'll be much easier to sort partially sorted list at release time than completely unsorted.

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

void MacroExpands(const Token &MacroNameToken,
     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)
+    std::string TestCaseNameMaybeDisabled = PP->getSpelling(*TestCaseNameToken);
+    StringRef TestCaseName = TestCaseNameMaybeDisabled;
+    TestCaseName.consume_front(KDisabledTestPrefix);
+    if (TestCaseName.contains('_'))
       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);

clang-tools-extra/docs/ReleaseNotes.rst

- Improved :doc:`bugprone-use-after-move
   <clang-tidy/checks/bugprone/use-after-move>` to understand that there is a
   sequence point between designated initializers.
 
 - Fixed an issue in :doc:`readability-identifier-naming
   <clang-tidy/checks/readability/identifier-naming>` when specifying an empty
   string for ``Prefix`` or ``Suffix`` options could result in the style not
   being used.
 
+- Fixed an issue in :doc:`google-avoid-underscore-in-googletest-name
+  <clang-tidy/checks/google/avoid-underscore-in-googletest-name>` when using
+  ``DISABLED_`` in the test suite name.
+
 Removed checks
 ^^^^^^^^^^^^^^
 
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 

clang-tools-extra/test/clang-tidy/checkers/google/avoid-underscore-in-googletest-name.cpp

 TYPED_TEST_P(Illegal_TypeParameterizedTestCaseName, Illegal_TestName) {}
 // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: avoid using "_" in test case name "Illegal_TypeParameterizedTestCaseName" according to Googletest FAQ [google-readability-avoid-underscore-in-googletest-name]
 // CHECK-MESSAGES: :[[@LINE-2]]:53: warning: avoid using "_" in test name "Illegal_TestName" according to Googletest FAQ [google-readability-avoid-underscore-in-googletest-name]
 
 TYPED_TEST_P(Illegal_Type_ParameterizedTestCaseName, TestName) {}
 // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: avoid using "_" in test case name "Illegal_Type_ParameterizedTestCaseName" according to Googletest FAQ [google-readability-avoid-underscore-in-googletest-name]
 
 // Underscores are allowed to disable a test with the DISABLED_ prefix.
-// https://github.com/google/googletest/blob/master/googletest/docs/faq.md#why-should-test-suite-names-and-test-names-not-contain-underscore
+// https://google.github.io/googletest/faq.html#why-should-test-suite-names-and-test-names-not-contain-underscore
 TEST(TestCaseName, TestName) {}
 TEST(TestCaseName, DISABLED_TestName) {}
+TEST(DISABLED_TestCaseName, TestName) {}
+TEST(DISABLED_TestCaseName, DISABLED_TestName) {}
 
 TEST_F(TestCaseFixtureName, TestName) {}
 TEST_F(TestCaseFixtureName, DISABLED_TestName) {}
+TEST_F(DISABLED_TestCaseFixtureName, TestName) {}
+TEST_F(DISABLED_TestCaseFixtureName, DISABLED_TestName) {}
 
 TEST_P(ParameterizedTestCaseFixtureName, TestName) {}
 TEST_P(ParameterizedTestCaseFixtureName, DISABLED_TestName) {}
+TEST_P(DISABLED_ParameterizedTestCaseFixtureName, TestName) {}
+TEST_P(DISABLED_ParameterizedTestCaseFixtureName, DISABLED_TestName) {}
 
 TYPED_TEST(TypedTestName, TestName) {}
 TYPED_TEST(TypedTestName, DISABLED_TestName) {}
+TYPED_TEST(DISABLED_TypedTestName, TestName) {}
+TYPED_TEST(DISABLED_TypedTestName, DISABLED_TestName) {}
 
 TYPED_TEST_P(TypeParameterizedTestName, TestName) {}
 TYPED_TEST_P(TypeParameterizedTestName, DISABLED_TestName) {}
+TYPED_TEST_P(DISABLED_TypeParameterizedTestName, TestName) {}
+TYPED_TEST_P(DISABLED_TypeParameterizedTestName, DISABLED_TestName) {}
 
 FRIEND_TEST(FriendTest, Is_NotChecked) {}
 FRIEND_TEST(Friend_Test, IsNotChecked) {}
 FRIEND_TEST(Friend_Test, Is_NotChecked) {}