Add OfflinePageSuggestionsProviderTest

components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc

Index: components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc
diff --git a/components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc b/components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..fe4a27cbdcebd1d429ef61292e2e96974eec24e0
--- /dev/null
+++ b/components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc
@@ -0,0 +1,412 @@
+// 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 MockDismissedSuggestionsCallback

[Marc Treib, 2016/08/26 12:14:50]

I find this pattern pretty weird - why not just make a MOCK_METHOD on the test
class? Is it mostly to circumvent the lack of move support in gtest?

[Philipp Keck, 2016/08/26 12:41:45]

Normally, a testing::MockFunction would do the job, but yes, here it doesn't
work because of the non-movable type. So we need a MOCK_METHOD and a helper
implementation of the real method.

Moved the two methods into the main test class (and renamed them).

+    : public ContentSuggestionsProvider::DismissedSuggestionsCallback {
+ public:
+  void Run(std::vector<ContentSuggestion> dismissed_suggestions) {
+    Call(dismissed_suggestions);
+  }
+  MOCK_METHOD1(
+      Call,
+      void(const std::vector<ContentSuggestion>& dismissed_suggestions));
+};
+
+class OfflinePageSuggestionsProviderTest : public testing::Test {
+ public:
+  OfflinePageSuggestionsProviderTest()
+      : pref_service_(new TestingPrefServiceSimple()) {
+    OfflinePageSuggestionsProvider::RegisterProfilePrefs(
+        pref_service()->registry());
+  }
+
+  ~OfflinePageSuggestionsProviderTest() override { provider_.reset(); }

[Marc Treib, 2016/08/26 12:14:50]

I don't think this is necessary?

[Philipp Keck, 2016/08/26 12:41:45]

Done.

+
+  void SetUp() override { CreateProvider(true, true, true); }

[Marc Treib, 2016/08/26 12:14:50]

Can this just happen in the ctor instead?

[Philipp Keck, 2016/08/26 12:41:45]

Done.

+
+  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);
+  }
+
+ protected:

[Marc Treib, 2016/08/26 12:14:50]

What's the difference between public and protected here?

[Philipp Keck, 2016/08/26 12:41:45]

None. Moved all to public.

+  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_;
+  // Last so that the dependencies are deleted after the provider.

[Marc Treib, 2016/08/26 12:14:50]

I think this will actually not be the case if you manually delete the provider
in the dtor.

[Philipp Keck, 2016/08/26 12:41:45]

Removed the comment.

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

Now that the dtor is gone, the comment might actually be relevant again? You
probably want to provider to be destroyed before the model.

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

The comment applies now. But is it important? I'm fine putting it back in, but
it doesn't really help. Like many other things you commented on in this CL, it
was only here because it's in NTPSnippetsServiceTest.

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

Yes, it is important: The provider calls model_->RemoveObserver in its dtor,
which is illegal if the model has already been destroyed.

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

Done.

+  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()),

[Marc Treib, 2016/08/26 12:14:50]

nit: parens not necessary

[Philipp Keck, 2016/08/26 12:41:45]

Done.

+      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()),

[Marc Treib, 2016/08/26 12:14:50]

also here (and a bunch more cases below)

[Philipp Keck, 2016/08/26 12:41:45]

Done.

+              OnNewSuggestions(
+                  _, downloads_category(),
+                  UnorderedElementsAre(AllOf(
+                      Property(&ContentSuggestion::url,
+                               GURL("file:///some/folder/test101.mhtml")),
+                      Property(&ContentSuggestion::title,
+                               base::UTF8ToUTF16("http://dummy.com/101"))))));
+
+  RecreateProvider(true, true, true);

[Marc Treib, 2016/08/26 12:14:50]

Why do you need to recreate the provider here? Wouldn't a "FireModelChanged" or
something like that be better?

[Philipp Keck, 2016/08/26 12:41:45]

Done.

+}
+
+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, dsiable downloads.

[Marc Treib, 2016/08/26 12:14:50]

typo

[Philipp Keck, 2016/08/26 12:41:45]

Done.

+  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_THAT(

[Marc Treib, 2016/08/26 12:14:50]

EXPECT_FALSE?

[Philipp Keck, 2016/08/26 12:41:45]

Done.

+      provider()->GetCategoryInfo(recent_tabs_category()).has_more_button(),
+      Eq(false));
+  EXPECT_THAT(
+      provider()->GetCategoryInfo(downloads_category()).has_more_button(),
+      Eq(true));
+  RecreateProvider(true, true, false);
+  EXPECT_THAT(
+      provider()->GetCategoryInfo(recent_tabs_category()).has_more_button(),
+      Eq(false));
+  EXPECT_THAT(
+      provider()->GetCategoryInfo(downloads_category()).has_more_button(),
+      Eq(false));
+}
+
+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.
+  MockDismissedSuggestionsCallback callback;
+  EXPECT_CALL(
+      callback,
+      Call(ElementsAre(Property(&ContentSuggestion::url,

[Marc Treib, 2016/08/26 12:14:50]

Unordered?

[Philipp Keck, 2016/08/26 12:41:45]

Done.

+                                GURL("file:///some/folder/test2.mhtml")),
+                       Property(&ContentSuggestion::url,
+                                GURL("file:///some/folder/test3.mhtml")))));
+  provider()->GetDismissedSuggestionsForDebugging(
+      recent_tabs_category(), base::Bind(&MockDismissedSuggestionsCallback::Run,
+                                         base::Unretained(&callback)));
+  Mock::VerifyAndClearExpectations(&callback);
+
+  // The other category should have no dismissed suggestions.
+  EXPECT_CALL(callback, Call(IsEmpty()));
+  provider()->GetDismissedSuggestionsForDebugging(
+      downloads_category(), base::Bind(&MockDismissedSuggestionsCallback::Run,
+                                       base::Unretained(&callback)));
+  Mock::VerifyAndClearExpectations(&callback);
+
+  // Clear dismissed suggestions.
+  provider()->ClearDismissedSuggestionsForDebugging(recent_tabs_category());
+
+  // They should be gone from the dismissed suggestions.
+  EXPECT_CALL(callback, Call(IsEmpty()));
+  provider()->GetDismissedSuggestionsForDebugging(
+      recent_tabs_category(), base::Bind(&MockDismissedSuggestionsCallback::Run,
+                                         base::Unretained(&callback)));
+  Mock::VerifyAndClearExpectations(&callback);
+
+  // 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