[Suggestions Service] Add support for expiring the SuggestionsStore

components/suggestions/suggestions_store_unittest.cc

 // Copyright 2014 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/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"
 
 using user_prefs::TestingPrefServiceSyncable;
 
 namespace suggestions {
 
 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();
   suggestion->set_url(kTestUrl);
   suggestion->set_title(kTestTitle);
   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(),
               actual.suggestions(i).thumbnail());
   }
 }
 
 }  // 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());
   SuggestionsStore suggestions_store(&prefs);
 
   const SuggestionsProfile suggestions = CreateTestSuggestions();
   const SuggestionsProfile empty_suggestions;
   SuggestionsProfile recovered_suggestions;
 
   // Attempt to load when prefs are empty.
   EXPECT_FALSE(suggestions_store.LoadSuggestions(&recovered_suggestions));
   ValidateSuggestions(empty_suggestions, recovered_suggestions);
 
   // Store then reload.
   EXPECT_TRUE(suggestions_store.StoreSuggestions(suggestions));
   EXPECT_TRUE(suggestions_store.LoadSuggestions(&recovered_suggestions));
   ValidateSuggestions(suggestions, recovered_suggestions);
 
   // Clear.
   suggestions_store.ClearSuggestions();
   EXPECT_FALSE(suggestions_store.LoadSuggestions(&recovered_suggestions));
   ValidateSuggestions(empty_suggestions, recovered_suggestions);
 }
 
 }  // namespace suggestions