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 <map>
 #include <utility>
 
 #include "base/json/json_reader.h"
@@ skipping 191 matching lines @@
   class RequestBuilderWithMockedFlagsForTesting
       : public NTPSnippetsFetcher::RequestBuilder {
    public:
     RequestBuilderWithMockedFlagsForTesting() : RequestBuilder() {}
 
    private:
     bool IsSendingUserClassEnabled() const override { return true; }
     bool IsSendingTopLanguagesEnabled() const override { return true; }
   };
 
+  class ScopedInjectedTestURLFetcherFactory {

[tschumann, 2016/12/08 13:27:53]

now that I better understand the usage pattern, maybe we can find a shorter
name.
ScopedTestURLFetcherFactory?

Also please:
-- move it out of the test class's scope. There's no need for that coupling.
-- add a class comment explaining it's use case, e.g.:
// <class-name> can be used to temporarily inject fake url fetcher instances.
Client code can access created fetchers to verify expected properties. When the
factory gets destroyed, all initiated fetches will be completed. 

[fhorschig, 2016/12/09 12:37:55]

Moved.
Commented.
It is now a Factory itself and implements the "ScopedURLFetcherFactory"
interface.
Therefore, we can focus on what it actually does: Ensuring the callback is
called.
This is most important and everything else can be done by the existing class.

+   public:
+    ScopedInjectedTestURLFetcherFactory() {}
+    ~ScopedInjectedTestURLFetcherFactory() {
+      net::TestURLFetcher* fetcher = GetLastCreatedUrlFetcher();

[tschumann, 2016/12/08 13:27:53]

either clearly document that we only support creating a single fetcher (in which
case GetLastCreatedFetcher should check that there's only a single one
existing), or support multiple fetchers at destruction -- just iterating over
all of them (no need to provide more accessors though as no test needs them).

[fhorschig, 2016/12/09 12:37:55]

We support now creation of multiple fetchers [1], but access to the last.
|GetLastCreatedFetcher| cannot check for multiple instances [2]. 
Therefore this class _is_ now a factory (which makes sense) that keeps track of
created fetchers which still need their delegates called.

[1] There is a delegate for testing purposes which allows calling the delegate
during destruction of the fetcher.
[2] No access to the map of fetchers and even if: by default new fetchers
receive the ID 0.

+      if (fetcher) {
+        // An 4XX response needs the least configuration to successfully invoke
+        // the  callback properly.
+        fetcher->set_response_code(net::HTTP_NOT_FOUND);
+        fetcher->set_status(net::URLRequestStatus(net::URLRequestStatus::FAILED,
+                                                  net::ERR_FAILED));
+        fetcher->delegate()->OnURLFetchComplete(fetcher);
+        test_url_fetcher_factory_.RemoveFetcherFromMap(0);
+      }
+    }
+
+    net::TestURLFetcher* GetLastCreatedUrlFetcher() const {
+      // If there is a fetcher, the first fetcher is always at 0.
+      // If there is no fetcher, the resulting pointer will be nulltpr, no
+      // matter
+      // which index was used.

[Marc Treib, 2016/12/08 10:52:06]

nit: some extra newlines here

[fhorschig, 2016/12/09 12:37:55]

Done.

+      // If you try to access another fetcher, the old fetcher must be destroyed
+      // manuall first (by calling |CloseLastUrlFetcher|) in order to secure

[Marc Treib, 2016/12/08 10:52:06]

s/manuall/manually/

[fhorschig, 2016/12/09 12:37:55]

Done.

+      // that
+      // the delegate was called. This wouldn't happen by default and would leak
+      // a once-bound SnippetsAvailableCallback.
+      return test_url_fetcher_factory_.GetFetcherByID(0);
+    }
+
+   private:
+    net::TestURLFetcherFactory test_url_fetcher_factory_;
+  };
+
   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 21 matching lines @@
         fake_signin_manager_.get(), fake_token_service_.get(),
         scoped_refptr<net::TestURLRequestContextGetter>(
             new net::TestURLRequestContextGetter(mock_task_runner_.get())),
         pref_service_.get(), &category_factory_, nullptr,
         base::Bind(&ParseJsonDelayed), kAPIKey, user_classifier_.get());
 
     snippets_fetcher_->SetTickClockForTesting(
         mock_task_runner_->GetMockTickClock());
   }
 
