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 326 matching lines @@
 };
 
 class NTPSnippetsContentSuggestionsFetcherTest : public NTPSnippetsFetcherTest {
  public:
   NTPSnippetsContentSuggestionsFetcherTest()
       : NTPSnippetsFetcherTest(
             GURL(kTestChromeContentSuggestionsUrl),
             {{"content_suggestions_backend", kContentSuggestionsServer}}) {}
 };
 
+// If you need access to the generated URLFetcher (i.e. all outgoing requests)
+// or don't need to know about the SnippetsAvailableCallback, use this class.
+// Tests derived from this class will never leak a SnippetsAvailableCallback.
+// TODO(fhorschig): Move this class and related tests to separate test file when
+// the NTPSnippetsFetcher::RequestBuilder gets moved to snippets::internal.
+class NTPSnippetsInternalUrlFetcherTest : public NTPSnippetsFetcherTest {

[tschumann, 2016/12/06 18:18:25]

sorry for the bystander, but it seems like inheritance is the wrong tool here
and aggregation would be better suited.

Why not make this a helper class which can be instantiated within the TEST()
blocks? This would make all the life-time issues go away.
Something like:

// ScopedInjectedURLFetcher injects url fetcher instances and executes the
fetcher with hard-coded responses on deletion.
class ScopedInjectedURLFetcher {
 public:
  ScopedInjectedURLFetcher() {}
  ~ScopedInjectedURLFetcher() {
    net::TestURLFetcher* fetcher = GetLastCreatedUrlFetcher();
    if (fetcher) {
      // inline CloseLastUrlFetcher() here. You can actually also just iterate
over all created fetchers and close all calls -- shouldn't make a difference.
      ...
    }
  }
  NTPSnippetsFetcher::SnippetsAvailableCallback
  CreateSnippetsAvailableCallback(
      MockSnippetsAvailableCallback* callback) {
    // Make sure the callback gets executed.
    EXPECT_CALL(...);
    ...
  }

 private:
  net::TestURLFetcherFactory test_url_fetcher_factory_;
};


In your TEST() you can then simply instantiate an instance and if you want to
verify the effects after the fetcher returned, you can simply put them in a
block, like:
TEST_F(...) {
  {
    ScopedInjectedURLFetcher url_fetcher;
    // set up call, do something.
  }
  // verify state after the execution.

[fhorschig, 2016/12/08 10:20:58]

Thank you for the bystander comment! I was already looking for a better
alternative (to be consistent with CL 2552813005) and this idea works great and
with much less explanation.

+ public:
+  NTPSnippetsInternalUrlFetcherTest() : NTPSnippetsFetcherTest() {}
+
+  void TearDown() override {
+    net::TestURLFetcher* fetcher = GetLastCreatedUrlFetcher();
+    if (fetcher) {
+      CloseLastUrlFetcher();
+    }
+  }
+
+  net::TestURLFetcher* GetLastCreatedUrlFetcher() const {
+    net::TestURLFetcher* fetcher = test_url_fetcher_factory_.GetFetcherByID(0);

[Marc Treib, 2016/12/06 16:06:56]

optional, to make this slightly less magic: It's possible to specify an ID when
creating the URLFetcher (in NTPSnippetsFetcher). Or maybe just add a comment why
ID 0 is all we need here?

[fhorschig, 2016/12/08 10:20:58]

Done.

+    return fetcher;

[Marc Treib, 2016/12/06 16:06:56]

nit: just
return test_url_fetcher_factory_.GetFetcherByID(0);

[fhorschig, 2016/12/08 10:20:58]

Done.

+  }
+
+  NTPSnippetsFetcher::SnippetsAvailableCallback
+  CreateSnippetsAvailableCallback() {
+    // If this expectation is triggered, the SnippetsAvailableCallback was not
+    // called. Did you trigger the creation of a new URLFetcher without calling
+    // |CloseLastUrlFetcher| to properly call the delegate of the last?
+    EXPECT_CALL(mock_callback(), Run(/*snippets=*/_)).Times(1);
+    return ToSnippetsAvailableCallback(&mock_callback());
+  }
+
+  // Call the last 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.
+  void CloseLastUrlFetcher() {
+    // An 4XX response needs the least configuration to successfully invoke the
+    // callback properly.
+    net::TestURLFetcher* fetcher = GetLastCreatedUrlFetcher();
+    ASSERT_THAT(fetcher->delegate(), NotNull());

[Marc Treib, 2016/12/06 16:06:56]

Also assert that |fetcher| itself isn't null?

[fhorschig, 2016/12/08 10:20:58]

Done.

+    fetcher->set_response_code(net::HTTP_NOT_FOUND);
+    fetcher->set_status(
+        net::URLRequestStatus(net::URLRequestStatus::FAILED, -2));

[Marc Treib, 2016/12/06 16:06:56]

nit: What's -2? Maybe add a /*name=*/?

[fhorschig, 2016/12/08 10:20:58]

Done.

+    fetcher->delegate()->OnURLFetchComplete(fetcher);
+    test_url_fetcher_factory_.RemoveFetcherFromMap(0);
+  }
+
+ private:
+  net::TestURLFetcherFactory test_url_fetcher_factory_;
+};
+
 TEST_F(NTPSnippetsFetcherTest, BuildRequestAuthenticated) {
   NTPSnippetsFetcher::RequestBuilder builder;
   NTPSnippetsFetcher::Params params;
   params.hosts = {"chromium.org"};
   params.excluded_ids = {"1234567890"};
   params.count_to_fetch = 25;
   params.interactive_request = false;
   builder.SetParams(params)
       .SetAuthentication("0BFUSGAIA", "headerstuff")
       .SetPersonalization(NTPSnippetsFetcher::Personalization::kPersonal)
@@ skipping 542 matching lines @@
   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;
+TEST_F(NTPSnippetsInternalUrlFetcherTest, ShouldRestrictToHosts) {
   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);
+  snippets_fetcher().FetchSnippets(params, CreateSnippetsAvailableCallback());
+  net::TestURLFetcher* fetcher = GetLastCreatedUrlFetcher();
   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(/*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(NTPSnippetsInternalUrlFetcherTest, RetryOnlyInteractiveRequests) {
+  NTPSnippetsFetcher::Params params = test_params();
+
+  // Fetch interactive and expect that the created fechter would retry on 5xx.

[Marc Treib, 2016/12/06 16:06:56]

nit: s/fechter/fetcher/

[fhorschig, 2016/12/08 10:20:58]

Done.

+  params.interactive_request = true;
+  snippets_fetcher().FetchSnippets(params, CreateSnippetsAvailableCallback());
+  EXPECT_THAT(GetLastCreatedUrlFetcher()->GetMaxRetriesOn5xx(), Eq(2));
+
+  CloseLastUrlFetcher();

[Marc Treib, 2016/12/06 16:06:56]

nit: Split this into two tests, so that this explicit call isn't needed?

[fhorschig, 2016/12/08 10:20:58]

Done.

+
+  // Fetch non-interactive and expect that the created fechter would not retry.
+  params.interactive_request = false;
+  snippets_fetcher().FetchSnippets(params, CreateSnippetsAvailableCallback());
+  EXPECT_THAT(GetLastCreatedUrlFetcher()->GetMaxRetriesOn5xx(), Eq(0));
 }
 
 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