[Doodle] Don't refresh more than once per 15 mins

components/doodle/doodle_service_unittest.cc

 // Copyright 2017 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/doodle/doodle_service.h"
 
 #include <memory>
 #include <utility>
 #include <vector>
 
@@ skipping 57 matching lines @@
   DoodleServiceTest()
       : task_runner_(new base::TestMockTimeTaskRunner()),
         task_runner_handle_(task_runner_),
         tick_clock_(task_runner_->GetMockTickClock()),
         fetcher_(nullptr),
         expiry_timer_(nullptr) {
     DoodleService::RegisterProfilePrefs(pref_service_.registry());
 
     task_runner_->FastForwardBy(base::TimeDelta::FromHours(12345));
 
-    RecreateService();
+    // Set the minimum refresh interval to 0 by default, so tests don't have to
+    // worry about it. The tests that care set it explicitly.
+    RecreateServiceWithZeroRefreshInterval();
   }
 
   void DestroyService() { service_ = nullptr; }
 
-  void RecreateService() {
+  void RecreateServiceWithZeroRefreshInterval() {
+    RecreateService(/*min_refresh_interval=*/base::TimeDelta());
+  }
+
+  void RecreateService(base::Optional<base::TimeDelta> refresh_interval) {
     auto expiry_timer = base::MakeUnique<base::OneShotTimer>(tick_clock_.get());
     expiry_timer->SetTaskRunner(task_runner_);
     expiry_timer_ = expiry_timer.get();
 
     auto fetcher = base::MakeUnique<FakeDoodleFetcher>();
     fetcher_ = fetcher.get();
 
     service_ = base::MakeUnique<DoodleService>(
         &pref_service_, std::move(fetcher), std::move(expiry_timer),
-        task_runner_->GetMockClock(), task_runner_->GetMockTickClock());
+        task_runner_->GetMockClock(), task_runner_->GetMockTickClock(),
+        refresh_interval);
   }
 
   DoodleService* service() { return service_.get(); }
   FakeDoodleFetcher* fetcher() { return fetcher_; }
 
   base::TestMockTimeTaskRunner* task_runner() { return task_runner_.get(); }
 
  private:
   TestingPrefServiceSimple pref_service_;
 
@@ skipping 43 matching lines @@
   // Load some doodle config.
   service()->Refresh();
   DoodleConfig config = CreateConfig(DoodleType::SIMPLE);
   config.large_image.url = GURL("https://doodle.com/doodle.jpg");
   fetcher()->ServeAllCallbacks(DoodleState::AVAILABLE,
                                base::TimeDelta::FromHours(1), config);
   ASSERT_THAT(service()->config(), Eq(config));
 
   // Re-create the service. It should have persisted the config, and load it
   // again automatically.
-  RecreateService();
+  RecreateServiceWithZeroRefreshInterval();
   EXPECT_THAT(service()->config(), Eq(config));
 }
 
 TEST_F(DoodleServiceTest, PersistsExpiryDate) {
   // Load some doodle config.
   service()->Refresh();
   DoodleConfig config = CreateConfig(DoodleType::SIMPLE);
   config.large_image.url = GURL("https://doodle.com/doodle.jpg");
   fetcher()->ServeAllCallbacks(DoodleState::AVAILABLE,
                                base::TimeDelta::FromHours(1), config);
   ASSERT_THAT(service()->config(), Eq(config));
 
   // Destroy the service, and let some time pass.
   DestroyService();
   task_runner()->FastForwardBy(base::TimeDelta::FromMinutes(15));
 
   // Remove the abandoned expiry task from the task runner.
   ASSERT_THAT(task_runner()->GetPendingTaskCount(), Eq(1u));
   task_runner()->ClearPendingTasks();
 
   // Re-create the service. The persisted config should have been loaded again.
-  RecreateService();
+  RecreateServiceWithZeroRefreshInterval();
   EXPECT_THAT(service()->config(), Eq(config));
 
   // Its time-to-live should have been updated.
   EXPECT_THAT(task_runner()->GetPendingTaskCount(), Eq(1u));
   EXPECT_THAT(task_runner()->NextPendingTaskDelay(),
               Eq(base::TimeDelta::FromMinutes(45)));
 }
 
 TEST_F(DoodleServiceTest, PersistedConfigExpires) {
   // Load some doodle config.
   service()->Refresh();
   DoodleConfig config = CreateConfig(DoodleType::SIMPLE);
   config.large_image.url = GURL("https://doodle.com/doodle.jpg");
   fetcher()->ServeAllCallbacks(DoodleState::AVAILABLE,
                                base::TimeDelta::FromHours(1), config);
   ASSERT_THAT(service()->config(), Eq(config));
 
   // Destroy the service, and let enough time pass so that the config expires.
   DestroyService();
   task_runner()->FastForwardBy(base::TimeDelta::FromHours(1));
 
   // Re-create the service. The persisted config should have been discarded
   // because it is expired.
-  RecreateService();
+  RecreateServiceWithZeroRefreshInterval();
   EXPECT_THAT(service()->config(), Eq(base::nullopt));
 }
 
