[Feed] Remove Time::Now() from Feed unittests.

RefreshThrottlerTest tests broke on the day daylight savings time
ended. To fix this, set a TestClock's time to a specific date so the
real date will never affect tests going forward. Removed all unittest
usage of Time::Now in Feed code.

Bug: 901698
Change-Id: I76b0b092aa12b7c4fef65a353caa317a381f32c1
Reviewed-on: https://chromium-review.googlesource.com/c/1324414
Commit-Queue: Sky Malice <skym@chromium.org>
Reviewed-by: Yuri Wiitala <miu@chromium.org>
Reviewed-by: Filip Gorski <fgorski@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608441}

chrome/browser/android/feed/feed_host_service_factory.cc

@@ -126,9 +126,10 @@ KeyedService* FeedHostServiceFactory::BuildServiceInstanceFor(
       HistoryServiceFactory::GetForProfile(profile,
                                            ServiceAccessType::EXPLICIT_ACCESS);
   auto history_helper = std::make_unique<FeedHistoryHelper>(history_service);
-  auto logging_metrics =
-      std::make_unique<FeedLoggingMetrics>(base::BindRepeating(
-          &FeedHistoryHelper::CheckURL, std::move(history_helper)));
+  auto logging_metrics = std::make_unique<FeedLoggingMetrics>(
+      base::BindRepeating(&FeedHistoryHelper::CheckURL,
+                          std::move(history_helper)),
+      base::DefaultClock::GetInstance());
 
   return new FeedHostService(
       std::move(logging_metrics), std::move(image_manager),

chrome/browser/android/feed/history/feed_history_helper_unittest.cc

@@ -36,7 +36,7 @@ class FeedHistoryHelperTest : public testing::Test {
     feed_history_helper_ =
         std::make_unique<FeedHistoryHelper>(history_service_);
     history_service_->AddPage(
-        GURL(kURL1), base::Time::Now(), /*context_id=*/nullptr,
+        GURL(kURL1), base::Time(), /*context_id=*/nullptr,
         /*nav_entry_id=*/0,
         /*referrer=*/GURL(), history::RedirectList(), ui::PAGE_TRANSITION_TYPED,
         history::SOURCE_BROWSED, /*did_replace_entry=*/false);

components/feed/core/feed_image_database.cc

@@ -11,6 +11,8 @@
 #include "base/task/post_task.h"
 #include "base/task/task_traits.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "base/time/clock.h"
+#include "base/time/default_clock.h"
 #include "base/time/time.h"
 #include "components/feed/core/proto/cached_image.pb.h"
 #include "components/feed/core/time_serialization.h"
@@ -37,14 +39,17 @@ FeedImageDatabase::FeedImageDatabase(const base::FilePath& database_dir)
           std::make_unique<leveldb_proto::ProtoDatabaseImpl<CachedImageProto>>(
               base::CreateSequencedTaskRunnerWithTraits(
                   {base::MayBlock(), base::TaskPriority::BEST_EFFORT,
-                   base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN}))) {}
+                   base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN})),
+          base::DefaultClock::GetInstance()) {}
 
 FeedImageDatabase::FeedImageDatabase(
     const base::FilePath& database_dir,
     std::unique_ptr<leveldb_proto::ProtoDatabase<CachedImageProto>>
-        image_database)
+        image_database,
+    base::Clock* clock)
     : database_status_(UNINITIALIZED),
       image_database_(std::move(image_database)),
