Add OfflinePageSuggestionsProviderTest

components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc

+// Copyright 2016 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/ntp_snippets/offline_pages/offline_page_suggestions_provider.h"
+
+#include <string>
+#include <vector>
+
+#include "base/bind.h"
+#include "base/files/file_path.h"
+#include "base/guid.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/strings/utf_string_conversions.h"
+#include "base/time/time.h"
+#include "components/ntp_snippets/category.h"
+#include "components/ntp_snippets/category_factory.h"
+#include "components/ntp_snippets/content_suggestions_provider.h"
+#include "components/ntp_snippets/mock_content_suggestions_provider_observer.h"
+#include "components/offline_pages/client_namespace_constants.h"
+#include "components/offline_pages/offline_page_item.h"
+#include "components/offline_pages/stub_offline_page_model.h"
+#include "components/prefs/testing_pref_service.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using offline_pages::ClientId;
+using offline_pages::MultipleOfflinePageItemCallback;
+using offline_pages::OfflinePageItem;
+using offline_pages::StubOfflinePageModel;
+using testing::AllOf;
+using testing::Eq;
+using testing::Invoke;
+using testing::IsEmpty;
+using testing::Property;
+using testing::ElementsAre;
+using testing::Mock;
+using testing::UnorderedElementsAre;
+using testing::UnorderedElementsAreArray;
+using testing::SizeIs;
+using testing::WhenSortedBy;
+using testing::_;
+
+namespace ntp_snippets {
+
+namespace {
+
+struct OrderByMostRecentlyVisited {
+  bool operator()(const OfflinePageItem* left,
+                  const OfflinePageItem* right) const {
+    return left->last_access_time > right->last_access_time;
+  }
+};
+
+OfflinePageItem CreateDummyItem(std::string name_space, int id) {
+  std::string strid = base::IntToString(id);
+  return OfflinePageItem(GURL("http://dummy.com/" + strid), id,
+                         ClientId(name_space, base::GenerateGUID()),
+                         base::FilePath("some/folder/test" + strid + ".mhtml"),
+                         0, base::Time::Now());
+}
+
+OfflinePageItem CreateDummyRecentTab(int id) {
+  return CreateDummyItem(offline_pages::kLastNNamespace, id);
+}
+
+OfflinePageItem CreateDummyRecentTab(int id, base::Time time) {
+  OfflinePageItem item = CreateDummyRecentTab(id);
+  item.last_access_time = time;
+  return item;
+}
+
+OfflinePageItem CreateDummyDownload(int id) {
+  return CreateDummyItem(offline_pages::kAsyncNamespace, id);
+}
+
+}  // namespace
+
+class MockOfflinePageModel : public StubOfflinePageModel {
+ public:
+  MockOfflinePageModel() {}
+
+  void GetAllPages(const MultipleOfflinePageItemCallback& callback) override {
+    callback.Run(items_);
+  }
+
+  std::vector<OfflinePageItem>* items() { return &items_; }
+
+ private:
+  std::vector<OfflinePageItem> items_;
+};
+
+class OfflinePageSuggestionsProviderTest : public testing::Test {
+ public:
+  OfflinePageSuggestionsProviderTest()
+      : pref_service_(new TestingPrefServiceSimple()) {
+    OfflinePageSuggestionsProvider::RegisterProfilePrefs(
+        pref_service()->registry());
+    CreateProvider(true, true, true);
+  }
+
+  void RecreateProvider(bool recent_tabs_enabled,
+                        bool downloads_enabled,
+                        bool download_manager_ui_enabled) {
+    provider_.reset();
+    CreateProvider(recent_tabs_enabled, downloads_enabled,
+                   download_manager_ui_enabled);
+  }
+
+  void CreateProvider(bool recent_tabs_enabled,
+                      bool downloads_enabled,
+                      bool download_manager_ui_enabled) {
+    DCHECK(!provider_);
+
+    provider_.reset(new OfflinePageSuggestionsProvider(
+        recent_tabs_enabled, downloads_enabled, download_manager_ui_enabled,
+        &observer_, &category_factory_, &model_, pref_service()));
+  }
+
+  Category recent_tabs_category() {
+    return category_factory_.FromKnownCategory(KnownCategories::RECENT_TABS);
+  }
+
+  Category downloads_category() {
+    return category_factory_.FromKnownCategory(KnownCategories::DOWNLOADS);
+  }
+
+  void AddItem(OfflinePageItem item) { model()->items()->push_back(item); }
+
+  std::string GetDummySuggestionId(Category category, int id) {
+    return provider_->MakeUniqueID(category, base::IntToString(id));
+  }
+
+  ContentSuggestion CreateDummySuggestion(Category category, int id) {
+    std::string strid = base::IntToString(id);
+    ContentSuggestion result(
+        GetDummySuggestionId(category, id),
+        GURL("file:///some/folder/test" + strid + ".mhtml"));
+    result.set_title(base::UTF8ToUTF16("http://dummy.com/" + strid));
+    return result;
+  }
+
+  void FireOfflinePageModelChanged() {
+    provider_->OfflinePageModelChanged(model());
+  }
+
+  void FireOfflinePageDeleted(const OfflinePageItem& item) {
+    provider_->OfflinePageDeleted(item.offline_id, item.client_id);
+  }
+
+  std::set<std::string> ReadDismissedIDsFromPrefs(Category category) {
+    return provider_->ReadDismissedIDsFromPrefs(category);
+  }
+
+  void ReceiveDismissedSuggestions(
+      std::vector<ContentSuggestion> dismissed_suggestions) {
+    ReceivedDismissedSuggestions(dismissed_suggestions);
+  }
+  MOCK_METHOD1(

[Marc Treib, 2016/08/26 12:52:03]

nit: Add a comment explaining the workaround?
It might also be good to give the two methods more distinguishable names.

[Philipp Keck, 2016/08/26 17:19:55]

Done.
Now they have distinguishable names, but I'm not perfectly happy with either of
the names, so let me know if you have better ideas.

[Marc Treib, 2016/08/29 09:28:48]

No better ideas, no...
One suggestion Tim had in a similar situation: Use the same name, but a
different type (std::list instead of std::vector) in the second method (the
helper will have to copy stuff from the vector into the list). That way, the
test body looks less confusing (same method name), and matchers will still work
the same. And if gmock ever supports movable types, we just remove the helper
and that's it.
But I'm fine with it either way - your call.

[Philipp Keck, 2016/08/29 11:32:42]

I tried, but this std::list workaround is not possible. The std::Bind call
"couldn't infer template argument 'Functor'".

[tschumann, 2016/08/29 12:45:14]

in this particular case, I wouldn't use the MOCK macro anyways as you're not
implementing a mock but much more a fake.
The mock is for verifying behavior, the fake is more concerned about
before/after states.
You could simply capture the state and after the test verify the proper data is
stored. Would be cleaner IMO as no other methods are mocked out.

[Marc Treib, 2016/08/29 13:01:51]

I disagree: I find a mock method and EXPECT_CALLs much easier to follow than
manually storing and checking some state. With EXPECT_CALL, all the expectations
are right in the test body; otherwise, I have to look up into the test class to
see what happens.

[Philipp Keck, 2016/08/29 13:35:06]

@Tim: Yes, currently, there are no other methods mocked out. But there will be,
for example when we want to use the ImageFetchedCallback once the image fetching
is supported by the provider. It might something like this:
EXPECT_CALL(*this, ReceivedSuggestionImage(AnyNonEmptyImageMatcher()));
and there are other, similar cases in other unit tests where the same reasoning
applies (but we currently use MOCK and EXPECT_CALL). Implementing a fake class
to store away the data to be checked later is more work and more code to read.

And I also see Marc's point that it would make the test harder to understand
(more data flows), even though I agree that the testing situation here is
somewhat a corner case between mock and fake.

[tschumann, 2016/08/29 14:13:32]

IMO, adding MOCK methods on a test is abusing the mock framework and
methodology.

For example, you use this function and a mock because all you need is a way to
store some results.
Let's look at one of the usages (from below):
 305   // And appear in the dismissed suggestions for the right category.
 306   EXPECT_CALL(*this, ReceivedDismissedSuggestions(UnorderedElementsAre(
 307                          Property(&ContentSuggestion::url,
 308                                   GURL("file:///some/folder/test2.mhtml")),
 309                          Property(&ContentSuggestion::url,
 310                                  
GURL("file:///some/folder/test3.mhtml")))));
 311   provider()->GetDismissedSuggestionsForDebugging(
 312       recent_tabs_category(),
 313       base::Bind(
 314           &OfflinePageSuggestionsProviderTest::ReceiveDismissedSuggestions,
 315           base::Unretained(this)));
 316   Mock::VerifyAndClearExpectations(this);
 317 

What you want to verify is, that the callback you're passing into the function
get's called with a certain set of parameters.
If you want to make sure the callback gets only called once, you should pass in
a MockFunction.
If you just want to verify the state, then you should do something like

namespace {

void CaptureSuggestionUrls(std::vector<std::string>>* captured_urls,
std::vector<ContentSuggestion> suggestions) {
  //... copy urls to output parameter
}

} // namespace.

and have a test like:

std::vector<std::string>> captured_urls;
provider()->GetDismissedSuggestionsForDebugging(
    recent_tabs_category(),
    base::Bind(
        &CaptureSuggestionUrls, &captured_urls));
EXPECT_THAT(captured_urls, UnorderedElementsAre(...));

Which makes the test self-contained and easy to follow.

+      ReceivedDismissedSuggestions,
+      void(const std::vector<ContentSuggestion>& dismissed_suggestions));
+
+  ContentSuggestionsProvider* provider() { return provider_.get(); }
+  MockOfflinePageModel* model() { return &model_; }
+  MockContentSuggestionsProviderObserver* observer() { return &observer_; }
+  TestingPrefServiceSimple* pref_service() { return pref_service_.get(); }
+
+ private:
+  MockOfflinePageModel model_;
+  MockContentSuggestionsProviderObserver observer_;
+  CategoryFactory category_factory_;
+  std::unique_ptr<TestingPrefServiceSimple> pref_service_;
+  std::unique_ptr<OfflinePageSuggestionsProvider> provider_;
+
+  DISALLOW_COPY_AND_ASSIGN(OfflinePageSuggestionsProviderTest);
+};
+
+TEST_F(OfflinePageSuggestionsProviderTest, ShouldSplitAndConvertToSuggestions) {
+  AddItem(CreateDummyRecentTab(1));
+  AddItem(CreateDummyRecentTab(2));
+  AddItem(CreateDummyRecentTab(3));
+  AddItem(CreateDummyDownload(101));
+
+  EXPECT_CALL(
+      *observer(),
+      OnNewSuggestions(_, recent_tabs_category(),
+                       UnorderedElementsAre(
+                           Property(&ContentSuggestion::url,
+                                    GURL("file:///some/folder/test1.mhtml")),
+                           Property(&ContentSuggestion::url,
+                                    GURL("file:///some/folder/test2.mhtml")),
+                           Property(&ContentSuggestion::url,
+                                    GURL("file:///some/folder/test3.mhtml")))));
+
+  EXPECT_CALL(*observer(),
+              OnNewSuggestions(
+                  _, downloads_category(),
+                  UnorderedElementsAre(AllOf(
+                      Property(&ContentSuggestion::url,
+                               GURL("file:///some/folder/test101.mhtml")),
+                      Property(&ContentSuggestion::title,
+                               base::UTF8ToUTF16("http://dummy.com/101"))))));
+
+  FireOfflinePageModelChanged();
+}
+
+TEST_F(OfflinePageSuggestionsProviderTest, ShouldIgnoreDisabledCategories) {
+  AddItem(CreateDummyRecentTab(1));
+  AddItem(CreateDummyRecentTab(2));
+  AddItem(CreateDummyRecentTab(3));
+  AddItem(CreateDummyDownload(101));
+
+  // Disable recent tabs, enable downloads.
+  EXPECT_CALL(*observer(), OnNewSuggestions(_, recent_tabs_category(), _))
+      .Times(0);
+  EXPECT_CALL(
+      *observer(),
+      OnNewSuggestions(_, downloads_category(),
+                       UnorderedElementsAre(Property(
+                           &ContentSuggestion::url,
+                           GURL("file:///some/folder/test101.mhtml")))));
+  RecreateProvider(false, true, true);
+  Mock::VerifyAndClearExpectations(observer());
+
+  // Enable recent tabs, disable downloads.
+  EXPECT_CALL(
+      *observer(),
+      OnNewSuggestions(_, recent_tabs_category(),
+                       UnorderedElementsAre(
+                           Property(&ContentSuggestion::url,
+                                    GURL("file:///some/folder/test1.mhtml")),
+                           Property(&ContentSuggestion::url,
+                                    GURL("file:///some/folder/test2.mhtml")),
+                           Property(&ContentSuggestion::url,
+                                    GURL("file:///some/folder/test3.mhtml")))));
+  EXPECT_CALL(*observer(), OnNewSuggestions(_, downloads_category(), _))
+      .Times(0);
+  RecreateProvider(true, false, true);
+  Mock::VerifyAndClearExpectations(observer());
+}
+
+TEST_F(OfflinePageSuggestionsProviderTest, ShouldSortByMostRecentlyVisited) {
+  base::Time now = base::Time::Now();
+  base::Time yesterday = now - base::TimeDelta::FromDays(1);
+  base::Time tomorrow = now + base::TimeDelta::FromDays(1);
+  AddItem(CreateDummyRecentTab(1, now));
+  AddItem(CreateDummyRecentTab(2, yesterday));
+  AddItem(CreateDummyRecentTab(3, tomorrow));
+
+  EXPECT_CALL(
+      *observer(),
+      OnNewSuggestions(
+          _, recent_tabs_category(),
+          ElementsAre(Property(&ContentSuggestion::url,
+                               GURL("file:///some/folder/test3.mhtml")),
+                      Property(&ContentSuggestion::url,
+                               GURL("file:///some/folder/test1.mhtml")),
+                      Property(&ContentSuggestion::url,
+                               GURL("file:///some/folder/test2.mhtml")))));
+  EXPECT_CALL(*observer(), OnNewSuggestions(_, downloads_category(), _));
+  FireOfflinePageModelChanged();
+}
+
+TEST_F(OfflinePageSuggestionsProviderTest, ShouldDeliverCorrectCategoryInfo) {
+  EXPECT_FALSE(
+      provider()->GetCategoryInfo(recent_tabs_category()).has_more_button());
+  EXPECT_TRUE(
+      provider()->GetCategoryInfo(downloads_category()).has_more_button());
+  RecreateProvider(true, true, false);
+  EXPECT_FALSE(
+      provider()->GetCategoryInfo(recent_tabs_category()).has_more_button());
+  EXPECT_FALSE(
+      provider()->GetCategoryInfo(downloads_category()).has_more_button());
+}
+
+TEST_F(OfflinePageSuggestionsProviderTest, ShouldDismiss) {
+  AddItem(CreateDummyRecentTab(1));
+  AddItem(CreateDummyRecentTab(2));
+  AddItem(CreateDummyRecentTab(3));
+  AddItem(CreateDummyRecentTab(4));
+  FireOfflinePageModelChanged();
+
+  // Dismiss 2 and 3.
+  EXPECT_CALL(*observer(), OnNewSuggestions(_, _, _)).Times(0);
+  provider()->DismissSuggestion(
+      GetDummySuggestionId(recent_tabs_category(), 2));
+  provider()->DismissSuggestion(
+      GetDummySuggestionId(recent_tabs_category(), 3));
+  Mock::VerifyAndClearExpectations(observer());
+
+  // They should disappear from the reported suggestions.
+  EXPECT_CALL(
+      *observer(),
+      OnNewSuggestions(_, recent_tabs_category(),
+                       UnorderedElementsAre(
+                           Property(&ContentSuggestion::url,
+                                    GURL("file:///some/folder/test1.mhtml")),
+                           Property(&ContentSuggestion::url,
+                                    GURL("file:///some/folder/test4.mhtml")))));
+  EXPECT_CALL(*observer(),
+              OnNewSuggestions(_, downloads_category(), IsEmpty()));
+  FireOfflinePageModelChanged();
+  Mock::VerifyAndClearExpectations(observer());
+
+  // And appear in the dismissed suggestions for the right category.
+  EXPECT_CALL(*this, ReceivedDismissedSuggestions(UnorderedElementsAre(
+                         Property(&ContentSuggestion::url,
+                                  GURL("file:///some/folder/test2.mhtml")),
+                         Property(&ContentSuggestion::url,
+                                  GURL("file:///some/folder/test3.mhtml")))));
+  provider()->GetDismissedSuggestionsForDebugging(
+      recent_tabs_category(),
+      base::Bind(
+          &OfflinePageSuggestionsProviderTest::ReceiveDismissedSuggestions,
+          base::Unretained(this)));
+  Mock::VerifyAndClearExpectations(this);
+
+  // The other category should have no dismissed suggestions.
+  EXPECT_CALL(*this, ReceivedDismissedSuggestions(IsEmpty()));
+  provider()->GetDismissedSuggestionsForDebugging(
+      downloads_category(),
+      base::Bind(
+          &OfflinePageSuggestionsProviderTest::ReceiveDismissedSuggestions,
+          base::Unretained(this)));
+  Mock::VerifyAndClearExpectations(this);
+
+  // Clear dismissed suggestions.
+  provider()->ClearDismissedSuggestionsForDebugging(recent_tabs_category());
+
+  // They should be gone from the dismissed suggestions.
+  EXPECT_CALL(*this, ReceivedDismissedSuggestions(IsEmpty()));
+  provider()->GetDismissedSuggestionsForDebugging(
+      recent_tabs_category(),
+      base::Bind(
+          &OfflinePageSuggestionsProviderTest::ReceiveDismissedSuggestions,
+          base::Unretained(this)));
+  Mock::VerifyAndClearExpectations(this);
+
+  // And appear in the reported suggestions for the category again.
+  EXPECT_CALL(*observer(),
+              OnNewSuggestions(_, recent_tabs_category(), SizeIs(4)));
+  EXPECT_CALL(*observer(),
+              OnNewSuggestions(_, downloads_category(), IsEmpty()));
+  FireOfflinePageModelChanged();
+  Mock::VerifyAndClearExpectations(observer());
+}
+
+TEST_F(OfflinePageSuggestionsProviderTest,
+       ShouldInvalidateWhenOfflinePageDeleted) {
+  AddItem(CreateDummyRecentTab(1));
+  AddItem(CreateDummyRecentTab(2));
+  AddItem(CreateDummyRecentTab(3));
+  FireOfflinePageModelChanged();
+
+  // Invalidation of suggestion 2 should be forwarded.
+  EXPECT_CALL(
+      *observer(),
+      OnSuggestionInvalidated(_, recent_tabs_category(),
+                              GetDummySuggestionId(recent_tabs_category(), 2)));
+  FireOfflinePageDeleted(model()->items()->at(1));
+}
+
+TEST_F(OfflinePageSuggestionsProviderTest, ShouldClearDismissedOnInvalidate) {
+  AddItem(CreateDummyRecentTab(1));
+  AddItem(CreateDummyRecentTab(2));
+  AddItem(CreateDummyRecentTab(3));
+  FireOfflinePageModelChanged();
+  EXPECT_THAT(ReadDismissedIDsFromPrefs(recent_tabs_category()), IsEmpty());
+  EXPECT_THAT(ReadDismissedIDsFromPrefs(downloads_category()), IsEmpty());
+
+  provider()->DismissSuggestion(
+      GetDummySuggestionId(recent_tabs_category(), 2));
+  EXPECT_THAT(ReadDismissedIDsFromPrefs(recent_tabs_category()), SizeIs(1));
+  EXPECT_THAT(ReadDismissedIDsFromPrefs(downloads_category()), IsEmpty());
+
+  FireOfflinePageDeleted(model()->items()->at(1));
+  EXPECT_THAT(ReadDismissedIDsFromPrefs(recent_tabs_category()), IsEmpty());
+  EXPECT_THAT(ReadDismissedIDsFromPrefs(downloads_category()), IsEmpty());
+}
+
+TEST_F(OfflinePageSuggestionsProviderTest, ShouldClearDismissedOnFetch) {
+  AddItem(CreateDummyDownload(1));
+  AddItem(CreateDummyDownload(2));
+  AddItem(CreateDummyDownload(3));
+  FireOfflinePageModelChanged();
+
+  provider()->DismissSuggestion(GetDummySuggestionId(downloads_category(), 2));
+  provider()->DismissSuggestion(GetDummySuggestionId(downloads_category(), 3));
+  EXPECT_THAT(ReadDismissedIDsFromPrefs(recent_tabs_category()), IsEmpty());
+  EXPECT_THAT(ReadDismissedIDsFromPrefs(downloads_category()), SizeIs(2));
+
+  model()->items()->clear();
+  AddItem(CreateDummyDownload(2));
+  FireOfflinePageModelChanged();
+  EXPECT_THAT(ReadDismissedIDsFromPrefs(recent_tabs_category()), IsEmpty());
+  EXPECT_THAT(ReadDismissedIDsFromPrefs(downloads_category()), SizeIs(1));
+
+  model()->items()->clear();
+  FireOfflinePageModelChanged();
+  EXPECT_THAT(ReadDismissedIDsFromPrefs(recent_tabs_category()), IsEmpty());
+  EXPECT_THAT(ReadDismissedIDsFromPrefs(downloads_category()), IsEmpty());
+}
+
+}  // namespace ntp_snippets