[AppList] Add a Suggestions Provider to applist.

chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider_unittest.cc

Index: chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider_unittest.cc
diff --git a/chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider_unittest.cc b/chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..a168fda69e901e7bd5a70e255e60491428e20d16
--- /dev/null
+++ b/chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider_unittest.cc
@@ -0,0 +1,145 @@
+// 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 "chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider.h"
+
+#include <algorithm>
+#include <string>
+#include <vector>
+
+#include "base/basictypes.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/strings/utf_string_conversions.h"
+#include "base/time/time.h"
+#include "chrome/browser/sync/profile_sync_service_mock.h"
+#include "chrome/browser/ui/app_list/app_list_test_util.h"
+#include "chrome/test/base/testing_pref_service_syncable.h"
+#include "chrome/test/base/testing_profile.h"
+#include "components/suggestions/proto/suggestions.pb.h"
+#include "components/suggestions/suggestions_store.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "ui/app_list/search_result.h"
+
+using suggestions::ChromeSuggestion;
+using suggestions::SuggestionsProfile;
+using suggestions::SuggestionsStore;
+
+namespace app_list {
+namespace test {
+
+namespace {
+
+// Used to expire suggestions in the future.
+const int kTimeGapUsec = 100000;  // 0.1s.
+
+void AddSuggestion(SuggestionsProfile* suggestions, const char* title,
+                   const char* url) {
+  ChromeSuggestion* suggestion = suggestions->add_suggestions();
+  suggestion->set_url(title);
+  suggestion->set_title(url);
+  int64 now_usec = (base::Time::NowFromSystemTime() - base::Time::UnixEpoch())

[calamity, 2014/12/05 04:01:58]

I think there are some issues with using base::Time::Now() in tests. Maybe
mgiuca@ can elaborate?

[Matt Giuca, 2014/12/05 06:49:06]

Well, it's generally bad to be using Now() in tests. It's not clear whether
there is an actual problem here.

At best, using Now() means the test will have different values coursing through
it on every run, so there is potential for different behaviour (even if there
shouldn't be, bugs could cause the test to pass sometimes and fail other times
--- the very bugs the test is trying to guard against).

But most of the time, using Now() in tests usually implies that the code under
test will also be using Now(), which means there are going to be timing issues.
I haven't looked at the code under test, but this test has me quite worried
since you are setting an expiry time of Now() + 0.1s. This hints that if the
test were to happen to take longer than 0.1s to run, it would fail. Which means
it is flaky.

I haven't looked deep enough into this to determine whether my suspicions are
correct (it will have to wait till Monday), but I think even if there is no
actual flakiness, it is still best practice to use dummy (hard-coded) time
values in tests.

[Mathieu, 2014/12/05 20:14:32]

Hard-coded an expiry in 40 years. Not to be pessimistic, but it's likely this
code won't be around then :).

[Matt Giuca, 2014/12/09 05:34:54]

OK, that's better (I think), but it still means that the code under test is
using Now(), which is not ideal. (In theory, code being tested should be running
the same data on every test run. What if, for example, there was a bug that only
occurs on Wednesdays? Then the test would flake out on Wednesdays, whereas if
you can force the code to think the time is a fixed value, then it will either
always pass or always fail.)

I have filed a bug for this (http://crbug.com/440252) and made a suggestion
about how to fix SuggestionStore. If that fix is taken, it should be very easy
for this test to call SuggestionStore::SetNowUsecForTest(), and make the tests
fully deterministic.

I will leave it up to you whether you want to do that before this CL, or just
commit this first and fix it up later. It isn't a big deal and I think now that
you've hard-coded a 40-year expiry time, it is fine to submit as-is. But please
add a TODO mentioning the bug number I filed. (And you should make sure you get
around to it some time within the next 40 years :p).

[Mathieu, 2014/12/09 21:18:25]

Appreciate the filed bugs. Will do it soon, but not now.

+                       .ToInternalValue();
+  suggestion->set_expiry_ts(now_usec + kTimeGapUsec);
+}
+
+bool MostRelevant(const SearchResult* result1, const SearchResult* result2) {
+  return result1->relevance() > result2->relevance();
+}
+
+}  // namespace
+
+class SuggestionsSearchProviderTest : public AppListTestBase {
+ public:
+  SuggestionsSearchProviderTest() {}
+  ~SuggestionsSearchProviderTest() override {}
+
+  // AppListTestBase overrides:
+  void SetUp() override {
+    AppListTestBase::SetUp();
+
+    profile_.reset(ProfileSyncServiceMock::MakeSignedInTestingProfile());
+    TestingPrefServiceSyncable* pref_service =
+        profile_->GetTestingPrefService();
+    SuggestionsStore::RegisterProfilePrefs(pref_service->registry());
+    suggestions_store_.reset(new SuggestionsStore(pref_service));
+
+    // Under test.
+    suggestions_search_.reset(
+        new SuggestionsSearchProvider(profile_.get(), NULL));
+  }
+
+  void TearDown() override {
+    // Making sure suggestions are cleared between tests.
+    suggestions_store_->ClearSuggestions();
+    profile_.reset();
+  }
+
+  std::string RunQuery(const std::string& query) {
+    suggestions_search_->Start(base::UTF8ToUTF16(query));
+
+    // Sort results from most to least relevant.
+    std::vector<SearchResult*> sorted_results(
+        suggestions_search_->results().begin(),
+        suggestions_search_->results().end());
+    std::sort(sorted_results.begin(), sorted_results.end(), &MostRelevant);
+
+    std::string result_str;
+    for (size_t i = 0; i < sorted_results.size(); ++i) {
+      if (!result_str.empty()) result_str += ',';
+
+      result_str += base::UTF16ToUTF8(sorted_results[i]->title());
+    }
+    return result_str;
+  }
+
+ protected:
+  // Used to store fake suggestions data.
+  scoped_ptr<SuggestionsStore> suggestions_store_;
+
+ private:
+  // Under test.
+  scoped_ptr<SuggestionsSearchProvider> suggestions_search_;
+
+  // Helpers.
+  ProfileSyncServiceMock* mock_pss_;
+
+  DISALLOW_COPY_AND_ASSIGN(SuggestionsSearchProviderTest);
+};
+
+TEST_F(SuggestionsSearchProviderTest, Basic) {
+  EXPECT_EQ("", RunQuery("anything other than empty string"));
+
+  // Empty query with no suggestions in cache returns nothing.
+  EXPECT_EQ("", RunQuery(""));
+}
+
+TEST_F(SuggestionsSearchProviderTest, OneSuggestion) {
+  SuggestionsProfile profile;
+  AddSuggestion(&profile, "http://foo.com", "title 1");
+  EXPECT_TRUE(suggestions_store_->StoreSuggestions(profile));
+
+  // Empty query with 1 suggestion in cache returns it.
+  EXPECT_EQ("title 1", RunQuery(""));
+
+  // Non-empty query returns nothing.
+  EXPECT_EQ("", RunQuery("t"));
+}
+
+TEST_F(SuggestionsSearchProviderTest, ManySuggestions) {
+  SuggestionsProfile profile;
+  AddSuggestion(&profile, "http://foo.com", "foo");
+  AddSuggestion(&profile, "http://bar.com", "bar");
+  EXPECT_TRUE(suggestions_store_->StoreSuggestions(profile));
+
+  // Empty query done, while there are 2 suggestions in the cache, returns
+  // those suggestions, with "foo" being more relevant since it came first in
+  // the suggestions profile.
+  EXPECT_EQ("foo,bar", RunQuery(""));
+
+  // Non-empty query returns nothing.
+  EXPECT_EQ("", RunQuery("t"));
+}
+
+}  // namespace test
+}  // namespace app_list