[Suggestions Service] Add support for expiring the SuggestionsStore

components/suggestions/suggestions_store_unittest.cc

Index: components/suggestions/suggestions_store_unittest.cc
diff --git a/components/suggestions/suggestions_store_unittest.cc b/components/suggestions/suggestions_store_unittest.cc
index 08930133f01adbb6a669c99c446ff4394d7407e1..d79f1f4b35ab22b1db96e8d4a00b28acc058a82f 100644
--- a/components/suggestions/suggestions_store_unittest.cc
+++ b/components/suggestions/suggestions_store_unittest.cc
@@ -4,6 +4,8 @@
 
 #include "components/suggestions/suggestions_store.h"
 
+#include "base/time/time.h"
+

[Mathieu, 2014/08/04 19:54:55]

remove extra newline

[gayane -on leave until 09-2017, 2014/08/05 14:39:15]

Done.

[manzagop (departed), 2014/08/05 15:30:26]

Nope! ;)

[gayane -on leave until 09-2017, 2014/08/06 14:40:55]

Done this time.

 #include "components/pref_registry/testing_pref_service_syncable.h"
 #include "components/suggestions/proto/suggestions.pb.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -17,11 +19,40 @@ namespace {
 const char kTestTitle[] = "Foo site";
 const char kTestUrl[] = "http://foo.com/";
 
+const int kTimeGapUsec = 1000;

[manzagop (departed), 2014/08/04 19:50:26]

IIUC this is the timespan within which tests must complete in order for them
valid suggestions to be ensured validity. As discussed in the first round, a
time dependency isn't ideal usually. You can usually pass in the cutoff
timestamp as argument to avoid it. That said it's ok here.

However, 1ms sounds short. Perhaps increase to 100ms?

[gayane -on leave until 09-2017, 2014/08/05 14:39:16]

Done.

[manzagop (departed), 2014/08/05 15:30:26]

Probably warrants a comment.

+
+void AddSuggestion(SuggestionsProfile* suggestions, const char *title,
+                   const char *url, int64 expiry_ts) {
+  ChromeSuggestion* suggestion = suggestions->add_suggestions();
+  suggestion->set_url(title);
+  suggestion->set_title(url);
+  suggestion->set_expiry_ts(expiry_ts);
+}
+
 SuggestionsProfile CreateTestSuggestions() {
   SuggestionsProfile suggestions;
   ChromeSuggestion* suggestion = suggestions.add_suggestions();
-  suggestion->set_url(kTestUrl);
-  suggestion->set_title(kTestTitle);
+  suggestion->set_url(kTestTitle);
+  suggestion->set_title(kTestUrl);
+  return suggestions;
+}
+
+
+SuggestionsProfile CreateTestSuggestionsProfileWithExpiry(int expired_count,
+                                                          int valid_count) {
+  int64 now_usec = (base::Time::NowFromSystemTime() - base::Time::UnixEpoch())
+      .ToInternalValue();
+  srand(7);  // Constant seed for rand() function.
+  int64 offset_limit_usec = 30 * base::Time::kMicrosecondsPerDay;
+  SuggestionsProfile suggestions;
+  for (int i = 0; i < valid_count; i++){
+    int64 offset_usec = rand() % offset_limit_usec + kTimeGapUsec;
+    AddSuggestion(&suggestions, kTestTitle, kTestUrl, now_usec + offset_usec);
+  }
+  for (int i = 0; i < expired_count; i++){
+    int64 offset_usec = rand() % offset_limit_usec - kTimeGapUsec;
+    AddSuggestion(&suggestions, kTestTitle, kTestUrl, now_usec - offset_usec);
+  }
   return suggestions;
 }
 