+TEST_F(DoodleServiceTest, RespectsMinRefreshInterval) {
+  // Create a service with the default refresh interval.
+  RecreateService(/*min_refresh_interval=*/base::nullopt);
+
+  // Load some doodle config.
+  service()->Refresh();
+  DoodleConfig config = CreateConfig(DoodleType::SIMPLE);
+  config.large_image.url = GURL("https://doodle.com/doodle.jpg");
+  fetcher()->ServeAllCallbacks(DoodleState::AVAILABLE,
+                               base::TimeDelta::FromHours(1), config);
+  ASSERT_THAT(service()->config(), Eq(config));
+
+  // Let some time pass (less than the refresh interval).
+  task_runner()->FastForwardBy(base::TimeDelta::FromMinutes(10));
+
+  // Request a refresh. This should get ignored.
+  service()->Refresh();
+  EXPECT_THAT(fetcher()->num_pending_callbacks(), Eq(0u));
+
+  // Let more time pass, so we get past the refresh interval.
+  task_runner()->FastForwardBy(base::TimeDelta::FromMinutes(6));
+
+  // Now the refresh request should be honored again.
+  service()->Refresh();
+  EXPECT_THAT(fetcher()->num_pending_callbacks(), Eq(1u));
+}
+
 TEST_F(DoodleServiceTest, CallsObserverOnConfigReceived) {
   StrictMock<MockDoodleObserver> observer;
   service()->AddObserver(&observer);
 
   ASSERT_THAT(service()->config(), Eq(base::nullopt));
 
   // Request a refresh of the doodle config.
   service()->Refresh();
   // The request should have arrived at the fetcher.
   ASSERT_THAT(fetcher()->num_pending_callbacks(), Eq(1u));
@@ skipping 204 matching lines @@
   ASSERT_THAT(service()->config(), Eq(base::nullopt));
 
   // The outcome should have been recorded, but not the time - we only record
   // that for successful downloads.
   histograms.ExpectUniqueSample("Doodle.ConfigDownloadOutcome",
                                 5,  // OUTCOME_DOWNLOAD_ERROR
                                 1);
   histograms.ExpectTotalCount("Doodle.ConfigDownloadTime", 0);
 }
 
+TEST_F(DoodleServiceTest, RecordsMetricsForEarlyRefreshRequest) {
+  // Create a service with the default refresh interval.
+  RecreateService(/*min_refresh_interval=*/base::nullopt);

[fhorschig, 2017/03/27 15:45:05]

nit: Can you please set some arbitrary value here?
(Just so we have one constructor call with value and one without)

[Marc Treib, 2017/03/27 16:02:22]

Done.

+
+  // Load a doodle config as usual.
+  service()->Refresh();
+  DoodleConfig config = CreateConfig(DoodleType::SIMPLE);
+  fetcher()->ServeAllCallbacks(DoodleState::AVAILABLE,
+                               base::TimeDelta::FromHours(1), config);
+  ASSERT_THAT(service()->config(), Eq(config));
+
+  base::HistogramTester histograms;
+
+  // Request a refresh before the min refresh interval has passed.
+  task_runner()->FastForwardBy(base::TimeDelta::FromMinutes(5));
+  service()->Refresh();
+
+  // This should not have resulted in a request.
+  ASSERT_THAT(fetcher()->num_pending_callbacks(), Eq(0u));
+
+  // The outcome should still have been recorded, but not the time - we only
+  // record that for successful downloads.
+  histograms.ExpectUniqueSample("Doodle.ConfigDownloadOutcome",
+                                7,  // OUTCOME_REFRESH_INTERVAL_NOT_PASSED
+                                1);
+  histograms.ExpectTotalCount("Doodle.ConfigDownloadTime", 0);
+}
+
 TEST_F(DoodleServiceTest, DoesNotRecordMetricsAtStartup) {
   // Creating the service should not emit any histogram samples.
   base::HistogramTester histograms;
-  RecreateService();
+  RecreateServiceWithZeroRefreshInterval();
   ASSERT_THAT(service()->config(), Eq(base::nullopt));
   histograms.ExpectTotalCount("Doodle.ConfigDownloadOutcome", 0);
   histograms.ExpectTotalCount("Doodle.ConfigDownloadTime", 0);
 }
 
 TEST_F(DoodleServiceTest, DoesNotRecordMetricsAtStartupWithConfig) {
   // Load a doodle config as usual.
   service()->Refresh();
   DoodleConfig config = CreateConfig(DoodleType::SIMPLE);
   fetcher()->ServeAllCallbacks(DoodleState::AVAILABLE,
                                base::TimeDelta::FromHours(1), config);
   ASSERT_THAT(service()->config(), Eq(config));
 
   // Recreating the service should not emit any histogram samples, even though
   // a config gets loaded.
   base::HistogramTester histograms;
-  RecreateService();
+  RecreateServiceWithZeroRefreshInterval();
   ASSERT_THAT(service()->config(), Eq(config));
   histograms.ExpectTotalCount("Doodle.ConfigDownloadOutcome", 0);
   histograms.ExpectTotalCount("Doodle.ConfigDownloadTime", 0);
 }
 
 TEST_F(DoodleServiceTest, DoesNotRecordMetricsWhenConfigExpires) {
   // Load some doodle config.
   service()->Refresh();
   DoodleConfig config = CreateConfig(DoodleType::SIMPLE);
   fetcher()->ServeAllCallbacks(DoodleState::AVAILABLE,
                                base::TimeDelta::FromHours(1), config);
   ASSERT_THAT(service()->config(), Eq(config));
 
   base::HistogramTester histograms;
 
   // Fast-forward time so that the config expires.
   task_runner()->FastForwardBy(base::TimeDelta::FromHours(1));
   ASSERT_THAT(service()->config(), Eq(base::nullopt));
 
   // This should not have resulted in any metrics being emitted.
   histograms.ExpectTotalCount("Doodle.ConfigDownloadOutcome", 0);
   histograms.ExpectTotalCount("Doodle.ConfigDownloadTime", 0);
 }
 
 }  // namespace doodle