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 135 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(
+  void CaptureDismissedSuggestions(

[Marc Treib, 2016/08/29 14:54:29]

This could be a global function, or at least a static method. That'll make
Bind'ing easier.

[tschumann, 2016/08/29 15:19:11]

Let's make this a function in an unnamed namespace, as it's independent from the
test fixture.

[Philipp Keck, 2016/08/29 15:27:05]

Done.

+      std::vector<ContentSuggestion>* captured_suggestions,
       std::vector<ContentSuggestion> dismissed_suggestions) {
-    ReceivedDismissedSuggestions(dismissed_suggestions);
+    std::move(dismissed_suggestions.begin(), dismissed_suggestions.end(),
+              std::back_inserter(*captured_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_;
@@ skipping 125 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)));
+          &OfflinePageSuggestionsProviderTest::CaptureDismissedSuggestions,
+          base::Unretained(this), &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"))));
   Mock::VerifyAndClearExpectations(this);

[tschumann, 2016/08/29 15:19:11]

you can drop this line here and in other places now.

You should only need that call rarely. Setting up multiple expectations creates
a stack and the first expectation that matches will be executed. 
And usually, the matchers of the expectation are precise enough to prevent this.
If you really need something like this, calling .RetiresOnSaturation(); at the
end of the EXPECT_CALL() statement makes sure the expectation gets removed once
saturated (but again, use that rarely).

(and if you really want to get everything verified at a specific place, consider
splitting up the test).

[Philipp Keck, 2016/08/29 15:27:05]

Done.

 
   // 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)));
+          &OfflinePageSuggestionsProviderTest::CaptureDismissedSuggestions,
+          base::Unretained(this), &dismissed_suggestions));
+  EXPECT_THAT(dismissed_suggestions, IsEmpty());
   Mock::VerifyAndClearExpectations(this);
 
   // 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)));
+          &OfflinePageSuggestionsProviderTest::CaptureDismissedSuggestions,
+          base::Unretained(this), &dismissed_suggestions));
+  EXPECT_THAT(dismissed_suggestions, IsEmpty());
   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());
 }
@@ skipping 48 matching lines @@
   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