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..6824f9a2679644106a55e8815ae4af73e16d89bc 100644
--- a/components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc
+++ b/components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc
@@ -4,6 +4,7 @@
 
 #include "components/ntp_snippets/remote/ntp_snippets_fetcher.h"
 
+#include <list>
 #include <map>
 #include <utility>
 
@@ -16,6 +17,7 @@
 #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"
@@ -158,6 +160,81 @@ class MockSnippetsAvailableCallback {
       void(NTPSnippetsFetcher::OptionalFetchedCategories* fetched_categories));
 };
 
+// DelegateCallingTestURLFetcherFactory can be used to temporarily inject fake
+// url fetcher instances into a scope.

[Marc Treib, 2016/12/09 12:59:24]

s/fake url fetcher/TestURLFetcher/ ?
Since both TestURLFetcher and FakeURLFetcher exist, we shouldn't mix the terms.
(I can never remember which is which anyway...)

[fhorschig, 2016/12/12 09:54:03]

Done.

+// 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

[tschumann, 2016/12/09 12:53:38]

hmm... I wonder why the previous (much simpler version) didn't work. Was there
anything missing in particular?
Might be nice to add a comment why we need to inherit from the factory instead
of just using it.
(did I mention that inheritance is something we should only do if we have to?
;-)).

[fhorschig, 2016/12/12 09:54:03]

Yes, you mentioned that. I replaced that multiple times with composition but 
this is the best thing I ended up with.

What I could do is inheriting from the base factory interface [1, 2] and make 
this class a facade that uses another factory internally [3].
But this way less readable and from a logic perspective: this class really IS a
TestURLFetcher.

[1] I need to control the point where fetchers are created.
[2] I needed to implement other interfaces like ScopedURLFetcherFactory, too.
    ... just to reach the same functionality again.
[3] It's doing the same thing and this would be a major code duplication
otherwise.

[tschumann, 2016/12/12 10:00:42]

to be more precise, what's the new bit of functionality that requires us from
inheriting from TestURLFetcherFactory?
My concern is that your code now has to deal with a lot of factory
implementation details which we didn't have to do before (dealing with
delegates, creating the fetchers).
And in the end, i'm not sure it's truely a factory -- the creation is still
forwarded to the factory we inherit from. And the test doesn't want to deal with
a factory -- we just want to inject our fetchers somehow.

I'm not saying we can't do it like you propose, but I'd like to understand why
we need to deal with all the details (which have a good potential for bugs) ;-)

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

It's true that we deal with a lot of implementation details. The reason here is,
that we never tested for the correct configuration of the fetcher before.
Sadly, this is nothing, we could see just by interacting with the life of a
request.

And I also agree that it's tricky to see the real issue here.
The tests don't want to deal with a factory, but they have to use one for
injecting
the fetchers. These fetchers all have the same ID. We need to identify fetchers
by
ID to call a callback. Calling every callback is probably the only things that
tests want to deal less with: 
1. It's not connected to the tests themselves.
2. Unrelated to the real use case, where you don't have to know when this
happens.
3. If you forget to do so (which is more than likely), the test will crash
   everything ASan-related. 

Automatically callback calling is complicated as it must not happen multiple
times.
Therefore, we have to track the calls. This is only possible we know of a
fetcher's
life time.

+    : public net::TestURLFetcherFactory,
+      public net::TestURLFetcherDelegateForTests {
+ public:
+  DelegateCallingTestURLFetcherFactory() {
+    SetDelegateForTests(this);
+    set_remove_fetcher_on_delete(true);
+  }
+
+  ~DelegateCallingTestURLFetcherFactory() override {
+    while (!fetcher_id_queue_.empty()) {
+      DropAndCallDelegate(fetcher_id_queue_.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(ERROR) << "The ID " << id << " was already assigned to a fetcher."

[Marc Treib, 2016/12/09 12:59:24]

Is this actually an error? If so, should we just fail the test?

[fhorschig, 2016/12/12 09:54:03]

It's a warning now.

I thought about just failing but if you don't care about the timing
of a callback that much or if it's empty, it's just an unnecessary
restriction. Warning is probably more appropriate, because it might
still work out.

+                 << "Its delegate will thereforde be called right now.";
+      DropAndCallDelegate(id);
+    }
+    fetcher_id_queue_.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 (fetcher_id_queue_.empty()) {
+      return nullptr;
+    }
+    return GetFetcherByID(fetcher_id_queue_.front());

[tschumann, 2016/12/09 12:53:38]

shouldn't this be back?

[fhorschig, 2016/12/12 09:54:03]

What came in first should be handled first (--> queue ;D).
I used push_back for adding elements, so the newest is at front().

+  }
+
+ private:
+  std::list<int> fetcher_id_queue_;  // Use a list as searching is essential.

[tschumann, 2016/12/09 12:53:38]

drop the type in the variable name (it's outdated already ;-)). Just call it
fetchers_.

[Marc Treib, 2016/12/09 12:59:24]

You mean, this can't be std::queue because that doesn't support iterating? Then
I'd say that, because the message is really "don't use queue" more than "use
list".
std::deque would also work btw :)

nit: data members usually come after methods

[fhorschig, 2016/12/12 09:54:03]

@tschumann: Done.
@treib: std::deque it is. And done.

+
+  // 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(fetcher_id_queue_.begin(),
+                                   fetcher_id_queue_.end(), fetcher_id);
+    if (found_id_iter == fetcher_id_queue_.end()) {
+      return;
+    }
+    fetcher_id_queue_.erase(found_id_iter);
+    net::TestURLFetcher* fetcher = GetFetcherByID(fetcher_id);
+    if (!fetcher) {
+      return;
+    }
+    if (!fetcher->delegate()) {

[Marc Treib, 2016/12/09 12:59:24]

nit: Could merge the two ifs:
if (!fetcher || !fetcher->delegate())

[fhorschig, 2016/12/12 09:54:03]

Okay.

+      return;
+    }
+    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);
+  }
+};
+
 // Factory for FakeURLFetcher objects that always generate errors.
 class FailingFakeURLFetcherFactory : public net::URLFetcherFactory {
  public:
@@ -209,6 +286,11 @@ class NTPSnippetsFetcherTest : public testing::Test {
     bool IsSendingTopLanguagesEnabled() const override { return true; }
   };
 
+  struct ExpectationForVariationParam {
+    std::string param_value;
+    int expected_value;
+  };
+
   NTPSnippetsFetcherTest()
       : NTPSnippetsFetcherTest(GURL(kTestChromeReaderUrl),
                                std::map<std::string, std::string>()) {}
@@ -290,6 +372,15 @@ class NTPSnippetsFetcherTest : public testing::Test {
             ntp_snippets::kStudyName, params);
   }
 
+  void SetVariationParametersForFeatures(

[Marc Treib, 2016/12/09 12:59:24]

Hm, now SetFetchingPersonalizationVariation and
SetVariationParametersForFeatures conflict (as in, you can't call both). Could
we get rid of SetFetchingPersonalizationVariation? Looks like it provides a
subset of SetVariationParametersForFeatures's functionality.

[fhorschig, 2016/12/12 09:54:03]

The basic difference is that one is tied to a feature. But in CL 2552813005
|SetVariationParametersForFeatures| will disappear for the reason you mentioned.

+      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) {
@@ -907,13 +998,15 @@ TEST_F(NTPSnippetsFetcherTest, ShouldFetchSuccessfullyEmptyList) {
 }
 
 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());
@@ -936,16 +1029,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) {
+  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.
+      {"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) {