+  NTPSnippetsFetcher::SnippetsAvailableCallback
+  CreateSnippetsAvailableCallback() {
+    // Make sure the callback gets executed.
+    EXPECT_CALL(mock_callback(), Run(/*snippets=*/_)).Times(1);
+    return ToSnippetsAvailableCallback(&mock_callback());

[Marc Treib, 2016/12/08 10:52:06]

Hm, how about inlining this into the tests themselves? I think it's nicer to
have the expectations in the test directly.

[tschumann, 2016/12/08 13:27:53]

+1

[fhorschig, 2016/12/09 12:37:55]

Removed. As they cannot happen anymore with the delegate-calling factory.
This expectation was pretty much independent from the test itself and
just serves to catches memory leaks without usage of local ASan builds.

+  }
+
   NTPSnippetsFetcher::SnippetsAvailableCallback ToSnippetsAvailableCallback(
       MockSnippetsAvailableCallback* callback) {
     return base::BindOnce(&MockSnippetsAvailableCallback::WrappedRun,
                           base::Unretained(callback));
   }
 
   NTPSnippetsFetcher& snippets_fetcher() { return *snippets_fetcher_; }
   MockSnippetsAvailableCallback& mock_callback() { return mock_callback_; }
   void FastForwardUntilNoTasksRemain() {
     mock_task_runner_->FastForwardUntilNoTasksRemain();
@@ skipping 10 matching lines @@
   void InitFakeURLFetcherFactory() {
     if (fake_url_fetcher_factory_) {
       return;
     }
     // Instantiation of factory automatically sets itself as URLFetcher's
     // factory.
     fake_url_fetcher_factory_.reset(new net::FakeURLFetcherFactory(
         /*default_factory=*/&failing_url_fetcher_factory_));
   }
 
-  void SetFetchingPersonalizationVariation(
-      const std::string& personalization_string) {
+  void SetVariationParameters(
+      const std::map<std::string, std::string>& params) {
     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 SetFakeResponse(const std::string& response_data,
                        net::HttpStatusCode response_code,
                        net::URLRequestStatus::Status status) {
     InitFakeURLFetcherFactory();
     fake_url_fetcher_factory_->SetFakeResponse(test_url_, response_data,
@@ skipping 567 matching lines @@
               Eq(CategoryFactory().FromRemoteCategory(2).id()));
   ASSERT_THAT(category.snippets.size(), Eq(1u));
   EXPECT_THAT(category.snippets[0]->url().spec(), Eq("http://localhost/foo2"));
 }
 
 TEST_F(NTPSnippetsFetcherTest, PersonalizesDependingOnVariations) {
   // Default setting should be both personalization options.
   EXPECT_THAT(snippets_fetcher().personalization(),
               Eq(NTPSnippetsFetcher::Personalization::kBoth));
 
-  SetFetchingPersonalizationVariation("personal");
+  SetVariationParameters({{"fetching_personalization", "personal"}});
   ResetSnippetsFetcher();
   EXPECT_THAT(snippets_fetcher().personalization(),
               Eq(NTPSnippetsFetcher::Personalization::kPersonal));
 
-  SetFetchingPersonalizationVariation("non_personal");
+  SetVariationParameters({{"fetching_personalization", "non_personal"}});
   ResetSnippetsFetcher();
   EXPECT_THAT(snippets_fetcher().personalization(),
               Eq(NTPSnippetsFetcher::Personalization::kNonPersonal));
 
-  SetFetchingPersonalizationVariation("both");
+  SetVariationParameters({{"fetching_personalization", "both"}});
   ResetSnippetsFetcher();
   EXPECT_THAT(snippets_fetcher().personalization(),
               Eq(NTPSnippetsFetcher::Personalization::kBoth));
 }
 
 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(), ToSnippetsAvailableCallback(&mock_callback()));
   FastForwardUntilNoTasksRemain();
   EXPECT_THAT(snippets_fetcher().last_status(), Eq("OK"));
   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;
   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);
-  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());
+  std::unique_ptr<base::Value> value;
+  {
+    ScopedInjectedTestURLFetcherFactory url_fetcher_factory;
+    snippets_fetcher().FetchSnippets(params, CreateSnippetsAvailableCallback());
+
+    net::TestURLFetcher* fetcher =
+        url_fetcher_factory.GetLastCreatedUrlFetcher();
+    ASSERT_THAT(fetcher, NotNull());
+    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(/*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) {
+  NTPSnippetsFetcher::Params params = test_params();
+  params.interactive_request = true;
+  {
+    ScopedInjectedTestURLFetcherFactory url_fetcher_factory;
+    snippets_fetcher().FetchSnippets(params, CreateSnippetsAvailableCallback());
+
+    net::TestURLFetcher* fetcher =
+        url_fetcher_factory.GetLastCreatedUrlFetcher();
+    ASSERT_THAT(fetcher, NotNull());
+    EXPECT_THAT(fetcher->GetMaxRetriesOn5xx(), Eq(2));
+  }
+}
+
+TEST_F(NTPSnippetsFetcherTest, RetriesConfigurableOnNonInteractiveRequests) {
+  NTPSnippetsFetcher::Params params = test_params();
+  params.interactive_request = false;
+  std::map<std::string, int> expected_retries_for_configurations = {

[Marc Treib, 2016/12/08 10:52:06]

Make this const?
I'd also slightly prefer an array of structs, since you don't actually need the
map functionality. Also the values will have better names thant "first" and
"second" :)
It can be initialized in almost exactly the same way.

[tschumann, 2016/12/08 13:27:53]

Using a struct would be better indeed -- you seem to abusing the keys here (see
the example of the empty string key).
I'd vote for a vector<> instead of an array ;-)

[Marc Treib, 2016/12/08 13:31:56]

Right, vector also works, now that we have std::initializer_list. Doesn't really
make a difference IMO, since it'll only be used in a single range-based for loop
below, which works the same in either case.

[fhorschig, 2016/12/09 12:37:55]

Done.

+      {"", 0},  // Keep 2 retries if no variation is set.
+      {"0", 0},
+      {"-1", 0},  // Allow no negative retry count.
+      {"3", 3}};
+
+  for (auto retry_configuration : expected_retries_for_configurations) {

[Marc Treib, 2016/12/08 10:52:06]

const auto&
?

[fhorschig, 2016/12/09 12:37:55]

Done.

+    ScopedInjectedTestURLFetcherFactory url_fetcher_factory;
+    SetVariationParameters(
+        {{"number_of_non_interactive_5xx_retries", retry_configuration.first}});
+
+    snippets_fetcher().FetchSnippets(params, CreateSnippetsAvailableCallback());
+
+    net::TestURLFetcher* fetcher =
+        url_fetcher_factory.GetLastCreatedUrlFetcher();
+    ASSERT_THAT(fetcher, NotNull());
+    EXPECT_THAT(fetcher->GetMaxRetriesOn5xx(), Eq(retry_configuration.second));
+  }
 }
 
 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(), ToSnippetsAvailableCallback(&mock_callback()));
   FastForwardUntilNoTasksRemain();
   EXPECT_THAT(snippets_fetcher().last_status(),
@@ skipping 129 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