SuggestionsService undo blacklist functionality.

components/suggestions/blacklist_store_unittest.cc

Index: components/suggestions/blacklist_store_unittest.cc
diff --git a/components/suggestions/blacklist_store_unittest.cc b/components/suggestions/blacklist_store_unittest.cc
index d7d1fa8e0d14768926ff791924ee4fb20ac65b0c..a0403487ae401da5e8c97b3f5151e28c8ba2aa85 100644
--- a/components/suggestions/blacklist_store_unittest.cc
+++ b/components/suggestions/blacklist_store_unittest.cc
@@ -69,6 +69,7 @@ class BlacklistStoreTest : public testing::Test {
   DISALLOW_COPY_AND_ASSIGN(BlacklistStoreTest);
 };
 
+// Tests adding, removing to the blacklist and filtering.
 TEST_F(BlacklistStoreTest, BasicInteractions) {
   BlacklistStore blacklist_store(pref_service());
 
@@ -102,30 +103,69 @@ TEST_F(BlacklistStoreTest, BasicInteractions) {
   ValidateSuggestions(original_suggestions, suggestions);
 }
 
+TEST_F(BlacklistStoreTest, TestIsEmpty) {
+  BlacklistStore blacklist_store(pref_service());
+  EXPECT_TRUE(blacklist_store.IsEmpty());
+  EXPECT_TRUE(blacklist_store.BlacklistUrl(GURL(kTestUrlA)));
+  EXPECT_FALSE(blacklist_store.IsEmpty());
+  EXPECT_TRUE(blacklist_store.RemoveUrl(GURL(kTestUrlA)));
+  EXPECT_TRUE(blacklist_store.IsEmpty());
+}
+
 TEST_F(BlacklistStoreTest, BlacklistTwiceSuceeds) {
   BlacklistStore blacklist_store(pref_service());
   EXPECT_TRUE(blacklist_store.BlacklistUrl(GURL(kTestUrlA)));
   EXPECT_TRUE(blacklist_store.BlacklistUrl(GURL(kTestUrlA)));
 }
 
-TEST_F(BlacklistStoreTest, RemoveUnknownUrlSucceeds) {
+TEST_F(BlacklistStoreTest, RemoveUnknownUrlFails) {
   BlacklistStore blacklist_store(pref_service());
-  EXPECT_TRUE(blacklist_store.RemoveUrl(GURL(kTestUrlA)));
+  EXPECT_FALSE(blacklist_store.RemoveUrl(GURL(kTestUrlA)));
 }
 
-TEST_F(BlacklistStoreTest, GetFirstUrlFromBlacklist) {
-  BlacklistStore blacklist_store(pref_service());
+TEST_F(BlacklistStoreTest, TestGetTimeUntilReadyForUpload) {
+  // Tests assumes completion within 1 day.
+  base::TimeDelta upload_delay = base::TimeDelta::FromDays(1);
+  scoped_ptr<BlacklistStore> blacklist_store(
+      new BlacklistStore(pref_service(), upload_delay));
+  base::TimeDelta candidate_delta;
+  EXPECT_FALSE(blacklist_store->GetTimeUntilReadyForUpload(&candidate_delta));
+  EXPECT_FALSE(blacklist_store->GetTimeUntilReadyForUpload(GURL(kTestUrlA),
+                                                           &candidate_delta));
+  EXPECT_TRUE(blacklist_store->BlacklistUrl(GURL(kTestUrlA)));
+  EXPECT_TRUE(blacklist_store->GetTimeUntilReadyForUpload(&candidate_delta));
+  EXPECT_TRUE(candidate_delta <= upload_delay);
+  EXPECT_TRUE(blacklist_store->GetTimeUntilReadyForUpload(GURL(kTestUrlA),
+                                                          &candidate_delta));
+  EXPECT_TRUE(candidate_delta <= upload_delay);

[Mathieu, 2014/12/04 18:53:32]

reusing |candidate_delta| could mean that in case the function breaks and
doesn't modify |candidate_delta|, it wouldn't detect it.

[manzagop (departed), 2014/12/05 15:13:22]

Done. Good catch.

+  EXPECT_FALSE(blacklist_store->GetTimeUntilReadyForUpload(GURL(kTestUrlB),
+                                                           &candidate_delta));
+  // There should be no candidate for upload since the upload delay is 1 day.
+  // Note: this is a test that relies on timing.
+  GURL retrieved;
+  EXPECT_FALSE(blacklist_store->GetCandidateForUpload(&retrieved));
+
+  // Same, but with an upload delay of 0.
+  base::TimeDelta no_delay = base::TimeDelta::FromDays(0);
+  blacklist_store.reset(new BlacklistStore(pref_service(), no_delay));
+  EXPECT_TRUE(blacklist_store->BlacklistUrl(GURL(kTestUrlA)));
+  EXPECT_TRUE(blacklist_store->GetTimeUntilReadyForUpload(&candidate_delta));
+  EXPECT_LE(candidate_delta, no_delay);

[Mathieu, 2014/12/04 18:53:32]

are you not testing equal to zero here? If it's impractical to use another
function, add a comment.

[manzagop (departed), 2014/12/05 15:13:22]

I should be. Forgot to update the test after finding negative times cause issues
when scheduling tasks.

+  EXPECT_TRUE(blacklist_store->GetTimeUntilReadyForUpload(GURL(kTestUrlA),
+                                                          &candidate_delta));
+  EXPECT_LE(candidate_delta, no_delay);
+}
 
-  // Expect GetFirstUrlFromBlacklist fails when blacklist empty.
+TEST_F(BlacklistStoreTest, GetCandidateForUpload) {
+  BlacklistStore blacklist_store(pref_service(), base::TimeDelta::FromDays(0));
+  // Empty blacklist.
   GURL retrieved;
-  EXPECT_FALSE(blacklist_store.GetFirstUrlFromBlacklist(&retrieved));
+  EXPECT_FALSE(blacklist_store.GetCandidateForUpload(&retrieved));
 
-  // Blacklist A and B.
+  // Blacklist A and B. Expect to retrieve A or B.
   EXPECT_TRUE(blacklist_store.BlacklistUrl(GURL(kTestUrlA)));
   EXPECT_TRUE(blacklist_store.BlacklistUrl(GURL(kTestUrlB)));
-
-  // Expect to retrieve A or B.
-  EXPECT_TRUE(blacklist_store.GetFirstUrlFromBlacklist(&retrieved));
+  EXPECT_TRUE(blacklist_store.GetCandidateForUpload(&retrieved));
   std::string retrieved_string = retrieved.spec();
   EXPECT_TRUE(retrieved_string == std::string(kTestUrlA) ||
               retrieved_string == std::string(kTestUrlB));
@@ -137,7 +177,6 @@ TEST_F(BlacklistStoreTest, LogsBlacklistSize) {
   // Create a first store - blacklist is empty at this point.
   scoped_ptr<BlacklistStore> blacklist_store(
       new BlacklistStore(pref_service()));
-
   histogram_tester.ExpectTotalCount("Suggestions.LocalBlacklistSize", 1);
   histogram_tester.ExpectUniqueSample("Suggestions.LocalBlacklistSize", 0, 1);
 
@@ -147,7 +186,6 @@ TEST_F(BlacklistStoreTest, LogsBlacklistSize) {
 
   // Create a new BlacklistStore and verify the counts.
   blacklist_store.reset(new BlacklistStore(pref_service()));
-
   histogram_tester.ExpectTotalCount("Suggestions.LocalBlacklistSize", 2);
   histogram_tester.ExpectBucketCount("Suggestions.LocalBlacklistSize", 0, 1);
   histogram_tester.ExpectBucketCount("Suggestions.LocalBlacklistSize", 2, 1);