[Doodle] Move image fetching from LogoBridge to DoodleService

components/doodle/doodle_service_unittest.cc

 // Copyright 2017 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_service.h"
 
 #include <memory>
 #include <utility>
 #include <vector>
 
 #include "base/bind.h"
 #include "base/memory/ptr_util.h"
 #include "base/memory/ref_counted.h"
 #include "base/test/histogram_tester.h"
+#include "base/test/mock_callback.h"
 #include "base/test/simple_test_tick_clock.h"
 #include "base/test/test_mock_time_task_runner.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/time/time.h"
+#include "components/image_fetcher/core/image_fetcher.h"
+#include "components/image_fetcher/core/request_metadata.h"
 #include "components/prefs/testing_pref_service.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "ui/gfx/geometry/size.h"
+#include "ui/gfx/image/image.h"
+#include "ui/gfx/image/image_unittest_util.h"
 
+using image_fetcher::ImageFetcher;
+using image_fetcher::RequestMetadata;
+using testing::_;
 using testing::Eq;
 using testing::StrictMock;
+using testing::Not;
 
 namespace doodle {
 
 namespace {
 
 class FakeDoodleFetcher : public DoodleFetcher {
  public:
   FakeDoodleFetcher() = default;
   ~FakeDoodleFetcher() override = default;
 
@@ skipping 18 matching lines @@
   std::vector<FinishedCallback> callbacks_;
 };
 
 class MockDoodleObserver : public DoodleService::Observer {
  public:
   MOCK_METHOD1(OnDoodleConfigUpdated,
                void(const base::Optional<DoodleConfig>&));
   MOCK_METHOD1(OnDoodleConfigRevalidated, void(bool));
 };
 
+class FakeImageFetcher : public ImageFetcher {
+ public:
+  FakeImageFetcher() = default;
+  ~FakeImageFetcher() override {
+    // To make sure we didn't receive an unexpected image request.
+    CHECK(!HasPendingRequest());

[fhorschig, 2017/05/15 13:54:35]

s/CHECK/DCHECK?
Also: do we really want a crash here?
ASSERT_TRUE'ing that on TearDown would have the test fail (because the tested
code failed) and not crashing (which would imply the testing code is broken).

[Marc Treib, 2017/05/15 14:25:22]

CHECK or DCHECK doesn't really make a difference, since the trybots build with
DCHECK_ALWAYS_ON anyway. But sure, done, for consistency.

You're right that ASSERT would be better than (D)CHECK everywhere here, but
ASSERT doesn't work in all contexts, it only works in methods returning void.
And when used outside the test body itself, it might not do what you think - it
doesn't cancel the rest of the test.

Anyway. I've made this one an ASSERT in DestroyService. Just TearDown wouldn't
check if a test re-creates the service in the middle.

+  }
+
+  void SetImageFetcherDelegate(image_fetcher::ImageFetcherDelegate*) override {

[fhorschig, 2017/05/15 13:54:35]

Why not "// Ignored"?
(I am not sure, a test should _crash_ just because this function was used.)

[Marc Treib, 2017/05/15 14:25:22]

Well, if someone calls this, then they expect something that the fake doesn't
support. I'd say crashing the test is okay; I certainly want to be notified
about it in some way.

+    NOTREACHED();
+  }
+
+  void SetDataUseServiceName(DataUseServiceName) override {
+    // Ignored.
+  }
+
+  void SetImageDownloadLimit(base::Optional<int64_t>) override {
+    // Ignored.
+  }
+
+  void SetDesiredImageFrameSize(const gfx::Size&) override { NOTREACHED(); }

[fhorschig, 2017/05/15 13:54:35]

Again, why not "// Ignored"?

[Marc Treib, 2017/05/15 14:25:22]

Eh.. same argument as above, though I guess here it doesn't really matter. So,
done.

+
+  void StartOrQueueNetworkRequest(
+      const std::string& id,
+      const GURL& url,
+      const ImageFetcherCallback& callback) override {
+    // For simplicity, the fake doesn't support multiple concurrent requests.
+    CHECK(!HasPendingRequest());

[fhorschig, 2017/05/15 13:54:35]

nit: s/CHECK/DCHECK?

[Marc Treib, 2017/05/15 14:25:22]

Done.

+
+    pending_id_ = id;
+    pending_url_ = url;
+    pending_callback_ = callback;
+  }
+
+  image_fetcher::ImageDecoder* GetImageDecoder() override {
+    NOTREACHED();
+    return nullptr;
+  }
+
+  bool HasPendingRequest() const { return !pending_callback_.is_null(); }
+
+  const GURL& pending_url() const { return pending_url_; }
+
+  void RespondToPendingRequest(const gfx::Image& image) {
+    CHECK(HasPendingRequest());

[fhorschig, 2017/05/15 13:54:35]

nit: s/CHECK/DCHECK?

[Marc Treib, 2017/05/15 14:25:22]

Done.

+
+    RequestMetadata metadata;
+    metadata.http_response_code = 200;
+    pending_callback_.Run(pending_id_, image, metadata);
+
+    pending_id_.clear();
+    pending_url_ = GURL();
+    pending_callback_.Reset();
+  }
+
+ private:
+  std::string pending_id_;
+  GURL pending_url_;
+  ImageFetcherCallback pending_callback_;
+};
+
 DoodleConfig CreateConfig(DoodleType type) {
   return DoodleConfig(type, DoodleImage(GURL("https://doodle.com/image.jpg")));
 }
 
+MATCHER(IsEmptyImage, "") {
+  return arg.IsEmpty();
+}
+
 }  // namespace
 
 class DoodleServiceTest : public testing::Test {
  public:
   DoodleServiceTest()
       : task_runner_(new base::TestMockTimeTaskRunner()),
         task_runner_handle_(task_runner_),
         tick_clock_(task_runner_->GetMockTickClock()),
         fetcher_(nullptr),
         expiry_timer_(nullptr) {
     DoodleService::RegisterProfilePrefs(pref_service_.registry());
 
     task_runner_->FastForwardBy(base::TimeDelta::FromHours(12345));
 
     // Set the minimum refresh interval to 0 by default, so tests don't have to
     // worry about it. The tests that care set it explicitly.
     RecreateServiceWithZeroRefreshInterval();
   }
 
-  void DestroyService() { service_ = nullptr; }
+  void DestroyService() {
+    fetcher_ = nullptr;
+    expiry_timer_ = nullptr;
+    image_fetcher_ = nullptr;

[fhorschig, 2017/05/15 13:54:35]

This could be a good place to ASSERT HasPendingRequest ... if so, renaming this
to "VerifyAndDestroy..." or something might make sense.

[Marc Treib, 2017/05/15 14:25:22]

Yup, moved the ASSERT here. IMO the name is fine though.

+
+    service_ = nullptr;
+  }
 
   void RecreateServiceWithZeroRefreshInterval() {
     RecreateService(/*min_refresh_interval=*/base::TimeDelta());
   }
 
   void RecreateService(base::Optional<base::TimeDelta> refresh_interval) {
     auto expiry_timer = base::MakeUnique<base::OneShotTimer>(tick_clock_.get());
     expiry_timer->SetTaskRunner(task_runner_);
     expiry_timer_ = expiry_timer.get();
 
     auto fetcher = base::MakeUnique<FakeDoodleFetcher>();
     fetcher_ = fetcher.get();
 
+    auto image_fetcher = base::MakeUnique<FakeImageFetcher>();
+    image_fetcher_ = image_fetcher.get();
+
     service_ = base::MakeUnique<DoodleService>(
         &pref_service_, std::move(fetcher), std::move(expiry_timer),
         task_runner_->GetMockClock(), task_runner_->GetMockTickClock(),
-        refresh_interval);
+        refresh_interval, std::move(image_fetcher));
   }
 
   DoodleService* service() { return service_.get(); }
   FakeDoodleFetcher* fetcher() { return fetcher_; }
