Page MenuHomePhabricator

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

astrelni created this revision.Jun 6 2019, 1:09 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJun 6 2019, 1:09 PM

I think will be good idea to replace upgrade with modernize to be consistent with similar checks in other module.

clang-tools-extra/clang-tidy/google/UpgradeGoogletestCaseCheck.cpp
171

Function should be static instead on placed in anonymous namespace. See LLVM Coding Guidelines. Same for other places.

clang-tools-extra/clang-tidy/google/UpgradeGoogletestCaseCheck.h
14

Please remove unnecessary empty line.

clang-tools-extra/docs/clang-tidy/checks/google-upgrade-googletest-case.rst
14

Please remove space before colon.

astrelni updated this revision to Diff 204312.Jun 12 2019, 9:37 AM

Style updates.

astrelni updated this revision to Diff 204313.Jun 12 2019, 9:40 AM
astrelni marked 2 inline comments as done.

Fix space.

You need to upload the entire patch, not as compared to the previous patch.
Without seeing the tests - what version checks does this have?
It shouldn't fire if the googletest version is the one before that rename.

astrelni marked an inline comment as done.Jun 12 2019, 9:55 AM

I think will be good idea to replace upgrade with modernize to be consistent with similar checks in other module.

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.
b) The check is meant to be consistent with abseil-upgrade-duration-conversions, which is aimed to upgrade code before API breaking changes. So if we change this check, I would like to change the Abseil one as well. Maybe this would be better done together in a follow-up patch?

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.

astrelni updated this revision to Diff 204317.Jun 12 2019, 10:01 AM

Fix mistake not uploading full diff

astrelni updated this revision to Diff 204318.Jun 12 2019, 10:04 AM

Fix diff issues again

Generally this LGTM.
I'll take another pass after the comments are addressed.

clang-tools-extra/clang-tidy/google/UpgradeGoogletestCaseCheck.cpp
20

Could you rename this to better represent what the diagnostic text is saying, rather than just being diagnostic text?
I think it'll help readability below.

Why not put this inside the unnamed namespace instead?

Same with the function below.

202

Instead of walking the tree, can't you ask if the Node is dependent in some way?

295

Maybe add an assertion for this comment?

clang-tools-extra/clang-tidy/google/UpgradeGoogletestCaseCheck.h
24

https all the things!

clang-tools-extra/test/clang-tidy/google-upgrade-googletest-case.cpp
10

You probably don't have to test for the full message every time. but I don't know it matters either way.

lebedev.ri requested changes to this revision.Jun 24 2019, 1:34 PM

Without seeing the tests - what version checks does this have?
It shouldn't fire if the googletest version is the one before that rename.

I don't believe this question was answered.

This revision now requires changes to proceed.Jun 24 2019, 1:34 PM
EricWF added inline comments.Jun 24 2019, 1:49 PM
clang-tools-extra/clang-tidy/google/GoogleTidyModule.cpp
31

Just tried building this. I think you're missing an include for UpgradeGoogletestCaseCheck.

aaron.ballman added inline comments.Jun 25 2019, 5:08 AM
clang-tools-extra/clang-tidy/google/GoogleTidyModule.cpp
42–43

Please keep this sorted alphabetically.

clang-tools-extra/clang-tidy/google/UpgradeGoogletestCaseCheck.cpp
21

Should this be spelled Google Test instead?

108–109

No need to register this unless in C++ mode.

astrelni updated this revision to Diff 206679.Jun 26 2019, 8:22 AM
astrelni marked 13 inline comments as done.Jun 26 2019, 8:45 AM

Without seeing the tests - what version checks does this have?
It shouldn't fire if the googletest version is the one before that rename.

I don't believe this question was answered.

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.

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

Thanks for catching this, was having issues in the diff.

clang-tools-extra/clang-tidy/google/UpgradeGoogletestCaseCheck.cpp
20

I had a previous comment that llvm coding guidelines prefer static and like to keep anonymous namespaces as small as possible and only around type declarations.

21

Thanks for spotting this. There is some inconsistency within the project. The top of the github repository is named "Googletest" with directories being "googletest" instead of "google_test", but documentation does use "Google Test".

For now I have changed the message to use "Google Test", but have kept "googletest" in the check name. Let me know if you would prefer "GoogleTest" and "google-test" elsewhere as well.

202

