From 35b97a77be31c415b43e7ae0a2fe07d7bc3a2711 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Wed, 23 Jan 2019 20:13:31 -0500 Subject: [PATCH 01/18] all tests working Signed-off-by: Joshua Marantz --- include/envoy/common/time.h | 2 +- include/envoy/event/timer.h | 2 +- test/common/config/grpc_mux_impl_test.cc | 25 +++---- .../upstream/cluster_manager_impl_test.cc | 7 +- .../upstream/load_stats_reporter_test.cc | 7 +- test/mocks/event/mocks.cc | 2 +- test/mocks/event/mocks.h | 9 ++- test/test_common/BUILD | 2 + test/test_common/simulated_time_system.cc | 62 +++++++++------ test/test_common/simulated_time_system.h | 75 ++++++++++++++++--- test/test_common/test_time.cc | 33 ++++++++ test/test_common/test_time.h | 56 +++++++++++++- test/test_common/test_time_system.h | 63 ++++++++++++++++ 13 files changed, 281 insertions(+), 64 deletions(-) diff --git a/include/envoy/common/time.h b/include/envoy/common/time.h index 6e9bd6c825824..f48ca72f79816 100644 --- a/include/envoy/common/time.h +++ b/include/envoy/common/time.h @@ -21,7 +21,7 @@ typedef std::chrono::time_point MonotonicTime; */ class TimeSource { public: - virtual ~TimeSource() {} + virtual ~TimeSource() = default; /** * @return the current system time; not guaranteed to be monotonically increasing. diff --git a/include/envoy/event/timer.h b/include/envoy/event/timer.h index 7ebc8bb14787f..d6d90d1da39d1 100644 --- a/include/envoy/event/timer.h +++ b/include/envoy/event/timer.h @@ -55,7 +55,7 @@ typedef std::unique_ptr SchedulerPtr; */ class TimeSystem : public TimeSource { public: - virtual ~TimeSystem() {} + virtual ~TimeSystem() = default; using Duration = MonotonicTime::duration; diff --git a/test/common/config/grpc_mux_impl_test.cc b/test/common/config/grpc_mux_impl_test.cc index 380826fa3d9ef..7d69d61f04540 100644 --- a/test/common/config/grpc_mux_impl_test.cc +++ b/test/common/config/grpc_mux_impl_test.cc @@ -18,6 +18,7 @@ #include "test/mocks/runtime/mocks.h" #include "test/test_common/logging.h" #include "test/test_common/simulated_time_system.h" +#include "test/test_common/test_time.h" #include "test/test_common/utility.h" #include "gmock/gmock.h" @@ -35,13 +36,15 @@ namespace Envoy { namespace Config { namespace { +struct MockTimeProvider { + Event::DelegatingTestTimeSystem mock_time_system_; +}; + // We test some mux specific stuff below, other unit test coverage for singleton use of GrpcMuxImpl // is provided in [grpc_]subscription_impl_test.cc. -class GrpcMuxImplTest : public testing::Test { +class GrpcMuxImplTestBase : public testing::Test { public: - GrpcMuxImplTest() : async_client_(new Grpc::MockAsyncClient()) { - dispatcher_.setTimeSystem(time_system_); - } + GrpcMuxImplTestBase() : async_client_(new Grpc::MockAsyncClient()) {} void setup() { grpc_mux_ = std::make_unique( @@ -88,12 +91,13 @@ class GrpcMuxImplTest : public testing::Test { Grpc::MockAsyncStream async_stream_; std::unique_ptr grpc_mux_; NiceMock callbacks_; - Event::SimulatedTimeSystem time_system_; NiceMock local_info_; Stats::IsolatedStoreImpl stats_; Envoy::Config::RateLimitSettings rate_limit_settings_; }; +class GrpcMuxImplTest : public Event::SimulatedTimeProvider, public GrpcMuxImplTestBase {}; + // Validate behavior when multiple type URL watches are maintained, watches are created/destroyed // (via RAII). TEST_F(GrpcMuxImplTest, MultipleTypeUrlStreams) { @@ -327,12 +331,7 @@ TEST_F(GrpcMuxImplTest, WatchDemux) { // Exactly one test requires a mock time system to provoke behavior that cannot // easily be achieved with a SimulatedTimeSystem. -class GrpcMuxImplTestWithMockTimeSystem : public GrpcMuxImplTest { -protected: - GrpcMuxImplTestWithMockTimeSystem() { dispatcher_.setTimeSystem(mock_time_system_); } - - MockTimeSystem mock_time_system_; -}; +class GrpcMuxImplTestWithMockTimeSystem : public MockTimeProvider, public GrpcMuxImplTestBase {}; // Verifies that rate limiting is not enforced with defaults. TEST_F(GrpcMuxImplTestWithMockTimeSystem, TooManyRequestsWithDefaultSettings) { @@ -347,7 +346,7 @@ TEST_F(GrpcMuxImplTestWithMockTimeSystem, TooManyRequestsWithDefaultSettings) { })); // Validate that rate limiter is not created. - EXPECT_CALL(mock_time_system_, monotonicTime()).Times(0); + EXPECT_CALL(*mock_time_system_, monotonicTime()).Times(0); setup(); @@ -397,7 +396,7 @@ TEST_F(GrpcMuxImplTestWithMockTimeSystem, TooManyRequestsWithEmptyRateLimitSetti drain_request_timer = new Event::MockTimer(); return drain_request_timer; })); - EXPECT_CALL(mock_time_system_, monotonicTime()) + EXPECT_CALL(*mock_time_system_, monotonicTime()) .WillRepeatedly(Return(std::chrono::steady_clock::time_point{})); RateLimitSettings custom_rate_limit_settings; diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index b26e796377f39..4e9bef7b2e9e0 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -167,11 +167,9 @@ envoy::config::bootstrap::v2::Bootstrap parseBootstrapFromV2Yaml(const std::stri return bootstrap; } -class ClusterManagerImplTest : public testing::Test { +class ClusterManagerImplTest : public Event::SimulatedTimeProvider, public testing::Test { public: - ClusterManagerImplTest() : api_(Api::createApiForTest(stats_store_)) { - factory_.dispatcher_.setTimeSystem(time_system_); - } + ClusterManagerImplTest() : api_(Api::createApiForTest(stats_store_)) {} void create(const envoy::config::bootstrap::v2::Bootstrap& bootstrap) { cluster_manager_ = std::make_unique( @@ -251,7 +249,6 @@ class ClusterManagerImplTest : public testing::Test { std::unique_ptr cluster_manager_; AccessLog::MockAccessLogManager log_manager_; NiceMock admin_; - Event::SimulatedTimeSystem time_system_; MockLocalClusterUpdate local_cluster_update_; Http::ContextImpl http_context_; }; diff --git a/test/common/upstream/load_stats_reporter_test.cc b/test/common/upstream/load_stats_reporter_test.cc index 5ac86d9a80c6e..b3e923793a380 100644 --- a/test/common/upstream/load_stats_reporter_test.cc +++ b/test/common/upstream/load_stats_reporter_test.cc @@ -26,13 +26,11 @@ using testing::Return; namespace Envoy { namespace Upstream { -class LoadStatsReporterTest : public testing::Test { +class LoadStatsReporterTest : public Event::SimulatedTimeProvider, public testing::Test { public: LoadStatsReporterTest() : retry_timer_(new Event::MockTimer()), response_timer_(new Event::MockTimer()), - async_client_(new Grpc::MockAsyncClient()) { - dispatcher_.setTimeSystem(time_system_); - } + async_client_(new Grpc::MockAsyncClient()) {} void createLoadStatsReporter() { InSequence s; @@ -78,7 +76,6 @@ class LoadStatsReporterTest : public testing::Test { Event::TimerCb response_timer_cb_; Grpc::MockAsyncStream async_stream_; Grpc::MockAsyncClient* async_client_; - Event::SimulatedTimeSystem time_system_; NiceMock local_info_; }; diff --git a/test/mocks/event/mocks.cc b/test/mocks/event/mocks.cc index b5e2f9a1bee6c..34ed99e236e0f 100644 --- a/test/mocks/event/mocks.cc +++ b/test/mocks/event/mocks.cc @@ -13,7 +13,7 @@ using testing::SaveArg; namespace Envoy { namespace Event { -MockDispatcher::MockDispatcher() : time_system_(&test_time_.timeSystem()) { +MockDispatcher::MockDispatcher() { //: time_system_(&test_time_.timeSystem()) { ON_CALL(*this, clearDeferredDeleteList()).WillByDefault(Invoke([this]() -> void { to_delete_.clear(); })); diff --git a/test/mocks/event/mocks.h b/test/mocks/event/mocks.h index 6f11b90b1cc56..4716d969646d2 100644 --- a/test/mocks/event/mocks.h +++ b/test/mocks/event/mocks.h @@ -30,10 +30,10 @@ class MockDispatcher : public Dispatcher { MockDispatcher(); ~MockDispatcher(); - void setTimeSystem(TimeSystem& time_system) { time_system_ = &time_system; } + //void setTimeSystem(TimeSystem& time_system) { time_system_ = &time_system; } // Dispatcher - TimeSystem& timeSystem() override { return *time_system_; } + TimeSystem& timeSystem() override { return time_system_; } Network::ConnectionPtr createServerConnection(Network::ConnectionSocketPtr&& socket, Network::TransportSocketPtr&& transport_socket) override { @@ -110,8 +110,9 @@ class MockDispatcher : public Dispatcher { Buffer::WatermarkFactory& getWatermarkFactory() override { return buffer_factory_; } // TODO(jmarantz): Switch these to using mock-time. - DangerousDeprecatedTestTime test_time_; - TimeSystem* time_system_; + //DangerousDeprecatedTestTime test_time_; + //TimeSystem* time_system_; + GlobalTimeSystem time_system_; std::list to_delete_; MockBufferFactory buffer_factory_; diff --git a/test/test_common/BUILD b/test/test_common/BUILD index b60a122a9f2e5..656a26b864f23 100644 --- a/test/test_common/BUILD +++ b/test/test_common/BUILD @@ -160,6 +160,7 @@ envoy_cc_test_library( srcs = ["test_time.cc"], hdrs = ["test_time.h"], deps = [ + ":global_lib", ":test_time_system_interface", "//source/common/event:real_time_system_lib", ], @@ -169,6 +170,7 @@ envoy_cc_test_library( name = "test_time_system_interface", hdrs = ["test_time_system.h"], deps = [ + ":global_lib", "//include/envoy/event:timer_interface", "//source/common/common:thread_lib", ], diff --git a/test/test_common/simulated_time_system.cc b/test/test_common/simulated_time_system.cc index de936e345a94b..f3b88ffdda463 100644 --- a/test/test_common/simulated_time_system.cc +++ b/test/test_common/simulated_time_system.cc @@ -18,9 +18,9 @@ namespace Event { // mechanism used in RealTimeSystem timers is employed for simulated alarms. // Note that libevent is placed into thread-safe mode due to the call to // evthread_use_pthreads() in source/common/event/libevent.cc. -class SimulatedTimeSystem::Alarm : public TimerImpl { +class SimulatedTimeSystemHelper::Alarm : public TimerImpl { public: - Alarm(SimulatedTimeSystem& time_system, Libevent::BasePtr& libevent, TimerCb cb) + Alarm(SimulatedTimeSystemHelper& time_system, Libevent::BasePtr& libevent, TimerCb cb) : TimerImpl(libevent, [this, cb] { runAlarm(cb); }), time_system_(time_system), index_(time_system.nextIndex()), armed_(false) {} @@ -56,7 +56,7 @@ class SimulatedTimeSystem::Alarm : public TimerImpl { cb(); } - SimulatedTimeSystem& time_system_; + SimulatedTimeSystemHelper& time_system_; MonotonicTime time_; uint64_t index_; bool armed_; @@ -64,7 +64,8 @@ class SimulatedTimeSystem::Alarm : public TimerImpl { // Compare two alarms, based on wakeup time and insertion order. Returns true if // a comes before b. -bool SimulatedTimeSystem::CompareAlarms::operator()(const Alarm* a, const Alarm* b) const { +bool SimulatedTimeSystemHelper::CompareAlarms::operator()( + const Alarm* a, const Alarm* b) const { if (a != b) { if (a->time() < b->time()) { return true; @@ -79,33 +80,33 @@ bool SimulatedTimeSystem::CompareAlarms::operator()(const Alarm* a, const Alarm* // associated with a scheduler. The scheduler creates the timers with a libevent // context, so that the timer callbacks can be executed via Dispatcher::run() in // the expected thread. -class SimulatedTimeSystem::SimulatedScheduler : public Scheduler { +class SimulatedTimeSystemHelper::SimulatedScheduler : public Scheduler { public: - SimulatedScheduler(SimulatedTimeSystem& time_system, Libevent::BasePtr& libevent) + SimulatedScheduler(SimulatedTimeSystemHelper& time_system, Libevent::BasePtr& libevent) : time_system_(time_system), libevent_(libevent) {} TimerPtr createTimer(const TimerCb& cb) override { - return std::make_unique(time_system_, libevent_, cb); + return std::make_unique(time_system_, libevent_, cb); }; private: - SimulatedTimeSystem& time_system_; + SimulatedTimeSystemHelper& time_system_; Libevent::BasePtr& libevent_; }; -SimulatedTimeSystem::Alarm::~Alarm() { +SimulatedTimeSystemHelper::Alarm::Alarm::~Alarm() { if (armed_) { disableTimer(); } } -void SimulatedTimeSystem::Alarm::disableTimer() { +void SimulatedTimeSystemHelper::Alarm::Alarm::disableTimer() { if (armed_) { time_system_.removeAlarm(this); armed_ = false; } } -void SimulatedTimeSystem::Alarm::enableTimer(const std::chrono::milliseconds& duration) { +void SimulatedTimeSystemHelper::Alarm::Alarm::enableTimer(const std::chrono::milliseconds& duration) { disableTimer(); armed_ = true; if (duration.count() == 0) { @@ -125,33 +126,35 @@ static int instance_count = 0; // When we initialize our simulated time, we'll start the current time based on // the real current time. But thereafter, real-time will not be used, and time // will march forward only by calling sleep(). -SimulatedTimeSystem::SimulatedTimeSystem() +SimulatedTimeSystemHelper::SimulatedTimeSystemHelper() : monotonic_time_(MonotonicTime(std::chrono::seconds(0))), system_time_(real_time_source_.systemTime()), index_(0), pending_alarms_(0) { ++instance_count; ASSERT(instance_count <= 1); } -SimulatedTimeSystem::~SimulatedTimeSystem() { --instance_count; } +SimulatedTimeSystemHelper::~SimulatedTimeSystemHelper() { --instance_count; } -SystemTime SimulatedTimeSystem::systemTime() { +bool SimulatedTimeSystemHelper::hasInstance() { return instance_count > 0; } + +SystemTime SimulatedTimeSystemHelper::systemTime() { Thread::LockGuard lock(mutex_); return system_time_; } -MonotonicTime SimulatedTimeSystem::monotonicTime() { +MonotonicTime SimulatedTimeSystemHelper::monotonicTime() { Thread::LockGuard lock(mutex_); return monotonic_time_; } -void SimulatedTimeSystem::sleep(const Duration& duration) { +void SimulatedTimeSystemHelper::sleep(const Duration& duration) { mutex_.lock(); MonotonicTime monotonic_time = monotonic_time_ + std::chrono::duration_cast(duration); setMonotonicTimeAndUnlock(monotonic_time); } -Thread::CondVar::WaitStatus SimulatedTimeSystem::waitFor(Thread::MutexBasicLockable& mutex, +Thread::CondVar::WaitStatus SimulatedTimeSystemHelper::waitFor(Thread::MutexBasicLockable& mutex, Thread::CondVar& condvar, const Duration& duration) noexcept { const Duration real_time_poll_delay(std::chrono::milliseconds(50)); @@ -187,27 +190,27 @@ Thread::CondVar::WaitStatus SimulatedTimeSystem::waitFor(Thread::MutexBasicLocka return Thread::CondVar::WaitStatus::Timeout; } -int64_t SimulatedTimeSystem::nextIndex() { +int64_t SimulatedTimeSystemHelper::nextIndex() { Thread::LockGuard lock(mutex_); return index_++; } -void SimulatedTimeSystem::addAlarm(Alarm* alarm, const std::chrono::milliseconds& duration) { +void SimulatedTimeSystemHelper::addAlarm(Alarm* alarm, const std::chrono::milliseconds& duration) { Thread::LockGuard lock(mutex_); alarm->setTime(monotonic_time_ + duration); alarms_.insert(alarm); } -void SimulatedTimeSystem::removeAlarm(Alarm* alarm) { +void SimulatedTimeSystemHelper::removeAlarm(Alarm* alarm) { Thread::LockGuard lock(mutex_); alarms_.erase(alarm); } -SchedulerPtr SimulatedTimeSystem::createScheduler(Libevent::BasePtr& libevent) { +SchedulerPtr SimulatedTimeSystemHelper::createScheduler(Libevent::BasePtr& libevent) { return std::make_unique(*this, libevent); } -void SimulatedTimeSystem::setMonotonicTimeAndUnlock(const MonotonicTime& monotonic_time) { +void SimulatedTimeSystemHelper::setMonotonicTimeAndUnlock(const MonotonicTime& monotonic_time) { // We don't have a convenient LockGuard construct that allows temporarily // dropping the lock to run a callback. The main issue here is that we must // be careful not to be holding mutex_ when an exception can be thrown. @@ -243,7 +246,7 @@ void SimulatedTimeSystem::setMonotonicTimeAndUnlock(const MonotonicTime& monoton mutex_.unlock(); } -void SimulatedTimeSystem::setSystemTime(const SystemTime& system_time) { +void SimulatedTimeSystemHelper::setSystemTime(const SystemTime& system_time) { mutex_.lock(); if (system_time > system_time_) { MonotonicTime monotonic_time = @@ -256,5 +259,18 @@ void SimulatedTimeSystem::setSystemTime(const SystemTime& system_time) { } } +/* +SimulatedTimeSystem::SimulatedTimeSystem() { + TestTimeSystem* time_system = singleton_->timeSystem(); + if (time_system == nullptr) { + time_system_ = new SimulatedTimeSystemHelper; + singleton_->set(time_system_); + } else { + time_system_ = dynamic_cast(time_system); + ASSERT(time_system_); + } +} +*/ + } // namespace Event } // namespace Envoy diff --git a/test/test_common/simulated_time_system.h b/test/test_common/simulated_time_system.h index 9921581905507..f9144f77347bc 100644 --- a/test/test_common/simulated_time_system.h +++ b/test/test_common/simulated_time_system.h @@ -1,7 +1,10 @@ #pragma once +#include "envoy/event/timer.h" + #include "common/common/lock_guard.h" #include "common/common/thread.h" +//#include "common/common/utility.h" #include "common/common/utility.h" #include "test/test_common/test_time_system.h" @@ -13,10 +16,10 @@ namespace Event { // sleep(), setSystemTime(), or setMonotonicTime(). systemTime() and // monotonicTime() are maintained in the class, and alarms are fired in response // to adjustments in time. -class SimulatedTimeSystem : public TestTimeSystem { +class SimulatedTimeSystemHelper : public TestTimeSystem { public: - SimulatedTimeSystem(); - ~SimulatedTimeSystem(); + SimulatedTimeSystemHelper(); + ~SimulatedTimeSystemHelper() override; // TimeSystem SchedulerPtr createScheduler(Libevent::BasePtr&) override; @@ -43,9 +46,6 @@ class SimulatedTimeSystem : public TestTimeSystem { mutex_.lock(); setMonotonicTimeAndUnlock(monotonic_time); } - template void setMonotonicTime(const Duration& duration) { - setMonotonicTime(MonotonicTime(duration)); - } /** * Sets the system-time, whether forward or backward. If time moves forward, @@ -55,9 +55,8 @@ class SimulatedTimeSystem : public TestTimeSystem { * @param system_time The desired new system time. */ void setSystemTime(const SystemTime& system_time); - template void setSystemTime(const Duration& duration) { - setSystemTime(SystemTime(duration)); - } + + static bool hasInstance(); private: class SimulatedScheduler; @@ -100,5 +99,63 @@ class SimulatedTimeSystem : public TestTimeSystem { std::atomic pending_alarms_; }; +class SimulatedTimeSystem : public DelegatingTestTimeSystem { + public: + void setMonotonicTime(const MonotonicTime& monotonic_time) { + time_system_->setMonotonicTime(monotonic_time); + } + void setSystemTime(const SystemTime& system_time) { time_system_->setSystemTime(system_time); } + + template void setMonotonicTime(const Duration& duration) { + setMonotonicTime(MonotonicTime(duration)); + } + template void setSystemTime(const Duration& duration) { + setSystemTime(SystemTime(duration)); + } +}; + +class SimulatedTimeProvider { + public: + SimulatedTimeSystem time_system_; +}; + +/* +class SimulatedTimeSystem : public TestTimeSystem { + public: + SimulatedTimeSystem(); + + SimulatedTimeSystemHelper& operator*() { return *time_system_; } + SimulatedTimeSystemHelper* operator->() { return time_system_; } + + void sleep(const Duration& duration) override { time_system_->sleep(duration); } + Thread::CondVar::WaitStatus + waitFor(Thread::MutexBasicLockable& mutex, Thread::CondVar& condvar, + const Duration& duration) noexcept EXCLUSIVE_LOCKS_REQUIRED(mutex) override { + return time_system_->waitFor(mutex, condvar, duration); + } + + SchedulerPtr createScheduler(Libevent::BasePtr& base_ptr) override { + return time_system_->createScheduler(base_ptr); + } + SystemTime systemTime() override { return time_system_->systemTime(); } + MonotonicTime monotonicTime() override { return time_system_->monotonicTime(); } + + void setMonotonicTime(const MonotonicTime& monotonic_time) { + time_system_->setMonotonicTime(monotonic_time); + } + void setSystemTime(const SystemTime& system_time) { time_system_->setSystemTime(system_time); } + + template void setMonotonicTime(const Duration& duration) { + setMonotonicTime(MonotonicTime(duration)); + } + template void setSystemTime(const Duration& duration) { + setSystemTime(SystemTime(duration)); + } + + private: + Test::Global singleton_; + SimulatedTimeSystemHelper* time_system_; +}; +*/ } // namespace Event } // namespace Envoy diff --git a/test/test_common/test_time.cc b/test/test_common/test_time.cc index ffe2fb29c5128..81da92171eaf9 100644 --- a/test/test_common/test_time.cc +++ b/test/test_common/test_time.cc @@ -2,6 +2,8 @@ #include "common/common/utility.h" +#include "test/test_common/global.h" + namespace Envoy { DangerousDeprecatedTestTime::DangerousDeprecatedTestTime() {} @@ -16,5 +18,36 @@ Thread::CondVar::WaitStatus TestRealTimeSystem::waitFor(Thread::MutexBasicLockab return condvar.waitFor(lock, duration); } +SystemTime TestRealTimeSystem::systemTime() { + //ASSERT(!SimulatedTimeSystem::hasInstance()); + return real_time_system_.systemTime(); +} + +MonotonicTime TestRealTimeSystem::monotonicTime() { + //ASSERT(!SimulatedTimeSystem::hasInstance()); + return real_time_system_.monotonicTime(); +} + +/* +MockTimeSystem::MockTimeSystem() {} +MockTimeSystem::~MockTimeSystem() {} + +// TODO(#4160): Eliminate all uses of MockTimeSystem, replacing with SimulatedTimeSystem, +// where timer callbacks are triggered by the advancement of time. This implementation +// matches recent behavior, where real-time timers were created directly in libevent +// by dispatcher_impl.cc. +Event::SchedulerPtr MockTimeSystem::createScheduler(Event::Libevent::BasePtr& base) { + return real_time_system_.createScheduler(base); +} + +void MockTimeSystem::sleep(const Duration& duration) { real_time_system_.sleep(duration); } + +Thread::CondVar::WaitStatus +MockTimeSystem::waitFor(Thread::MutexBasicLockable& mutex, Thread::CondVar& condvar, + const Duration& duration) noexcept EXCLUSIVE_LOCKS_REQUIRED(mutex) { + return real_time_system_.waitFor(mutex, condvar, duration); +} +*/ + } // namespace Event } // namespace Envoy diff --git a/test/test_common/test_time.h b/test/test_common/test_time.h index 5f7501305b440..3d4e12c1781f2 100644 --- a/test/test_common/test_time.h +++ b/test/test_common/test_time.h @@ -2,6 +2,7 @@ #include "common/event/real_time_system.h" +#include "test/test_common/global.h" #include "test/test_common/test_time_system.h" namespace Envoy { @@ -21,13 +22,64 @@ class TestRealTimeSystem : public TestTimeSystem { } // TimeSource - SystemTime systemTime() override { return real_time_system_.systemTime(); } - MonotonicTime monotonicTime() override { return real_time_system_.monotonicTime(); } + SystemTime systemTime() override; + MonotonicTime monotonicTime() override; private: Event::RealTimeSystem real_time_system_; }; +class GlobalTimeSystem : public TestTimeSystem { +public: + void sleep(const Duration& duration) override { lazyInit().sleep(duration); } + Thread::CondVar::WaitStatus + waitFor(Thread::MutexBasicLockable& mutex, Thread::CondVar& condvar, + const Duration& duration) noexcept EXCLUSIVE_LOCKS_REQUIRED(mutex) override { + return lazyInit().waitFor(mutex, condvar, duration); + } + SchedulerPtr createScheduler(Libevent::BasePtr& base_ptr) override { + return lazyInit().createScheduler(base_ptr); + } + SystemTime systemTime() override { return lazyInit().systemTime(); } + MonotonicTime monotonicTime() override { return lazyInit().monotonicTime(); } + + TestTimeSystem& lazyInit() { + if (singleton_->timeSystem() == nullptr) { + singleton_->set(new TestRealTimeSystem); + } + return *singleton_->timeSystem(); + } + +private: + Test::Global singleton_; +}; + +/* +template class TestTime : public TestTimeSystem { +public: + TestTime() { global_time_system_.set(&(time_system_.get())); } + + void sleep(const Duration& duration) override { time_system_->sleep(duration); } + Thread::CondVar::WaitStatus + waitFor(Thread::MutexBasicLockable& mutex, Thread::CondVar& condvar, + const Duration& duration) noexcept EXCLUSIVE_LOCKS_REQUIRED(mutex) override { + return time_system_->waitFor(mutex, condvar, duration); + } + SchedulerPtr createScheduler(Libevent::BasePtr& base_ptr) override { + return time_system_->createScheduler(base_ptr); + } + SystemTime systemTime() override { return time_system_->systemTime(); } + MonotonicTime monotonicTime() override { return time_system_->monotonicTime(); } + + Type* operator->() { return &(*time_system_); } + Type& operator*() { return *time_system_; } + +private: + Test::Global time_system_; + GlobalTimeSystem global_time_system_; +}; +*/ + } // namespace Event // Instantiates real-time sources for testing purposes. In general, this is a diff --git a/test/test_common/test_time_system.h b/test/test_common/test_time_system.h index 60a8291249173..6283c079f1357 100644 --- a/test/test_common/test_time_system.h +++ b/test/test_common/test_time_system.h @@ -2,14 +2,40 @@ #include "envoy/event/timer.h" +#include "common/common/assert.h" #include "common/common/thread.h" +#include "test/test_common/global.h" + namespace Envoy { namespace Event { +class TestTimeSystem; + +// Ensures that only one type of time-system is instantiated at a time. +class SingletonTimeSystemHelper { +public: + explicit SingletonTimeSystemHelper() : time_system_(nullptr) {} + + void set(TestTimeSystem* time_system) { + if (time_system_ == nullptr) { + time_system_.reset(time_system); + } else { + ASSERT(time_system_.get() == time_system); + } + } + + TestTimeSystem* timeSystem() { return time_system_.get(); } + +private: + std::unique_ptr time_system_; +}; + // Adds sleep() and waitFor() interfaces to Event::TimeSystem. class TestTimeSystem : public Event::TimeSystem { public: + virtual ~TestTimeSystem() = default; + /** * Advances time forward by the specified duration, running any timers * along the way that have been scheduled to fire. @@ -41,5 +67,42 @@ class TestTimeSystem : public Event::TimeSystem { } }; +template +class DelegatingTestTimeSystem : public TestTimeSystem { + public: + DelegatingTestTimeSystem() { + TestTimeSystem* time_system = singleton_->timeSystem(); + if (time_system == nullptr) { + time_system_ = new TimeSystemVariant; + singleton_->set(time_system_); + } else { + time_system_ = dynamic_cast(time_system); + ASSERT(time_system_); + } + } + + void sleep(const Duration& duration) override { time_system_->sleep(duration); } + + Thread::CondVar::WaitStatus + waitFor(Thread::MutexBasicLockable& mutex, Thread::CondVar& condvar, + const Duration& duration) noexcept EXCLUSIVE_LOCKS_REQUIRED(mutex) override { + return time_system_->waitFor(mutex, condvar, duration); + } + + SchedulerPtr createScheduler(Libevent::BasePtr& base_ptr) override { + return time_system_->createScheduler(base_ptr); + } + SystemTime systemTime() override { return time_system_->systemTime(); } + MonotonicTime monotonicTime() override { return time_system_->monotonicTime(); } + + TimeSystemVariant& operator*() { return *time_system_; } + + protected: + TimeSystemVariant* time_system_; + + private: + Test::Global singleton_; +}; + } // namespace Event } // namespace Envoy From 43bcff1652d93f34163d5d2fd654b0b440d36e24 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Wed, 23 Jan 2019 20:23:18 -0500 Subject: [PATCH 02/18] cleanup Signed-off-by: Joshua Marantz --- test/mocks/event/mocks.h | 6 ---- test/test_common/simulated_time_system.h | 39 ------------------------ test/test_common/test_time.h | 27 +--------------- test/test_common/test_time_system.h | 3 +- 4 files changed, 2 insertions(+), 73 deletions(-) diff --git a/test/mocks/event/mocks.h b/test/mocks/event/mocks.h index 4716d969646d2..e3a31ce17ca35 100644 --- a/test/mocks/event/mocks.h +++ b/test/mocks/event/mocks.h @@ -30,8 +30,6 @@ class MockDispatcher : public Dispatcher { MockDispatcher(); ~MockDispatcher(); - //void setTimeSystem(TimeSystem& time_system) { time_system_ = &time_system; } - // Dispatcher TimeSystem& timeSystem() override { return time_system_; } Network::ConnectionPtr @@ -109,11 +107,7 @@ class MockDispatcher : public Dispatcher { MOCK_METHOD1(run, void(RunType type)); Buffer::WatermarkFactory& getWatermarkFactory() override { return buffer_factory_; } - // TODO(jmarantz): Switch these to using mock-time. - //DangerousDeprecatedTestTime test_time_; - //TimeSystem* time_system_; GlobalTimeSystem time_system_; - std::list to_delete_; MockBufferFactory buffer_factory_; }; diff --git a/test/test_common/simulated_time_system.h b/test/test_common/simulated_time_system.h index f9144f77347bc..c4dddb7ee60c0 100644 --- a/test/test_common/simulated_time_system.h +++ b/test/test_common/simulated_time_system.h @@ -4,7 +4,6 @@ #include "common/common/lock_guard.h" #include "common/common/thread.h" -//#include "common/common/utility.h" #include "common/common/utility.h" #include "test/test_common/test_time_system.h" @@ -119,43 +118,5 @@ class SimulatedTimeProvider { SimulatedTimeSystem time_system_; }; -/* -class SimulatedTimeSystem : public TestTimeSystem { - public: - SimulatedTimeSystem(); - - SimulatedTimeSystemHelper& operator*() { return *time_system_; } - SimulatedTimeSystemHelper* operator->() { return time_system_; } - - void sleep(const Duration& duration) override { time_system_->sleep(duration); } - Thread::CondVar::WaitStatus - waitFor(Thread::MutexBasicLockable& mutex, Thread::CondVar& condvar, - const Duration& duration) noexcept EXCLUSIVE_LOCKS_REQUIRED(mutex) override { - return time_system_->waitFor(mutex, condvar, duration); - } - - SchedulerPtr createScheduler(Libevent::BasePtr& base_ptr) override { - return time_system_->createScheduler(base_ptr); - } - SystemTime systemTime() override { return time_system_->systemTime(); } - MonotonicTime monotonicTime() override { return time_system_->monotonicTime(); } - - void setMonotonicTime(const MonotonicTime& monotonic_time) { - time_system_->setMonotonicTime(monotonic_time); - } - void setSystemTime(const SystemTime& system_time) { time_system_->setSystemTime(system_time); } - - template void setMonotonicTime(const Duration& duration) { - setMonotonicTime(MonotonicTime(duration)); - } - template void setSystemTime(const Duration& duration) { - setSystemTime(SystemTime(duration)); - } - - private: - Test::Global singleton_; - SimulatedTimeSystemHelper* time_system_; -}; -*/ } // namespace Event } // namespace Envoy diff --git a/test/test_common/test_time.h b/test/test_common/test_time.h index 3d4e12c1781f2..741642932e76c 100644 --- a/test/test_common/test_time.h +++ b/test/test_common/test_time.h @@ -45,6 +45,7 @@ class GlobalTimeSystem : public TestTimeSystem { TestTimeSystem& lazyInit() { if (singleton_->timeSystem() == nullptr) { + // TODO(jmarantz): Switch default to SimulatedTimeSystem. singleton_->set(new TestRealTimeSystem); } return *singleton_->timeSystem(); @@ -54,32 +55,6 @@ class GlobalTimeSystem : public TestTimeSystem { Test::Global singleton_; }; -/* -template class TestTime : public TestTimeSystem { -public: - TestTime() { global_time_system_.set(&(time_system_.get())); } - - void sleep(const Duration& duration) override { time_system_->sleep(duration); } - Thread::CondVar::WaitStatus - waitFor(Thread::MutexBasicLockable& mutex, Thread::CondVar& condvar, - const Duration& duration) noexcept EXCLUSIVE_LOCKS_REQUIRED(mutex) override { - return time_system_->waitFor(mutex, condvar, duration); - } - SchedulerPtr createScheduler(Libevent::BasePtr& base_ptr) override { - return time_system_->createScheduler(base_ptr); - } - SystemTime systemTime() override { return time_system_->systemTime(); } - MonotonicTime monotonicTime() override { return time_system_->monotonicTime(); } - - Type* operator->() { return &(*time_system_); } - Type& operator*() { return *time_system_; } - -private: - Test::Global time_system_; - GlobalTimeSystem global_time_system_; -}; -*/ - } // namespace Event // Instantiates real-time sources for testing purposes. In general, this is a diff --git a/test/test_common/test_time_system.h b/test/test_common/test_time_system.h index 6283c079f1357..9bd04942d831c 100644 --- a/test/test_common/test_time_system.h +++ b/test/test_common/test_time_system.h @@ -67,8 +67,7 @@ class TestTimeSystem : public Event::TimeSystem { } }; -template -class DelegatingTestTimeSystem : public TestTimeSystem { +template class DelegatingTestTimeSystem : public TestTimeSystem { public: DelegatingTestTimeSystem() { TestTimeSystem* time_system = singleton_->timeSystem(); From 43cf4ba7da485d52c345c888a1ed4eee544fa0dd Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Thu, 24 Jan 2019 12:34:48 -0500 Subject: [PATCH 03/18] Enforce TimeSystem being a true singleton. Share delagation logic via template & subclass. Signed-off-by: Joshua Marantz --- source/common/crypto/utility.h | 4 +- test/common/config/BUILD | 1 + .../config/config_provider_impl_test.cc | 4 +- test/common/router/BUILD | 1 + test/common/router/rds_impl_test.cc | 4 +- .../transport_sockets/tls/ssl_socket_test.cc | 55 ++++++++----------- test/integration/utility.cc | 8 +-- test/integration/utility.h | 3 - test/mocks/common.h | 8 +-- test/mocks/server/BUILD | 2 +- test/mocks/server/mocks.h | 10 ++-- test/server/BUILD | 1 + test/test_common/simulated_time_system.cc | 10 ++-- test/test_common/simulated_time_system.h | 8 +-- test/test_common/test_time.cc | 4 +- test/test_common/test_time.h | 20 ++----- test/test_common/test_time_system.h | 46 +++++++++------- 17 files changed, 92 insertions(+), 97 deletions(-) diff --git a/source/common/crypto/utility.h b/source/common/crypto/utility.h index d89c51fc75be0..69e3a1f91aafd 100644 --- a/source/common/crypto/utility.h +++ b/source/common/crypto/utility.h @@ -1,5 +1,7 @@ #pragma once +#include + #include "envoy/buffer/buffer.h" namespace Envoy { @@ -27,4 +29,4 @@ class Utility { } // namespace Crypto } // namespace Common -} // namespace Envoy \ No newline at end of file +} // namespace Envoy diff --git a/test/common/config/BUILD b/test/common/config/BUILD index d80c5b07dd781..f6b41dac69e14 100644 --- a/test/common/config/BUILD +++ b/test/common/config/BUILD @@ -223,5 +223,6 @@ envoy_cc_test( "//source/common/config:config_provider_lib", "//source/common/protobuf:utility_lib", "//test/mocks/server:server_mocks", + "//test/test_common:simulated_time_system_lib", ], ) diff --git a/test/common/config/config_provider_impl_test.cc b/test/common/config/config_provider_impl_test.cc index 7e928963aed9f..4704f1aa82b06 100644 --- a/test/common/config/config_provider_impl_test.cc +++ b/test/common/config/config_provider_impl_test.cc @@ -5,6 +5,7 @@ #include "test/common/config/dummy_config.pb.h" #include "test/mocks/server/mocks.h" +#include "test/test_common/simulated_time_system.h" #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -211,9 +212,10 @@ class ConfigProviderImplTest : public testing::Test { provider_manager_ = std::make_unique(factory_context_.admin_); } - Event::SimulatedTimeSystem& timeSystem() { return factory_context_.timeSystem(); } + Event::SimulatedTimeSystem& timeSystem() { return time_system_; } protected: + Event::SimulatedTimeSystem time_system_; NiceMock factory_context_; std::unique_ptr provider_manager_; }; diff --git a/test/common/router/BUILD b/test/common/router/BUILD index 3394cbd55fbd7..1c303657c3d05 100644 --- a/test/common/router/BUILD +++ b/test/common/router/BUILD @@ -67,6 +67,7 @@ envoy_cc_test( "//test/mocks/server:server_mocks", "//test/mocks/thread_local:thread_local_mocks", "//test/mocks/upstream:upstream_mocks", + "//test/test_common:simulated_time_system_lib", "//test/test_common:utility_lib", ], ) diff --git a/test/common/router/rds_impl_test.cc b/test/common/router/rds_impl_test.cc index 81e52bba4b995..7fa8cac0dc856 100644 --- a/test/common/router/rds_impl_test.cc +++ b/test/common/router/rds_impl_test.cc @@ -20,6 +20,7 @@ #include "test/mocks/thread_local/mocks.h" #include "test/mocks/upstream/mocks.h" #include "test/test_common/printers.h" +#include "test/test_common/simulated_time_system.h" #include "test/test_common/utility.h" #include "gmock/gmock.h" @@ -67,8 +68,9 @@ class RdsTestBase : public testing::Test { })); } - Event::SimulatedTimeSystem& timeSystem() { return factory_context_.timeSystem(); } + Event::SimulatedTimeSystem& timeSystem() { return time_system_; } + Event::SimulatedTimeSystem time_system_; NiceMock factory_context_; Http::MockAsyncClientRequest request_; Http::AsyncClient::Callbacks* callbacks_{}; diff --git a/test/extensions/transport_sockets/tls/ssl_socket_test.cc b/test/extensions/transport_sockets/tls/ssl_socket_test.cc index 9b2e9cc74fe50..d6aa02af41d97 100644 --- a/test/extensions/transport_sockets/tls/ssl_socket_test.cc +++ b/test/extensions/transport_sockets/tls/ssl_socket_test.cc @@ -183,9 +183,8 @@ void testUtil(const TestUtilOptions& options) { ServerSslSocketFactory server_ssl_socket_factory(std::move(server_cfg), manager, server_stats_store, std::vector{}); - DangerousDeprecatedTestTime test_time; Api::ApiPtr api = Api::createApiForTest(server_stats_store); - Event::DispatcherImpl dispatcher(test_time.timeSystem(), *api); + Event::DispatcherImpl dispatcher(time_system, *api); Network::TcpListenSocket socket(Network::Test::getCanonicalLoopbackAddress(options.version()), nullptr, true); Network::MockListenerCallbacks callbacks; @@ -400,9 +399,8 @@ const std::string testUtilV2(const TestUtilOptionsV2& options) { ServerSslSocketFactory server_ssl_socket_factory(std::move(server_cfg), manager, server_stats_store, server_names); - DangerousDeprecatedTestTime test_time; Api::ApiPtr api = Api::createApiForTest(server_stats_store); - Event::DispatcherImpl dispatcher(test_time.timeSystem(), *api); + Event::DispatcherImpl dispatcher(time_system, *api); Network::TcpListenSocket socket(Network::Test::getCanonicalLoopbackAddress(options.version()), nullptr, true); NiceMock callbacks; @@ -574,11 +572,15 @@ class SslSocketTest : public SslCertsTest, protected: SslSocketTest() : api_(Api::createApiForTest(stats_store_)), - dispatcher_(std::make_unique(test_time_.timeSystem(), *api_)) {} + dispatcher_(std::make_unique(time_system_, *api_)) {} + + void testClientSessionResumption(const std::string& server_ctx_yaml, + const std::string& client_ctx_yaml, bool expect_reuse, + const Network::Address::IpVersion version); + Event::SimulatedTimeSystem time_system_; Stats::IsolatedStoreImpl stats_store_; Api::ApiPtr api_; - DangerousDeprecatedTestTime test_time_; std::unique_ptr dispatcher_; }; @@ -1861,8 +1863,7 @@ TEST_P(SslSocketTest, FlushCloseDuringHandshake) { envoy::api::v2::auth::DownstreamTlsContext tls_context; MessageUtil::loadFromYaml(TestEnvironment::substitute(server_ctx_yaml), tls_context); auto server_cfg = std::make_unique(tls_context, factory_context_); - Event::SimulatedTimeSystem time_system; - ContextManagerImpl manager(time_system); + ContextManagerImpl manager(time_system_); Stats::IsolatedStoreImpl server_stats_store; ServerSslSocketFactory server_ssl_socket_factory(std::move(server_cfg), manager, server_stats_store, std::vector{}); @@ -1922,8 +1923,7 @@ TEST_P(SslSocketTest, HalfClose) { envoy::api::v2::auth::DownstreamTlsContext server_tls_context; MessageUtil::loadFromYaml(TestEnvironment::substitute(server_ctx_yaml), server_tls_context); auto server_cfg = std::make_unique(server_tls_context, factory_context_); - Event::SimulatedTimeSystem time_system; - ContextManagerImpl manager(time_system); + ContextManagerImpl manager(time_system_); Stats::IsolatedStoreImpl server_stats_store; ServerSslSocketFactory server_ssl_socket_factory(std::move(server_cfg), manager, server_stats_store, std::vector{}); @@ -2012,8 +2012,7 @@ TEST_P(SslSocketTest, ClientAuthMultipleCAs) { envoy::api::v2::auth::DownstreamTlsContext server_tls_context; MessageUtil::loadFromYaml(TestEnvironment::substitute(server_ctx_yaml), server_tls_context); auto server_cfg = std::make_unique(server_tls_context, factory_context); - Event::SimulatedTimeSystem time_system; - ContextManagerImpl manager(time_system); + ContextManagerImpl manager(time_system_); Stats::IsolatedStoreImpl server_stats_store; ServerSslSocketFactory server_ssl_socket_factory(std::move(server_cfg), manager, server_stats_store, std::vector{}); @@ -2116,9 +2115,8 @@ void testTicketSessionResumption(const std::string& server_ctx_yaml1, true); NiceMock callbacks; Network::MockConnectionHandler connection_handler; - DangerousDeprecatedTestTime test_time; Api::ApiPtr api = Api::createApiForTest(server_stats_store); - Event::DispatcherImpl dispatcher(test_time.timeSystem(), *api); + Event::DispatcherImpl dispatcher(time_system, *api); Network::ListenerPtr listener1 = dispatcher.createListener(socket1, callbacks, true, false); Network::ListenerPtr listener2 = dispatcher.createListener(socket2, callbacks, true, false); @@ -2507,8 +2505,7 @@ TEST_P(SslSocketTest, ClientAuthCrossListenerSessionResumption) { envoy::api::v2::auth::DownstreamTlsContext tls_context2; MessageUtil::loadFromYaml(TestEnvironment::substitute(server2_ctx_yaml), tls_context2); auto server2_cfg = std::make_unique(tls_context2, factory_context_); - Event::SimulatedTimeSystem time_system; - ContextManagerImpl manager(time_system); + ContextManagerImpl manager(time_system_); Stats::IsolatedStoreImpl server_stats_store; ServerSslSocketFactory server_ssl_socket_factory(std::move(server_cfg), manager, server_stats_store, std::vector{}); @@ -2609,14 +2606,14 @@ TEST_P(SslSocketTest, ClientAuthCrossListenerSessionResumption) { EXPECT_EQ(0UL, client_stats_store.counter("ssl.session_reused").value()); } -void testClientSessionResumption(const std::string& server_ctx_yaml, - const std::string& client_ctx_yaml, bool expect_reuse, - const Network::Address::IpVersion version) { +void SslSocketTest::testClientSessionResumption(const std::string& server_ctx_yaml, + const std::string& client_ctx_yaml, + bool expect_reuse, + const Network::Address::IpVersion version) { InSequence s; testing::NiceMock factory_context; - Event::SimulatedTimeSystem time_system; - ContextManagerImpl manager(time_system); + ContextManagerImpl manager(time_system_); envoy::api::v2::auth::DownstreamTlsContext server_ctx_proto; MessageUtil::loadFromYaml(TestEnvironment::substitute(server_ctx_yaml), server_ctx_proto); @@ -2630,7 +2627,7 @@ void testClientSessionResumption(const std::string& server_ctx_yaml, NiceMock callbacks; Network::MockConnectionHandler connection_handler; Api::ApiPtr api = Api::createApiForTest(server_stats_store); - Event::DispatcherImpl dispatcher(time_system, *api); + Event::DispatcherImpl dispatcher(time_system_, *api); Network::ListenerPtr listener = dispatcher.createListener(socket, callbacks, true, false); Network::ConnectionPtr server_connection; @@ -2883,8 +2880,7 @@ TEST_P(SslSocketTest, SslError) { envoy::api::v2::auth::DownstreamTlsContext tls_context; MessageUtil::loadFromYaml(TestEnvironment::substitute(server_ctx_yaml), tls_context); auto server_cfg = std::make_unique(tls_context, factory_context_); - Event::SimulatedTimeSystem time_system; - ContextManagerImpl manager(time_system); + ContextManagerImpl manager(time_system_); Stats::IsolatedStoreImpl server_stats_store; ServerSslSocketFactory server_ssl_socket_factory(std::move(server_cfg), manager, server_stats_store, std::vector{}); @@ -3486,8 +3482,7 @@ TEST_P(SslSocketTest, DownstreamNotReadySslSocket) { EXPECT_TRUE(server_cfg->tlsCertificates().empty()); EXPECT_FALSE(server_cfg->isReady()); - Event::SimulatedTimeSystem time_system; - ContextManagerImpl manager(time_system); + ContextManagerImpl manager(time_system_); ServerSslSocketFactory server_ssl_socket_factory(std::move(server_cfg), manager, stats_store, std::vector{}); auto transport_socket = server_ssl_socket_factory.createTransportSocket(nullptr); @@ -3510,7 +3505,6 @@ TEST_P(SslSocketTest, UpstreamNotReadySslSocket) { NiceMock random; NiceMock cluster_manager; NiceMock init_manager; - Event::SimulatedTimeSystem time_system; EXPECT_CALL(factory_context, localInfo()).WillOnce(ReturnRef(local_info)); EXPECT_CALL(factory_context, dispatcher()).WillOnce(ReturnRef(dispatcher)); EXPECT_CALL(factory_context, random()).WillOnce(ReturnRef(random)); @@ -3527,7 +3521,7 @@ TEST_P(SslSocketTest, UpstreamNotReadySslSocket) { EXPECT_TRUE(client_cfg->tlsCertificates().empty()); EXPECT_FALSE(client_cfg->isReady()); - ContextManagerImpl manager(time_system); + ContextManagerImpl manager(time_system_); ClientSslSocketFactory client_ssl_socket_factory(std::move(client_cfg), manager, stats_store); auto transport_socket = client_ssl_socket_factory.createTransportSocket(nullptr); EXPECT_EQ(EMPTY_STRING, transport_socket->protocol()); @@ -3546,8 +3540,7 @@ class SslReadBufferLimitTest : public SslSocketTest { downstream_tls_context_); auto server_cfg = std::make_unique(downstream_tls_context_, factory_context_); - Event::SimulatedTimeSystem time_system; - manager_ = std::make_unique(time_system); + manager_ = std::make_unique(time_system_); server_ssl_socket_factory_ = std::make_unique( std::move(server_cfg), *manager_, server_stats_store_, std::vector{}); @@ -3636,7 +3629,7 @@ class SslReadBufferLimitTest : public SslSocketTest { MockWatermarkBuffer* client_write_buffer = nullptr; MockBufferFactory* factory = new StrictMock; dispatcher_ = std::make_unique( - test_time_.timeSystem(), Buffer::WatermarkFactoryPtr{factory}, *api_); + time_system_, Buffer::WatermarkFactoryPtr{factory}, *api_); // By default, expect 4 buffers to be created - the client and server read and write buffers. EXPECT_CALL(*factory, create_(_, _)) diff --git a/test/integration/utility.cc b/test/integration/utility.cc index 76b280b4cbff8..5eb7c783cc751 100644 --- a/test/integration/utility.cc +++ b/test/integration/utility.cc @@ -54,8 +54,6 @@ void BufferingStreamDecoder::onComplete() { void BufferingStreamDecoder::onResetStream(Http::StreamResetReason) { ADD_FAILURE(); } -DangerousDeprecatedTestTime IntegrationUtil::evil_singleton_test_time_; - BufferingStreamDecoderPtr IntegrationUtil::makeSingleRequest(const Network::Address::InstanceConstSharedPtr& addr, const std::string& method, const std::string& url, @@ -64,7 +62,8 @@ IntegrationUtil::makeSingleRequest(const Network::Address::InstanceConstSharedPt NiceMock mock_stats_store; Api::Impl api(std::chrono::milliseconds(9000), Thread::threadFactoryForTest(), mock_stats_store); - Event::DispatcherPtr dispatcher(api.allocateDispatcher(evil_singleton_test_time_.timeSystem())); + Event::GlobalTimeSystem time_system; + Event::DispatcherPtr dispatcher(api.allocateDispatcher(*time_system)); std::shared_ptr cluster{new NiceMock()}; Upstream::HostDescriptionConstSharedPtr host_description{ Upstream::makeTestHostDescription(cluster, "tcp://127.0.0.1:80")}; @@ -112,7 +111,8 @@ RawConnectionDriver::RawConnectionDriver(uint32_t port, Buffer::Instance& initia ReadCallback data_callback, Network::Address::IpVersion version) { api_ = Api::createApiForTest(stats_store_); - dispatcher_ = api_->allocateDispatcher(IntegrationUtil::evil_singleton_test_time_.timeSystem()); + Event::GlobalTimeSystem time_system; + dispatcher_ = api_->allocateDispatcher(*time_system); callbacks_ = std::make_unique(); client_ = dispatcher_->createClientConnection( Network::Utility::resolveUrl( diff --git a/test/integration/utility.h b/test/integration/utility.h index 23630096ef289..0e3a483cd1bd8 100644 --- a/test/integration/utility.h +++ b/test/integration/utility.h @@ -146,9 +146,6 @@ class IntegrationUtil { const std::string& body, Http::CodecClient::Type type, Network::Address::IpVersion ip_version, const std::string& host = "host", const std::string& content_type = ""); - - // TODO(jmarantz): this should be injectable. - static DangerousDeprecatedTestTime evil_singleton_test_time_; }; // A set of connection callbacks which tracks connection state. diff --git a/test/mocks/common.h b/test/mocks/common.h index 3c787de7b8a74..5908a0f9a804c 100644 --- a/test/mocks/common.h +++ b/test/mocks/common.h @@ -49,18 +49,18 @@ class MockTimeSystem : public Event::TestTimeSystem { // matches recent behavior, where real-time timers were created directly in libevent // by dispatcher_impl.cc. Event::SchedulerPtr createScheduler(Event::Libevent::BasePtr& base) override { - return test_time_.timeSystem().createScheduler(base); + return real_time_.createScheduler(base); } - void sleep(const Duration& duration) override { test_time_.timeSystem().sleep(duration); } + void sleep(const Duration& duration) override { real_time_.sleep(duration); } Thread::CondVar::WaitStatus waitFor(Thread::MutexBasicLockable& mutex, Thread::CondVar& condvar, const Duration& duration) noexcept EXCLUSIVE_LOCKS_REQUIRED(mutex) override { - return test_time_.timeSystem().waitFor(mutex, condvar, duration); + return real_time_.waitFor(mutex, condvar, duration); // NO_CHECK_FORMAT(real_time) } MOCK_METHOD0(systemTime, SystemTime()); MOCK_METHOD0(monotonicTime, MonotonicTime()); - DangerousDeprecatedTestTime test_time_; + Event::TestRealTimeSystem real_time_; // NO_CHECK_FORMAT(real_time) }; class MockTokenBucket : public TokenBucket { diff --git a/test/mocks/server/BUILD b/test/mocks/server/BUILD index cf04ace8041f9..fd64aafbf173c 100644 --- a/test/mocks/server/BUILD +++ b/test/mocks/server/BUILD @@ -43,6 +43,6 @@ envoy_cc_mock( "//test/mocks/thread_local:thread_local_mocks", "//test/mocks/tracing:tracing_mocks", "//test/mocks/upstream:upstream_mocks", - "//test/test_common:simulated_time_system_lib", + "//test/test_common:test_time_lib", ], ) diff --git a/test/mocks/server/mocks.h b/test/mocks/server/mocks.h index 8f89258f65104..506b91d5a3d72 100644 --- a/test/mocks/server/mocks.h +++ b/test/mocks/server/mocks.h @@ -39,7 +39,7 @@ #include "test/mocks/thread_local/mocks.h" #include "test/mocks/tracing/mocks.h" #include "test/mocks/upstream/mocks.h" -#include "test/test_common/simulated_time_system.h" +#include "test/test_common/test_time_system.h" #include "absl/strings/string_view.h" #include "gmock/gmock.h" @@ -355,7 +355,7 @@ class MockInstance : public Instance { MOCK_METHOD0(localInfo, const LocalInfo::LocalInfo&()); MOCK_CONST_METHOD0(statsFlushInterval, std::chrono::milliseconds()); - Event::TestTimeSystem& timeSystem() override { return test_time_.timeSystem(); } + Event::TestTimeSystem& timeSystem() override { return time_system_; } std::unique_ptr secret_manager_; testing::NiceMock thread_local_; @@ -364,7 +364,7 @@ class MockInstance : public Instance { new testing::NiceMock()}; testing::NiceMock api_; testing::NiceMock admin_; - DangerousDeprecatedTestTime test_time_; + Event::GlobalTimeSystem time_system_; testing::NiceMock cluster_manager_; Thread::MutexBasicLockable access_log_lock_; testing::NiceMock runtime_loader_; @@ -429,7 +429,7 @@ class MockFactoryContext : public virtual FactoryContext { MOCK_CONST_METHOD0(localInfo, const LocalInfo::LocalInfo&()); MOCK_CONST_METHOD0(listenerMetadata, const envoy::api::v2::core::Metadata&()); MOCK_METHOD0(timeSource, TimeSource&()); - Event::SimulatedTimeSystem& timeSystem() { return time_system_; } + Event::TestTimeSystem& timeSystem() { return time_system_; } Http::Context& httpContext() override { return http_context_; } testing::NiceMock access_log_manager_; @@ -446,7 +446,7 @@ class MockFactoryContext : public virtual FactoryContext { Singleton::ManagerPtr singleton_manager_; testing::NiceMock admin_; Stats::IsolatedStoreImpl listener_scope_; - Event::SimulatedTimeSystem time_system_; + Event::GlobalTimeSystem time_system_; testing::NiceMock overload_manager_; Tracing::HttpNullTracer null_tracer_; Http::ContextImpl http_context_; diff --git a/test/server/BUILD b/test/server/BUILD index 8f0db6396d0ff..0ba84d4870b45 100644 --- a/test/server/BUILD +++ b/test/server/BUILD @@ -102,6 +102,7 @@ envoy_cc_test( "//test/mocks:common_lib", "//test/mocks/server:server_mocks", "//test/mocks/stats:stats_mocks", + "//test/test_common:simulated_time_system_lib", "//test/test_common:utility_lib", ], ) diff --git a/test/test_common/simulated_time_system.cc b/test/test_common/simulated_time_system.cc index f3b88ffdda463..6f20e61208f69 100644 --- a/test/test_common/simulated_time_system.cc +++ b/test/test_common/simulated_time_system.cc @@ -64,8 +64,7 @@ class SimulatedTimeSystemHelper::Alarm : public TimerImpl { // Compare two alarms, based on wakeup time and insertion order. Returns true if // a comes before b. -bool SimulatedTimeSystemHelper::CompareAlarms::operator()( - const Alarm* a, const Alarm* b) const { +bool SimulatedTimeSystemHelper::CompareAlarms::operator()(const Alarm* a, const Alarm* b) const { if (a != b) { if (a->time() < b->time()) { return true; @@ -106,7 +105,8 @@ void SimulatedTimeSystemHelper::Alarm::Alarm::disableTimer() { } } -void SimulatedTimeSystemHelper::Alarm::Alarm::enableTimer(const std::chrono::milliseconds& duration) { +void SimulatedTimeSystemHelper::Alarm::Alarm::enableTimer( + const std::chrono::milliseconds& duration) { disableTimer(); armed_ = true; if (duration.count() == 0) { @@ -155,8 +155,8 @@ void SimulatedTimeSystemHelper::sleep(const Duration& duration) { } Thread::CondVar::WaitStatus SimulatedTimeSystemHelper::waitFor(Thread::MutexBasicLockable& mutex, - Thread::CondVar& condvar, - const Duration& duration) noexcept { + Thread::CondVar& condvar, + const Duration& duration) noexcept { const Duration real_time_poll_delay(std::chrono::milliseconds(50)); const MonotonicTime end_time = monotonicTime() + duration; diff --git a/test/test_common/simulated_time_system.h b/test/test_common/simulated_time_system.h index c4dddb7ee60c0..6a09e648c53b3 100644 --- a/test/test_common/simulated_time_system.h +++ b/test/test_common/simulated_time_system.h @@ -99,11 +99,11 @@ class SimulatedTimeSystemHelper : public TestTimeSystem { }; class SimulatedTimeSystem : public DelegatingTestTimeSystem { - public: +public: void setMonotonicTime(const MonotonicTime& monotonic_time) { - time_system_->setMonotonicTime(monotonic_time); + timeSystem().setMonotonicTime(monotonic_time); } - void setSystemTime(const SystemTime& system_time) { time_system_->setSystemTime(system_time); } + void setSystemTime(const SystemTime& system_time) { timeSystem().setSystemTime(system_time); } template void setMonotonicTime(const Duration& duration) { setMonotonicTime(MonotonicTime(duration)); @@ -114,7 +114,7 @@ class SimulatedTimeSystem : public DelegatingTestTimeSystem { public: - void sleep(const Duration& duration) override { lazyInit().sleep(duration); } - Thread::CondVar::WaitStatus - waitFor(Thread::MutexBasicLockable& mutex, Thread::CondVar& condvar, - const Duration& duration) noexcept EXCLUSIVE_LOCKS_REQUIRED(mutex) override { - return lazyInit().waitFor(mutex, condvar, duration); - } - SchedulerPtr createScheduler(Libevent::BasePtr& base_ptr) override { - return lazyInit().createScheduler(base_ptr); - } - SystemTime systemTime() override { return lazyInit().systemTime(); } - MonotonicTime monotonicTime() override { return lazyInit().monotonicTime(); } - - TestTimeSystem& lazyInit() { + TestTimeSystem& timeSystem() override { if (singleton_->timeSystem() == nullptr) { // TODO(jmarantz): Switch default to SimulatedTimeSystem. singleton_->set(new TestRealTimeSystem); @@ -69,7 +57,9 @@ class DangerousDeprecatedTestTime { Event::TestTimeSystem& timeSystem() { return time_system_; } private: - Event::TestRealTimeSystem time_system_; + // Event::TestRealTimeSystem time_system_; + // Event::GlobalTimeSystem time_system_; + Event::DelegatingTestTimeSystem time_system_; }; } // namespace Envoy diff --git a/test/test_common/test_time_system.h b/test/test_common/test_time_system.h index 9bd04942d831c..577e6a312a908 100644 --- a/test/test_common/test_time_system.h +++ b/test/test_common/test_time_system.h @@ -67,8 +67,30 @@ class TestTimeSystem : public Event::TimeSystem { } }; -template class DelegatingTestTimeSystem : public TestTimeSystem { - public: +template class DelegatingTestTimeSystemBase : public TestTimeSystem { +public: + void sleep(const Duration& duration) override { timeSystem().sleep(duration); } + + Thread::CondVar::WaitStatus + waitFor(Thread::MutexBasicLockable& mutex, Thread::CondVar& condvar, + const Duration& duration) noexcept EXCLUSIVE_LOCKS_REQUIRED(mutex) override { + return timeSystem().waitFor(mutex, condvar, duration); + } + + SchedulerPtr createScheduler(Libevent::BasePtr& base_ptr) override { + return timeSystem().createScheduler(base_ptr); + } + SystemTime systemTime() override { return timeSystem().systemTime(); } + MonotonicTime monotonicTime() override { return timeSystem().monotonicTime(); } + + TimeSystemVariant& operator*() { return timeSystem(); } + + virtual TimeSystemVariant& timeSystem() PURE; +}; + +template +class DelegatingTestTimeSystem : public DelegatingTestTimeSystemBase { +public: DelegatingTestTimeSystem() { TestTimeSystem* time_system = singleton_->timeSystem(); if (time_system == nullptr) { @@ -80,26 +102,10 @@ template class DelegatingTestTimeSystem : public TestTi } } - void sleep(const Duration& duration) override { time_system_->sleep(duration); } - - Thread::CondVar::WaitStatus - waitFor(Thread::MutexBasicLockable& mutex, Thread::CondVar& condvar, - const Duration& duration) noexcept EXCLUSIVE_LOCKS_REQUIRED(mutex) override { - return time_system_->waitFor(mutex, condvar, duration); - } - - SchedulerPtr createScheduler(Libevent::BasePtr& base_ptr) override { - return time_system_->createScheduler(base_ptr); - } - SystemTime systemTime() override { return time_system_->systemTime(); } - MonotonicTime monotonicTime() override { return time_system_->monotonicTime(); } - - TimeSystemVariant& operator*() { return *time_system_; } + TimeSystemVariant& timeSystem() override { return *time_system_; } - protected: +private: TimeSystemVariant* time_system_; - - private: Test::Global singleton_; }; From 8ecee51b1bbf1566196e9d4e8a70a9a6c0da5638 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Thu, 24 Jan 2019 12:52:49 -0500 Subject: [PATCH 04/18] Removed some TimeProviders. Signed-off-by: Joshua Marantz --- .../upstream/cluster_manager_impl_test.cc | 3 +- .../upstream/load_stats_reporter_test.cc | 3 +- test/mocks/event/mocks.cc | 2 +- test/test_common/test_time.cc | 31 ++----------------- test/test_common/test_time.h | 5 +-- 5 files changed, 8 insertions(+), 36 deletions(-) diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index 3ed736c900cd5..0690de8fde0ed 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -167,7 +167,7 @@ envoy::config::bootstrap::v2::Bootstrap parseBootstrapFromV2Yaml(const std::stri return bootstrap; } -class ClusterManagerImplTest : public Event::SimulatedTimeProvider, public testing::Test { +class ClusterManagerImplTest : public testing::Test { public: ClusterManagerImplTest() : api_(Api::createApiForTest(stats_store_)) {} @@ -244,6 +244,7 @@ class ClusterManagerImplTest : public Event::SimulatedTimeProvider, public testi } Stats::IsolatedStoreImpl stats_store_; + Event::SimulatedTimeSystem time_system_; Api::ApiPtr api_; NiceMock factory_; std::unique_ptr cluster_manager_; diff --git a/test/common/upstream/load_stats_reporter_test.cc b/test/common/upstream/load_stats_reporter_test.cc index b3e923793a380..beed927a71704 100644 --- a/test/common/upstream/load_stats_reporter_test.cc +++ b/test/common/upstream/load_stats_reporter_test.cc @@ -26,7 +26,7 @@ using testing::Return; namespace Envoy { namespace Upstream { -class LoadStatsReporterTest : public Event::SimulatedTimeProvider, public testing::Test { +class LoadStatsReporterTest : public testing::Test { public: LoadStatsReporterTest() : retry_timer_(new Event::MockTimer()), response_timer_(new Event::MockTimer()), @@ -66,6 +66,7 @@ class LoadStatsReporterTest : public Event::SimulatedTimeProvider, public testin load_stats_reporter_->onReceiveMessage(std::move(response)); } + Event::SimulatedTimeSystem time_system_; NiceMock cm_; Event::MockDispatcher dispatcher_; Stats::IsolatedStoreImpl stats_store_; diff --git a/test/mocks/event/mocks.cc b/test/mocks/event/mocks.cc index 34ed99e236e0f..59d724f85e4da 100644 --- a/test/mocks/event/mocks.cc +++ b/test/mocks/event/mocks.cc @@ -13,7 +13,7 @@ using testing::SaveArg; namespace Envoy { namespace Event { -MockDispatcher::MockDispatcher() { //: time_system_(&test_time_.timeSystem()) { +MockDispatcher::MockDispatcher() { ON_CALL(*this, clearDeferredDeleteList()).WillByDefault(Invoke([this]() -> void { to_delete_.clear(); })); diff --git a/test/test_common/test_time.cc b/test/test_common/test_time.cc index 59052f084bcab..24bd072e6b4fb 100644 --- a/test/test_common/test_time.cc +++ b/test/test_common/test_time.cc @@ -18,36 +18,9 @@ Thread::CondVar::WaitStatus TestRealTimeSystem::waitFor(Thread::MutexBasicLockab return condvar.waitFor(lock, duration); } -SystemTime TestRealTimeSystem::systemTime() { - // ASSERT(!SimulatedTimeSystem::hasInstance()); - return real_time_system_.systemTime(); -} - -MonotonicTime TestRealTimeSystem::monotonicTime() { - // ASSERT(!SimulatedTimeSystem::hasInstance()); - return real_time_system_.monotonicTime(); -} - -/* -MockTimeSystem::MockTimeSystem() {} -MockTimeSystem::~MockTimeSystem() {} +SystemTime TestRealTimeSystem::systemTime() { return real_time_system_.systemTime(); } -// TODO(#4160): Eliminate all uses of MockTimeSystem, replacing with SimulatedTimeSystem, -// where timer callbacks are triggered by the advancement of time. This implementation -// matches recent behavior, where real-time timers were created directly in libevent -// by dispatcher_impl.cc. -Event::SchedulerPtr MockTimeSystem::createScheduler(Event::Libevent::BasePtr& base) { - return real_time_system_.createScheduler(base); -} - -void MockTimeSystem::sleep(const Duration& duration) { real_time_system_.sleep(duration); } - -Thread::CondVar::WaitStatus -MockTimeSystem::waitFor(Thread::MutexBasicLockable& mutex, Thread::CondVar& condvar, - const Duration& duration) noexcept EXCLUSIVE_LOCKS_REQUIRED(mutex) { - return real_time_system_.waitFor(mutex, condvar, duration); -} -*/ +MonotonicTime TestRealTimeSystem::monotonicTime() { return real_time_system_.monotonicTime(); } } // namespace Event } // namespace Envoy diff --git a/test/test_common/test_time.h b/test/test_common/test_time.h index 2028e8d124dd5..a30cff51034f6 100644 --- a/test/test_common/test_time.h +++ b/test/test_common/test_time.h @@ -48,8 +48,7 @@ class GlobalTimeSystem : public DelegatingTestTimeSystemBase { // Instantiates real-time sources for testing purposes. In general, this is a // bad idea, and tests should use simulated or mock time. // -// TODO(#4160): change all references to this class to instantiate instead to -// some kind of mock or simulated-time source. +// TODO(#4160): change most references to this class to SimulatedTimeSystem. class DangerousDeprecatedTestTime { public: DangerousDeprecatedTestTime(); @@ -57,8 +56,6 @@ class DangerousDeprecatedTestTime { Event::TestTimeSystem& timeSystem() { return time_system_; } private: - // Event::TestRealTimeSystem time_system_; - // Event::GlobalTimeSystem time_system_; Event::DelegatingTestTimeSystem time_system_; }; From d5d2c68d52c71068eb2c1e033dc11791481ff19d Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Thu, 24 Jan 2019 13:07:35 -0500 Subject: [PATCH 05/18] Remove rest of time-system providers. Signed-off-by: Joshua Marantz --- test/common/config/grpc_mux_impl_test.cc | 14 ++++++++------ test/test_common/simulated_time_system.h | 5 ----- 2 files changed, 8 insertions(+), 11 deletions(-) diff --git a/test/common/config/grpc_mux_impl_test.cc b/test/common/config/grpc_mux_impl_test.cc index b62b204acddc9..d20d699fbdec6 100644 --- a/test/common/config/grpc_mux_impl_test.cc +++ b/test/common/config/grpc_mux_impl_test.cc @@ -36,10 +36,6 @@ namespace Envoy { namespace Config { namespace { -struct MockTimeProvider { - Event::DelegatingTestTimeSystem mock_time_system_; -}; - // We test some mux specific stuff below, other unit test coverage for singleton use of GrpcMuxImpl // is provided in [grpc_]subscription_impl_test.cc. class GrpcMuxImplTestBase : public testing::Test { @@ -96,7 +92,10 @@ class GrpcMuxImplTestBase : public testing::Test { Envoy::Config::RateLimitSettings rate_limit_settings_; }; -class GrpcMuxImplTest : public Event::SimulatedTimeProvider, public GrpcMuxImplTestBase {}; +class GrpcMuxImplTest : public GrpcMuxImplTestBase { +public: + Event::SimulatedTimeSystem time_system_; +}; // Validate behavior when multiple type URL watches are maintained, watches are created/destroyed // (via RAII). @@ -332,7 +331,10 @@ TEST_F(GrpcMuxImplTest, WatchDemux) { // Exactly one test requires a mock time system to provoke behavior that cannot // easily be achieved with a SimulatedTimeSystem. -class GrpcMuxImplTestWithMockTimeSystem : public MockTimeProvider, public GrpcMuxImplTestBase {}; +class GrpcMuxImplTestWithMockTimeSystem : public GrpcMuxImplTestBase { +public: + Event::DelegatingTestTimeSystem mock_time_system_; +}; // Verifies that rate limiting is not enforced with defaults. TEST_F(GrpcMuxImplTestWithMockTimeSystem, TooManyRequestsWithDefaultSettings) { diff --git a/test/test_common/simulated_time_system.h b/test/test_common/simulated_time_system.h index 6a09e648c53b3..e00edc76ce7bd 100644 --- a/test/test_common/simulated_time_system.h +++ b/test/test_common/simulated_time_system.h @@ -113,10 +113,5 @@ class SimulatedTimeSystem : public DelegatingTestTimeSystem Date: Thu, 24 Jan 2019 19:33:50 -0500 Subject: [PATCH 06/18] Clean up comments, add unit test. Signed-off-by: Joshua Marantz --- test/test_common/BUILD | 11 ++++- test/test_common/test_time.h | 2 +- test/test_common/test_time_system.h | 55 ++++++++++++++--------- test/test_common/test_time_system_test.cc | 48 ++++++++++++++++++++ 4 files changed, 94 insertions(+), 22 deletions(-) create mode 100644 test/test_common/test_time_system_test.cc diff --git a/test/test_common/BUILD b/test/test_common/BUILD index 656a26b864f23..67fd473f6d9b2 100644 --- a/test/test_common/BUILD +++ b/test/test_common/BUILD @@ -191,6 +191,15 @@ envoy_cc_test( srcs = ["simulated_time_system_test.cc"], deps = [ ":simulated_time_system_lib", - "//test/test_common:utility_lib", + ":utility_lib", + ], +) + +envoy_cc_test( + name = "test_time_system_test", + srcs = ["test_time_system_test.cc"], + deps = [ + ":simulated_time_system_lib", + ":test_time_lib", ], ) diff --git a/test/test_common/test_time.h b/test/test_common/test_time.h index a30cff51034f6..02edee7a65f08 100644 --- a/test/test_common/test_time.h +++ b/test/test_common/test_time.h @@ -53,7 +53,7 @@ class DangerousDeprecatedTestTime { public: DangerousDeprecatedTestTime(); - Event::TestTimeSystem& timeSystem() { return time_system_; } + Event::TestTimeSystem& timeSystem() { return time_system_.timeSystem(); } private: Event::DelegatingTestTimeSystem time_system_; diff --git a/test/test_common/test_time_system.h b/test/test_common/test_time_system.h index 577e6a312a908..4d385a3c01313 100644 --- a/test/test_common/test_time_system.h +++ b/test/test_common/test_time_system.h @@ -12,25 +12,6 @@ namespace Event { class TestTimeSystem; -// Ensures that only one type of time-system is instantiated at a time. -class SingletonTimeSystemHelper { -public: - explicit SingletonTimeSystemHelper() : time_system_(nullptr) {} - - void set(TestTimeSystem* time_system) { - if (time_system_ == nullptr) { - time_system_.reset(time_system); - } else { - ASSERT(time_system_.get() == time_system); - } - } - - TestTimeSystem* timeSystem() { return time_system_.get(); } - -private: - std::unique_ptr time_system_; -}; - // Adds sleep() and waitFor() interfaces to Event::TimeSystem. class TestTimeSystem : public Event::TimeSystem { public: @@ -67,6 +48,33 @@ class TestTimeSystem : public Event::TimeSystem { } }; +// There should only be one instance of any time-system resident in a test +// process at once. This helper class is used with Test::Global to help enforce +// that with an ASSERT. Each time-system derivation should have a helper +// implementation which is referenced from a delegate (see +// DelegatingTestTimeSystemBase). In each delegate, a SingletonTimeSystemHelper +// should be instantiated via Test::Global. Only one +// instance of SingletonTimeSystemHelper per process, at a time. When all +// references to the delegates are destructed, the singleton will be destroyed +// as well, so each test-method will get a fresh start. +class SingletonTimeSystemHelper { +public: + SingletonTimeSystemHelper() : time_system_(nullptr) {} + + void set(TestTimeSystem* time_system) { + ASSERT(time_system_ == nullptr); + time_system_.reset(time_system); + } + + TestTimeSystem* timeSystem() { return time_system_.get(); } + +private: + std::unique_ptr time_system_; +}; + +// Implements the TestTimeSystem interface, delegating implementation of all +// methods to a TestTimeSystem reference supplied by a timeSystem() method in a +// subclass. template class DelegatingTestTimeSystemBase : public TestTimeSystem { public: void sleep(const Duration& duration) override { timeSystem().sleep(duration); } @@ -88,6 +96,13 @@ template class DelegatingTestTimeSystemBase : public T virtual TimeSystemVariant& timeSystem() PURE; }; +// Wraps a concrete time-system in a delegate that ensures thare is only one +// time-system of any variant resident in a process at a time. Attempts to +// instantiate multiple instances of the same type of time-system will simply +// reference the same shared delegate, which will be deleted when the last one +// goes out of scope. Attempts to instantiate different types of type-systems +// will result in a RELEASE_ASSERT. See the testcases in +// test_time_system_test.cc to understand the allowable sequences. template class DelegatingTestTimeSystem : public DelegatingTestTimeSystemBase { public: @@ -98,7 +113,7 @@ class DelegatingTestTimeSystem : public DelegatingTestTimeSystemBaseset(time_system_); } else { time_system_ = dynamic_cast(time_system); - ASSERT(time_system_); + RELEASE_ASSERT(time_system_, "Two different types of time-systems allocated"); } } diff --git a/test/test_common/test_time_system_test.cc b/test/test_common/test_time_system_test.cc new file mode 100644 index 0000000000000..b16966f4d59ef --- /dev/null +++ b/test/test_common/test_time_system_test.cc @@ -0,0 +1,48 @@ +#include "test/test_common/simulated_time_system.h" +#include "test/test_common/test_time.h" +#include "test/test_common/test_time_system.h" + +#include "gtest/gtest.h" + +namespace Envoy { +namespace Event { +namespace Test { + +class TestTimeSystemTest : public testing::Test { +protected: +}; + +TEST_F(TestTimeSystemTest, TwoSimsSameReference) { + SimulatedTimeSystem t1, t2; + EXPECT_EQ(&t1.timeSystem(), &t2.timeSystem()); +} + +TEST_F(TestTimeSystemTest, TwoRealsSameReference) { + DangerousDeprecatedTestTime t1, t2; + EXPECT_EQ(&t1.timeSystem(), &t2.timeSystem()); +} + +TEST_F(TestTimeSystemTest, SimThenRealConflict) { + SimulatedTimeSystem t1; + EXPECT_DEATH({ DangerousDeprecatedTestTime t2; }, + ".*Two different types of time-systems allocated"); +} + +TEST_F(TestTimeSystemTest, SimThenRealSerial) { + { SimulatedTimeSystem t1; } + { DangerousDeprecatedTestTime t2; } +} + +TEST_F(TestTimeSystemTest, RealThenSim) { + DangerousDeprecatedTestTime t1; + EXPECT_DEATH({ SimulatedTimeSystem t2; }, ".*Two different types of time-systems allocated"); +} + +TEST_F(TestTimeSystemTest, RealThenSimSerial) { + { DangerousDeprecatedTestTime t2; } + { SimulatedTimeSystem t1; } +} + +} // namespace Test +} // namespace Event +} // namespace Envoy From 27037c72130caf5e06efdd4bdcbdf22043c58353 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Thu, 24 Jan 2019 21:01:52 -0500 Subject: [PATCH 07/18] Retain a context for sharing the time-system in the contention test. Multi-threaded time-system creation really does not work. Signed-off-by: Joshua Marantz --- test/common/common/mutex_tracer_test.cc | 3 ++- test/exe/main_common_test.cc | 3 ++- test/test_common/contention.cc | 20 +++++++++----------- test/test_common/contention.h | 8 +++++--- 4 files changed, 18 insertions(+), 16 deletions(-) diff --git a/test/common/common/mutex_tracer_test.cc b/test/common/common/mutex_tracer_test.cc index 386dcf6a399ac..042b0469f1994 100644 --- a/test/common/common/mutex_tracer_test.cc +++ b/test/common/common/mutex_tracer_test.cc @@ -75,8 +75,9 @@ TEST_F(MutexTracerTest, TwoThreadsWithContention) { for (int i = 1; i <= 10; ++i) { int64_t curr_num_lifetime_wait_cycles = tracer_.lifetimeWaitCycles(); - Thread::TestUtil::ContentionGenerator::generateContention(tracer_); + Thread::TestUtil::ContentionGenerator contention_generator; + contention_generator.generateContention(tracer_); EXPECT_EQ(tracer_.numContentions(), i); EXPECT_GT(tracer_.currentWaitCycles(), 0); // This shouldn't be hardcoded. EXPECT_GT(tracer_.lifetimeWaitCycles(), 0); diff --git a/test/exe/main_common_test.cc b/test/exe/main_common_test.cc index 01a889d7de366..de7dfb56a7cd0 100644 --- a/test/exe/main_common_test.cc +++ b/test/exe/main_common_test.cc @@ -274,7 +274,8 @@ TEST_P(AdminRequestTest, AdminRequestContentionEnabled) { started_.WaitForNotification(); // Induce contention to guarantee a non-zero num_contentions count. - Thread::TestUtil::ContentionGenerator::generateContention(MutexTracerImpl::getOrCreateTracer()); + Thread::TestUtil::ContentionGenerator contention_generator; + contention_generator.generateContention(MutexTracerImpl::getOrCreateTracer()); std::string response = adminRequest("/contention", "GET"); EXPECT_THAT(response, Not(HasSubstr("not enabled"))); diff --git a/test/test_common/contention.cc b/test/test_common/contention.cc index 8461ef2d47cc6..7e4f493d99cd0 100644 --- a/test/test_common/contention.cc +++ b/test/test_common/contention.cc @@ -7,28 +7,26 @@ namespace Thread { namespace TestUtil { void ContentionGenerator::generateContention(MutexTracerImpl& tracer) { - MutexBasicLockable mu; - Envoy::Thread::ThreadPtr t1 = launchThread(tracer, &mu); - Envoy::Thread::ThreadPtr t2 = launchThread(tracer, &mu); + DangerousDeprecatedTestTime test_time; + Envoy::Thread::ThreadPtr t1 = launchThread(tracer); + Envoy::Thread::ThreadPtr t2 = launchThread(tracer); t1->join(); t2->join(); } -Envoy::Thread::ThreadPtr ContentionGenerator::launchThread(MutexTracerImpl& tracer, - MutexBasicLockable* mu) { +Envoy::Thread::ThreadPtr ContentionGenerator::launchThread(MutexTracerImpl& tracer) { return threadFactoryForTest().createThread( - [&tracer, mu]() -> void { holdUntilContention(tracer, mu); }); + [&tracer, this]() -> void { holdUntilContention(tracer); }); } -void ContentionGenerator::holdUntilContention(MutexTracerImpl& tracer, MutexBasicLockable* mu) { - DangerousDeprecatedTestTime test_time; +void ContentionGenerator::holdUntilContention(MutexTracerImpl& tracer) { int64_t curr_num_contentions = tracer.numContentions(); while (tracer.numContentions() == curr_num_contentions) { - test_time.timeSystem().sleep(std::chrono::milliseconds(1)); - LockGuard lock(*mu); + test_time_.timeSystem().sleep(std::chrono::milliseconds(1)); + LockGuard lock(mutex_); // We hold the lock 90% of the time to ensure both contention and eventual acquisition, which // is needed to bump numContentions(). - test_time.timeSystem().sleep(std::chrono::milliseconds(9)); + test_time_.timeSystem().sleep(std::chrono::milliseconds(9)); } } diff --git a/test/test_common/contention.h b/test/test_common/contention.h index ffa32d62ef1e7..77ce14d4872a8 100644 --- a/test/test_common/contention.h +++ b/test/test_common/contention.h @@ -23,12 +23,14 @@ class ContentionGenerator { /** * Generates at least once occurrence of mutex contention, as measured by tracer. */ - static void generateContention(MutexTracerImpl& tracer); + void generateContention(MutexTracerImpl& tracer); private: - static Envoy::Thread::ThreadPtr launchThread(MutexTracerImpl& tracer, MutexBasicLockable* mu); + ThreadPtr launchThread(MutexTracerImpl& tracer); + void holdUntilContention(MutexTracerImpl& tracer); - static void holdUntilContention(MutexTracerImpl& tracer, MutexBasicLockable* mu); + MutexBasicLockable mutex_; + DangerousDeprecatedTestTime test_time_; }; } // namespace TestUtil From ebc31152743455d7fdef7d6dc19ab9fd70d9fff2 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Thu, 24 Jan 2019 21:14:08 -0500 Subject: [PATCH 08/18] Cleanup some superfluous declarations and commented-out code. Signed-off-by: Joshua Marantz --- test/test_common/contention.cc | 1 - test/test_common/simulated_time_system.cc | 13 ------------- 2 files changed, 14 deletions(-) diff --git a/test/test_common/contention.cc b/test/test_common/contention.cc index 7e4f493d99cd0..10df692c4d8ef 100644 --- a/test/test_common/contention.cc +++ b/test/test_common/contention.cc @@ -7,7 +7,6 @@ namespace Thread { namespace TestUtil { void ContentionGenerator::generateContention(MutexTracerImpl& tracer) { - DangerousDeprecatedTestTime test_time; Envoy::Thread::ThreadPtr t1 = launchThread(tracer); Envoy::Thread::ThreadPtr t2 = launchThread(tracer); t1->join(); diff --git a/test/test_common/simulated_time_system.cc b/test/test_common/simulated_time_system.cc index 6f20e61208f69..26778f7c9c61b 100644 --- a/test/test_common/simulated_time_system.cc +++ b/test/test_common/simulated_time_system.cc @@ -259,18 +259,5 @@ void SimulatedTimeSystemHelper::setSystemTime(const SystemTime& system_time) { } } -/* -SimulatedTimeSystem::SimulatedTimeSystem() { - TestTimeSystem* time_system = singleton_->timeSystem(); - if (time_system == nullptr) { - time_system_ = new SimulatedTimeSystemHelper; - singleton_->set(time_system_); - } else { - time_system_ = dynamic_cast(time_system); - ASSERT(time_system_); - } -} -*/ - } // namespace Event } // namespace Envoy From 5a12891d6c850a15ea10bdc1ab2997755d446473 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Fri, 25 Jan 2019 11:33:42 -0500 Subject: [PATCH 09/18] Remove expected-output string from EXPECT_DEATH, as it seemed to fail on coverage. Signed-off-by: Joshua Marantz --- test/test_common/test_time_system_test.cc | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/test/test_common/test_time_system_test.cc b/test/test_common/test_time_system_test.cc index b16966f4d59ef..be9383d65195f 100644 --- a/test/test_common/test_time_system_test.cc +++ b/test/test_common/test_time_system_test.cc @@ -24,8 +24,7 @@ TEST_F(TestTimeSystemTest, TwoRealsSameReference) { TEST_F(TestTimeSystemTest, SimThenRealConflict) { SimulatedTimeSystem t1; - EXPECT_DEATH({ DangerousDeprecatedTestTime t2; }, - ".*Two different types of time-systems allocated"); + EXPECT_DEATH({ DangerousDeprecatedTestTime t2; }, ""); } TEST_F(TestTimeSystemTest, SimThenRealSerial) { @@ -35,7 +34,7 @@ TEST_F(TestTimeSystemTest, SimThenRealSerial) { TEST_F(TestTimeSystemTest, RealThenSim) { DangerousDeprecatedTestTime t1; - EXPECT_DEATH({ SimulatedTimeSystem t2; }, ".*Two different types of time-systems allocated"); + EXPECT_DEATH({ SimulatedTimeSystem t2; }, ""); } TEST_F(TestTimeSystemTest, RealThenSimSerial) { From 00e8e27d4c8a44d8ca2ed73fb720c45e885c2bf1 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Fri, 25 Jan 2019 13:04:49 -0500 Subject: [PATCH 10/18] Use EXPECT_DEATH_LOG_TO_STDERR which appears to work with assertion messages. Signed-off-by: Joshua Marantz --- test/test_common/BUILD | 1 + test/test_common/test_time_system_test.cc | 7 +++++-- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/test/test_common/BUILD b/test/test_common/BUILD index 67fd473f6d9b2..4b164f56c4b55 100644 --- a/test/test_common/BUILD +++ b/test/test_common/BUILD @@ -201,5 +201,6 @@ envoy_cc_test( deps = [ ":simulated_time_system_lib", ":test_time_lib", + ":utility_lib", ], ) diff --git a/test/test_common/test_time_system_test.cc b/test/test_common/test_time_system_test.cc index be9383d65195f..e1568e47cbdae 100644 --- a/test/test_common/test_time_system_test.cc +++ b/test/test_common/test_time_system_test.cc @@ -1,6 +1,7 @@ #include "test/test_common/simulated_time_system.h" #include "test/test_common/test_time.h" #include "test/test_common/test_time_system.h" +#include "test/test_common/utility.h" #include "gtest/gtest.h" @@ -24,7 +25,8 @@ TEST_F(TestTimeSystemTest, TwoRealsSameReference) { TEST_F(TestTimeSystemTest, SimThenRealConflict) { SimulatedTimeSystem t1; - EXPECT_DEATH({ DangerousDeprecatedTestTime t2; }, ""); + EXPECT_DEATH_LOG_TO_STDERR({ DangerousDeprecatedTestTime t2; }, + ".*Two different types of time-systems allocated.*"); } TEST_F(TestTimeSystemTest, SimThenRealSerial) { @@ -34,7 +36,8 @@ TEST_F(TestTimeSystemTest, SimThenRealSerial) { TEST_F(TestTimeSystemTest, RealThenSim) { DangerousDeprecatedTestTime t1; - EXPECT_DEATH({ SimulatedTimeSystem t2; }, ""); + EXPECT_DEATH_LOG_TO_STDERR( { SimulatedTimeSystem t2; }, + ".*Two different types of time-systems allocated.*"); } TEST_F(TestTimeSystemTest, RealThenSimSerial) { From e14f910b99c97c0676d4877671b1b7a21a6bf957 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Fri, 25 Jan 2019 15:19:55 -0500 Subject: [PATCH 11/18] format fix. Signed-off-by: Joshua Marantz --- test/test_common/test_time_system_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/test_common/test_time_system_test.cc b/test/test_common/test_time_system_test.cc index e1568e47cbdae..30268d3fe6bfa 100644 --- a/test/test_common/test_time_system_test.cc +++ b/test/test_common/test_time_system_test.cc @@ -36,7 +36,7 @@ TEST_F(TestTimeSystemTest, SimThenRealSerial) { TEST_F(TestTimeSystemTest, RealThenSim) { DangerousDeprecatedTestTime t1; - EXPECT_DEATH_LOG_TO_STDERR( { SimulatedTimeSystem t2; }, + EXPECT_DEATH_LOG_TO_STDERR({ SimulatedTimeSystem t2; }, ".*Two different types of time-systems allocated.*"); } From 52cfeb713d442d3f3fd6fed1ce9bf85175b306dc Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Fri, 25 Jan 2019 17:50:07 -0500 Subject: [PATCH 12/18] Change the commenting to encourage users to use SimulatedTimeSystem and not the helper. Signed-off-by: Joshua Marantz --- test/test_common/simulated_time_system.h | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/test/test_common/simulated_time_system.h b/test/test_common/simulated_time_system.h index e00edc76ce7bd..f14693b449bea 100644 --- a/test/test_common/simulated_time_system.h +++ b/test/test_common/simulated_time_system.h @@ -11,10 +11,11 @@ namespace Envoy { namespace Event { -// Represents a simulated time system, where time is advanced by calling -// sleep(), setSystemTime(), or setMonotonicTime(). systemTime() and -// monotonicTime() are maintained in the class, and alarms are fired in response -// to adjustments in time. +// Implements a simulated time system including a scheduler for timers. This is +// designed to be used as as the exclusive time-system resident in a process at +// any particular time, and as such should not be instantiated directly by +// tests. Instead it should be instantied via SimulatedTimeSystem, declared +// below. class SimulatedTimeSystemHelper : public TestTimeSystem { public: SimulatedTimeSystemHelper(); @@ -98,6 +99,10 @@ class SimulatedTimeSystemHelper : public TestTimeSystem { std::atomic pending_alarms_; }; +// Represents a simulated time system, where time is advanced by calling +// sleep(), setSystemTime(), or setMonotonicTime(). systemTime() and +// monotonicTime() are maintained in the class, and alarms are fired in response +// to adjustments in time. class SimulatedTimeSystem : public DelegatingTestTimeSystem { public: void setMonotonicTime(const MonotonicTime& monotonic_time) { From 70d4261bb0b197f7be4868ada9d44d7b80cc187d Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Mon, 28 Jan 2019 14:30:09 -0500 Subject: [PATCH 13/18] Convert ASSERT to RELEASE_ASSERT. Signed-off-by: Joshua Marantz --- test/test_common/test_time_system.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/test_common/test_time_system.h b/test/test_common/test_time_system.h index 4d385a3c01313..d7b1ec11b2e4e 100644 --- a/test/test_common/test_time_system.h +++ b/test/test_common/test_time_system.h @@ -62,7 +62,7 @@ class SingletonTimeSystemHelper { SingletonTimeSystemHelper() : time_system_(nullptr) {} void set(TestTimeSystem* time_system) { - ASSERT(time_system_ == nullptr); + RELEASE_ASSERT(time_system_ == nullptr, "Unexpected reset of SingletonTimeSystemHelper"); time_system_.reset(time_system); } From 4ab76c809f4624455998acf5f8dff4abdec8f57b Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Mon, 28 Jan 2019 16:37:58 -0500 Subject: [PATCH 14/18] Add description in test/README.md of the time-system and how to test with it. Signed-off-by: Joshua Marantz --- test/README.md | 28 ++++++++++++++++++++++++++++ tools/check_format.py | 2 ++ 2 files changed, 30 insertions(+) diff --git a/test/README.md b/test/README.md index e83a72fc11cdd..b45cb580b6544 100644 --- a/test/README.md +++ b/test/README.md @@ -69,3 +69,31 @@ Examples: EXPECT_THAT(response->headers(), IsSubsetOfHeaders(allowed_headers)); EXPECT_THAT(response->headers(), IsSupersetOfHeaders(required_headers)); ``` + +## Controlling time in tests + +In Envoy production code, time and timers are managed via +[`Event::TimeSystem`](https://github.com/envoyproxy/envoy/blob/master/include/envoy/event/timer.h), +which provides a mechanism for querying the time setting up time-based callbacks. Bypassing +this abstraction in Envoy code is flagged as a format violation in CI. + +In tests we use a deriviation +[`Event::TestTimeSystem`](test_common/test_time_system.h) which adds the ability +to sleep or do a blocking timed wait on a condition variable. There are two +implementations of the `Event::TestTimeSystem` interface: +`Event::TestRealTimeSystem`, and `Event::SimulatedTimeSystem`. The latter is +recommended for all new tests, as it helps avoid flaky tests on slow machines. + +Typically we do not want to have both real-time and simulated-time in the same +test; that could lead to hard-to-reproduce results. Thus both implementations +have a mechanism to enforce that only one of them can be instantiated at once. +A runtime assertion occurs if a `Event::TestRealTimeSystem` and +`Event::SimulatedTimeSystem` are instantiated at the same time. Once the +time-systems go out of scope, usually at the end of of a test method, the slate +is clean and a new test-method can use a different time system. + +There is also `Event::GlobalTimeSystem`, which can be instantiated in shared +test infrastructure that wants to be agnostic to which `TimeSystem` is used in a +test. When no `TimeSystem` is instantiated in a test, the `Event::GlobalTimeSystem` +will lazy-initialize itself into a concrete `TimeSystem` -- currently this is +`TestRealTimeSystem` but will be changed in the future to `SimulatedTimeSystem`. diff --git a/tools/check_format.py b/tools/check_format.py index efd065eb4b320..6468a275a97a7 100755 --- a/tools/check_format.py +++ b/tools/check_format.py @@ -190,6 +190,8 @@ def whitelistedForProtobufDeps(file_path): # specific cases. They should be passed down from where they are instantied to where # they need to be used, e.g. through the ServerInstance, Dispatcher, or ClusterManager. def whitelistedForRealTime(file_path): + if file_path.endswith(".md"): + return True return file_path in REAL_TIME_WHITELIST From 67127080f048502024f0ae335dc49722d578a290 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Mon, 28 Jan 2019 16:45:16 -0500 Subject: [PATCH 15/18] grammar tweaks Signed-off-by: Joshua Marantz --- test/README.md | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/test/README.md b/test/README.md index b45cb580b6544..5c8d160db68f3 100644 --- a/test/README.md +++ b/test/README.md @@ -82,18 +82,19 @@ In tests we use a deriviation to sleep or do a blocking timed wait on a condition variable. There are two implementations of the `Event::TestTimeSystem` interface: `Event::TestRealTimeSystem`, and `Event::SimulatedTimeSystem`. The latter is -recommended for all new tests, as it helps avoid flaky tests on slow machines. +recommended for all new tests, as it helps avoid flaky tests on slow machines, +and makes tests run faster. Typically we do not want to have both real-time and simulated-time in the same test; that could lead to hard-to-reproduce results. Thus both implementations have a mechanism to enforce that only one of them can be instantiated at once. -A runtime assertion occurs if a `Event::TestRealTimeSystem` and +A runtime assertion occurs if an `Event::TestRealTimeSystem` and `Event::SimulatedTimeSystem` are instantiated at the same time. Once the -time-systems go out of scope, usually at the end of of a test method, the slate +time-system goes out of scope, usually at the end of of a test method, the slate is clean and a new test-method can use a different time system. There is also `Event::GlobalTimeSystem`, which can be instantiated in shared test infrastructure that wants to be agnostic to which `TimeSystem` is used in a test. When no `TimeSystem` is instantiated in a test, the `Event::GlobalTimeSystem` -will lazy-initialize itself into a concrete `TimeSystem` -- currently this is +will lazy-initialize itself into a concrete `TimeSystem`. Currently this is `TestRealTimeSystem` but will be changed in the future to `SimulatedTimeSystem`. From a35d2235f13096a082febb2d32c9db7ba8946b11 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Mon, 28 Jan 2019 16:51:48 -0500 Subject: [PATCH 16/18] Switch TODO to reference #4160 Signed-off-by: Joshua Marantz --- test/test_common/test_time.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/test_common/test_time.h b/test/test_common/test_time.h index 02edee7a65f08..ea0411f6a2c95 100644 --- a/test/test_common/test_time.h +++ b/test/test_common/test_time.h @@ -33,7 +33,7 @@ class GlobalTimeSystem : public DelegatingTestTimeSystemBase { public: TestTimeSystem& timeSystem() override { if (singleton_->timeSystem() == nullptr) { - // TODO(jmarantz): Switch default to SimulatedTimeSystem. + // TODO(#4160): Switch default to SimulatedTimeSystem. singleton_->set(new TestRealTimeSystem); } return *singleton_->timeSystem(); From 32ce33ed0b87689448225ad3d2d312152ec6312a Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Mon, 28 Jan 2019 16:54:26 -0500 Subject: [PATCH 17/18] Fix typos. Signed-off-by: Joshua Marantz --- test/README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/README.md b/test/README.md index 5c8d160db68f3..e6637684c90c6 100644 --- a/test/README.md +++ b/test/README.md @@ -77,7 +77,7 @@ In Envoy production code, time and timers are managed via which provides a mechanism for querying the time setting up time-based callbacks. Bypassing this abstraction in Envoy code is flagged as a format violation in CI. -In tests we use a deriviation +In tests we use a derivation [`Event::TestTimeSystem`](test_common/test_time_system.h) which adds the ability to sleep or do a blocking timed wait on a condition variable. There are two implementations of the `Event::TestTimeSystem` interface: @@ -90,7 +90,7 @@ test; that could lead to hard-to-reproduce results. Thus both implementations have a mechanism to enforce that only one of them can be instantiated at once. A runtime assertion occurs if an `Event::TestRealTimeSystem` and `Event::SimulatedTimeSystem` are instantiated at the same time. Once the -time-system goes out of scope, usually at the end of of a test method, the slate +time-system goes out of scope, usually at the end of a test method, the slate is clean and a new test-method can use a different time system. There is also `Event::GlobalTimeSystem`, which can be instantiated in shared From d45dd4ae373d213bbfa1ed7cd147941690579d17 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Mon, 28 Jan 2019 19:59:47 -0500 Subject: [PATCH 18/18] fix grammar error. Signed-off-by: Joshua Marantz --- test/README.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/test/README.md b/test/README.md index e6637684c90c6..76f4c866f4f7c 100644 --- a/test/README.md +++ b/test/README.md @@ -74,8 +74,9 @@ EXPECT_THAT(response->headers(), IsSupersetOfHeaders(required_headers)); In Envoy production code, time and timers are managed via [`Event::TimeSystem`](https://github.com/envoyproxy/envoy/blob/master/include/envoy/event/timer.h), -which provides a mechanism for querying the time setting up time-based callbacks. Bypassing -this abstraction in Envoy code is flagged as a format violation in CI. +which provides a mechanism for querying the time and setting up time-based +callbacks. Bypassing this abstraction in Envoy code is flagged as a format +violation in CI. In tests we use a derivation [`Event::TestTimeSystem`](test_common/test_time_system.h) which adds the ability