[Doodle] Cleanups in DoodleFetcherImplTest

components/doodle/doodle_fetcher_impl_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/doodle/doodle_fetcher_impl.h"
 
+#include <memory>
 #include <string>
 #include <utility>
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/json/json_reader.h"
 #include "base/memory/ptr_util.h"
 #include "base/message_loop/message_loop.h"
+#include "base/test/mock_callback.h"
 #include "base/time/time.h"
 #include "base/values.h"
 #include "components/google/core/browser/google_switches.h"
 #include "components/google/core/browser/google_url_tracker.h"
 #include "net/http/http_status_code.h"
 #include "net/url_request/test_url_fetcher_factory.h"
 #include "net/url_request/url_request_status.h"
 #include "net/url_request/url_request_test_util.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+using testing::_;
+using testing::DoAll;
 using testing::Eq;
+using testing::SaveArg;
 
 namespace doodle {
 
 namespace {
 
 const char kDoodleConfigPath[] = "/async/ddljson";
 
 // Required to instantiate a GoogleUrlTracker in UNIT_TEST_MODE.
 class GoogleURLTrackerClientStub : public GoogleURLTrackerClient {
  public:
   GoogleURLTrackerClientStub() {}
   ~GoogleURLTrackerClientStub() override {}
 
   bool IsBackgroundNetworkingEnabled() override { return true; }
 
   PrefService* GetPrefs() override { return nullptr; }
 
   net::URLRequestContextGetter* GetRequestContext() override { return nullptr; }
 
  private:
   DISALLOW_COPY_AND_ASSIGN(GoogleURLTrackerClientStub);
 };
 
-std::string Resolve(const std::string& relative_url) {
-  return GURL(GoogleURLTracker::kDefaultGoogleHomepage)
-      .Resolve(relative_url)
-      .spec();
+GURL Resolve(const std::string& relative_url) {

[Marc Treib, 2017/03/22 13:44:07]

Should this be a member of the test class and use GetGoogleBaseURL?

[fhorschig, 2017/03/22 14:27:41]

Sounds reasonable.

[Marc Treib, 2017/03/22 15:35:37]

Done.

+  return GURL(GoogleURLTracker::kDefaultGoogleHomepage).Resolve(relative_url);
 }
 
 void ParseJson(
     const std::string& json,
     const base::Callback<void(std::unique_ptr<base::Value> json)>& success,
     const base::Callback<void(const std::string&)>& error) {
   base::JSONReader json_reader;
   std::unique_ptr<base::Value> value = json_reader.ReadToValue(json);
   if (value) {
     success.Run(std::move(value));
   } else {
     error.Run(json_reader.GetErrorMessage());
   }
 }
 
 }  // namespace
 
 class DoodleFetcherImplTest : public testing::Test {
  public:
   DoodleFetcherImplTest()
-      : url_(GURL(GoogleURLTracker::kDefaultGoogleHomepage)),
-        google_url_tracker_(base::MakeUnique<GoogleURLTrackerClientStub>(),
+      : google_url_tracker_(base::MakeUnique<GoogleURLTrackerClientStub>(),
                             GoogleURLTracker::UNIT_TEST_MODE),
         doodle_fetcher_(
             new net::TestURLRequestContextGetter(message_loop_.task_runner()),
             &google_url_tracker_,
             base::Bind(ParseJson)) {}
 
   void RespondWithData(const std::string& data) {
-    RespondToFetcherWithData(GetRunningFetcher(), data);
-  }
-
-  void RespondToFetcherWithData(net::TestURLFetcher* url_fetcher,
-                                const std::string& data) {
+    net::TestURLFetcher* url_fetcher = GetRunningFetcher();
     url_fetcher->set_status(net::URLRequestStatus());
     url_fetcher->set_response_code(net::HTTP_OK);
     url_fetcher->SetResponseString(data);
     // Call the URLFetcher delegate to continue the test.
     url_fetcher->delegate()->OnURLFetchComplete(url_fetcher);
   }
 
   void RespondWithError(int error_code) {
     net::TestURLFetcher* url_fetcher = GetRunningFetcher();
     url_fetcher->set_status(net::URLRequestStatus::FromError(error_code));
     url_fetcher->SetResponseString("");
     // Call the URLFetcher delegate to continue the test.
     url_fetcher->delegate()->OnURLFetchComplete(url_fetcher);
   }
 
   net::TestURLFetcher* GetRunningFetcher() {
     // All created TestURLFetchers have ID 0 by default.
     net::TestURLFetcher* url_fetcher = url_fetcher_factory_.GetFetcherByID(0);
     DCHECK(url_fetcher);
     return url_fetcher;
   }
 
   // TODO(treib): Replace with a MockCallback plus testing::SaveArgs?

[Marc Treib, 2017/03/22 13:44:07]

I'm not quite sure about this one. I've converted one test to the
MockCallback/SaveArg style as an example, see below. I think that's a bit more
explicit/less magic. WDYT? If you agree, I'll convert the rest too.

[fhorschig, 2017/03/22 14:27:41]

Less magic is relative but it's certainly easier to debug.
So I would be in favor of the SaveArg-style.

[Marc Treib, 2017/03/22 15:35:37]

Done.

   DoodleFetcherImpl::FinishedCallback CreateResponseSavingCallback(
       DoodleState* state_out,
       base::TimeDelta* time_to_live_out,
       base::Optional<DoodleConfig>* config_out) {
     return base::BindOnce(
         [](DoodleState* state_out, base::TimeDelta* time_to_live_out,
            base::Optional<DoodleConfig>* config_out, DoodleState state,
            base::TimeDelta time_to_live,
            const base::Optional<DoodleConfig>& config) {
           if (state_out) {
             *state_out = state;
           }
           if (time_to_live_out) {
             *time_to_live_out = time_to_live;
           }
           if (config_out) {
             *config_out = config;
           }
         },
         state_out, time_to_live_out, config_out);
   }
 
   DoodleFetcherImpl* doodle_fetcher() { return &doodle_fetcher_; }
 
   GURL GetGoogleBaseURL() { return google_url_tracker_.google_url(); }
 
  private:
   base::MessageLoop message_loop_;
-  GURL url_;
   net::TestURLFetcherFactory url_fetcher_factory_;
   GoogleURLTracker google_url_tracker_;
   DoodleFetcherImpl doodle_fetcher_;
 };
 
 TEST_F(DoodleFetcherImplTest, ReturnsFromFetchWithoutError) {
+  base::MockCallback<DoodleFetcherImpl::FinishedCallback> callback;
+  doodle_fetcher()->FetchDoodle(callback.Get());
+
   DoodleState state(DoodleState::NO_DOODLE);
   base::Optional<DoodleConfig> response;
-
-  doodle_fetcher()->FetchDoodle(
-      CreateResponseSavingCallback(&state, nullptr, &response));
+  EXPECT_CALL(callback, Run(_, _, _))

[fhorschig, 2017/03/22 14:27:41]

It's somewhat confusing to see the expectation after the call, but of
course it makes sense given that the callback isn't triggered until the
server responds ....

[Marc Treib, 2017/03/22 15:35:37]

I read this as: I expect the call when I send the response. But if you prefer, I
can move the expectation up.

[fhorschig, 2017/03/22 17:00:56]

No, don't move it up. It also makes sense that the expectation can never be
triggered before we send the
response.
It's just unusual because we are usually forced to have expectations
earlier. I would prefer it to always be near the place where the call
happens. Like here :D

+      .WillOnce(DoAll(SaveArg<0>(&state), SaveArg<2>(&response)));
   RespondWithData(R"json({"ddljson": {
         "time_to_live_ms":55000,
         "large_image": {"url":"/logos/doodles/2015/some.gif"}
       }})json");
 
   EXPECT_THAT(state, Eq(DoodleState::AVAILABLE));
   EXPECT_TRUE(response.has_value());
 }
 
 TEST_F(DoodleFetcherImplTest, ReturnsFrom404FetchWithError) {
@@ skipping 183 matching lines @@
           "url":"/logos/doodles/2015/new-years-eve-2015-59854387958251-thp.png",
           "width":510
         }
       }})json");
 
   EXPECT_THAT(state, Eq(DoodleState::AVAILABLE));
   ASSERT_TRUE(response.has_value());
   DoodleConfig config = response.value();
 
   EXPECT_TRUE(config.large_image.url.is_valid());
