test: fix several clang-tidy warnings from #5811#5826
test: fix several clang-tidy warnings from #5811#5826jmarantz merged 8 commits intoenvoyproxy:masterfrom
Conversation
…st and variants. Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
htuch
left a comment
There was a problem hiding this comment.
Are all of these set to yield failures on regular sized builds? How come clang-tidy doesn't scale?
| )EOF"; | ||
|
|
||
| SetUpTest(json); | ||
| setUpTest(json); |
There was a problem hiding this comment.
There's a weird lexical/grammar thing going on here with setUp; I would write it as setupTest. I think you are paying homage to gtest's SetUp, which is also pretty weird IMHO; is it a pun on "up"?
There was a problem hiding this comment.
:) I was just changing one character in an already-existing test-method to reflect the fact that this is a locally defined class method, not associated with gtest at all (and easy to confuse with it). So it should use Envoy's convention of a lower-cased first character.
But you're right, this looks weird, so I might as well clean this up and change it to setupTest. Any other issues?
There was a problem hiding this comment.
Nup, that's all good. I'd like to make more of these warnings act as errors though, I always prefer cleanups that don't need to ever be done again :) Thanks for doing the grungey work in this PR.
|
RE why does clang-tidy not scale? Not sure. It's processing diffs very slowly, and on a large PR there are a metric ton of diffs. @lizan ? |
|
And none of these were failing individually. They were just warnings. |
Signed-off-by: Joshua Marantz <jmarantz@google.com>
|
Please merge master to pick up #5827. |
…y#5826) * test: fix several clang-tidy warnings from envoyproxy#5811 Signed-off-by: Joshua Marantz <jmarantz@google.com> Signed-off-by: Fred Douglas <fredlas@google.com>
Description: #5811 failed its clang-tidy test just due to its size, but in examining the results I found a bunch of reasonable warnings to fix, which I did thinking it might make clang-tidy CI pass. However the root cause of the CI failure was just size, and not actual errors.
Risk Level: low
Testing: //test/..., clang-tidy
Docs Changes: n/a
Release Notes: n/a