NTPSnippets: Set MaxRetriesOn5xx only for interactive requests.

components/ntp_snippets/remote/ntp_snippets_fetcher_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/remote/ntp_snippets_fetcher.h"
 
+#include <deque>
 #include <map>
 #include <utility>
 
 #include "base/json/json_reader.h"
 #include "base/memory/ptr_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/test/histogram_tester.h"
 #include "base/test/test_mock_time_task_runner.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/time/time.h"
 #include "base/values.h"
 #include "components/ntp_snippets/category_factory.h"
+#include "components/ntp_snippets/features.h"
 #include "components/ntp_snippets/ntp_snippets_constants.h"
 #include "components/ntp_snippets/remote/ntp_snippet.h"
 #include "components/ntp_snippets/user_classifier.h"
 #include "components/prefs/testing_pref_service.h"
 #include "components/signin/core/browser/account_tracker_service.h"
 #include "components/signin/core/browser/fake_profile_oauth2_token_service.h"
 #include "components/signin/core/browser/fake_signin_manager.h"
 #include "components/signin/core/browser/test_signin_client.h"
 #include "components/variations/entropy_provider.h"
 #include "components/variations/variations_params_manager.h"
@@ skipping 124 matching lines @@
       NTPSnippetsFetcher::OptionalFetchedCategories fetched_categories) {
     Run(fetch_result, &fetched_categories);
   }
 
   MOCK_METHOD2(
       Run,
       void(NTPSnippetsFetcher::FetchResult fetch_result,
            NTPSnippetsFetcher::OptionalFetchedCategories* fetched_categories));
 };
 
