[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..5599aee8fcc4fd146e886235295939b2a7b26d8c 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"
+
 #include "components/pref_registry/testing_pref_service_syncable.h"
 #include "components/suggestions/proto/suggestions.pb.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -17,6 +19,8 @@ namespace {
 const char kTestTitle[] = "Foo site";
 const char kTestUrl[] = "http://foo.com/";
 
+const int time_gap = 1000;

[manzagop (departed), 2014/07/31 15:31:52]

Put the unit in the name.

[gayane -on leave until 09-2017, 2014/08/04 13:46:32]

Done.

+
 SuggestionsProfile CreateTestSuggestions() {
   SuggestionsProfile suggestions;
   ChromeSuggestion* suggestion = suggestions.add_suggestions();
@@ -25,12 +29,47 @@ SuggestionsProfile CreateTestSuggestions() {
   return suggestions;
 }
 
+
+SuggestionsProfile CreateTestSuggestions_LoadExpiryTests(int expired,

[manzagop (departed), 2014/07/31 15:31:52]

I don't think CreateTestSuggestions_LoadExpiryTests follows the style.
CreateTestSuggestionsProfileWithExpiry?

You might use it for something other than the expiry tests.

[manzagop (departed), 2014/07/31 15:31:53]

expired_count? valid_count?

[gayane -on leave until 09-2017, 2014/08/04 13:46:31]

Done.

[gayane -on leave until 09-2017, 2014/08/04 13:46:32]

Done.

+                                                         int valid) {
+  int64 now = (base::Time::NowFromSystemTime()
+              -base::Time::UnixEpoch()).ToInternalValue();

[manzagop (departed), 2014/07/31 15:31:51]

Fix indentation.

[gayane -on leave until 09-2017, 2014/08/04 13:46:31]

Done.

+

[manzagop (departed), 2014/07/31 15:31:52]

No need for the space.

[gayane -on leave until 09-2017, 2014/08/04 13:46:32]

Done.

+  // constant seed for rand() function.

[manzagop (departed), 2014/07/31 15:31:51]

Move the comment to the srand line, 2 spaces after the ';'

[gayane -on leave until 09-2017, 2014/08/04 13:46:31]

Done.

+  srand(7);
+
+  const int rand_limit = 1000000000;
+
+  SuggestionsProfile suggestions;
+

[manzagop (departed), 2014/07/31 15:31:51]

Too much vertical whitespace.

[gayane -on leave until 09-2017, 2014/08/04 13:46:31]

Done.

+  for (int i = 0; i < valid; i++){
+    ChromeSuggestion* suggestion = suggestions.add_suggestions();
+    suggestion->set_url(kTestUrl);
+    suggestion->set_title(kTestTitle);

[manzagop (departed), 2014/07/31 15:31:52]

These 3 lines are duplicated 3 times. Have a AddSuggestion function?

[gayane -on leave until 09-2017, 2014/08/04 13:46:32]

Done.

+    int r = rand()%rand_limit;

[manzagop (departed), 2014/07/31 15:31:51]

r isn't very descriptive. Something like future_offset_usec?

[gayane -on leave until 09-2017, 2014/08/04 13:46:31]

Done.

+    suggestion->set_expiry_ts(now + r + time_gap);

[manzagop (departed), 2014/07/31 15:31:53]

I'd move the time gap into the computation of r.

[gayane -on leave until 09-2017, 2014/08/04 13:46:31]

Done.

+
+  }
+

[manzagop (departed), 2014/07/31 15:31:52]

No need for this line.

[gayane -on leave until 09-2017, 2014/08/04 13:46:32]

Done.

+  for (int i = 0; i < expired; i++){
+    ChromeSuggestion* suggestion_expired = suggestions.add_suggestions();
+    suggestion_expired->set_url(kTestUrl);
+    suggestion_expired->set_title(kTestTitle);
+    int r = rand()%rand_limit;
+    suggestion_expired->set_expiry_ts(now  - r - time_gap);
+  }
+
+  return suggestions;
+}
+
 void ValidateSuggestions(const SuggestionsProfile& expected,
                          const SuggestionsProfile& actual) {
   EXPECT_EQ(expected.suggestions_size(), actual.suggestions_size());
   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 +79,101 @@ void ValidateSuggestions(const SuggestionsProfile& expected,
 
 }  // namespace
 
+// tests LoadSuggestions function to filter expired suggestions

[manzagop (departed), 2014/07/31 15:31:52]

Capitalize.

[gayane -on leave until 09-2017, 2014/08/04 13:46:32]

Done.

+TEST(SuggestionsStoreTest, LoadOneExpired) {
+  TestingPrefServiceSyncable prefs;
+  SuggestionsStore::RegisterProfilePrefs(prefs.registry());
+  SuggestionsStore suggestions_store(&prefs);
+
+  SuggestionsProfile suggestions = CreateTestSuggestions_LoadExpiryTests(1, 0);
+  SuggestionsProfile filtered_suggestions;
+
+  // store and load. Expired suggestion should not be loaded.

[manzagop (departed), 2014/07/31 15:31:52]

Capitalize.

[gayane -on leave until 09-2017, 2014/08/04 13:46:32]

Done.

+  EXPECT_TRUE(suggestions_store.StoreSuggestions(suggestions));
+  EXPECT_FALSE(suggestions_store.LoadSuggestions(&filtered_suggestions));
+  EXPECT_EQ(0, filtered_suggestions.suggestions_size());
+
+  // Clear.
+  suggestions_store.ClearSuggestions();

[manzagop (departed), 2014/07/31 15:31:52]

Does this do anything? Isn't clean up done from TestingPrefServiceSyncable's
destructor?

[gayane -on leave until 09-2017, 2014/08/04 13:46:32]

Removed

+}
+
+// tests LoadSuggestions function to filter expired suggestions
+TEST(SuggestionsStoreTest, LoadOneValid) {

[manzagop (departed), 2014/07/31 15:31:52]

Hm, this test is identical to the previous up to 3 characters. Perhaps a fixture
would help?

[gayane -on leave until 09-2017, 2014/08/04 13:46:32]

What is fixture?

+  TestingPrefServiceSyncable prefs;
+  SuggestionsStore::RegisterProfilePrefs(prefs.registry());
+  SuggestionsStore suggestions_store(&prefs);
+
+  SuggestionsProfile suggestions = CreateTestSuggestions_LoadExpiryTests(0, 1);
+  SuggestionsProfile filtered_suggestions;
+
+  // store and load. Suggestion should be loaded.
+  EXPECT_TRUE(suggestions_store.StoreSuggestions(suggestions));
+  EXPECT_TRUE(suggestions_store.LoadSuggestions(&filtered_suggestions));
+  EXPECT_EQ(1, filtered_suggestions.suggestions_size());
+
+  // Clear.
+  suggestions_store.ClearSuggestions();
+}
+
+// tests LoadSuggestions function to filter expired suggestions
+TEST(SuggestionsStoreTest, LoadMultipleExpired) {

[manzagop (departed), 2014/07/31 15:31:51]

What is the value of this test compared to the single expired? Tests are good,
but long files have a cost in mental complexity for the reader.

[gayane -on leave until 09-2017, 2014/08/04 13:46:31]

well I just thought of testing different cases. Unless you messed up the loop in
filtration both should work or both should fail.

+  TestingPrefServiceSyncable prefs;
+  SuggestionsStore::RegisterProfilePrefs(prefs.registry());
+  SuggestionsStore suggestions_store(&prefs);
+
+  SuggestionsProfile suggestions = CreateTestSuggestions_LoadExpiryTests(5, 0);
+  SuggestionsProfile filtered_suggestions;
+
+  // store and load. expired suggestions should not be loaded.
+  EXPECT_TRUE(suggestions_store.StoreSuggestions(suggestions));
+  EXPECT_FALSE(suggestions_store.LoadSuggestions(&filtered_suggestions));
+  EXPECT_EQ(0, filtered_suggestions.suggestions_size());
+
+  // Clear.
+  suggestions_store.ClearSuggestions();
+}
+
+// tests LoadSuggestions function to filter expired suggestions
+TEST(SuggestionsStoreTest, LoadValidAndExpired) {
+  TestingPrefServiceSyncable prefs;
+  SuggestionsStore::RegisterProfilePrefs(prefs.registry());
+  SuggestionsStore suggestions_store(&prefs);
+
+  SuggestionsProfile suggestions = CreateTestSuggestions_LoadExpiryTests(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());
+
+  // Clear.
+  suggestions_store.ClearSuggestions();
+}
+
+// tests LoadSuggestions function to filter expired suggestions
+TEST(SuggestionsStoreTest, LoadValidAndExpiredStore) {

[manzagop (departed), 2014/07/31 15:31:52]

Perhaps keep only this test and the AllExpired one?

[manzagop (departed), 2014/07/31 15:31:52]

I don't understand what the test does from the name (the store suffix).

[gayane -on leave until 09-2017, 2014/08/04 13:46:31]

Done.

[gayane -on leave until 09-2017, 2014/08/04 13:46:32]

yeah I changed the name, but I am not happy about this test actually. It is
intended to check that after filtering expired suggestions it stores the updated
list. Currently I check that by loading it again which is wrong for more than
one reason. First Load might not store and filter each time. Second, if more
suggestions get expired before second load is called because of time difference
then the test will fail.

+  TestingPrefServiceSyncable prefs;
+  SuggestionsStore::RegisterProfilePrefs(prefs.registry());
+  SuggestionsStore suggestions_store(&prefs);
+
+  SuggestionsProfile suggestions = CreateTestSuggestions_LoadExpiryTests(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());
+
+  SuggestionsProfile loaded_suggestions;
+  EXPECT_TRUE(suggestions_store.LoadSuggestions(&loaded_suggestions));
+  EXPECT_EQ(3, loaded_suggestions.suggestions_size());
+  ValidateSuggestions(filtered_suggestions, loaded_suggestions);
+
+  // Clear.
+  suggestions_store.ClearSuggestions();
+}
+
 TEST(SuggestionsStoreTest, LoadStoreClear) {
   TestingPrefServiceSyncable prefs;
   SuggestionsStore::RegisterProfilePrefs(prefs.registry());