envoyproxy · jmarantz · Jan 29, 2019 · Jan 24, 2019 · Jan 24, 2019 · Jan 24, 2019
diff --git a/include/envoy/common/time.h b/include/envoy/common/time.h
@@ -21,7 +21,7 @@ typedef std::chrono::time_point<std::chrono::steady_clock> 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
@@ -55,7 +55,7 @@ typedef std::unique_ptr<Scheduler> SchedulerPtr;
  */
 class TimeSystem : public TimeSource {
 public:
-  virtual ~TimeSystem() {}
+  virtual ~TimeSystem() = default;
 
   using Duration = MonotonicTime::duration;
 

diff --git a/test/README.md b/test/README.md
@@ -69,3 +69,33 @@ 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 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
+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,
+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 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 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/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/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
@@ -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<DummyConfigProviderManager>(factory_context_.admin_);
   }
 
-  Event::SimulatedTimeSystem& timeSystem() { return factory_context_.timeSystem(); }
+  Event::SimulatedTimeSystem& timeSystem() { return time_system_; }
 
 protected:
+  Event::SimulatedTimeSystem time_system_;
   NiceMock<Server::Configuration::MockFactoryContext> factory_context_;
   std::unique_ptr<DummyConfigProviderManager> provider_manager_;
 };

diff --git 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"
@@ -37,11 +38,9 @@ namespace {
 
 // 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<GrpcMuxImpl>(
@@ -88,12 +87,16 @@ class GrpcMuxImplTest : public testing::Test {
   Grpc::MockAsyncStream async_stream_;
   std::unique_ptr<GrpcMuxImpl> grpc_mux_;
   NiceMock<MockGrpcMuxCallbacks> callbacks_;
-  Event::SimulatedTimeSystem time_system_;
   NiceMock<LocalInfo::MockLocalInfo> local_info_;
   Stats::IsolatedStoreImpl stats_;
   Envoy::Config::RateLimitSettings rate_limit_settings_;
 };
 
+class GrpcMuxImplTest : public GrpcMuxImplTestBase {
+public:
+  Event::SimulatedTimeSystem time_system_;
+};
+
 // Validate behavior when multiple type URL watches are maintained, watches are created/destroyed
 // (via RAII).
 TEST_F(GrpcMuxImplTest, MultipleTypeUrlStreams) {
@@ -328,11 +331,9 @@ 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 GrpcMuxImplTestBase {
+public:
+  Event::DelegatingTestTimeSystem<MockTimeSystem> mock_time_system_;
 };
 
 //  Verifies that rate limiting is not enforced with defaults.
@@ -348,7 +349,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();
 
@@ -398,7 +399,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/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
@@ -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<Server::Configuration::MockFactoryContext> factory_context_;
   Http::MockAsyncClientRequest request_;
   Http::AsyncClient::Callbacks* callbacks_{};

diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc
@@ -164,9 +164,7 @@ envoy::config::bootstrap::v2::Bootstrap parseBootstrapFromV2Yaml(const std::stri
 
 class ClusterManagerImplTest : 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<ClusterManagerImpl>(
@@ -241,12 +239,12 @@ class ClusterManagerImplTest : public testing::Test {
   }
 
   Stats::IsolatedStoreImpl stats_store_;
+  Event::SimulatedTimeSystem time_system_;
   Api::ApiPtr api_;
   NiceMock<TestClusterManagerFactory> factory_;
   std::unique_ptr<ClusterManagerImpl> cluster_manager_;
   AccessLog::MockAccessLogManager log_manager_;
   NiceMock<Server::MockAdmin> 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
@@ -30,9 +30,7 @@ class LoadStatsReporterTest : 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;
@@ -68,6 +66,7 @@ class LoadStatsReporterTest : public testing::Test {
     load_stats_reporter_->onReceiveMessage(std::move(response));
   }
 
+  Event::SimulatedTimeSystem time_system_;
   NiceMock<Upstream::MockClusterManager> cm_;
   Event::MockDispatcher dispatcher_;
   Stats::IsolatedStoreImpl stats_store_;
@@ -78,7 +77,6 @@ class LoadStatsReporterTest : public testing::Test {
   Event::TimerCb response_timer_cb_;
   Grpc::MockAsyncStream async_stream_;
   Grpc::MockAsyncClient* async_client_;
-  Event::SimulatedTimeSystem time_system_;
   NiceMock<LocalInfo::MockLocalInfo> local_info_;
 };
 

diff --git 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")));