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..92dec91301716d7f178da6eb3fe5f408e22c3d80
--- /dev/null
+++ b/components/doodle/doodle_fetcher_unittest.cc
@@ -0,0 +1,279 @@
+// 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/json/json_reader.h"
+#include "base/memory/ptr_util.h"
+#include "base/test/test_mock_time_task_runner.h"
+#include "base/threading/sequenced_task_runner_handle.h"
+#include "base/threading/thread_task_runner_handle.h"
+#include "base/values.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;
+using testing::IsEmpty;
+
+namespace doodle {
+
+namespace {
+const char kSampleResponse[] = R"json()]}'{

[Marc Treib, 2017/02/01 11:53:14]

nitty nit: I'd insert a newline after R"json( to make clear where the actual
data begins

[Marc Treib, 2017/02/01 14:08:05]

Hm, on second thought: That would put an extra newline into the string, which
currently we don't handle correctly :D

[fhorschig, 2017/02/03 13:21:05]

I guess it would be weird to add code that just unbreaks tests and has no
productive value.

[Marc Treib, 2017/02/03 15:45:48]

Well, we could decide that it's okay if the response contains whitespace before
the preamble. Then it wouldn't strictly be test-only code. But I don't feel
strongly either way.

[fhorschig, 2017/02/08 09:10:23]

Acknowledged.

+      "ddljson": {
+      "alt_text":"Mouseover Text",
+      "doodle_type":"SIMPLE",
+      "id":0,
+      "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":514220215000,
+      "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";

[Marc Treib, 2017/02/01 11:53:14]

similar here: newline after the {{

[fhorschig, 2017/02/03 13:21:05]

Done.

+
+const char kBaseUrl[] = "https://www.google.com/";
+
+std::string AbsoluteUrl(const std::string& relative_url) {

[Marc Treib, 2017/02/01 11:53:14]

MakeAbsoluteUrl, or just Resolve?

[fhorschig, 2017/02/03 13:21:05]

I am ashamed. Changed.

+  return GURL(kBaseUrl).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(kBaseUrl).Resolve(kDoodleConfigPath)),
+        url_fetcher_factory_(nullptr),

[Marc Treib, 2017/02/01 11:53:14]

Add a comment on what the nullptr is? (I first interpreted this as
"url_fetcher_factory_ is a pointer", and wondered where it gets initialized.)

[fhorschig, 2017/02/03 13:21:05]

Comment added. I will probably use a failing fake default
when testing the BaseUrl functionality.

[Marc Treib, 2017/02/03 15:45:48]

FWIW, there's two different versions of URLFetcher things for testing, the
"Fake" ones and the "Test" ones. I *think* the other one doesn't need all the
TaskRunner stuff.

Let's maybe discuss this next week when you're back.

[fhorschig, 2017/02/08 09:10:23]

Done.

+        mock_task_runner_(new base::TestMockTimeTaskRunner()),

[Marc Treib, 2017/02/01 11:53:14]

Hm, I'm not very familiar with this, but it looks like you only need the
TestMockTimeTaskRunner if the tests depend on delayed tasks?
Actually, I'm not convinced that the tests need a taskrunner at all, since
nothing in DoodleFetcher actually posts any tasks.

[fhorschig, 2017/02/03 13:21:05]

The URLFetcher posts tasks and needs a context.
The TestMockTimeTaskRunner also provides me with that context
and also allows me to use its MockClock.

If you insist, I could try using a TestSimpleTaskRunner and look for 
another MockClock.

(I think net:: has one, but I wouldn't like to depend on more classes
than I absolutely need to).

[Marc Treib, 2017/02/03 15:45:48]

Couldn't you just instantiate a SimpleTestClock yourself?

Generally, I'm a bit wary of unit tests that need task runners, message loops,
RunUntilIdle and all that stuff. Sometimes it can't be avoided, but I think here
it maybe can.

Anyway, this mostly boils down to the comment above. Let's discuss in person.

[fhorschig, 2017/02/08 09:10:23]

As discussed, tests redesigned to use TestUrlFetcherFactory instead.

+        mock_task_runner_handle_(mock_task_runner_),
+        context_getter(
+            new net::TestURLRequestContextGetter(mock_task_runner_.get())) {
+    // Random difference to 0 ensures that expiry_dates are really relative.
+    mock_task_runner_->FastForwardBy(base::TimeDelta::FromMilliseconds(80082));
+  }
+
+  void RespondWithData(const std::string& data) {

[Marc Treib, 2017/02/01 11:53:14]

nit: Maybe SetResponse or something like that? IMO the current name is a bit
misleading, since it implies that it would actually cause a response right now.

[fhorschig, 2017/02/03 13:21:05]

Done.

+    url_fetcher_factory_.SetFakeResponse(url_, data, net::HTTP_OK,
+                                         net::URLRequestStatus::SUCCESS);
+  }
+
+  void RespondWithError() {
+    url_fetcher_factory_.SetFakeResponse(url_, "", net::HTTP_NOT_FOUND,
+                                         net::URLRequestStatus::FAILED);
+  }
+
+  DoodleState FetchDoodle(base::Optional<DoodleConfig>* config) {
+    DoodleState state;
+    std::unique_ptr<DoodleFetcher> fetcher = CreateDoodleFetcher();
+    TriggerFetch(fetcher.get(), &state, config);
+    WaitForCallbacksToReturn();
+    return state;
+  }
+
+  std::unique_ptr<DoodleFetcher> CreateDoodleFetcher() {
+    // TODO(fhorschig): Create Fake for GoogleUrlTracker.
+    std::unique_ptr<DoodleFetcher> fetcher = base::MakeUnique<DoodleFetcher>(
+        context_getter.get(),
+        /*google_url_tracker=*/nullptr, base::Bind(ParseJson));
+    fetcher->SetClockForTesting(mock_task_runner_->GetMockClock());
+    return fetcher;
+  }
+
+  void TriggerFetch(DoodleFetcher* fetcher,
+                    DoodleState* state,
+                    base::Optional<DoodleConfig>* config) {
+    fetcher->FetchDoodle(base::BindOnce(
+        [](DoodleState* state_out, base::Optional<DoodleConfig>* config_out,
+           DoodleState state, base::Optional<DoodleConfig> config) {
+          *state_out = std::move(state);
+          *config_out = std::move(config);
+        },
+        state, config));
+  }
+
+  base::Time TimeFromNow(uint64_t milliseconds) {
+    return mock_task_runner_->GetMockClock()->Now() +
+           base::TimeDelta::FromMilliseconds(milliseconds);
+  }
+
+  void WaitForCallbacksToReturn() {
+    mock_task_runner_->FastForwardUntilNoTasksRemain();
+  }
+
+  size_t NumberOfPendingTasks() {
+    return mock_task_runner_->GetPendingTaskCount();
+  }
+
+ private:
+  GURL url_;
+  net::FakeURLFetcherFactory url_fetcher_factory_;
+  scoped_refptr<base::TestMockTimeTaskRunner> mock_task_runner_;
+  base::ThreadTaskRunnerHandle mock_task_runner_handle_;

[Marc Treib, 2017/02/01 11:53:14]

Is this needed?

[fhorschig, 2017/02/03 13:21:05]

Yes. (Its constructor replace the base::ThreadTaskRunnerHandle.
That way, when the static base::ThreadTaskRunnerHandle::Get()
is called, it will use the handle of mock_task_runner_.)

+  scoped_refptr<net::TestURLRequestContextGetter> context_getter;
+};
+
+TEST_F(DoodleFetcherTest, ReturnsFromFetchWithoutError) {
+  RespondWithData(kSampleResponse);
+
+  base::Optional<DoodleConfig> response;
+  EXPECT_THAT(FetchDoodle(&response), Eq(DoodleState::AVAILABLE));
+  EXPECT_TRUE(response.has_value());
+}
+
+TEST_F(DoodleFetcherTest, ReturnsFrom404FetchWithError) {
+  RespondWithError();
+
+  base::Optional<DoodleConfig> response;
+  EXPECT_THAT(FetchDoodle(&response), Eq(DoodleState::DOWNLOAD_ERROR));
+  EXPECT_FALSE(response.has_value());
+}
+
+TEST_F(DoodleFetcherTest, ReturnsErrorForInvalidJson) {
+  RespondWithData("{}");
+
+  base::Optional<DoodleConfig> response;
+  EXPECT_THAT(FetchDoodle(&response), Eq(DoodleState::PARSING_ERROR));
+  EXPECT_FALSE(response.has_value());
+}
+
+TEST_F(DoodleFetcherTest, ResponseContainsValidBaseInformation) {
+  RespondWithData(kSampleResponse);
+
+  base::Optional<DoodleConfig> response;
+  EXPECT_THAT(FetchDoodle(&response), Eq(DoodleState::AVAILABLE));
+  ASSERT_TRUE(response.has_value());
+  DoodleConfig config = response.value();
+
+  EXPECT_TRUE(config.search_url.is_valid());
+  EXPECT_THAT(config.search_url, Eq(AbsoluteUrl("/search?q\u003dtest")));
+  EXPECT_THAT(config.fullpage_interactive_url, Eq(GURL()));
+  EXPECT_TRUE(config.target_url.is_valid());
+  EXPECT_THAT(config.target_url,
+              Eq(AbsoluteUrl("/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.expiry_date, Eq(TimeFromNow(514220215000)));
+}
+
+TEST_F(DoodleFetcherTest, EmptyResponsesCausesNoDoodleState) {
+  RespondWithData("{\"ddljson\":{}}");
+
+  base::Optional<DoodleConfig> response;
+  EXPECT_THAT(FetchDoodle(&response), Eq(DoodleState::NO_DOODLE));
+  EXPECT_FALSE(response.has_value());
+}
+
+TEST_F(DoodleFetcherTest, ResponseContainsExactlyTheSampleImages) {
+  RespondWithData(kSampleResponse);
+
+  base::Optional<DoodleConfig> response;
+  EXPECT_THAT(FetchDoodle(&response), Eq(DoodleState::AVAILABLE));
+  ASSERT_TRUE(response.has_value());
+  DoodleConfig config = response.value();
+
+  EXPECT_TRUE(config.transparent_large_image.url.is_valid());
+  EXPECT_THAT(config.transparent_large_image.url.spec(),
+              Eq(AbsoluteUrl("/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(AbsoluteUrl("/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(AbsoluteUrl("/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, RespondsToMultipleRequestsWithSameResponse) {
+  RespondWithData(kSampleResponse);
+
+  DoodleState state1;
+  DoodleState state2;
+  base::Optional<DoodleConfig> response1;
+  base::Optional<DoodleConfig> response2;
+  std::unique_ptr<DoodleFetcher> fetcher = CreateDoodleFetcher();
+
+  TriggerFetch(fetcher.get(), &state1, &response1);
+  TriggerFetch(fetcher.get(), &state2, &response2);
+
+  EXPECT_THAT(NumberOfPendingTasks(), Eq(1ul));
+  WaitForCallbacksToReturn();
+
+  EXPECT_THAT(state1, Eq(DoodleState::AVAILABLE));
+  EXPECT_THAT(state2, Eq(DoodleState::AVAILABLE));
+  EXPECT_TRUE(response1.has_value());
+  EXPECT_TRUE(response2.has_value());
+}
+
+}  // namespace doodle