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..084caaeae50e4d84282ffaf007ed28a625f84f43 100644
--- a/components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc
+++ b/components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc
@@ -344,6 +344,55 @@ class NTPSnippetsContentSuggestionsFetcherTest : public NTPSnippetsFetcherTest {
             {{"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;
@@ -906,14 +955,12 @@ TEST_F(NTPSnippetsFetcherTest, ShouldFetchSuccessfullyEmptyList) {
               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());
@@ -936,16 +983,22 @@ 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(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) {