I think it is a trade-off. Given where this gets called, what we are really looking to gather are all places that are inside a template that are not instantiation dependent. If we only use a check against instantiation dependent, then the hash set gets a lot bigger because it holds every matched location and is not filtered down to the ones inside templates. Let me know if you think I should explore that route instead.

lebedev.ri requested changes to this revision.Jun 26 2019, 8:51 AM

Without seeing the tests - what version checks does this have?
It shouldn't fire if the googletest version is the one before that rename.

I don't believe this question was answered.

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.

I'm not fully current on what can and can't be done in clang-tidy, but i suppose if the
check has matched something, then it must mean that the googletest headers were parsed.
Can you perhaps look in the AST that the new names are present?
E.g. the new macros, specified in getNewMacroName().

This revision now requires changes to proceed.Jun 26 2019, 8:51 AM
astrelni updated this revision to Diff 206720.Jun 26 2019, 11:46 AM
astrelni marked 4 inline comments as done.

Without seeing the tests - what version checks does this have?
It shouldn't fire if the googletest version is the one before that rename.

I don't believe this question was answered.

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.

I'm not fully current on what can and can't be done in clang-tidy, but i suppose if the
check has matched something, then it must mean that the googletest headers were parsed.
Can you perhaps look in the AST that the new names are present?
E.g. the new macros, specified in getNewMacroName().

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.

It sounds correct, but i don't see accompanying test changes, so i can't be sure.

It sounds correct, but i don't see accompanying test changes, so i can't be sure.

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?

It sounds correct, but i don't see accompanying test changes, so i can't be sure.

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.

Yep

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?

If that is what it takes to get the test coverage, i suppose so.

astrelni updated this revision to Diff 206855.Jun 27 2019, 6:55 AM

It sounds correct, but i don't see accompanying test changes, so i can't be sure.

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.

Yep

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?

If that is what it takes to get the test coverage, i suppose so.

What do you think of this?

lebedev.ri resigned from this revision.Jun 27 2019, 7:49 AM

Looks reasonable. I did not review the check itself though.
Are test/clang-tidy/google-upgrade-googletest-case-nosuite.cpp and test/clang-tidy/google-upgrade-googletest-case.cpp identical other than the included header and expected output?
I'd recommend to condense it into a single file, and just have two RUN lines each one checking different message prefixes

clang-tools-extra/clang-tidy/google/UpgradeGoogletestCaseCheck.h
35

Have you tried llvm::DenseSet instead?
This may not matter *here*, but std::unordered_set usually results in horrible perf.

clang-tools-extra/test/clang-tidy/google-upgrade-googletest-case-nosuite.cpp
9 ↗(On Diff #206855)

Hm?

astrelni updated this revision to Diff 206889.Jun 27 2019, 11:03 AM
astrelni marked 4 inline comments as done.Jun 27 2019, 11:07 AM

Looks reasonable. I did not review the check itself though.
Are test/clang-tidy/google-upgrade-googletest-case-nosuite.cpp and test/clang-tidy/google-upgrade-googletest-case.cpp identical other than the included header and expected output?
I'd recommend to condense it into a single file, and just have two RUN lines each one checking different message prefixes

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.

clang-tools-extra/clang-tidy/google/UpgradeGoogletestCaseCheck.h
35

Thanks, I wasn't aware of what is available.

clang-tools-extra/test/clang-tidy/google-upgrade-googletest-case-nosuite.cpp
9 ↗(On Diff #206855)

I added a comment to better explain this in the combined test. check_clang_tidy.py asserts that there is at least one message, fix or note comment present in the file. If there isn't one, the script returns without even running the test. I couldn't see an option to pass that would turn of this check, please let me know if you are aware of one.

EricWF accepted this revision.Jun 28 2019, 1:13 PM

I don't have any other comments.

LGTM.

This revision is now accepted and ready to land.Jun 28 2019, 1:13 PM
EricWF closed this revision.Jul 29 2019, 2:40 PM

Committed in r367263. Thanks for the change, sorry about the delay.

I'll go watch the bots now.

tsatsulin marked an inline comment as done.Aug 30 2019, 4:22 AM
tsatsulin added a subscriber: tsatsulin.
tsatsulin added inline comments.
clang-tools-extra/clang-tidy/google/UpgradeGoogletestCaseCheck.cpp
83

The body of this method is the same as Ifdef. Please check.