diff --git a/ci/run_clang_tidy.sh b/ci/run_clang_tidy.sh index 2cc20142023e5..e9df93bfcb652 100755 --- a/ci/run_clang_tidy.sh +++ b/ci/run_clang_tidy.sh @@ -16,14 +16,20 @@ echo "build ${BAZEL_BUILD_OPTIONS}" >> .bazelrc mv ./compile_commands.json "${ENVOY_SRCDIR}/compile_commands.json" cd "${ENVOY_SRCDIR}" +# Do not run incremental clang-tidy on check_format testdata files. +function exclude_testdata() { + grep -v tools/testdata/check_format/ +} + if [[ "${RUN_FULL_CLANG_TIDY}" == 1 ]]; then echo "Running full clang-tidy..." run-clang-tidy-7 elif [[ -z "${CIRCLE_PR_NUMBER}" && "$CIRCLE_BRANCH" == "master" ]]; then echo "On master branch, running clang-tidy-diff against previous commit..." - git diff HEAD^ | clang-tidy-diff-7.py -p 1 + git diff HEAD^ | exclude_testdata | clang-tidy-diff-7.py -p 1 else echo "Running clang-tidy-diff against master branch..." git fetch https://github.com/envoyproxy/envoy.git master - git diff $(git merge-base HEAD FETCH_HEAD)..HEAD | clang-tidy-diff-7.py -p 1 + git diff $(git merge-base HEAD FETCH_HEAD)..HEAD | exclude_testdata | \ + clang-tidy-diff-7.py -p 1 fi diff --git a/test/BUILD b/test/BUILD index f7044560db8fa..d1e28211c9e40 100644 --- a/test/BUILD +++ b/test/BUILD @@ -29,6 +29,7 @@ envoy_cc_test_library( "//source/common/event:libevent_lib", "//test/mocks/access_log:access_log_mocks", "//test/test_common:environment_lib", + "//test/test_common:global_lib", "//test/test_common:printers_lib", ] + select({ "//bazel:disable_signal_trace": [], diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index b04ddcf264078..e7bd2a26276c1 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -55,12 +55,11 @@ using testing::Ref; using testing::Return; using testing::ReturnRef; using testing::Sequence; -using testing::Test; namespace Envoy { namespace Http { -class HttpConnectionManagerImplTest : public Test, public ConnectionManagerConfig { +class HttpConnectionManagerImplTest : public testing::Test, public ConnectionManagerConfig { public: struct RouteConfigProvider : public Router::RouteConfigProvider { RouteConfigProvider(TimeSource& time_source) : time_source_(time_source) {} diff --git a/test/common/network/connection_impl_test.cc b/test/common/network/connection_impl_test.cc index a176458e250f6..fe6ea3a672067 100644 --- a/test/common/network/connection_impl_test.cc +++ b/test/common/network/connection_impl_test.cc @@ -36,7 +36,6 @@ using testing::Return; using testing::SaveArg; using testing::Sequence; using testing::StrictMock; -using testing::Test; namespace Envoy { namespace Network { diff --git a/test/common/tracing/http_tracer_impl_test.cc b/test/common/tracing/http_tracer_impl_test.cc index c05ebf0dac08d..99e07658df811 100644 --- a/test/common/tracing/http_tracer_impl_test.cc +++ b/test/common/tracing/http_tracer_impl_test.cc @@ -29,7 +29,6 @@ using testing::NiceMock; using testing::Return; using testing::ReturnPointee; using testing::ReturnRef; -using testing::Test; namespace Envoy { namespace Tracing { @@ -324,7 +323,7 @@ TEST(HttpNullTracerTest, BasicFunctionality) { EXPECT_NE(nullptr, span_ptr->spawnChild(config, "foo", SystemTime())); } -class HttpTracerImplTest : public Test { +class HttpTracerImplTest : public testing::Test { public: HttpTracerImplTest() { driver_ = new MockDriver(); diff --git a/test/extensions/filters/network/thrift_proxy/decoder_test.cc b/test/extensions/filters/network/thrift_proxy/decoder_test.cc index b7abc2b6d7027..2808a799e84bf 100644 --- a/test/extensions/filters/network/thrift_proxy/decoder_test.cc +++ b/test/extensions/filters/network/thrift_proxy/decoder_test.cc @@ -26,7 +26,6 @@ using testing::Return; using testing::ReturnRef; using testing::SetArgReferee; using testing::StrictMock; -using testing::Test; using testing::TestParamInfo; using testing::TestWithParam; using testing::Values; @@ -206,7 +205,7 @@ INSTANTIATE_TEST_CASE_P(NonValueProtocolStates, DecoderStateMachineNonValueTest, ProtocolState::SetBegin, ProtocolState::SetEnd), protoStateParamToString); -class DecoderStateMachineTest : public DecoderStateMachineTestBase, public Test {}; +class DecoderStateMachineTest : public DecoderStateMachineTestBase, public testing::Test {}; class DecoderStateMachineValueTest : public DecoderStateMachineTestBase, public TestWithParam {}; diff --git a/test/extensions/filters/network/thrift_proxy/route_matcher_test.cc b/test/extensions/filters/network/thrift_proxy/route_matcher_test.cc index ffc9093de7227..e4cbff2dcc935 100644 --- a/test/extensions/filters/network/thrift_proxy/route_matcher_test.cc +++ b/test/extensions/filters/network/thrift_proxy/route_matcher_test.cc @@ -10,7 +10,6 @@ #include "gtest/gtest.h" using testing::_; -using testing::Test; namespace Envoy { namespace Extensions { diff --git a/test/extensions/filters/network/thrift_proxy/router_test.cc b/test/extensions/filters/network/thrift_proxy/router_test.cc index d35809d97f678..de2d084c5708a 100644 --- a/test/extensions/filters/network/thrift_proxy/router_test.cc +++ b/test/extensions/filters/network/thrift_proxy/router_test.cc @@ -29,7 +29,6 @@ using testing::NiceMock; using testing::Ref; using testing::Return; using testing::ReturnRef; -using testing::Test; using testing::TestWithParam; using testing::Values; @@ -330,14 +329,12 @@ class ThriftRouterTestBase { NiceMock upstream_connection_; }; -class ThriftRouterTest : public ThriftRouterTestBase, public Test { +class ThriftRouterTest : public ThriftRouterTestBase, public testing::Test { public: - ThriftRouterTest() {} }; class ThriftRouterFieldTypeTest : public ThriftRouterTestBase, public TestWithParam { public: - ThriftRouterFieldTypeTest() {} }; INSTANTIATE_TEST_CASE_P(PrimitiveFieldTypes, ThriftRouterFieldTypeTest, @@ -347,7 +344,6 @@ INSTANTIATE_TEST_CASE_P(PrimitiveFieldTypes, ThriftRouterFieldTypeTest, class ThriftRouterContainerTest : public ThriftRouterTestBase, public TestWithParam { public: - ThriftRouterContainerTest() {} }; INSTANTIATE_TEST_CASE_P(ContainerFieldTypes, ThriftRouterContainerTest, diff --git a/test/extensions/filters/network/thrift_proxy/thrift_object_impl_test.cc b/test/extensions/filters/network/thrift_proxy/thrift_object_impl_test.cc index 3e8d12403dcd8..79df66bc1b309 100644 --- a/test/extensions/filters/network/thrift_proxy/thrift_object_impl_test.cc +++ b/test/extensions/filters/network/thrift_proxy/thrift_object_impl_test.cc @@ -18,7 +18,6 @@ using testing::NiceMock; using testing::Ref; using testing::Return; using testing::ReturnRef; -using testing::Test; using testing::TestWithParam; using testing::Values; @@ -144,7 +143,7 @@ class ThriftObjectImplTestBase { Buffer::OwnedImpl buffer_; }; -class ThriftObjectImplTest : public ThriftObjectImplTestBase, public Test {}; +class ThriftObjectImplTest : public ThriftObjectImplTestBase, public testing::Test {}; // Test parsing an empty struct (just a stop field). TEST_F(ThriftObjectImplTest, ParseEmptyStruct) { diff --git a/test/extensions/tracers/common/ot/opentracing_driver_impl_test.cc b/test/extensions/tracers/common/ot/opentracing_driver_impl_test.cc index 5172d9d0aeb6c..c826495f54bce 100644 --- a/test/extensions/tracers/common/ot/opentracing_driver_impl_test.cc +++ b/test/extensions/tracers/common/ot/opentracing_driver_impl_test.cc @@ -11,8 +11,6 @@ #include "opentracing/mocktracer/in_memory_recorder.h" #include "opentracing/mocktracer/tracer.h" -using testing::Test; - namespace Envoy { namespace Extensions { namespace Tracers { @@ -46,7 +44,7 @@ class TestDriver : public OpenTracingDriver { std::shared_ptr tracer_; }; -class OpenTracingDriverTest : public Test { +class OpenTracingDriverTest : public testing::Test { public: void setupValidDriver(OpenTracingDriver::PropagationMode propagation_mode = diff --git a/test/extensions/tracers/datadog/datadog_tracer_impl_test.cc b/test/extensions/tracers/datadog/datadog_tracer_impl_test.cc index 9d964110ccd1b..2da3a766aee79 100644 --- a/test/extensions/tracers/datadog/datadog_tracer_impl_test.cc +++ b/test/extensions/tracers/datadog/datadog_tracer_impl_test.cc @@ -32,14 +32,13 @@ using testing::Invoke; using testing::NiceMock; using testing::Return; using testing::ReturnRef; -using testing::Test; namespace Envoy { namespace Extensions { namespace Tracers { namespace Datadog { -class DatadogDriverTest : public Test { +class DatadogDriverTest : public testing::Test { public: void setup(envoy::config::trace::v2::DatadogConfig& datadog_config, bool init_timer) { ON_CALL(cm_, httpAsyncClientForCluster("fake_cluster")) diff --git a/test/extensions/tracers/dynamic_ot/dynamic_opentracing_driver_impl_test.cc b/test/extensions/tracers/dynamic_ot/dynamic_opentracing_driver_impl_test.cc index 9232518de202f..3e75240289bc4 100644 --- a/test/extensions/tracers/dynamic_ot/dynamic_opentracing_driver_impl_test.cc +++ b/test/extensions/tracers/dynamic_ot/dynamic_opentracing_driver_impl_test.cc @@ -13,14 +13,12 @@ #include "gmock/gmock.h" #include "gtest/gtest.h" -using testing::Test; - namespace Envoy { namespace Extensions { namespace Tracers { namespace DynamicOt { -class DynamicOpenTracingDriverTest : public Test { +class DynamicOpenTracingDriverTest : public testing::Test { public: void setup(const std::string& library, const std::string& tracer_config) { driver_ = std::make_unique(stats_, library, tracer_config); diff --git a/test/extensions/tracers/lightstep/lightstep_tracer_impl_test.cc b/test/extensions/tracers/lightstep/lightstep_tracer_impl_test.cc index 62925cd2b0c2a..5bbd6c6c2e02f 100644 --- a/test/extensions/tracers/lightstep/lightstep_tracer_impl_test.cc +++ b/test/extensions/tracers/lightstep/lightstep_tracer_impl_test.cc @@ -33,14 +33,13 @@ using testing::Invoke; using testing::NiceMock; using testing::Return; using testing::ReturnRef; -using testing::Test; namespace Envoy { namespace Extensions { namespace Tracers { namespace Lightstep { -class LightStepDriverTest : public Test { +class LightStepDriverTest : public testing::Test { public: void setup(envoy::config::trace::v2::LightstepConfig& lightstep_config, bool init_timer, Common::Ot::OpenTracingDriver::PropagationMode propagation_mode = diff --git a/test/extensions/tracers/zipkin/zipkin_tracer_impl_test.cc b/test/extensions/tracers/zipkin/zipkin_tracer_impl_test.cc index 8c0c92ea52ccb..c6db7bb3d9a91 100644 --- a/test/extensions/tracers/zipkin/zipkin_tracer_impl_test.cc +++ b/test/extensions/tracers/zipkin/zipkin_tracer_impl_test.cc @@ -31,14 +31,13 @@ using testing::Invoke; using testing::NiceMock; using testing::Return; using testing::ReturnRef; -using testing::Test; namespace Envoy { namespace Extensions { namespace Tracers { namespace Zipkin { -class ZipkinDriverTest : public Test { +class ZipkinDriverTest : public testing::Test { public: ZipkinDriverTest() : time_source_(test_time_.timeSystem()) {} diff --git a/test/extensions/transport_sockets/alts/tsi_frame_protector_test.cc b/test/extensions/transport_sockets/alts/tsi_frame_protector_test.cc index d98217a9f5375..724638b3d4a10 100644 --- a/test/extensions/transport_sockets/alts/tsi_frame_protector_test.cc +++ b/test/extensions/transport_sockets/alts/tsi_frame_protector_test.cc @@ -16,14 +16,13 @@ using testing::InSequence; using testing::Invoke; using testing::NiceMock; using testing::SaveArg; -using testing::Test; using namespace std::string_literals; /** * Test with fake frame protector. The protected frame header is 4 byte length (little endian, * include header itself) and following the body. */ -class TsiFrameProtectorTest : public Test { +class TsiFrameProtectorTest : public testing::Test { public: TsiFrameProtectorTest() : raw_frame_protector_(tsi_create_fake_frame_protector(nullptr)), diff --git a/test/extensions/transport_sockets/alts/tsi_handshaker_test.cc b/test/extensions/transport_sockets/alts/tsi_handshaker_test.cc index 611479298277e..75ff19c0f32ed 100644 --- a/test/extensions/transport_sockets/alts/tsi_handshaker_test.cc +++ b/test/extensions/transport_sockets/alts/tsi_handshaker_test.cc @@ -16,7 +16,6 @@ using testing::InSequence; using testing::Invoke; using testing::NiceMock; using testing::SaveArg; -using testing::Test; class MockTsiHandshakerCallbacks : public TsiHandshakerCallbacks { public: @@ -33,7 +32,7 @@ class MockTsiHandshakerCallbacks : public TsiHandshakerCallbacks { } }; -class TsiHandshakerTest : public Test { +class TsiHandshakerTest : public testing::Test { public: TsiHandshakerTest() : server_handshaker_({tsi_create_fake_handshaker(0)}, dispatcher_), diff --git a/test/test_common/BUILD b/test/test_common/BUILD index 84593f9108f30..0d77934a2ffce 100644 --- a/test/test_common/BUILD +++ b/test/test_common/BUILD @@ -131,6 +131,24 @@ envoy_cc_library( ], ) +envoy_cc_test_library( + name = "global_lib", + srcs = ["global.cc"], + hdrs = ["global.h"], + deps = [ + "//source/common/common:assert_lib", + "//source/common/common:thread_lib", + ], +) + +envoy_cc_test( + name = "global_test", + srcs = ["global_test.cc"], + deps = [ + ":global_lib", + ], +) + envoy_cc_test_library( name = "test_time_lib", srcs = ["test_time.cc"], diff --git a/test/test_common/global.cc b/test/test_common/global.cc new file mode 100644 index 0000000000000..4c827922ee91f --- /dev/null +++ b/test/test_common/global.cc @@ -0,0 +1,26 @@ +#include "test/test_common/global.h" + +#include "common/common/assert.h" + +namespace Envoy { +namespace Test { + +Globals& Globals::instance() { + static Globals* globals = new Globals; + return *globals; +} + +std::string Globals::describeActiveSingletonsHelper() { + std::string ret; + Thread::ReleasableLockGuard map_lock(map_mutex_); + for (auto& p : singleton_map_) { + SingletonSharedPtr singleton = p.second.lock(); + if (singleton != nullptr) { + absl::StrAppend(&ret, "Unexpected active singleton: ", p.first, "\n"); + } + } + return ret; +} + +} // namespace Test +} // namespace Envoy diff --git a/test/test_common/global.h b/test/test_common/global.h new file mode 100644 index 0000000000000..c00f847667f08 --- /dev/null +++ b/test/test_common/global.h @@ -0,0 +1,128 @@ +#pragma once + +#include "common/common/lock_guard.h" +#include "common/common/thread.h" + +#include "absl/container/flat_hash_map.h" + +namespace Envoy { +namespace Test { + +/** + * Helper class for managing Globals. + * + * This class is instantiated as a process-scoped singleton. It manages a map + * from type-name to weak_ptr. That map accumulates + * over a process lifetime and never shrinks. However, the weak_ptr will + * generally be cleared after each test, as all shared_ptr references held in + * Global instances are destroyed. This way, each unit-test gets a fresh + * start. + */ +class Globals { +public: + /** + * Walks through all global singletons and ensures that none of them are + * active. No singletons should be allocaed at the end of unit tests, so + * this is called at the end of Envoy::TestRunner::RunTests(). + * + * @return std::string empty string if quiescent, otherwise newline-separated + * error messages. + */ + static std::string describeActiveSingletons() { + return instance().describeActiveSingletonsHelper(); + } + + /** + * Manages Singleton objects that are cleaned up after all references are + * dropped. This class must not be templatized because as a map value where + * every singleton in the map represents a different type. Instead we + * templatize the ptr() and ref() methods. + */ + struct Singleton { + virtual ~Singleton() = default; + virtual void* ptrHelper() PURE; + template Type* ptr() { return static_cast(ptrHelper()); } + template Type& ref() { return *ptr(); } + }; + using SingletonSharedPtr = std::shared_ptr; + + /** + * @return Type a singleton instance of Type. T must be default-constructible. + */ + template static SingletonSharedPtr get() { return instance().getHelper(); } + + ~Globals() = delete; // GlobalHeler is constructed once and never destroyed. + +private: + /** + * Templatized derived class of Singleton which holds the Type object and is + * responsible for deleting it using the correct destructor. + */ + template struct TypedSingleton : public Singleton { + ~TypedSingleton() override = default; + void* ptrHelper() override { return ptr_.get(); } + + private: + std::unique_ptr ptr_{std::make_unique()}; + }; + + Globals() = default; // Construct via Globals::instance(). + + /** + * @return Globals& a singleton for Globals. + */ + static Globals& instance(); + + template SingletonSharedPtr getHelper() { + Thread::LockGuard map_lock(map_mutex_); + std::weak_ptr& weak_singleton_ref = singleton_map_[typeid(Type).name()]; + SingletonSharedPtr singleton = weak_singleton_ref.lock(); + + if (singleton == nullptr) { + singleton = std::make_shared>(); + weak_singleton_ref = singleton; + } + return singleton; + } + + std::string describeActiveSingletonsHelper(); + + Thread::MutexBasicLockable map_mutex_; + absl::flat_hash_map> singleton_map_ GUARDED_BY(map_mutex_); +}; + +/** + * Helps manage classes that need to be instantiated once per server. In + * production they must be be plumbed through call/class hierarchy, but + * in test-code the zero-arg-constructor Mock pattern makes this impractical. + * Instead we use self-cleaning singletons. + * + * Say for example you need a FooImpl plumbed through the system. In production + * code you must propagate a FooImpl through constructors to provide access + * where needed. For tests, everywhere a common FooImpl is required, + * instantiate: + * + * Global foo; + * + * You can ghen access the singleton FooImpl via foo.get(). The underlying + * FooImpl is ref-counted, and when the last TestGlobal is freed, the singleton + * FooImpl will be destructed and the singleton pointer nulled. + * + * The templated type must have a zero-arg constructor. Templatizing this on an + * int will compile, but will be hard to use as the memory will be uninitialized + * and you will not know when instantiating it whether it needs to be + * initialized. + */ +template class Global { +public: + Global() : singleton_(Globals::get()) {} + Type& get() { return singleton_->ref(); } + Type* operator->() { return singleton_->ptr(); } + Type& operator*() { return singleton_->ref(); } + +private: + Globals::SingletonSharedPtr singleton_; +}; + +} // namespace Test +} // namespace Envoy diff --git a/test/test_common/global_test.cc b/test/test_common/global_test.cc new file mode 100644 index 0000000000000..bd6bb56746600 --- /dev/null +++ b/test/test_common/global_test.cc @@ -0,0 +1,46 @@ +#include +#include + +#include "test/test_common/global.h" + +#include "gtest/gtest.h" + +namespace Envoy { +namespace Test { + +class GlobalTest : public testing::Test { +protected: +}; + +TEST_F(GlobalTest, SingletonStringAndVector) { + { + Global s1; + Global> v1; + EXPECT_EQ("", *s1); + *s1 = "foo"; + EXPECT_TRUE(v1->empty()); + v1->push_back(42); + + Global s2; + Global> v2; + EXPECT_EQ("foo", *s2); + ASSERT_EQ(1, v2->size()); + EXPECT_EQ(42, (*v2)[0]); + } + + // The system is now quiescent, having dropped all references to the globals. + EXPECT_EQ("", Globals::describeActiveSingletons()); + + // After the globals went out of scope, referencing them again we start + // from clean objects; + Global s3; + Global> v3; + EXPECT_EQ("", *s3); + EXPECT_TRUE(v3->empty()); + + // With s3 and v3 on the stack, there are active singletons. + EXPECT_NE("", Globals::describeActiveSingletons()); +} + +} // namespace Test +} // namespace Envoy diff --git a/test/test_runner.h b/test/test_runner.h index 77cd8389867cc..1c4a003e623d2 100644 --- a/test/test_runner.h +++ b/test/test_runner.h @@ -7,6 +7,7 @@ #include "test/mocks/access_log/mocks.h" #include "test/test_common/environment.h" +#include "test/test_common/global.h" #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -41,7 +42,16 @@ class TestRunner { file_logger = std::make_unique( TestEnvironment::getOptions().logPath(), access_log_manager, Logger::Registry::getSink()); } - return RUN_ALL_TESTS(); + int exit_status = RUN_ALL_TESTS(); + + // Check that all singletons have been destroyed. + std::string active_singletons = Test::Globals::describeActiveSingletons(); + if (!active_singletons.empty()) { + std::cerr << "\n\nFAIL: Active singletons exist:\n" << active_singletons << std::endl; + exit_status = EXIT_FAILURE; + } + + return exit_status; } }; } // namespace Envoy diff --git a/tools/check_format.py b/tools/check_format.py index e10ba2595a923..d94ad2e26e6c2 100755 --- a/tools/check_format.py +++ b/tools/check_format.py @@ -286,6 +286,8 @@ def checkSourceLine(line, file_path, reportError): if ' ?: ' in line: # The ?: operator is non-standard, it is a GCC extension reportError("Don't use the '?:' operator, it is a non-standard GCC extension") + if line.startswith('using testing::Test;'): + reportError("Don't use 'using testing::Test;, elaborate the type instead") def checkBuildLine(line, file_path, reportError): diff --git a/tools/check_format_test_helper.py b/tools/check_format_test_helper.py index 99d541b75113f..7cf24a49a6d88 100755 --- a/tools/check_format_test_helper.py +++ b/tools/check_format_test_helper.py @@ -190,6 +190,8 @@ def checkFileExpectingOK(filename): errors += checkUnfixableError("attribute_packed.cc", "Don't use __attribute__((packed))") errors += checkUnfixableError("designated_initializers.cc", "Don't use designated initializers") errors += checkUnfixableError("elvis_operator.cc", "Don't use the '?:' operator") + errors += checkUnfixableError("testing_test.cc", + "Don't use 'using testing::Test;, elaborate the type instead") # The following files have errors that can be automatically fixed. errors += checkAndFixError("over_enthusiastic_spaces.cc", diff --git a/tools/testdata/check_format/testing_test.cc b/tools/testdata/check_format/testing_test.cc new file mode 100644 index 0000000000000..187deb4621250 --- /dev/null +++ b/tools/testdata/check_format/testing_test.cc @@ -0,0 +1,5 @@ +namespace Envoy { + +using testing::Test; + +} // namespace Envoy