Refactor 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"
@@ skipping 57 matching lines @@
 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);
 }
 
+void CaptureDismissedSuggestions(
+    std::vector<ContentSuggestion>* captured_suggestions,
+    std::vector<ContentSuggestion> dismissed_suggestions) {
+  std::move(dismissed_suggestions.begin(), dismissed_suggestions.end(),
+            std::back_inserter(*captured_suggestions));
+}
+
 }  // namespace
 
 class MockOfflinePageModel : public StubOfflinePageModel {

[tschumann, 2016/08/29 15:47:00]

nit: while we're at it. This is more a fake than a Mock. Rename?

[Philipp Keck, 2016/08/29 17:04:15]

Done.

  public:
   MockOfflinePageModel() {}
 
   void GetAllPages(const MultipleOfflinePageItemCallback& callback) override {
     callback.Run(items_);
   }
 
   std::vector<OfflinePageItem>* items() { return &items_; }

[tschumann, 2016/08/29 15:47:00]

a proto-inspired convention is to call these functions mutable_items().
then you can have a const version of item() which returns a reference and can be
used when verifying data.

[Philipp Keck, 2016/08/29 17:04:15]

Done.

 
  private:
   std::vector<OfflinePageItem> items_;
 };
 
 class OfflinePageSuggestionsProviderTest : public testing::Test {
  public:
   OfflinePageSuggestionsProviderTest()
       : pref_service_(new TestingPrefServiceSimple()) {
     OfflinePageSuggestionsProvider::RegisterProfilePrefs(
@@ skipping 47 matching lines @@
   }
 
   void FireOfflinePageDeleted(const OfflinePageItem& item) {
     provider_->OfflinePageDeleted(item.offline_id, item.client_id);
   }
 
   std::set<std::string> ReadDismissedIDsFromPrefs(Category category) {
     return provider_->ReadDismissedIDsFromPrefs(category);
   }
 
-  // Workaround to realize a DismissedSuggestionsCallback. Because gMock can't
-  // handle non-movable parameters, a helper method is needed to forward the
-  // call to the actual MOCK_METHOD.
-  void DismissedSuggestionsHelper(
-      std::vector<ContentSuggestion> dismissed_suggestions) {
-    ReceivedDismissedSuggestions(dismissed_suggestions);
-  }
-  MOCK_METHOD1(
-      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_;
@@ skipping 124 matching lines @@
                            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")))));
+  std::vector<ContentSuggestion> dismissed_suggestions;
   provider()->GetDismissedSuggestionsForDebugging(
       recent_tabs_category(),
-      base::Bind(
-          &OfflinePageSuggestionsProviderTest::DismissedSuggestionsHelper,
-          base::Unretained(this)));
-  Mock::VerifyAndClearExpectations(this);
+      base::Bind(&CaptureDismissedSuggestions, &dismissed_suggestions));
+  EXPECT_THAT(
+      dismissed_suggestions,
+      UnorderedElementsAre(Property(&ContentSuggestion::url,
+                                    GURL("file:///some/folder/test2.mhtml")),
+                           Property(&ContentSuggestion::url,
+                                    GURL("file:///some/folder/test3.mhtml"))));
 
   // The other category should have no dismissed suggestions.
-  EXPECT_CALL(*this, ReceivedDismissedSuggestions(IsEmpty()));
+  dismissed_suggestions.clear();
   provider()->GetDismissedSuggestionsForDebugging(
       downloads_category(),
-      base::Bind(
-          &OfflinePageSuggestionsProviderTest::DismissedSuggestionsHelper,
-          base::Unretained(this)));
-  Mock::VerifyAndClearExpectations(this);
+      base::Bind(&CaptureDismissedSuggestions, &dismissed_suggestions));
+  EXPECT_THAT(dismissed_suggestions, IsEmpty());
 
   // Clear dismissed suggestions.
   provider()->ClearDismissedSuggestionsForDebugging(recent_tabs_category());
 
   // They should be gone from the dismissed suggestions.
-  EXPECT_CALL(*this, ReceivedDismissedSuggestions(IsEmpty()));
+  dismissed_suggestions.clear();
   provider()->GetDismissedSuggestionsForDebugging(
       recent_tabs_category(),
-      base::Bind(
-          &OfflinePageSuggestionsProviderTest::DismissedSuggestionsHelper,
-          base::Unretained(this)));
-  Mock::VerifyAndClearExpectations(this);
+      base::Bind(&CaptureDismissedSuggestions, &dismissed_suggestions));
+  EXPECT_THAT(dismissed_suggestions, IsEmpty());
 
   // 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());
 }
 
@@ skipping 24 matching lines @@
       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));

[tschumann, 2016/08/29 15:47:00]

for symmetry, I'd also call model() here and inline the convenience method. It
just adds a layer of indirection, and because you call clear() below, it doesn't
even provide abstraction.

[Philipp Keck, 2016/08/29 17:04:15]

Done.

   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