@@ -31,6 +62,8 @@ void ValidateSuggestions(const SuggestionsProfile& expected,
   for (int i = 0; i < expected.suggestions_size(); ++i) {
     EXPECT_EQ(expected.suggestions(i).url(), actual.suggestions(i).url());
     EXPECT_EQ(expected.suggestions(i).title(), actual.suggestions(i).title());
+    EXPECT_EQ(expected.suggestions(i).expiry_ts(),
+              actual.suggestions(i).expiry_ts());
     EXPECT_EQ(expected.suggestions(i).favicon_url(),
               actual.suggestions(i).favicon_url());
     EXPECT_EQ(expected.suggestions(i).thumbnail(),
@@ -40,6 +73,56 @@ void ValidateSuggestions(const SuggestionsProfile& expected,
 
 }  // namespace
 
+// Tests LoadSuggestions function to filter expired suggestions.
+TEST(SuggestionsStoreTest, LoadAllExpired) {
+  TestingPrefServiceSyncable prefs;

[Mathieu, 2014/08/04 19:54:55]

since this is now duplicated many times, could you create the
SuggestionsStoreTest class and store things such as |prefs| there? The test
class can also own a scoped_ptr<SuggestionsStore> so you won't need to create it
in every test.

Example:
https://code.google.com/p/chromium/codesearch#chromium/src/components/suggest...

(Note TEST_F instead of TEST: it means that tests inherit the test class). 

[gayane -on leave until 09-2017, 2014/08/05 14:39:16]

test class created. but didn't manage to put scoped_ptr<SuggestionStore> in the
test class. Maybe you can help me with that.

[gayane -on leave until 09-2017, 2014/08/06 15:05:28]

Done.

+  SuggestionsStore::RegisterProfilePrefs(prefs.registry());
+  SuggestionsStore suggestions_store(&prefs);
+
+  SuggestionsProfile suggestions = CreateTestSuggestionsProfileWithExpiry(5, 0);
+  SuggestionsProfile filtered_suggestions;
+
+  // store and load. expired suggestions should not be loaded.

[manzagop (departed), 2014/08/04 19:50:26]

Capitals, here and below.

[Mathieu, 2014/08/04 19:54:55]

nit: Capital letter to start sentences.

[gayane -on leave until 09-2017, 2014/08/05 14:39:16]

Done.

[gayane -on leave until 09-2017, 2014/08/05 14:39:16]

Done.

+  EXPECT_TRUE(suggestions_store.StoreSuggestions(suggestions));
+  EXPECT_FALSE(suggestions_store.LoadSuggestions(&filtered_suggestions));
+  EXPECT_EQ(0, filtered_suggestions.suggestions_size());
+}
+
+// Tests LoadSuggestions function to filter expired suggestions.
+TEST(SuggestionsStoreTest, LoadValidAndExpired) {
+  TestingPrefServiceSyncable prefs;
+  SuggestionsStore::RegisterProfilePrefs(prefs.registry());
+  SuggestionsStore suggestions_store(&prefs);
+
+  SuggestionsProfile suggestions = CreateTestSuggestionsProfileWithExpiry(5, 3);
+  SuggestionsProfile filtered_suggestions;
+
+  // store and load. expired suggestions should not be loaded.
+  EXPECT_TRUE(suggestions_store.StoreSuggestions(suggestions));
+  EXPECT_TRUE(suggestions_store.LoadSuggestions(&filtered_suggestions));
+  EXPECT_EQ(3, filtered_suggestions.suggestions_size());
+}
+
+// Tests LoadSuggestions function to filter expired suggestions.
+TEST(SuggestionsStoreTest, CheckStoreAfterLoadExpired) {
+  TestingPrefServiceSyncable prefs;
+  SuggestionsStore::RegisterProfilePrefs(prefs.registry());
+  SuggestionsStore suggestions_store(&prefs);
+
+  SuggestionsProfile suggestions = CreateTestSuggestionsProfileWithExpiry(5, 3);
+  SuggestionsProfile filtered_suggestions;
+
+  // store and load. expired suggestions should not be loaded.
+  EXPECT_TRUE(suggestions_store.StoreSuggestions(suggestions));
+  EXPECT_TRUE(suggestions_store.LoadSuggestions(&filtered_suggestions));

[Mathieu, 2014/08/04 19:54:55]

you can remove the lines that are replicated in the test above (related to
filtered_suggestions)

[gayane -on leave until 09-2017, 2014/08/05 14:39:16]

Done.

+  EXPECT_EQ(3, filtered_suggestions.suggestions_size());
+
+  SuggestionsProfile loaded_suggestions;
+  EXPECT_TRUE(suggestions_store.LoadSuggestions(&loaded_suggestions));
+  EXPECT_EQ(3, loaded_suggestions.suggestions_size());
+  ValidateSuggestions(filtered_suggestions, loaded_suggestions);
+}
+
 TEST(SuggestionsStoreTest, LoadStoreClear) {
   TestingPrefServiceSyncable prefs;
   SuggestionsStore::RegisterProfilePrefs(prefs.registry());