-  EXPECT_THAT(config.large_image.url.spec(),
+  EXPECT_THAT(config.large_image.url,
               Eq(Resolve("/logos/doodles/2015/new-years-eve-2015-5985438795"
                          "8251-hp.gif")));
   EXPECT_THAT(config.large_image.width, Eq(489));
   EXPECT_THAT(config.large_image.height, Eq(225));
   EXPECT_TRUE(config.large_image.is_animated_gif);
   EXPECT_FALSE(config.large_image.is_cta);
 
   ASSERT_TRUE(config.transparent_large_image.has_value());
   EXPECT_TRUE(config.transparent_large_image->url.is_valid());
-  EXPECT_THAT(config.transparent_large_image->url.spec(),
+  EXPECT_THAT(config.transparent_large_image->url,
               Eq(Resolve("/logos/doodles/2015/new-years-eve-2015-5985438795"
                          "8251-thp.png")));
   EXPECT_THAT(config.transparent_large_image->width, Eq(510));
   EXPECT_THAT(config.transparent_large_image->height, Eq(225));
   EXPECT_FALSE(config.transparent_large_image->is_animated_gif);
   EXPECT_FALSE(config.transparent_large_image->is_cta);
 
   ASSERT_TRUE(config.large_cta_image.has_value());
   EXPECT_TRUE(config.large_cta_image->url.is_valid());
-  EXPECT_THAT(config.large_cta_image->url.spec(),
+  EXPECT_THAT(config.large_cta_image->url,
               Eq(Resolve("/logos/doodles/2015/new-years-eve-2015-5985438795"
                          "8251-cta.gif")));
   EXPECT_THAT(config.large_cta_image->width, Eq(489));
   EXPECT_THAT(config.large_cta_image->height, Eq(225));
   EXPECT_TRUE(config.large_cta_image->is_animated_gif);
   EXPECT_TRUE(config.large_cta_image->is_cta);
 }
 
 TEST_F(DoodleFetcherImplTest, RespondsToMultipleRequestsWithSameFetcher) {
   DoodleState state1(DoodleState::NO_DOODLE);
@@ skipping 22 matching lines @@
   EXPECT_TRUE(response1.has_value());
   EXPECT_THAT(state2, Eq(DoodleState::AVAILABLE));
   EXPECT_TRUE(response2.has_value());
 }
 
 TEST_F(DoodleFetcherImplTest, ReceivesBaseUrlFromTracker) {
   doodle_fetcher()->FetchDoodle(CreateResponseSavingCallback(
       /*state=*/nullptr, nullptr, /*response=*/nullptr));
 
   EXPECT_THAT(GetRunningFetcher()->GetOriginalURL(),
               Eq(GetGoogleBaseURL().Resolve(kDoodleConfigPath)));

[Marc Treib, 2017/03/22 13:44:07]

This doesn't really test anything, does it? The Google base URL has the default
value anyway.

[fhorschig, 2017/03/22 14:27:41]

Yes. Meanwhile, this case is covered by pretty much every other test.

[Marc Treib, 2017/03/22 15:35:37]

GoogleBaseURLTracker doesn't seem to have an easy way to override the URL,
unfortunately :-/  I've added a TODO for now.

[fhorschig, 2017/03/22 17:00:56]

sgtm (I can remotely remember to have stumbled upon the
same issue.)

 }
 
 TEST_F(DoodleFetcherImplTest, OverridesBaseUrlWithCommandLineArgument) {
   base::CommandLine::ForCurrentProcess()->AppendSwitchASCII(
       switches::kGoogleBaseURL, "http://www.google.kz");
 
   doodle_fetcher()->FetchDoodle(CreateResponseSavingCallback(
       /*state=*/nullptr, nullptr, /*response=*/nullptr));
 
   EXPECT_THAT(GetRunningFetcher()->GetOriginalURL(),
               Eq(GURL("http://www.google.kz").Resolve(kDoodleConfigPath)));
 }
 
 }  // namespace doodle