Refactor 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
index bdceafd23489f3695232dba18db693e0f91036ba..64288f6cf2e063f8d3aa430dba0c18ed71b3bc4c 100644
--- a/components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc
+++ b/components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc
@@ -153,16 +153,12 @@ class OfflinePageSuggestionsProviderTest : public testing::Test {
     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_; }
@@ -308,37 +304,41 @@ TEST_F(OfflinePageSuggestionsProviderTest, ShouldDismiss) {
   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.