Skip to content
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions ci/run_clang_tidy.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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
1 change: 1 addition & 0 deletions test/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -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": [],
Expand Down
3 changes: 1 addition & 2 deletions test/common/http/conn_manager_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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) {}
Expand Down
1 change: 0 additions & 1 deletion test/common/network/connection_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ using testing::Return;
using testing::SaveArg;
using testing::Sequence;
using testing::StrictMock;
using testing::Test;

namespace Envoy {
namespace Network {
Expand Down
3 changes: 1 addition & 2 deletions test/common/tracing/http_tracer_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ using testing::NiceMock;
using testing::Return;
using testing::ReturnPointee;
using testing::ReturnRef;
using testing::Test;

namespace Envoy {
namespace Tracing {
Expand Down Expand Up @@ -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();
Expand Down
3 changes: 1 addition & 2 deletions test/extensions/filters/network/thrift_proxy/decoder_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<FieldType> {};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
#include "gtest/gtest.h"

using testing::_;
using testing::Test;

namespace Envoy {
namespace Extensions {
Expand Down
9 changes: 4 additions & 5 deletions test/extensions/filters/network/thrift_proxy/router_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -330,14 +329,14 @@ class ThriftRouterTestBase {
NiceMock<Network::MockClientConnection> upstream_connection_;
};

class ThriftRouterTest : public ThriftRouterTestBase, public Test {
class ThriftRouterTest : public ThriftRouterTestBase, public testing::Test {
public:
ThriftRouterTest() {}
ThriftRouterTest() = default;
Comment thread
jmarantz marked this conversation as resolved.
Outdated
};

class ThriftRouterFieldTypeTest : public ThriftRouterTestBase, public TestWithParam<FieldType> {
public:
ThriftRouterFieldTypeTest() {}
ThriftRouterFieldTypeTest() = default;
};

INSTANTIATE_TEST_CASE_P(PrimitiveFieldTypes, ThriftRouterFieldTypeTest,
Expand All @@ -347,7 +346,7 @@ INSTANTIATE_TEST_CASE_P(PrimitiveFieldTypes, ThriftRouterFieldTypeTest,

class ThriftRouterContainerTest : public ThriftRouterTestBase, public TestWithParam<FieldType> {
public:
ThriftRouterContainerTest() {}
ThriftRouterContainerTest() = default;
};

INSTANTIATE_TEST_CASE_P(ContainerFieldTypes, ThriftRouterContainerTest,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -46,7 +44,7 @@ class TestDriver : public OpenTracingDriver {
std::shared_ptr<opentracing::mocktracer::MockTracer> tracer_;
};

class OpenTracingDriverTest : public Test {
class OpenTracingDriverTest : public testing::Test {
public:
void
setupValidDriver(OpenTracingDriver::PropagationMode propagation_mode =
Expand Down
3 changes: 1 addition & 2 deletions test/extensions/tracers/datadog/datadog_tracer_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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"))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<DynamicOpenTracingDriver>(stats_, library, tracer_config);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 =
Expand Down
3 changes: 1 addition & 2 deletions test/extensions/tracers/zipkin/zipkin_tracer_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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()) {}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ using testing::InSequence;
using testing::Invoke;
using testing::NiceMock;
using testing::SaveArg;
using testing::Test;

class MockTsiHandshakerCallbacks : public TsiHandshakerCallbacks {
public:
Expand All @@ -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_),
Expand Down
18 changes: 18 additions & 0 deletions test/test_common/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -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"],
Expand Down
68 changes: 68 additions & 0 deletions test/test_common/global.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
#include "test/test_common/global.h"

#include "common/common/assert.h"

namespace Envoy {
namespace Test {

Globals& Globals::instance() {
static Globals* h = new Globals;
return *h;
}

std::string Globals::describeActiveSingletonsHelper() {
std::string ret;
Thread::ReleasableLockGuard map_lock(map_mutex_);
for (auto& p : singleton_map_) {
std::unique_ptr<Singleton>& singleton = p.second;
ASSERT(singleton != nullptr);
if (singleton->ptr_ != nullptr) {
absl::StrAppend(&ret, "Unexpected active singleton: ", p.first, "\n");
}
}
return ret;
}

Globals::Singleton& Globals::get(const std::string& type_name, const MakeObjectFn& make_object) {
Thread::ReleasableLockGuard map_lock(map_mutex_);
std::unique_ptr<Singleton>& singleton = singleton_map_[type_name];

if (singleton == nullptr) {
// The first time constructing this object, we'll be constructing the
// Singleton for the first time, populating it with a fresh instance,
// so no need to take the singleton's mutex. But we do need to hold
// the map mutex as we are installing the singleton object in the
// singleton_map.
singleton = std::make_unique<Singleton>(make_object(), map_mutex_);
return *singleton;
}

// Subsequent accesses will must lock the singleton's mutex to safely populate
// .second. However, we can relinquish map_mutex_ as we are no longer
// modifying the map; just the .second of the MutexSingleton pointed to by the
// map.
Comment thread
jmarantz marked this conversation as resolved.
Outdated
if (singleton->ptr_ == nullptr) {
ASSERT(singleton->ref_count_ == 0);
singleton->ptr_ = make_object();
}
++singleton->ref_count_;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How come you're not using a shared_ptr/weak_ptr pattern here but doing it by hand?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about this for a bit; couldn't figure out how to make the pieces fit given the heterogenous map and casting.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about std::shared_ptr<Singleton> (and corresponding weak_ptr), with a void * still inside the object to provide the type cast abilities?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought of that, but thought it annoying that I'd have to hold a functor for calling the class-specific delete in the singleton.

Then I thought I'd give it a shot and it wasn't too bad to just do that, so switching to weak_ptr.

return *singleton;
}

void Globals::Singleton::releaseHelper(const DeleteObjectFn& delete_object) {
void* obj_to_delete = nullptr;
{
Thread::LockGuard singleton_lock(mutex_);
ASSERT(ptr_ != nullptr);
if (--ref_count_ == 0) {
obj_to_delete = ptr_;
ptr_ = nullptr;
}
}
if (obj_to_delete != nullptr) {
delete_object(obj_to_delete);
}
}

} // namespace Test
} // namespace Envoy
Loading