test: add a common test base class

[jmarantz]

Description: There's been a few cases where I wish I could get a global teardown/setup hook but Envoy lacked a common test base-class. This giant PR adds one. Sorry it's incredibly huge...I don't know if it's worth breaking up. This was mostly done with sed scripts with minor manual fixups where needed, and no functional logic was changed.

Currently in the common per-test teardown we just check that all the test-scoped singletons are freed. But there may be other uses for this hook.
Risk Level: low
Testing: //test/..., in fastbuild and tsan
Docs Changes: n/a
Release Notes: n/a

[jmarantz]

Note: after this PR there are still 879 instances of TEST( in 164 _test.cc files which would need to be cleaned up to make the global common TestBase actually work. That can be done in a follow-up PR (with appropriate check_format.py tweak to make sure that doesn't recur).

[mattklein123]

Currently in the common per-test teardown we just check that all the test-scoped singletons are freed. But there may be other uses for this hook.

The diff is so big that I can't easily find this. Can you link it or potentially just do a PR where we review the base class and a single test? I don't have a strong opinion about this if people think having this is worth it (as long as we have it checked by the formatter). Also it looks like clang-tidy is broken.

/wait

[lizan]

feel free to ignore the broken clang-tidy build, it is because the diff is too big (across too many files)

[jmarantz]

The new class is:
header: https://github.com/envoyproxy/envoy/blob/00f86e1d2f18864339d3ca0c99f1a19383cd8d5b/test/test_common/test_base.h
impl:
https://github.com/envoyproxy/envoy/blob/00f86e1d2f18864339d3ca0c99f1a19383cd8d5b/test/test_common/test_base.cc

This gets implicitly linked implicitly with envoy_cc_test_library via a new dependency in envoy_build_system.bzl:

  repository + "//test/test_common:test_base",

An example test change is in

envoy/test/common/buffer/owned_impl_test.cc

Line 18 in 00f86e1

class OwnedImplTest : public TestBase {

The bulk of the changes are pretty simple like that. The other changes are almost 100% from a script that does a file-tree string-replacement. From my shell history there were several of these:

sed_multi_rep '#include "gtest/gtest.h"' '#include "test/test_common/test_base.h"' cc h
sed_multi_rep " ::testing::Test" " TestBase" cc h
sed_multi_rep " testing::Test" " TestBase" cc h
sed_multi_rep  TestBaseParamInfo ::testing::TestParamInfo cc h
sed_multi_rep  TestBaseInfo ::testing::TestInfo cc h

After this there were a few tweaks required to deal with patterns like using testing::Test; which becamse using TestBase; which of course makes no sense.

@lizan you are right I think clang-tidy is really hard on this PR. I have been iterating on it locally and it takes hours. There were a lot of legacy clang-tidy violations which this PR awakened. I've fixed a bunch of them and committed them locally, but did not push the change yet. It'd be better to merge this first if you think this is worth it, as it's a real merge-conflict magnet :)

I'm not too invested in this; IMO it's a good hook to have.

[jmarantz]

For the record I went through all the clang-tidy warnings and fixed most of them. I could find no errors, so I guess in CI it just failed due to excessive size. Some of the clang-tidy suggestions are wrong: IMO:

• executing the suggestions results in compilable errors: replacing {} with = default in constructors when there are arg-dependent initialization values.

Others are annoying:

• changing a const std::string& arg to an rvalue ref with std::move, which seems not general to all call-sites which might not want to give up their ownership.

I will send a follow-up PR with the clang-tidy changes that made sense after this is merged.

[mattklein123]

@jmarantz thanks for the links. If you want to go with this and are willing to do the work to replace TEST somehow and also wire up the format checker, no objections from me. Do you want to go ahead and merge this?

[jmarantz]

Yup will merge but need approval I think per tool. Will follow up with ^TEST( replacement. Should be pretty straightforward.