+  FakeImageFetcher* image_fetcher() { return image_fetcher_; }
 
   base::TestMockTimeTaskRunner* task_runner() { return task_runner_.get(); }
 
  private:
   TestingPrefServiceSimple pref_service_;
 
   scoped_refptr<base::TestMockTimeTaskRunner> task_runner_;
   base::ThreadTaskRunnerHandle task_runner_handle_;
   std::unique_ptr<base::TickClock> tick_clock_;
 
   std::unique_ptr<DoodleService> service_;
 
   // Weak, owned by the service.
   FakeDoodleFetcher* fetcher_;
   base::OneShotTimer* expiry_timer_;
+  FakeImageFetcher* image_fetcher_;
 };
 
 TEST_F(DoodleServiceTest, FetchesConfigOnRefresh) {
   ASSERT_THAT(service()->config(), Eq(base::nullopt));
 
   // Request a refresh of the doodle config.
   service()->Refresh();
   // The request should have arrived at the fetcher.
   EXPECT_THAT(fetcher()->num_pending_callbacks(), Eq(1u));
 
@@ skipping 457 matching lines @@
 
   // Fast-forward time so that the config expires.
   task_runner()->FastForwardBy(base::TimeDelta::FromHours(1));
   ASSERT_THAT(service()->config(), Eq(base::nullopt));
 
   // This should not have resulted in any metrics being emitted.
   histograms.ExpectTotalCount("Doodle.ConfigDownloadOutcome", 0);
   histograms.ExpectTotalCount("Doodle.ConfigDownloadTime", 0);
 }
 
