[Doodle] Cleanups in DoodleFetcherImplTest

components/doodle/doodle_fetcher_impl_unittest.cc

Index: components/doodle/doodle_fetcher_impl_unittest.cc
diff --git a/components/doodle/doodle_fetcher_impl_unittest.cc b/components/doodle/doodle_fetcher_impl_unittest.cc
index bc67bf7de3d8b6bda623d3d41e298d7cafc9858d..3b450be88d7f1e787a87b66b3afd8b99a874b6b8 100644
--- a/components/doodle/doodle_fetcher_impl_unittest.cc
+++ b/components/doodle/doodle_fetcher_impl_unittest.cc
@@ -4,6 +4,7 @@
 
 #include "components/doodle/doodle_fetcher_impl.h"
 
+#include <memory>
 #include <string>
 #include <utility>
 
@@ -12,6 +13,7 @@
 #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"
@@ -23,7 +25,10 @@
 #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 {
 
@@ -47,10 +52,8 @@ class GoogleURLTrackerClientStub : public GoogleURLTrackerClient {
   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(
@@ -71,8 +74,7 @@ void ParseJson(
 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()),
@@ -80,11 +82,7 @@ class DoodleFetcherImplTest : public testing::Test {
             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);
@@ -136,18 +134,19 @@ class DoodleFetcherImplTest : public testing::Test {
 
  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"}
@@ -351,7 +350,7 @@ TEST_F(DoodleFetcherImplTest, ResponseContainsExactlyTheSampleImages) {
   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));
@@ -361,7 +360,7 @@ TEST_F(DoodleFetcherImplTest, ResponseContainsExactlyTheSampleImages) {
 
   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));
@@ -371,7 +370,7 @@ TEST_F(DoodleFetcherImplTest, ResponseContainsExactlyTheSampleImages) {
 
   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));