[NTP Snippets] FetchMore backend

components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc

Index: components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc
diff --git a/components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc b/components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc
index c48f88fc60012248507091a2b3707a49448c0aa4..655c57f26fbc0a883001370e7299fb38e18aaec6 100644
--- a/components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc
+++ b/components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc
@@ -173,15 +173,26 @@ class NTPSnippetsFetcherTest : public testing::Test {
         pref_service_.get(), &category_factory_, nullptr,
         base::Bind(&ParseJsonDelayed), kAPIKey, user_classifier_.get());
 
-    snippets_fetcher_->SetCallback(
-        base::Bind(&MockSnippetsAvailableCallback::WrappedRun,
-                   base::Unretained(&mock_callback_)));
     snippets_fetcher_->SetTickClockForTesting(
         mock_task_runner_->GetMockTickClock());
     // Increase initial time such that ticks are non-zero.
     mock_task_runner_->FastForwardBy(base::TimeDelta::FromMilliseconds(1234));
   }
 
+  NTPSnippetsFetcher::SnippetsAvailableCallback MakeMockCallback() {
+    return base::BindOnce(&MockSnippetsAvailableCallback::WrappedRun,
+                          base::Unretained(&mock_callback_));
+  }
+
+  base::Optional<Category> NoExclusiveCategory() {
+    return base::Optional<Category>();
+  }
+
+  base::Optional<Category> OptionalArticlesCategory() {
+    return base::Optional<Category>(
+        CategoryFactory().FromKnownCategory(KnownCategories::ARTICLES));
+  }
+
   NTPSnippetsFetcher& snippets_fetcher() { return *snippets_fetcher_; }
   MockSnippetsAvailableCallback& mock_callback() { return mock_callback_; }
   void FastForwardUntilNoTasksRemain() {
@@ -508,7 +519,7 @@ TEST_F(NTPSnippetsFetcherTest, ShouldFetchSuccessfully) {
   SetFakeResponse(/*response_data=*/kJsonStr, net::HTTP_OK,
                   net::URLRequestStatus::SUCCESS);
   EXPECT_CALL(mock_callback(), Run(IsSingleArticle("http://localhost/foobar")));
-  snippets_fetcher().FetchSnippets(test_params());
+  snippets_fetcher().FetchSnippets(test_params(), MakeMockCallback());
   FastForwardUntilNoTasksRemain();
   EXPECT_THAT(snippets_fetcher().last_status(), Eq("OK"));
   EXPECT_THAT(snippets_fetcher().last_json(), Eq(kJsonStr));
@@ -541,7 +552,7 @@ TEST_F(NTPSnippetsContentSuggestionsFetcherTest, ShouldFetchSuccessfully) {
   SetFakeResponse(/*response_data=*/kJsonStr, net::HTTP_OK,
                   net::URLRequestStatus::SUCCESS);
   EXPECT_CALL(mock_callback(), Run(IsSingleArticle("http://localhost/foobar")));
-  snippets_fetcher().FetchSnippets(test_params());
+  snippets_fetcher().FetchSnippets(test_params(), MakeMockCallback());
   FastForwardUntilNoTasksRemain();
   EXPECT_THAT(snippets_fetcher().last_status(), Eq("OK"));
   EXPECT_THAT(snippets_fetcher().last_json(), Eq(kJsonStr));
@@ -562,7 +573,7 @@ TEST_F(NTPSnippetsContentSuggestionsFetcherTest, EmptyCategoryIsOK) {
   SetFakeResponse(/*response_data=*/kJsonStr, net::HTTP_OK,
                   net::URLRequestStatus::SUCCESS);
   EXPECT_CALL(mock_callback(), Run(IsEmptyArticleList()));
-  snippets_fetcher().FetchSnippets(test_params());
+  snippets_fetcher().FetchSnippets(test_params(), MakeMockCallback());
   FastForwardUntilNoTasksRemain();
   EXPECT_THAT(snippets_fetcher().last_status(), Eq("OK"));
   EXPECT_THAT(snippets_fetcher().last_json(), Eq(kJsonStr));
@@ -612,7 +623,7 @@ TEST_F(NTPSnippetsContentSuggestionsFetcherTest, ServerCategories) {
   NTPSnippetsFetcher::OptionalFetchedCategories fetched_categories;
   EXPECT_CALL(mock_callback(), Run(_))
       .WillOnce(WithArg<0>(MovePointeeTo(&fetched_categories)));
-  snippets_fetcher().FetchSnippets(test_params());
+  snippets_fetcher().FetchSnippets(test_params(), MakeMockCallback());
   FastForwardUntilNoTasksRemain();
 
   ASSERT_TRUE(fetched_categories);
@@ -645,12 +656,66 @@ TEST_F(NTPSnippetsContentSuggestionsFetcherTest, ServerCategories) {
                                        /*count=*/1)));
 }
 
+TEST_F(NTPSnippetsContentSuggestionsFetcherTest, ExclusiveCategoryOnly) {
+  const std::string kJsonStr =
+      "{\"categories\" : [{"
+      "  \"id\": 1,"
+      "  \"localizedTitle\": \"Articles for You\","
+      "  \"suggestions\" : [{"
+      "    \"ids\" : [\"http://localhost/foobar\"],"
+      "    \"title\" : \"Foo Barred from Baz\","
+      "    \"snippet\" : \"...\","
+      "    \"fullPageUrl\" : \"http://localhost/foobar\","
+      "    \"creationTime\" : \"2016-06-30T11:01:37.000Z\","
+      "    \"expirationTime\" : \"2016-07-01T11:01:37.000Z\","
+      "    \"attribution\" : \"Foo News\","
+      "    \"imageUrl\" : \"http://localhost/foobar.jpg\","
+      "    \"ampUrl\" : \"http://localhost/amp\","
+      "    \"faviconUrl\" : \"http://localhost/favicon.ico\" "
+      "  }]"
+      "}, {"
+      "  \"id\": 2,"
+      "  \"localizedTitle\": \"Articles for Me\","
+      "  \"suggestions\" : [{"
+      "    \"ids\" : [\"http://localhost/foo2\"],"
+      "    \"title\" : \"Foo Barred from Baz\","
+      "    \"snippet\" : \"...\","
+      "    \"fullPageUrl\" : \"http://localhost/foo2\","
+      "    \"creationTime\" : \"2016-06-30T11:01:37.000Z\","
+      "    \"expirationTime\" : \"2016-07-01T11:01:37.000Z\","
+      "    \"attribution\" : \"Foo News\","
+      "    \"imageUrl\" : \"http://localhost/foo2.jpg\","
+      "    \"ampUrl\" : \"http://localhost/amp\","
+      "    \"faviconUrl\" : \"http://localhost/favicon.ico\" "
+      "  }]"
+      "}]}";
+  SetFakeResponse(/*response_data=*/kJsonStr, net::HTTP_OK,
+                  net::URLRequestStatus::SUCCESS);
+  NTPSnippetsFetcher::OptionalFetchedCategories fetched_categories;
+  EXPECT_CALL(mock_callback(), Run(_))
+      .WillOnce(WithArg<0>(MovePointeeTo(&fetched_categories)));

[Bernhard Bauer, 2016/10/28 16:26:00]

I realize this is just copied from existing code but... this seems like it's
past the point where using GMock actually provides value. The only advantage the
mock seems to provide is that you can verify it's been called, which is a
somewhat contentious pattern anyway (see eg.
http://martinfowler.com/articles/mocksArentStubs.html), and you can verify the
final _state_ by inspecting |fetched_categories| anyway.

[fhorschig, 2016/11/02 05:06:12]

On one hand, it is true that a stub would suffice in most cases. 
On the other hand, the behavior has to be tested at least once: even if there
was no new article fetched, the callback has to return. In this special case
(see test "EmptyCategoryIsOK"), we need a mock.

If it is considered better style, I can define a stub for every case that
doesn't need the explicit verification of the behavior. I would prefer to do
this change in a separate CL as it affects tests that are not explicitly tied to
these changes.

+  NTPSnippetsFetcher::Params params = test_params();
+  params.exclusive_category = OptionalArticlesCategory();
+  snippets_fetcher().FetchSnippets(params, MakeMockCallback());
+  FastForwardUntilNoTasksRemain();
+
+  ASSERT_TRUE(fetched_categories);
+  ASSERT_THAT(fetched_categories->size(), Eq(1u));
+  const auto& fetched_category = (*fetched_categories)[0];
+  EXPECT_TRUE(
+      fetched_category.category.IsKnownCategory(KnownCategories::ARTICLES));
+  const auto& articles = fetched_category.snippets;
+  ASSERT_THAT(articles.size(), Eq(1u));
+  EXPECT_THAT(articles[0]->best_source().url.spec(),
+              Eq("http://localhost/foobar"));
+}
+
 TEST_F(NTPSnippetsFetcherTest, ShouldFetchSuccessfullyEmptyList) {
   const std::string kJsonStr = "{\"recos\": []}";
   SetFakeResponse(/*response_data=*/kJsonStr, net::HTTP_OK,
                   net::URLRequestStatus::SUCCESS);
   EXPECT_CALL(mock_callback(), Run(IsEmptyArticleList()));
-  snippets_fetcher().FetchSnippets(test_params());
+  snippets_fetcher().FetchSnippets(test_params(), MakeMockCallback());
   FastForwardUntilNoTasksRemain();
   EXPECT_THAT(snippets_fetcher().last_status(), Eq("OK"));
   EXPECT_THAT(snippets_fetcher().last_json(), Eq(kJsonStr));
@@ -667,7 +732,7 @@ TEST_F(NTPSnippetsFetcherTest, ShouldRestrictToHosts) {
   NTPSnippetsFetcher::Params params = test_params();
   params.hosts = {"www.somehost1.com", "www.somehost2.com"};
   params.count_to_fetch = 17;
-  snippets_fetcher().FetchSnippets(params);
+  snippets_fetcher().FetchSnippets(params, MakeMockCallback());
   net::TestURLFetcher* fetcher = test_url_fetcher_factory.GetFetcherByID(0);
   ASSERT_THAT(fetcher, NotNull());
   std::unique_ptr<base::Value> value =
@@ -697,7 +762,7 @@ TEST_F(NTPSnippetsFetcherTest, ShouldReportUrlStatusError) {
   SetFakeResponse(/*response_data=*/std::string(), net::HTTP_NOT_FOUND,
                   net::URLRequestStatus::FAILED);
   EXPECT_CALL(mock_callback(), Run(/*snippets=*/Not(HasValue()))).Times(1);
-  snippets_fetcher().FetchSnippets(test_params());
+  snippets_fetcher().FetchSnippets(test_params(), MakeMockCallback());
   FastForwardUntilNoTasksRemain();
   EXPECT_THAT(snippets_fetcher().last_status(),
               Eq("URLRequestStatus error -2"));
@@ -716,7 +781,7 @@ TEST_F(NTPSnippetsFetcherTest, ShouldReportHttpError) {
   SetFakeResponse(/*response_data=*/std::string(), net::HTTP_NOT_FOUND,
                   net::URLRequestStatus::SUCCESS);
   EXPECT_CALL(mock_callback(), Run(/*snippets=*/Not(HasValue()))).Times(1);
-  snippets_fetcher().FetchSnippets(test_params());
+  snippets_fetcher().FetchSnippets(test_params(), MakeMockCallback());
   FastForwardUntilNoTasksRemain();
   EXPECT_THAT(snippets_fetcher().last_json(), IsEmpty());
   EXPECT_THAT(
@@ -734,7 +799,7 @@ TEST_F(NTPSnippetsFetcherTest, ShouldReportJsonError) {
   SetFakeResponse(/*response_data=*/kInvalidJsonStr, net::HTTP_OK,
                   net::URLRequestStatus::SUCCESS);
   EXPECT_CALL(mock_callback(), Run(/*snippets=*/Not(HasValue()))).Times(1);
-  snippets_fetcher().FetchSnippets(test_params());
+  snippets_fetcher().FetchSnippets(test_params(), MakeMockCallback());
   FastForwardUntilNoTasksRemain();
   EXPECT_THAT(snippets_fetcher().last_status(),
               StartsWith("Received invalid JSON (error "));
@@ -754,7 +819,7 @@ TEST_F(NTPSnippetsFetcherTest, ShouldReportJsonErrorForEmptyResponse) {
   SetFakeResponse(/*response_data=*/std::string(), net::HTTP_OK,
                   net::URLRequestStatus::SUCCESS);
   EXPECT_CALL(mock_callback(), Run(/*snippets=*/Not(HasValue()))).Times(1);
-  snippets_fetcher().FetchSnippets(test_params());
+  snippets_fetcher().FetchSnippets(test_params(), MakeMockCallback());
   FastForwardUntilNoTasksRemain();
   EXPECT_THAT(snippets_fetcher().last_json(), std::string());
   EXPECT_THAT(
@@ -771,7 +836,7 @@ TEST_F(NTPSnippetsFetcherTest, ShouldReportInvalidListError) {
   SetFakeResponse(/*response_data=*/kJsonStr, net::HTTP_OK,
                   net::URLRequestStatus::SUCCESS);
   EXPECT_CALL(mock_callback(), Run(/*snippets=*/Not(HasValue()))).Times(1);
-  snippets_fetcher().FetchSnippets(test_params());
+  snippets_fetcher().FetchSnippets(test_params(), MakeMockCallback());
   FastForwardUntilNoTasksRemain();
   EXPECT_THAT(snippets_fetcher().last_json(), Eq(kJsonStr));
   EXPECT_THAT(
@@ -789,7 +854,7 @@ TEST_F(NTPSnippetsFetcherTest, ShouldReportInvalidListError) {
 TEST_F(NTPSnippetsFetcherTest, ShouldReportHttpErrorForMissingBakedResponse) {
   InitFakeURLFetcherFactory();
   EXPECT_CALL(mock_callback(), Run(/*snippets=*/Not(HasValue()))).Times(1);
-  snippets_fetcher().FetchSnippets(test_params());
+  snippets_fetcher().FetchSnippets(test_params(), MakeMockCallback());
   FastForwardUntilNoTasksRemain();
 }
 
@@ -798,10 +863,10 @@ TEST_F(NTPSnippetsFetcherTest, ShouldCancelOngoingFetch) {
   SetFakeResponse(/*response_data=*/kJsonStr, net::HTTP_OK,
                   net::URLRequestStatus::SUCCESS);
   EXPECT_CALL(mock_callback(), Run(IsEmptyArticleList()));
-  snippets_fetcher().FetchSnippets(test_params());
+  snippets_fetcher().FetchSnippets(test_params(), MakeMockCallback());
   // Second call to FetchSnippets() overrides/cancels the previous.
   // Callback is expected to be called once.
-  snippets_fetcher().FetchSnippets(test_params());
+  snippets_fetcher().FetchSnippets(test_params(), MakeMockCallback());
   FastForwardUntilNoTasksRemain();
   EXPECT_THAT(
       histogram_tester().GetAllSamples("NewTabPage.Snippets.FetchResult"),