+TEST_F(DoodleServiceTest, GetImageWithEmptyConfigReturnsImmediately) {
+  ASSERT_THAT(service()->config(), Eq(base::nullopt));
+
+  base::MockCallback<DoodleService::ImageCallback> callback;
+  EXPECT_CALL(callback, Run(IsEmptyImage()));
+
+  service()->GetImage(callback.Get());

[fhorschig, 2017/05/15 13:54:35]

ASSERT_FALSE(HasPendingRequest) might ensure this doesn't accidentally pass if
it doesn't return immediately but fails instead.

[Marc Treib, 2017/05/15 14:25:22]

Er.. what?

Anyway, that assert happens during TearDown now.

+}
+
+TEST_F(DoodleServiceTest, GetImageFetchesLargeImage) {
+  service()->Refresh();
+  DoodleConfig config = CreateConfig(DoodleType::SIMPLE);
+  fetcher()->ServeAllCallbacks(DoodleState::AVAILABLE,
+                               base::TimeDelta::FromHours(1), config);
+  ASSERT_THAT(service()->config(), Eq(config));
+
+  base::MockCallback<DoodleService::ImageCallback> callback;
+  service()->GetImage(callback.Get());
+
+  EXPECT_EQ(config.large_image.url, image_fetcher()->pending_url());
+
+  EXPECT_CALL(callback, Run(Not(IsEmptyImage())));
+  gfx::Image image = gfx::test::CreateImage(1, 1);
+  image_fetcher()->RespondToPendingRequest(image);

[fhorschig, 2017/05/15 13:54:35]

ASSERT_TRUE(HasPendingRequest) before that would have the test fail and not
crash.

[Marc Treib, 2017/05/15 14:25:22]

Done.

+}
+
+TEST_F(DoodleServiceTest, GetImageFetchesCTAImage) {
+  service()->Refresh();
+  DoodleConfig config = CreateConfig(DoodleType::SIMPLE);
+  // Set a CTA image, which should take precedence over the regular image.
+  config.large_image.is_animated_gif = true;
+  config.large_cta_image = DoodleImage(GURL("https://doodle.com/cta.jpg"));
+  config.large_cta_image->is_cta = true;
+  fetcher()->ServeAllCallbacks(DoodleState::AVAILABLE,
+                               base::TimeDelta::FromHours(1), config);
+  ASSERT_THAT(service()->config(), Eq(config));
+
+  base::MockCallback<DoodleService::ImageCallback> callback;
+  service()->GetImage(callback.Get());
+
+  // If the doodle has a CTA image, that should loaded instead of the regular
+  // large image.
+  EXPECT_EQ(config.large_cta_image->url, image_fetcher()->pending_url());
+
+  EXPECT_CALL(callback, Run(Not(IsEmptyImage())));
+  gfx::Image image = gfx::test::CreateImage(1, 1);
+  image_fetcher()->RespondToPendingRequest(image);

[fhorschig, 2017/05/15 13:54:35]

again ASSERT_FALSE(HasPendingRequest)?

[Marc Treib, 2017/05/15 14:25:22]

ASSERT_TRUE :)  but done.

+}
+
 }  // namespace doodle