Add TestScopedOfflineClock

Setting the offline clock pointer is error-prone, as a test must ensure it
resets the pointer back to null, or a later test will fail. TestScopedOfflineClock
overrides the clock in a more safe way.

Bug: 906903
Change-Id: Idd88c35da90bddcb3b7006c7b33547c5b36b02d2
Reviewed-on: https://chromium-review.googlesource.com/c/1351748
Commit-Queue: Dan H <harringtond@google.com>
Reviewed-by: Carlos Knippschild <carlosk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#611970}

components/offline_pages/core/BUILD.gn

@@ -123,6 +123,8 @@ static_library("test_support") {
     "stub_offline_page_model.h",
     "stub_system_download_manager.cc",
     "stub_system_download_manager.h",
+    "test_scoped_offline_clock.cc",
+    "test_scoped_offline_clock.h",
   ]
 
   deps = [

components/offline_pages/core/offline_clock.cc

@@ -4,6 +4,7 @@
 
 #include "components/offline_pages/core/offline_clock.h"
 
+#include "base/logging.h"
 #include "base/time/default_clock.h"
 
 namespace offline_pages {
@@ -19,6 +20,9 @@ base::Clock* OfflineClock() {
 }
 
 void SetOfflineClockForTesting(base::Clock* clock) {
+  DCHECK(clock == nullptr || custom_clock_ == nullptr)
+      << "Offline clock is being overridden a second time, which might "
+         "indicate a bug.";
   custom_clock_ = clock;
 }
 

components/offline_pages/core/offline_clock.h

@@ -15,7 +15,8 @@ namespace offline_pages {
 // can be called from any threads.
 base::Clock* OfflineClock();
 
-// Allows tests to override the clock returned by |OfflineClock()|.
+// Allows tests to override the clock returned by |OfflineClock()|. For safety,
+// use |TestScopedOfflineClock| instead if possible.
 void SetOfflineClockForTesting(base::Clock* clock);
 
 }  // namespace offline_pages

components/offline_pages/core/prefetch/tasks/generate_page_bundle_task_unittest.cc

@@ -8,9 +8,7 @@
 
 #include "base/logging.h"
 #include "base/test/mock_callback.h"
-#include "base/test/simple_test_clock.h"
 #include "base/time/time.h"
-#include "components/offline_pages/core/offline_clock.h"
 #include "components/offline_pages/core/prefetch/prefetch_item.h"
 #include "components/offline_pages/core/prefetch/prefetch_types.h"
 #include "components/offline_pages/core/prefetch/store/prefetch_store.h"
@@ -19,6 +17,7 @@
 #include "components/offline_pages/core/prefetch/tasks/prefetch_task_test_base.h"
 #include "components/offline_pages/core/prefetch/test_prefetch_dispatcher.h"
 #include "components/offline_pages/core/prefetch/test_prefetch_gcm_handler.h"
+#include "components/offline_pages/core/test_scoped_offline_clock.h"
 #include "components/offline_pages/task/task.h"
 #include "services/network/test/test_utils.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -72,7 +71,7 @@ TEST_F(GeneratePageBundleTaskTest, EmptyTask) {
 TEST_F(GeneratePageBundleTaskTest, TaskMakesNetworkRequest) {
   base::MockCallback<PrefetchRequestFinishedCallback> request_callback;
 
-  base::SimpleTestClock clock;
+  TestScopedOfflineClock clock;
 
   // This item will be sent with the bundle request.
   PrefetchItem item1 =
@@ -107,7 +106,6 @@ TEST_F(GeneratePageBundleTaskTest, TaskMakesNetworkRequest) {
   GeneratePageBundleTask task(dispatcher(), store(), gcm_handler(),
                               prefetch_request_factory(),
                               request_callback.Get());
-  SetOfflineClockForTesting(&clock);
   RunTask(&task);
 
   // Note: even though the requested URLs checked further below are in undefined

components/offline_pages/core/prefetch/tasks/stale_entry_finalizer_task_unittest.cc

@@ -10,16 +10,15 @@
 #include "base/bind.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/test/metrics/histogram_tester.h"
-#include "base/test/simple_test_clock.h"
 #include "base/test/test_simple_task_runner.h"
 #include "base/threading/thread_task_runner_handle.h"
-#include "components/offline_pages/core/offline_clock.h"
 #include "components/offline_pages/core/prefetch/mock_prefetch_item_generator.h"
 #include "components/offline_pages/core/prefetch/prefetch_item.h"
 #include "components/offline_pages/core/prefetch/prefetch_types.h"
 #include "components/offline_pages/core/prefetch/store/prefetch_store_test_util.h"
 #include "components/offline_pages/core/prefetch/tasks/prefetch_task_test_base.h"
 #include "components/offline_pages/core/prefetch/test_prefetch_dispatcher.h"
+#include "components/offline_pages/core/test_scoped_offline_clock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace offline_pages {
@@ -52,7 +51,7 @@ class StaleEntryFinalizerTaskTest : public PrefetchTaskTestBase {
  protected:
   TestPrefetchDispatcher dispatcher_;
   std::unique_ptr<StaleEntryFinalizerTask> stale_finalizer_task_;
-  base::SimpleTestClock simple_test_clock_;
+  TestScopedOfflineClock simple_test_clock_;
 };
 
 void StaleEntryFinalizerTaskTest::SetUp() {
@@ -60,7 +59,6 @@ void StaleEntryFinalizerTaskTest::SetUp() {
   stale_finalizer_task_ =
       std::make_unique<StaleEntryFinalizerTask>(dispatcher(), store());
   simple_test_clock_.SetNow(base::Time() + base::TimeDelta::FromDays(100));
-  SetOfflineClockForTesting(&simple_test_clock_);
 }
 
 void StaleEntryFinalizerTaskTest::TearDown() {

components/offline_pages/core/test_scoped_offline_clock.cc

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "components/offline_pages/core/test_scoped_offline_clock.h"
+
+#include "components/offline_pages/core/offline_clock.h"
+
+namespace offline_pages {
+
+TestScopedOfflineClock::TestScopedOfflineClock() {
+  SetOfflineClockForTesting(this);
+}
+TestScopedOfflineClock::~TestScopedOfflineClock() {
+  SetOfflineClockForTesting(nullptr);
+}
+
+}  // namespace offline_pages

components/offline_pages/core/test_scoped_offline_clock.h

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef COMPONENTS_OFFLINE_PAGES_CORE_TEST_SCOPED_OFFLINE_CLOCK_H_
+#define COMPONENTS_OFFLINE_PAGES_CORE_TEST_SCOPED_OFFLINE_CLOCK_H_
+
+#include "base/macros.h"
+#include "base/test/simple_test_clock.h"
+
+namespace offline_pages {
+
+// Overrides |OfflineClock()| with |this| upon construction. Returns
+// |OfflineClock()| to its original state upon destruction.
+class TestScopedOfflineClock : public base::SimpleTestClock {
+ public:
+  TestScopedOfflineClock();
+  ~TestScopedOfflineClock() override;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(TestScopedOfflineClock);
+};
+
+}  // namespace offline_pages
+
+#endif  // COMPONENTS_OFFLINE_PAGES_CORE_TEST_SCOPED_OFFLINE_CLOCK_H_