+      clock_(clock),
       weak_ptr_factory_(this) {
   leveldb_env::Options options = leveldb_proto::CreateSimpleOptions();
   if (base::SysInfo::IsLowEndDevice()) {
@@ -76,7 +81,7 @@ void FeedImageDatabase::SaveImage(const std::string& url,
   CachedImageProto image_proto;
   image_proto.set_url(url);
   image_proto.set_data(image_data);
-  image_proto.set_last_used_time(ToDatabaseTime(base::Time::Now()));
+  image_proto.set_last_used_time(ToDatabaseTime(clock_->Now()));
 
   SaveImageImpl(url, image_proto);
 }
@@ -167,7 +172,7 @@ void FeedImageDatabase::OnImageLoaded(std::string url,
   std::move(callback).Run(entry->data());
 
   // Update timestamp for image.
-  entry->set_last_used_time(ToDatabaseTime(base::Time::Now()));
+  entry->set_last_used_time(ToDatabaseTime(clock_->Now()));
   SaveImageImpl(std::move(url), *entry);
 }
 

components/feed/core/feed_image_database.h

@@ -13,6 +13,10 @@
 #include "base/memory/weak_ptr.h"
 #include "components/leveldb_proto/proto_database.h"
 
+namespace base {
+class Clock;
+}  // namespace base
+
 namespace feed {
 
 class CachedImageProto;
@@ -42,7 +46,8 @@ class FeedImageDatabase {
   FeedImageDatabase(
       const base::FilePath& database_dir,
       std::unique_ptr<leveldb_proto::ProtoDatabase<CachedImageProto>>
-          image_database);
+          image_database,
+      base::Clock* clock);
   ~FeedImageDatabase();
 
   // Returns true if initialization has finished successfully, else false.
@@ -111,6 +116,10 @@ class FeedImageDatabase {
 
   std::unique_ptr<leveldb_proto::ProtoDatabase<CachedImageProto>>
       image_database_;
+
+  // Used to access current time, injected for testing.
+  base::Clock* clock_;
+
   std::vector<std::pair<std::string, FeedImageDatabaseCallback>>
       pending_image_callbacks_;
 

components/feed/core/feed_image_database_unittest.cc

@@ -7,6 +7,7 @@
 #include <map>
 
 #include "base/test/scoped_task_environment.h"
+#include "base/test/simple_test_clock.h"
 #include "components/feed/core/proto/cached_image.pb.h"
 #include "components/feed/core/time_serialization.h"
 #include "components/leveldb_proto/testing/fake_db.h"
@@ -21,6 +22,9 @@ namespace feed {
 
 namespace {
 
+// Fixed "now" to make tests more deterministic.
+char kNowString[] = "2018-06-11 15:41";
+
 constexpr char kImageURL[] = "http://pie.com/";
 constexpr char kImageData[] = "pie image";
 
@@ -28,7 +32,11 @@ constexpr char kImageData[] = "pie image";
 
 class FeedImageDatabaseTest : public testing::Test {
  public:
-  FeedImageDatabaseTest() : image_db_(nullptr) {}
+  FeedImageDatabaseTest() : image_db_(nullptr) {
+    base::Time now;
+    EXPECT_TRUE(base::Time::FromUTCString(kNowString, &now));
+    test_clock_.SetNow(now);
+  }
 
   void CreateDatabase() {
     // The FakeDBs are owned by |feed_db_|, so clear our pointers before
@@ -41,8 +49,8 @@ class FeedImageDatabaseTest : public testing::Test {
         std::make_unique<FakeDB<CachedImageProto>>(&image_db_storage_);
 
     image_db_ = image_db.get();
-    feed_db_ = std::make_unique<FeedImageDatabase>(base::FilePath(),
-                                                   std::move(image_db));
+    feed_db_ = std::make_unique<FeedImageDatabase>(
+        base::FilePath(), std::move(image_db), &test_clock_);
   }
 
   int64_t GetImageLastUsedTime(const std::string& url) {
@@ -60,8 +68,8 @@ class FeedImageDatabaseTest : public testing::Test {
   }
 
   FakeDB<CachedImageProto>* image_db() { return image_db_; }
-
   FeedImageDatabase* db() { return feed_db_.get(); }
+  base::SimpleTestClock* test_clock() { return &test_clock_; }
 
   void RunUntilIdle() { scoped_task_environment_.RunUntilIdle(); }
 
@@ -76,6 +84,8 @@ class FeedImageDatabaseTest : public testing::Test {
   // Owned by |feed_db_|.
   FakeDB<CachedImageProto>* image_db_;
 
+  base::SimpleTestClock test_clock_;
+
   std::unique_ptr<FeedImageDatabase> feed_db_;
 
   DISALLOW_COPY_AND_ASSIGN(FeedImageDatabaseTest);
@@ -252,7 +262,7 @@ TEST_F(FeedImageDatabaseTest, GarbageCollectImagesTest) {
   image_db()->InitCallback(true);
   ASSERT_TRUE(db()->IsInitialized());
 
-  base::Time now = base::Time::Now();
+  base::Time now = test_clock()->Now();
   base::Time expired_time = now - base::TimeDelta::FromDays(30);
   base::Time very_old_time = now - base::TimeDelta::FromDays(100);
 

components/feed/core/feed_logging_metrics.cc

@@ -13,6 +13,7 @@
 #include "base/metrics/histogram_macros.h"
 #include "base/metrics/user_metrics.h"
 #include "base/strings/stringprintf.h"
+#include "base/time/clock.h"
 #include "base/time/time.h"
 #include "ui/base/mojo/window_open_disposition.mojom.h"
 
@@ -37,12 +38,12 @@ const char kHistogramArticlesUsageTimeLocal[] =
 // Records ContentSuggestions usage. Therefore the day is sliced into 20min
 // buckets. Depending on the current local time the count of the corresponding
 // bucket is increased.
-void RecordContentSuggestionsUsage() {
+void RecordContentSuggestionsUsage(base::Time now) {
   const int kBucketSizeMins = 20;
   const int kNumBuckets = 24 * 60 / kBucketSizeMins;
 
   base::Time::Exploded now_exploded;
-  base::Time::Now().LocalExplode(&now_exploded);
+  now.LocalExplode(&now_exploded);
   int bucket = (now_exploded.hour * 60 + now_exploded.minute) / kBucketSizeMins;
 
   const char* kWeekdayNames[] = {"Sunday",   "Monday", "Tuesday", "Wednesday",
@@ -78,8 +79,10 @@ void RecordSuggestionPageVisited(bool return_to_ntp) {
 }  // namespace
 
 FeedLoggingMetrics::FeedLoggingMetrics(
-    HistoryURLCheckCallback history_url_check_callback)
+    HistoryURLCheckCallback history_url_check_callback,
+    base::Clock* clock)
     : history_url_check_callback_(std::move(history_url_check_callback)),
+      clock_(clock),
       weak_ptr_factory_(this) {}
 
 FeedLoggingMetrics::~FeedLoggingMetrics() = default;
@@ -97,7 +100,7 @@ void FeedLoggingMetrics::OnSuggestionShown(int position,
   UMA_HISTOGRAM_EXACT_LINEAR("NewTabPage.ContentSuggestions.Shown", position,
                              kMaxSuggestionsTotal);
 
-  base::TimeDelta age = base::Time::Now() - publish_date;
+  base::TimeDelta age = clock_->Now() - publish_date;
   UMA_HISTOGRAM_CUSTOM_TIMES("NewTabPage.ContentSuggestions.ShownAge.Articles",
                              age, base::TimeDelta::FromSeconds(1),
                              base::TimeDelta::FromDays(7), 100);
@@ -109,14 +112,14 @@ void FeedLoggingMetrics::OnSuggestionShown(int position,
   // Records the time since the fetch time of the displayed snippet.
   UMA_HISTOGRAM_CUSTOM_TIMES(
       "NewTabPage.ContentSuggestions.TimeSinceSuggestionFetched",
-      base::Time::Now() - fetch_date, base::TimeDelta::FromSeconds(1),
+      clock_->Now() - fetch_date, base::TimeDelta::FromSeconds(1),
       base::TimeDelta::FromDays(7),
       /*bucket_count=*/100);
 
   // When the first of the articles suggestions is shown, then we count this as
   // a single usage of content suggestions.
   if (position == 0) {
-    RecordContentSuggestionsUsage();
+    RecordContentSuggestionsUsage(clock_->Now());
   }
 }
 
@@ -126,7 +129,7 @@ void FeedLoggingMetrics::OnSuggestionOpened(int position,
   UMA_HISTOGRAM_EXACT_LINEAR("NewTabPage.ContentSuggestions.Opened", position,
                              kMaxSuggestionsTotal);
 
-  base::TimeDelta age = base::Time::Now() - publish_date;
+  base::TimeDelta age = clock_->Now() - publish_date;
   UMA_HISTOGRAM_CUSTOM_TIMES("NewTabPage.ContentSuggestions.OpenedAge.Articles",
                              age, base::TimeDelta::FromSeconds(1),
                              base::TimeDelta::FromDays(7), 100);
@@ -135,7 +138,7 @@ void FeedLoggingMetrics::OnSuggestionOpened(int position,
       "NewTabPage.ContentSuggestions.OpenedScoreNormalized.Articles",
       ToUMAScore(score), 11);
 
-  RecordContentSuggestionsUsage();
+  RecordContentSuggestionsUsage(clock_->Now());
 
   base::RecordAction(base::UserMetricsAction("Suggestions.Content.Opened"));
 }
@@ -160,7 +163,7 @@ void FeedLoggingMetrics::OnSuggestionMenuOpened(int position,
   UMA_HISTOGRAM_EXACT_LINEAR("NewTabPage.ContentSuggestions.MenuOpened",
                              position, kMaxSuggestionsTotal);
 
-  base::TimeDelta age = base::Time::Now() - publish_date;
+  base::TimeDelta age = clock_->Now() - publish_date;
   UMA_HISTOGRAM_CUSTOM_TIMES(
       "NewTabPage.ContentSuggestions.MenuOpenedAge.Articles", age,
       base::TimeDelta::FromSeconds(1), base::TimeDelta::FromDays(7), 100);

components/feed/core/feed_logging_metrics.h

@@ -14,6 +14,7 @@
 #include "url/gurl.h"
 
 namespace base {
+class Clock;
 class Time;
 class TimeDelta;
 }  // namespace base
@@ -33,7 +34,8 @@ class FeedLoggingMetrics {
   using HistoryURLCheckCallback =
       base::RepeatingCallback<void(const GURL&, CheckURLVisitCallback)>;
 
-  explicit FeedLoggingMetrics(HistoryURLCheckCallback callback);
+  explicit FeedLoggingMetrics(HistoryURLCheckCallback callback,
+                              base::Clock* clock);
   ~FeedLoggingMetrics();
 
   // |suggestions_count| contains how many cards show to users. It does not
@@ -72,6 +74,9 @@ class FeedLoggingMetrics {
 
   const HistoryURLCheckCallback history_url_check_callback_;
 
+  // Used to access current time, injected for testing.
+  base::Clock* clock_;
+
   base::WeakPtrFactory<FeedLoggingMetrics> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(FeedLoggingMetrics);

components/feed/core/feed_logging_metrics_unittest.cc

@@ -5,18 +5,22 @@
 #include "components/feed/core/feed_logging_metrics.h"
 
 #include "base/test/metrics/histogram_tester.h"
+#include "base/test/simple_test_clock.h"
 #include "base/time/time.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/base/mojo/window_open_disposition.mojom.h"
 
+using testing::ElementsAre;
+using testing::IsEmpty;
+
 namespace feed {
 namespace {
 
-GURL VISITED_URL("http://visited_url.com");
+GURL kVisitedUrl("http://visited_url.com/");
 
-using testing::ElementsAre;
-using testing::IsEmpty;
+// Fixed "now" to make tests more deterministic.
+char kNowString[] = "2018-06-11 15:41";
 
 // This needs to keep in sync with ActionType in third_party/feed/src/src/main/
 // java/com/google/android/libraries/feed/host/logging/ActionType.java.
@@ -31,7 +35,7 @@ enum FeedActionType {
 
 void CheckURLVisit(const GURL& url,
                    FeedLoggingMetrics::CheckURLVisitCallback callback) {
-  if (url == VISITED_URL) {
+  if (url == kVisitedUrl) {
     std::move(callback).Run(true);
   } else {
     std::move(callback).Run(false);
@@ -43,15 +47,22 @@ void CheckURLVisit(const GURL& url,
 class FeedLoggingMetricsTest : public testing::Test {
  public:
   FeedLoggingMetricsTest() {
+    base::Time now;
+    EXPECT_TRUE(base::Time::FromUTCString(kNowString, &now));
+    test_clock_.SetNow(now);
+
     feed_logging_metrics_ = std::make_unique<FeedLoggingMetrics>(
-        base::BindRepeating(&CheckURLVisit));
+        base::BindRepeating(&CheckURLVisit), &test_clock_);
   }
 
   FeedLoggingMetrics* feed_logging_metrics() {
     return feed_logging_metrics_.get();
   }
+  base::SimpleTestClock* test_clock() { return &test_clock_; }
 
  private:
+  base::SimpleTestClock test_clock_;
+
   std::unique_ptr<FeedLoggingMetrics> feed_logging_metrics_;
 
   DISALLOW_COPY_AND_ASSIGN(FeedLoggingMetricsTest);
@@ -60,18 +71,18 @@ class FeedLoggingMetricsTest : public testing::Test {
 TEST_F(FeedLoggingMetricsTest, ShouldLogOnSuggestionsShown) {
   base::HistogramTester histogram_tester;
   feed_logging_metrics()->OnSuggestionShown(
-      /*position=*/1, base::Time::Now(),
-      /*score=*/0.01f, base::Time::Now() - base::TimeDelta::FromHours(2));
+      /*position=*/1, test_clock()->Now(),
+      /*score=*/0.01f, test_clock()->Now() - base::TimeDelta::FromHours(2));
   // Test corner cases for score.
   feed_logging_metrics()->OnSuggestionShown(
-      /*position=*/2, base::Time::Now(),
-      /*score=*/0.0f, base::Time::Now() - base::TimeDelta::FromHours(2));
+      /*position=*/2, test_clock()->Now(),
+      /*score=*/0.0f, test_clock()->Now() - base::TimeDelta::FromHours(2));
   feed_logging_metrics()->OnSuggestionShown(
-      /*position=*/3, base::Time::Now(),
-      /*score=*/1.0f, base::Time::Now() - base::TimeDelta::FromHours(2));
+      /*position=*/3, test_clock()->Now(),
+      /*score=*/1.0f, test_clock()->Now() - base::TimeDelta::FromHours(2));
   feed_logging_metrics()->OnSuggestionShown(
-      /*position=*/4, base::Time::Now(),
-      /*score=*/8.0f, base::Time::Now() - base::TimeDelta::FromHours(2));
+      /*position=*/4, test_clock()->Now(),
+      /*score=*/8.0f, test_clock()->Now() - base::TimeDelta::FromHours(2));
 
   EXPECT_THAT(
       histogram_tester.GetAllSamples("NewTabPage.ContentSuggestions.Shown"),
@@ -99,16 +110,16 @@ TEST_F(FeedLoggingMetricsTest, ShouldLogOnPageShown) {
 TEST_F(FeedLoggingMetricsTest, ShouldLogOnSuggestionOpened) {
   base::HistogramTester histogram_tester;
   feed_logging_metrics()->OnSuggestionOpened(
-      /*position=*/11, base::Time::Now(),
+      /*position=*/11, test_clock()->Now(),
       /*score=*/1.0f);
   feed_logging_metrics()->OnSuggestionOpened(
-      /*position=*/13, base::Time::Now(),
+      /*position=*/13, test_clock()->Now(),
       /*score=*/1.0f);
   feed_logging_metrics()->OnSuggestionOpened(
-      /*position=*/15, base::Time::Now(),
+      /*position=*/15, test_clock()->Now(),
       /*score=*/1.0f);
   feed_logging_metrics()->OnSuggestionOpened(
-      /*position=*/23, base::Time::Now(),
+      /*position=*/23, test_clock()->Now(),
       /*score=*/1.0f);
 
   EXPECT_THAT(
@@ -140,7 +151,7 @@ TEST_F(FeedLoggingMetricsTest, ShouldLogOnSuggestionWindowOpened) {
 
 TEST_F(FeedLoggingMetricsTest, ShouldLogOnSuggestionDismissedIfVisited) {
   base::HistogramTester histogram_tester;
-  feed_logging_metrics()->OnSuggestionDismissed(/*position=*/10, VISITED_URL);
+  feed_logging_metrics()->OnSuggestionDismissed(/*position=*/10, kVisitedUrl);
   EXPECT_THAT(histogram_tester.GetAllSamples(
                   "NewTabPage.ContentSuggestions.DismissedVisited"),
               ElementsAre(base::Bucket(/*min=*/10, /*count=*/1)));

components/feed/core/feed_scheduler_host_unittest.cc

@@ -24,13 +24,17 @@
 #include "net/base/network_change_notifier.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+using base::Time;
+using base::TimeDelta;
+
 namespace feed {
 
+namespace {
+
 // Fixed "now" to make tests more deterministic.
 char kNowString[] = "2018-06-11 15:41";
 
-using base::Time;
-using base::TimeDelta;
+}  // namespace
 
 class ForceDeviceOffline : public net::NetworkChangeNotifier {
  public:
@@ -880,7 +884,7 @@ TEST_F(FeedSchedulerHostTest, RefreshThrottler) {
 
   for (int i = 0; i < 5; i++) {
     scheduler()->OnForegrounded();
-    ResetRefreshState(base::Time());
+    ResetRefreshState(Time());
     EXPECT_EQ(std::min(i + 1, 3), refresh_call_count());
   }
 }

components/feed/core/refresh_throttler_unittest.cc

@@ -18,11 +18,21 @@
 
 namespace feed {
 
+namespace {
+
+// Fixed "now" to make tests more deterministic.
+char kNowString[] = "2018-06-11 15:41";
+
+}  // namespace
+
 class RefreshThrottlerTest : public testing::Test {
  public:
   RefreshThrottlerTest() {
     RefreshThrottler::RegisterProfilePrefs(test_prefs_.registry());
-    test_clock_.SetNow(base::Time::Now());
+
+    base::Time now;
+    EXPECT_TRUE(base::Time::FromUTCString(kNowString, &now));
+    test_clock_.SetNow(now);
 
     variations::testing::VariationParamsManager variation_params(
         kInterestFeedContentSuggestions.name,
@@ -49,8 +59,7 @@ TEST_F(RefreshThrottlerTest, QuotaExceeded) {
   EXPECT_FALSE(throttler_->RequestQuota());
 }
 
-// Disabled since this is not robust against DST https://crbug.com/901698
-TEST_F(RefreshThrottlerTest, DISABLED_QuotaIsPerDay) {
+TEST_F(RefreshThrottlerTest, QuotaIsPerDay) {
   EXPECT_TRUE(throttler_->RequestQuota());
   EXPECT_TRUE(throttler_->RequestQuota());
   EXPECT_FALSE(throttler_->RequestQuota());
@@ -59,8 +68,7 @@ TEST_F(RefreshThrottlerTest, DISABLED_QuotaIsPerDay) {
   EXPECT_TRUE(throttler_->RequestQuota());
 }
 
-// Disabled since this is not robust against DST https://crbug.com/901698
-TEST_F(RefreshThrottlerTest, DISABLED_RollOver) {
+TEST_F(RefreshThrottlerTest, RollOver) {
   // Exhaust our quota so the for loop can verify everything as false.
   EXPECT_TRUE(throttler_->RequestQuota());
   EXPECT_TRUE(throttler_->RequestQuota());