NTPSnippets: Set MaxRetriesOn5xx only for interactive requests.

components/ntp_snippets/remote/ntp_snippets_fetcher.h

 // 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.
 
 #ifndef COMPONENTS_NTP_SNIPPETS_REMOTE_NTP_SNIPPETS_FETCHER_H_
 #define COMPONENTS_NTP_SNIPPETS_REMOTE_NTP_SNIPPETS_FETCHER_H_
 
 #include <memory>
 #include <queue>
 #include <set>
@@ skipping 164 matching lines @@
   FRIEND_TEST_ALL_PREFIXES(NTPSnippetsFetcherTest, BuildRequestAuthenticated);
   FRIEND_TEST_ALL_PREFIXES(NTPSnippetsFetcherTest, BuildRequestUnauthenticated);
   FRIEND_TEST_ALL_PREFIXES(NTPSnippetsFetcherTest, BuildRequestExcludedIds);
   FRIEND_TEST_ALL_PREFIXES(NTPSnippetsFetcherTest, BuildRequestNoUserClass);
   FRIEND_TEST_ALL_PREFIXES(NTPSnippetsFetcherTest,
                            BuildRequestWithTwoLanguages);
   FRIEND_TEST_ALL_PREFIXES(NTPSnippetsFetcherTest,
                            BuildRequestWithUILanguageOnly);
   FRIEND_TEST_ALL_PREFIXES(NTPSnippetsFetcherTest,
                            BuildRequestWithOtherLanguageOnly);
+  FRIEND_TEST_ALL_PREFIXES(NTPSnippetsFetcherTest,
+                           BuildRequestFetcherDependingOnInteractivity);
   friend class NTPSnippetsFetcherTest;
 
   enum FetchAPI {
     CHROME_READER_API,
     CHROME_CONTENT_SUGGESTIONS_API,
   };
 
   class JsonRequest;
 
   // A class that builds authenticated and non-authenticated JsonRequests.
@@ skipping 21 matching lines @@
     RequestBuilder& SetPersonalization(Personalization personalization);
     // The tick_clock borrowed from the fetcher will be injected into the
     // request. It will be used at build time and after the fetch returned.
     // It has to be alive until the request is destroyed.
     RequestBuilder& SetTickClock(base::TickClock* tick_clock);
     RequestBuilder& SetUrl(const GURL& url);
     RequestBuilder& SetUrlRequestContextGetter(
         const scoped_refptr<net::URLRequestContextGetter>& context_getter);
     RequestBuilder& SetUserClassifier(const UserClassifier& user_classifier);
 
-    std::string PreviewRequestBodyForTesting() { return BuildBody(); }
-    std::string PreviewRequestHeadersForTesting() { return BuildHeaders(); }
+    std::string PreviewRequestBodyForTesting() const { return BuildBody(); }

[tschumann, 2016/12/05 17:26:33]

hmm... this has a smell and doesn't seem to scale well.

Usually, a test should use the public interface and observe the expected
behavior or output.

In this case it's tricky as the built request object wont' have the necessary
information. What you want to verify is an interaction with the external world
(through the URL fetcher).

Can't we inject a fake or mock fetcher in the test? One like this
https://codesearch.chromium.org/chromium/src/net/url_request/test_url_fetcher...
via
https://codesearch.chromium.org/chromium/src/net/url_request/url_fetcher_impl...
?

[fhorschig, 2016/12/06 14:31:40]

Reverted changes made here and added a TODO. In order to inject the URLFetcher
for a specific builder, we would need to build and start the request or expose
the request. Both alternatives don't improve the code in any way. As soon as the
Builder and JsonRequest have moved to snippets::internal, these functions are
completely unnecessary.

+    std::string PreviewRequestHeadersForTesting() const {
+      return BuildHeaders();
+    }
+    // Creates a valid net::URLFetcher that has no request and can therefore not
+    // fetch and trigger a delegate. Body and Header are empty.
+    std::unique_ptr<net::URLFetcher> PreviewURLFetcherForTesting() const;
     RequestBuilder& SetUserClassForTesting(const std::string& user_class) {
       user_class_ = user_class;
       return *this;
     }
 
    protected:
     // TODO(fhorschig): As soon as crbug.com/crbug.com/645447 is resolved,
     // make these an implementation detail in the body and remove the mock.
     virtual bool IsSendingTopLanguagesEnabled() const;
     virtual bool IsSendingUserClassEnabled() const;
@@ skipping 119 matching lines @@
   std::string last_status_;
   std::string last_fetch_json_;
 
   base::WeakPtrFactory<NTPSnippetsFetcher> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(NTPSnippetsFetcher);
 };
 }  // namespace ntp_snippets
 
 #endif  // COMPONENTS_NTP_SNIPPETS_REMOTE_NTP_SNIPPETS_FETCHER_H_