NTPSnippets: Set MaxRetriesOn5xx only for interactive requests.

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 c02ffa4753d5056f26eddc47663323831c844361..c264940a072f3b4bf9366730de7c35b456e3ed7e 100644
--- a/components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc
+++ b/components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc
@@ -209,6 +209,39 @@ class NTPSnippetsFetcherTest : public testing::Test {
     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>()) {}
@@ -250,6 +283,13 @@ class NTPSnippetsFetcherTest : public testing::Test {
         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,
@@ -280,11 +320,9 @@ class NTPSnippetsFetcherTest : public testing::Test {
         /*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);
@@ -872,17 +910,17 @@ TEST_F(NTPSnippetsFetcherTest, PersonalizesDependingOnVariations) {
   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));
@@ -907,18 +945,21 @@ TEST_F(NTPSnippetsFetcherTest, ShouldFetchSuccessfullyEmptyList) {
 }
 
 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;
@@ -936,16 +977,43 @@ TEST_F(NTPSnippetsFetcherTest, ShouldRestrictToHosts) {
   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) {