Introduce a new check to upgrade user code based on API changes in Googletest.
The check finds uses of old Googletest APIs with "case" in their name and replaces them with the new APIs named with "suite".
Paths
| Differential D62977
[clang-tidy]: Google: new check 'google-upgrade-googletest-case' ClosedPublic Authored by astrelni on Jun 6 2019, 1:09 PM.
Details Summary Introduce a new check to upgrade user code based on API changes in Googletest. The check finds uses of old Googletest APIs with "case" in their name and replaces them with the new APIs named with "suite".
Diff Detail Event TimelineHerald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJun 6 2019, 1:09 PM Comment Actions I think will be good idea to replace upgrade with modernize to be consistent with similar checks in other module.
Comment Actions You need to upload the entire patch, not as compared to the previous patch. Comment Actions
I will work on changing this in the next diff update, if your opinion holds. The context for currently using "upgrade" is: a) The modernize module seems to contain checks that are geared at using more modern language features as opposed to API breaking changes. With some exceptions, they seem to be about changing from old to new practice while both ways remain valid code. The goal of this check is to take code that will break with future googletest API changes and turn it into working code, allowing users to upgrade the version of googletest they use. Maybe upgrade is not the best term, but I don't think modernize is a better fit. I don't feel very strongly, just thought it would be good to provide the context. Please let me know how you'd like me to proceed. Comment Actions Generally this LGTM.
Comment Actions
I don't believe this question was answered. This revision now requires changes to proceed.Jun 24 2019, 1:34 PM
Comment Actions
Sorry, the original comment got lost between me updating two patches as I noticed I did it wrong without seeing your comment. Unfortunately there are no versioning macros in googletest, so I'm not sure how to keep this check from providing warnings if it is run while depending on a version that is too early. The new names are in master and will be part of an upcoming version 1.9 release. I have tried to update the documentation to clarify which version of googletest this intended for. Please let me know how you think we should proceed.
Comment Actions
I'm not fully current on what can and can't be done in clang-tidy, but i suppose if the This revision now requires changes to proceed.Jun 26 2019, 8:51 AM astrelni marked 4 inline comments as done. Comment Actions
Yes, makes sense. Let me know what you think of the approach I've added in the latest diff. I think it is sufficient to check for the existence of one of the new macros in the right file and one new method in each matched class. Comment Actions
The tests as they are cover the positive case in that they would not show warning or fixes if we didn't find the replacements. The only way to test the negative is to make a second test with a second set of mock googletest headers that declare things in the v1.8 way. Is this what you would prefer? Comment Actions
Yep
If that is what it takes to get the test coverage, i suppose so. Comment Actions
What do you think of this? Comment Actions Looks reasonable. I did not review the check itself though.
Comment Actions
The nosuite test was a strict subset. I've combined them with a few lines that needed to be turned via preprocessor branches. Thank you, I actually wasn't aware that these tests can have multiple RUN lines.
This revision is now accepted and ready to land.Jun 28 2019, 1:13 PM Comment Actions Committed in r367263. Thanks for the change, sorry about the delay. I'll go watch the bots now.
Revision Contents
Diff 206889 clang-tools-extra/clang-tidy/google/CMakeLists.txt
clang-tools-extra/clang-tidy/google/GoogleTidyModule.cpp
clang-tools-extra/clang-tidy/google/UpgradeGoogletestCaseCheck.h
clang-tools-extra/clang-tidy/google/UpgradeGoogletestCaseCheck.cpp
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/google-upgrade-googletest-case.rst
clang-tools-extra/docs/clang-tidy/checks/list.rst
clang-tools-extra/test/clang-tidy/Inputs/gtest/gtest-typed-test.h
clang-tools-extra/test/clang-tidy/Inputs/gtest/gtest.h
clang-tools-extra/test/clang-tidy/Inputs/gtest/nosuite/gtest/gtest-typed-test.h
clang-tools-extra/test/clang-tidy/Inputs/gtest/nosuite/gtest/gtest.h
clang-tools-extra/test/clang-tidy/google-upgrade-googletest-case.cpp
llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/google/BUILD.gn
|
Just tried building this. I think you're missing an include for UpgradeGoogletestCaseCheck.