+// DelegateCallingTestURLFetcherFactory can be used to temporarily inject
+// TestURLFetcher instances into a scope.
+// Client code can access the last created fetcher to verify expected
+// properties. When the factory gets destroyed, all available delegates of still
+// valid fetchers will be called.
+// This ensures once-bound callbacks (like SnippetsAvailableCallback) will be
+// called at some point and are not leaked.
+class DelegateCallingTestURLFetcherFactory
+    : public net::TestURLFetcherFactory,
+      public net::TestURLFetcherDelegateForTests {
+ public:
+  DelegateCallingTestURLFetcherFactory() {
+    SetDelegateForTests(this);
+    set_remove_fetcher_on_delete(true);
+  }
+
+  ~DelegateCallingTestURLFetcherFactory() override {
+    while (!fetchers_.empty()) {
+      DropAndCallDelegate(fetchers_.front());
+    }
+  }
+
+  std::unique_ptr<net::URLFetcher> CreateURLFetcher(
+      int id,
+      const GURL& url,
+      net::URLFetcher::RequestType request_type,
+      net::URLFetcherDelegate* d) override {
+    if (GetFetcherByID(id)) {
+      LOG(WARNING) << "The ID " << id << " was already assigned to a fetcher."
+                   << "Its delegate will thereforde be called right now.";
+      DropAndCallDelegate(id);
+    }
+    fetchers_.push_back(id);
+    return TestURLFetcherFactory::CreateURLFetcher(id, url, request_type, d);
+  }
+
+  // Returns the raw pointer of the last created URL fetcher.
+  // If it was destroyed or no fetcher was created, it will return a nulltpr.
+  net::TestURLFetcher* GetLastCreatedFetcher() {
+    if (fetchers_.empty()) {
+      return nullptr;
+    }
+    return GetFetcherByID(fetchers_.front());
+  }
+
+ private:
+  // The fetcher can either be destroyed because the delegate was called during
+  // execution or because we called it on destruction.
+  void DropAndCallDelegate(int fetcher_id) {
+    auto found_id_iter =
+        std::find(fetchers_.begin(), fetchers_.end(), fetcher_id);
+    if (found_id_iter == fetchers_.end()) {
+      return;
+    }
+    fetchers_.erase(found_id_iter);
+    net::TestURLFetcher* fetcher = GetFetcherByID(fetcher_id);
+    if (!fetcher || !fetcher->delegate()) {
+      return;

[Marc Treib, 2016/12/12 10:17:16]

When would |fetcher| be null here? Add a comment to explain?

[fhorschig, 2016/12/12 11:05:10]

Removed. It cannot be 0. If it is, crashing is an appropriate action.

+    }
+    fetcher->delegate()->OnURLFetchComplete(fetcher);
+  }
+
+  // net::TestURLFetcherDelegateForTests overrides:
+  void OnRequestStart(int fetcher_id) override {}
+  void OnChunkUpload(int fetcher_id) override {}
+  void OnRequestEnd(int fetcher_id) override {
+    DropAndCallDelegate(fetcher_id);
+  }
+
+  std::deque<int> fetchers_;  // std::queue doesn't support std::find.
+};
+
 // Factory for FakeURLFetcher objects that always generate errors.
 class FailingFakeURLFetcherFactory : public net::URLFetcherFactory {
  public:
   std::unique_ptr<net::URLFetcher> CreateURLFetcher(
       int id, const GURL& url, net::URLFetcher::RequestType request_type,
       net::URLFetcherDelegate* d) override {
     return base::MakeUnique<net::FakeURLFetcher>(
         url, d, /*response_data=*/std::string(), net::HTTP_NOT_FOUND,
         net::URLRequestStatus::FAILED);
   }
@@ skipping 31 matching lines @@
   class RequestBuilderWithMockedFlagsForTesting
       : public NTPSnippetsFetcher::RequestBuilder {
    public:
     RequestBuilderWithMockedFlagsForTesting() : RequestBuilder() {}
 
    private:
     bool IsSendingUserClassEnabled() const override { return true; }
     bool IsSendingTopLanguagesEnabled() const override { return true; }
   };
 
+  struct ExpectationForVariationParam {

[Marc Treib, 2016/12/12 10:17:16]

nit: Move this into the test that uses it.

[fhorschig, 2016/12/12 11:05:10]

Done.

+    std::string param_value;
+    int expected_value;
+  };
+
   NTPSnippetsFetcherTest()
       : NTPSnippetsFetcherTest(GURL(kTestChromeReaderUrl),
                                std::map<std::string, std::string>()) {}
 
   NTPSnippetsFetcherTest(const GURL& gurl,
                          const std::map<std::string, std::string>& params)
       : params_manager_(
             base::MakeUnique<variations::testing::VariationParamsManager>(
                 ntp_snippets::kStudyName,
                 params)),
@@ skipping 61 matching lines @@
   void SetFetchingPersonalizationVariation(
       const std::string& personalization_string) {
     params_manager_.reset();
     std::map<std::string, std::string> params = {
         {"fetching_personalization", personalization_string}};
     params_manager_ =
         base::MakeUnique<variations::testing::VariationParamsManager>(
             ntp_snippets::kStudyName, params);
   }
 
+  void SetVariationParametersForFeatures(
+      const std::map<std::string, std::string>& params,
+      const std::set<std::string>& features) {
+    params_manager_.reset();
+    params_manager_ =
+        base::MakeUnique<variations::testing::VariationParamsManager>(
+            ntp_snippets::kStudyName, params, features);
+  }
+
   void SetFakeResponse(const std::string& response_data,
                        net::HttpStatusCode response_code,
                        net::URLRequestStatus::Status status) {
     InitFakeURLFetcherFactory();
     fake_url_fetcher_factory_->SetFakeResponse(test_url_, response_data,
                                                response_code, status);
   }
 
   std::unique_ptr<translate::LanguageModel> MakeLanguageModel(
       const std::vector<std::string>& codes) {
@@ skipping 600 matching lines @@
   EXPECT_THAT(snippets_fetcher().last_json(), Eq(kJsonStr));
   EXPECT_THAT(
       histogram_tester().GetAllSamples("NewTabPage.Snippets.FetchResult"),
       ElementsAre(base::Bucket(/*min=*/0, /*count=*/1)));
   EXPECT_THAT(histogram_tester().GetAllSamples(
                   "NewTabPage.Snippets.FetchHttpResponseOrErrorCode"),
               ElementsAre(base::Bucket(/*min=*/200, /*count=*/1)));
 }
 
 TEST_F(NTPSnippetsFetcherTest, ShouldRestrictToHosts) {
-  net::TestURLFetcherFactory test_url_fetcher_factory;
+  DelegateCallingTestURLFetcherFactory fetcher_factory;
   NTPSnippetsFetcher::Params params = test_params();
   params.hosts = {"www.somehost1.com", "www.somehost2.com"};
   params.count_to_fetch = 17;
+
   snippets_fetcher().FetchSnippets(
       params, ToSnippetsAvailableCallback(&mock_callback()));
-  net::TestURLFetcher* fetcher = test_url_fetcher_factory.GetFetcherByID(0);
+
+  net::TestURLFetcher* fetcher = fetcher_factory.GetLastCreatedFetcher();
   ASSERT_THAT(fetcher, NotNull());
   std::unique_ptr<base::Value> value =
       base::JSONReader::Read(fetcher->upload_data());
   ASSERT_TRUE(value) << " failed to parse JSON: "
                      << PrintToString(fetcher->upload_data());
   const base::DictionaryValue* dict = nullptr;
   ASSERT_TRUE(value->GetAsDictionary(&dict));
   const base::DictionaryValue* local_scoring_params = nullptr;
   ASSERT_TRUE(dict->GetDictionary("advanced_options.local_scoring_params",
                                   &local_scoring_params));
   const base::ListValue* content_selectors = nullptr;
   ASSERT_TRUE(
       local_scoring_params->GetList("content_selectors", &content_selectors));
   ASSERT_THAT(content_selectors->GetSize(), Eq(static_cast<size_t>(2)));
   const base::DictionaryValue* content_selector = nullptr;
   ASSERT_TRUE(content_selectors->GetDictionary(0, &content_selector));
   std::string content_selector_value;
   EXPECT_TRUE(content_selector->GetString("value", &content_selector_value));
   EXPECT_THAT(content_selector_value, Eq("www.somehost1.com"));
   ASSERT_TRUE(content_selectors->GetDictionary(1, &content_selector));
   EXPECT_TRUE(content_selector->GetString("value", &content_selector_value));
   EXPECT_THAT(content_selector_value, Eq("www.somehost2.com"));
-  // Call the delegate callback manually as the TestURLFetcher deletes any
-  // call to the delegate that usually happens on |Start|.
-  // Without the call to the delegate, it leaks the request that owns itself.
-  ASSERT_THAT(fetcher->delegate(), NotNull());
-  EXPECT_CALL(mock_callback(),
-              Run(NTPSnippetsFetcher::FetchResult::URL_REQUEST_STATUS_ERROR,
-                  /*snippets=*/Not(HasValue())))
-      .Times(1);
-  // An 4XX response needs the least configuration to successfully invoke the
-  // callback properly as the results are not important in this test.
-  fetcher->set_response_code(net::HTTP_NOT_FOUND);
-  fetcher->set_status(net::URLRequestStatus(net::URLRequestStatus::FAILED, -2));
-  fetcher->delegate()->OnURLFetchComplete(fetcher);
+}
+
+TEST_F(NTPSnippetsFetcherTest, RetryOnInteractiveRequests) {
+  DelegateCallingTestURLFetcherFactory fetcher_factory;
+  NTPSnippetsFetcher::Params params = test_params();
+  params.interactive_request = true;
+
+  snippets_fetcher().FetchSnippets(
+      params, ToSnippetsAvailableCallback(&mock_callback()));
+
+  net::TestURLFetcher* fetcher = fetcher_factory.GetLastCreatedFetcher();
+  ASSERT_THAT(fetcher, NotNull());
+  EXPECT_THAT(fetcher->GetMaxRetriesOn5xx(), Eq(2));
+}
+
+TEST_F(NTPSnippetsFetcherTest, RetriesConfigurableOnNonInteractiveRequests) {
+  NTPSnippetsFetcher::Params params = test_params();
+  params.interactive_request = false;
+  const std::vector<ExpectationForVariationParam> retry_config_expectation = {
+      {"", 0},  // Keep 2 retries if no variation is set.

[Marc Treib, 2016/12/12 10:17:16]

Comment is out of date.
One alternative is adding a "description" string to each test case. Then when
one of them fails, we can print it, so it's clear what exactly failed.

[fhorschig, 2016/12/12 11:05:10]

Good idea.

[tschumann, 2016/12/12 11:55:48]

This is fine by me, but we're getting close into the realms of discouraged
data-driven tests.
https://big.corp.google.com/~jmcmaster/testing/2008/07/episode-97-blast-from-...
is an interesting read.

[fhorschig, 2016/12/12 12:16:45]

Looks like a valid concern even though this very episode was very
controversially discussed: http://g/unittest-discuss/7bi0kIjCgak

I added it to go/zine-testing and the agenda of go/zine-testing-notes. 
We should discuss where we draw the line. For this particular test,
I see no huge problem, especially with the descriptions in place...

+      {"0", 0},
+      {"-1", 0},  // Allow no negative retry count.
+      {"3", 3}};
+
+  for (const auto& retry_config : retry_config_expectation) {
+    DelegateCallingTestURLFetcherFactory fetcher_factory;
+    SetVariationParametersForFeatures(
+        {{"background_5xx_retries_count", retry_config.param_value}},
+        {ntp_snippets::kArticleSuggestionsFeature.name});
+
+    snippets_fetcher().FetchSnippets(
+        params, ToSnippetsAvailableCallback(&mock_callback()));
+
+    net::TestURLFetcher* fetcher = fetcher_factory.GetLastCreatedFetcher();
+    ASSERT_THAT(fetcher, NotNull());
+    EXPECT_THAT(fetcher->GetMaxRetriesOn5xx(), Eq(retry_config.expected_value));
+  }
 }
 
 TEST_F(NTPSnippetsFetcherTest, ShouldReportUrlStatusError) {
   SetFakeResponse(/*response_data=*/std::string(), net::HTTP_NOT_FOUND,
                   net::URLRequestStatus::FAILED);
   EXPECT_CALL(mock_callback(),
               Run(NTPSnippetsFetcher::FetchResult::URL_REQUEST_STATUS_ERROR,
                   /*snippets=*/Not(HasValue())))
       .Times(1);
   snippets_fetcher().FetchSnippets(
@@ skipping 149 matching lines @@
     const NTPSnippetsFetcher::OptionalFetchedCategories& fetched_categories) {
   if (fetched_categories) {
     // Matchers above aren't any more precise than this, so this is sufficient
     // for test-failure diagnostics.
     return os << "list with " << fetched_categories->size() << " elements";
   }
   return os << "null";
 }
 
 }  // namespace ntp_snippets