Introduce a Doodle Fetcher for NTP

components/doodle/doodle_fetcher_unittest.cc

Index: components/doodle/doodle_fetcher_unittest.cc
diff --git a/components/doodle/doodle_fetcher_unittest.cc b/components/doodle/doodle_fetcher_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..f7dd6653688f65bd576605b6e07d27d9ac4553bc
--- /dev/null
+++ b/components/doodle/doodle_fetcher_unittest.cc
@@ -0,0 +1,373 @@
+// 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.h"
+
+#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/simple_test_clock.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::Eq;
+
+namespace doodle {
+
+namespace {
+
+const char kDoodleConfigPath[] = "/async/ddljson";
+
+const char kSampleResponse[] = R"json()]}'{
+      "ddljson": {
+      "alt_text":"Mouseover Text",
+      "doodle_type":"SIMPLE",
+      "id":0,
+      "interactive_html":"\u003cstyle\u003e\u003c\/style\u003e",
+      "interactive_js":"(function(){}).call(this);",
+      "search_url":"/search?q\u003dtest",
+      "share_text":"Share Text #GoogleDoodle\nhttps://g.co/doodle/8hfqzq",
+      "short_link":"//g.co/doodle/8hfqzq",
+      "show_now_header_search_affordance":false,
+      "show_now_header_share_button":true,
+      "target_url":"/search?q\u003dtest\u0026sa\u003dX\u0026ved\u003d0ahUKEwjm",
+      "time_to_live_ms":55000,
+      "large_image": {
+        "background_color":"#ffffff",
+        "height":225,
+        "image_id":0,
+        "is_animated_gif":true,
+        "is_cta":false,
+        "slot":1,
+        "url":"/logos/doodles/2015/new-years-eve-2015-5985438795825152-hp.gif",
+        "width":489
+      },
+      "large_cta_image": {
+        "background_color":"#ffffff",
+        "height":225,
+        "image_id":0,
+        "is_animated_gif":true,
+        "is_cta":true,
+        "slot":8,
+        "url":"/logos/doodles/2015/new-years-eve-2015-5985438795825152-cta.gif",
+        "width":489
+      },
+      "transparent_large_image": {
+        "background_color":"",
+        "height":225,
+        "image_id":0,
+        "is_animated_gif":false,
+        "is_cta":false,
+        "slot":2,
+        "url":"/logos/doodles/2015/new-years-eve-2015-5985438795825152-thp.png",
+        "width":510
+      }
+    }})json";
+
+const char kMinimalAvailableDoodleResponse[] = R"json({"ddljson": {
+      "large_image": {
+        "url":"/logos/doodles/2015/new-years-eve-2015-5985438795825152-hp.gif"
+      }}})json";
+
+const char kMalformedImageUrlResponse[] = R"json({"ddljson": {
+      "time_to_live_ms":55000,
+      "large_image": {
+        "height":225,
+        "width":489
+      }}})json";
+
+// 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();
+}
+
+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 DoodleFetcherTest : public testing::Test {
+ public:
+  DoodleFetcherTest()
+      : url_(GURL(GoogleURLTracker::kDefaultGoogleHomepage)),
+        last_state_(DoodleState::NO_DOODLE),
+        last_response_(base::nullopt),

[Marc Treib, 2017/02/08 10:51:36]

Isn't this the default value anyway?

[fhorschig, 2017/02/08 18:37:26]

Documentation says you are right. Removed.

+        context_getter(new net::TestURLRequestContextGetter(
+            base::ThreadTaskRunnerHandle::Get())),

[Marc Treib, 2017/02/08 10:51:36]

I think it'd be clearer to explicitly pass in the TaskRunner from message_loop_
(it should work out to the exact same thing, but IMO makes clearer what's going
on).

[fhorschig, 2017/02/08 18:37:25]

Reverted to how I did this intermediately. 
It does work the same way.

+        google_url_tracker_(base::MakeUnique<GoogleURLTrackerClientStub>(),
+                            GoogleURLTracker::UNIT_TEST_MODE),
+        doodle_fetcher_(context_getter.get(),
+                        &google_url_tracker_,
+                        base::Bind(ParseJson)) {
+    // Random difference to 0 ensures that expiry_dates are really relative.
+    clock_.Advance(base::TimeDelta::FromMilliseconds(1123581321));
+    auto clock = base::MakeUnique<base::SimpleTestClock>();
+    clock->SetNow(clock_.Now());
+    doodle_fetcher_.SetClockForTesting(std::move(clock));

[Marc Treib, 2017/02/08 10:51:37]

I guess we never need to set anything in the clock we pass to the fetcher?

[fhorschig, 2017/02/08 18:37:26]

Exactly.
I just noticed that we have exactly one fetcher that lives for the duration
of the test, so there is no reason to make a copy of the clock.
A pointer to this clock suffices.

+  }
+
+  void RespondWithData(const std::string& data) {
+    RespondToFetcherWithData(GetRunningFetcher(), data);
+  }
+
+  void RespondToFetcherWithData(net::TestURLFetcher* url_fetcher,
+                                const std::string& data) {
+    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() {
+    net::TestURLFetcher* url_fetcher = GetRunningFetcher();
+    url_fetcher->set_status(
+        net::URLRequestStatus::FromError(net::ERR_INVALID_URL));
+    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.

[Marc Treib, 2017/02/08 10:51:36]

I guess there's never more than one URLFetcher at a time?
More to the point: What would happen if there's more than one?

[fhorschig, 2017/02/08 18:37:25]

A test would break (RespondsToMultipleRequestsWithSameFetcher)
and also the old fetcher would silently be overridden.
Other tests would break but not crash.

[Marc Treib, 2017/02/09 10:36:24]

Thanks! (I was mostly wondering what GetFetcherByID would return - I guess it's
just the most-recently-created one.)

[fhorschig, 2017/02/09 13:23:56]

Exactly.

+    net::TestURLFetcher* url_fetcher = url_fetcher_factory_.GetFetcherByID(0);
+    DCHECK(url_fetcher);
+    return url_fetcher;
+  }
+
+  void TriggerFetch() {

[Marc Treib, 2017/02/08 10:51:36]

I think this is a case where it's better to put this code into the test itself.
IMO the helper makes it harder to follow the tests.
If we remove this, then also the last_* members shouldn't be required anymore.

[fhorschig, 2017/02/08 18:37:26]

I did this to make the tests readable like natural language :/
It's now inlined but the callback is still wrapped.

[Marc Treib, 2017/02/09 10:36:24]

I think that's okay. The alternative would be to use EXPECT_CALL, but then we'd
probably need a bunch of custom matchers (which would be fine for checking if
the returned config has a value at all, but probably not for e.g.
ResponseContainsValidBaseInformation).

[fhorschig, 2017/02/09 13:23:56]

I agree, the benefit would not be very huge.

+    doodle_fetcher_.FetchDoodle(base::BindOnce(
+        [](DoodleState* state_out, base::Optional<DoodleConfig>* config_out,
+           DoodleState state, const base::Optional<DoodleConfig>& config) {
+          *state_out = state;
+          *config_out = config;
+        },
+        &last_state_, &last_response_));
+  }
+
+  base::Time TimeFromNow(uint64_t milliseconds) {
+    return clock_.Now() + base::TimeDelta::FromMilliseconds(milliseconds);
+  }
+
+  void SetLastState(DoodleState state) { last_state_ = state; }
+
+  DoodleState last_state() const { return last_state_; }
+
+  const base::Optional<DoodleConfig>& last_response() const {
+    return last_response_;
+  }
+
+ private:
+  base::MessageLoop message_loop_;
+  GURL url_;
+  DoodleState last_state_;
+  base::Optional<DoodleConfig> last_response_;
+  net::TestURLFetcherFactory url_fetcher_factory_;
+  base::SimpleTestClock clock_;
+  scoped_refptr<net::TestURLRequestContextGetter> context_getter;
+  GoogleURLTracker google_url_tracker_;
+  DoodleFetcher doodle_fetcher_;
+};
+
+TEST_F(DoodleFetcherTest, ReturnsFromFetchWithoutError) {
+  TriggerFetch();
+  RespondWithData(kSampleResponse);
+  EXPECT_THAT(last_state(), Eq(DoodleState::AVAILABLE));
+  EXPECT_TRUE(last_response().has_value());

[Marc Treib, 2017/02/08 10:51:37]

This is what I mean above: Here we're EXPECTing stuff that isn't mentioned
anywhere in the test before. So it's not really clear why the setup above should
produce these results.
WDYT?

(OTOH, I think the Respond* helpers are fine. Those abstract away details which
aren't really interesting for the test itself.)

[fhorschig, 2017/02/08 18:37:25]

I think I get your thought.

+}
+
+TEST_F(DoodleFetcherTest, ReturnsFrom404FetchWithError) {

[Marc Treib, 2017/02/08 10:51:36]

RespondWithError doesn't actually produce a 404.
How about passing the error code to RespondWithError as a param?

[fhorschig, 2017/02/08 18:37:26]

Done.
(And thanks for pointing out the wrong code.)

+  TriggerFetch();
+  RespondWithError();
+  EXPECT_THAT(last_state(), Eq(DoodleState::DOWNLOAD_ERROR));
+  EXPECT_FALSE(last_response().has_value());
+}
+
+TEST_F(DoodleFetcherTest, ReturnsErrorForInvalidJson) {
+  TriggerFetch();
+  RespondWithData("{}");
+  EXPECT_THAT(last_state(), Eq(DoodleState::PARSING_ERROR));
+  EXPECT_FALSE(last_response().has_value());
+}
+
+TEST_F(DoodleFetcherTest, ResponseContainsValidBaseInformation) {
+  TriggerFetch();
+  RespondWithData(kSampleResponse);
+  EXPECT_THAT(last_state(), Eq(DoodleState::AVAILABLE));
+  ASSERT_TRUE(last_response().has_value());
+  DoodleConfig config = last_response().value();
+
+  EXPECT_TRUE(config.search_url.is_valid());
+  EXPECT_THAT(config.search_url, Eq(Resolve("/search?q\u003dtest")));
+  EXPECT_THAT(config.fullpage_interactive_url, Eq(GURL()));

[Marc Treib, 2017/02/08 10:51:37]

nit: EXPECT_TRUE(...is_empty()) ?

[fhorschig, 2017/02/08 18:37:25]

Done.

+  EXPECT_TRUE(config.target_url.is_valid());
+  EXPECT_THAT(config.target_url,
+              Eq(Resolve("/search?q\u003dtest\u0026sa\u003dX\u0026ved\u003d"
+                         "0ahUKEwjm")));
+  EXPECT_THAT(config.doodle_type, Eq(DoodleType::SIMPLE));
+  EXPECT_THAT(config.alt_text, Eq("Mouseover Text"));
+  EXPECT_THAT(config.interactive_html,
+              Eq("\u003cstyle\u003e\u003c/style\u003e"));
+  EXPECT_THAT(config.interactive_js, Eq("(function(){}).call(this);"));
+
+  EXPECT_THAT(config.expiry_date, Eq(TimeFromNow(55000)));
+}
+
+TEST_F(DoodleFetcherTest, ResponseContainsExpiresWithinThirtyDays) {
+  TriggerFetch();
+  RespondWithData(R"json({"ddljson": {
+      "time_to_live_ms":5184000000,
+      "large_image": {"url":"/logos/doodles/2015/some.gif"}
+    }})json");  // 60 days
+
+  EXPECT_THAT(last_state(), Eq(DoodleState::AVAILABLE));
+  ASSERT_TRUE(last_response().has_value());
+  EXPECT_THAT(last_response().value().expiry_date,
+              Eq(TimeFromNow(24ul * 60 * 60 * 1000 /* ms */)));  // 1 day
+}
+
+TEST_F(DoodleFetcherTest, AvailableDoodleExpiresNowWithNegativeTTL) {
+  TriggerFetch();
+  RespondWithData(R"json({"ddljson": {
+      "time_to_live_ms":-1,
+      "large_image": {"url":"/logos/doodles/2015/some.gif"}
+    }})json");
+
+  EXPECT_THAT(last_state(), Eq(DoodleState::AVAILABLE));
+  ASSERT_TRUE(last_response().has_value());
+  EXPECT_THAT(last_response().value().expiry_date, Eq(TimeFromNow(0)));
+}
+
+TEST_F(DoodleFetcherTest, AvailableDoodleExpiresNowWithoutValidTTL) {
+  TriggerFetch();
+  RespondWithData(kMinimalAvailableDoodleResponse);
+
+  EXPECT_THAT(last_state(), Eq(DoodleState::AVAILABLE));
+  ASSERT_TRUE(last_response().has_value());
+  EXPECT_THAT(last_response().value().expiry_date, Eq(TimeFromNow(0)));
+}
+
+TEST_F(DoodleFetcherTest, ExpectToReturnNoDoodleForMalfomedImageUrls) {
+  SetLastState(DoodleState::AVAILABLE);
+
+  TriggerFetch();
+  RespondWithData(kMalformedImageUrlResponse);
+
+  EXPECT_THAT(last_state(), Eq(DoodleState::NO_DOODLE));
+  EXPECT_FALSE(last_response().has_value());
+}
+
+TEST_F(DoodleFetcherTest, EmptyResponsesCausesNoDoodleState) {
+  SetLastState(DoodleState::AVAILABLE);
+
+  TriggerFetch();
+  RespondWithData("{\"ddljson\":{}}");
+
+  EXPECT_THAT(last_state(), Eq(DoodleState::NO_DOODLE));
+  EXPECT_FALSE(last_response().has_value());
+}
+
+TEST_F(DoodleFetcherTest, ResponseContainsExactlyTheSampleImages) {
+  TriggerFetch();
+  RespondWithData(kSampleResponse);
+
+  EXPECT_THAT(last_state(), Eq(DoodleState::AVAILABLE));
+  ASSERT_TRUE(last_response().has_value());
+  DoodleConfig config = last_response().value();
+
+  EXPECT_TRUE(config.transparent_large_image.url.is_valid());
+  EXPECT_THAT(config.transparent_large_image.url.spec(),
+              Eq(Resolve("/logos/doodles/2015/new-years-eve-2015-5985438795"
+                         "825152-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);
+
+  EXPECT_TRUE(config.large_image.url.is_valid());
+  EXPECT_THAT(config.large_image.url.spec(),
+              Eq(Resolve("/logos/doodles/2015/new-years-eve-2015-5985438795"
+                         "825152-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);
+
+  EXPECT_TRUE(config.large_cta_image.url.is_valid());
+  EXPECT_THAT(config.large_cta_image.url.spec(),
+              Eq(Resolve("/logos/doodles/2015/new-years-eve-2015-5985438795"
+                         "825152-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(DoodleFetcherTest, RespondsToMultipleRequestsWithSameFetcher) {
+  TriggerFetch();
+  net::URLFetcher* first_created_fetcher = GetRunningFetcher();
+  TriggerFetch();
+  net::URLFetcher* second_created_fetcher = GetRunningFetcher();
+
+  EXPECT_THAT(first_created_fetcher, Eq(second_created_fetcher));
+}
+
+TEST_F(DoodleFetcherTest, ReceivesBaseUrlFromTracker) {
+  TriggerFetch();
+
+  EXPECT_THAT(GetRunningFetcher()->GetOriginalURL(),
+              Eq(GURL(GoogleURLTracker::kDefaultGoogleHomepage)

[Marc Treib, 2017/02/08 10:51:36]

google_url_tracker_->google_url() ?

[fhorschig, 2017/02/08 18:37:25]

Done. Kind of.

+                     .Resolve(kDoodleConfigPath)));
+}
+
+TEST_F(DoodleFetcherTest, OverridesBaseUrlWithCommandLineArgument) {
+  base::CommandLine::ForCurrentProcess()->AppendSwitchASCII(
+      switches::kGoogleBaseURL, "http://www.google.kz");
+
+  TriggerFetch();
+
+  EXPECT_THAT(GetRunningFetcher()->GetOriginalURL(),
+              Eq(GURL("http://www.google.kz").Resolve(kDoodleConfigPath)));
+}
+
+}  // namespace doodle