Add a unittest for image_data_fetcher

components/image_fetcher/image_data_fetcher_unittest.cc

Index: components/image_fetcher/image_data_fetcher_unittest.cc
diff --git a/components/image_fetcher/image_data_fetcher_unittest.cc b/components/image_fetcher/image_data_fetcher_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..26f3cec95b09aa1135afd974bbdd0e2858aca870
--- /dev/null
+++ b/components/image_fetcher/image_data_fetcher_unittest.cc
@@ -0,0 +1,133 @@
+// 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/image_fetcher/image_data_fetcher.h"
+
+#include <memory>
+
+#include "base/callback.h"
+#include "base/macros.h"
+#include "base/memory/ref_counted.h"
+#include "base/message_loop/message_loop.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/gtest/include/gtest/gtest.h"
+
+namespace {
+
+const GURL kImageURL("http://www.example.com/image1");
+std::string kURLResponseData("EncodedImageData");

[Bernhard Bauer, 2016/06/20 13:36:16]

These will create static initializers. What you want to do is store the literals
as Plain Old Data, i.e. const char[].

[markusheintz_, 2016/06/20 14:30:17]

Done.

+
+}  // namespace
+
+class ImageDataFetcherTest : public testing::Test {
+ public:
+  ImageDataFetcherTest() : num_callbacks_run_(0) {}
+  ~ImageDataFetcherTest() override {}
+
+  // testing::Test overrides:
+  void SetUp() override {
+    test_request_context_getter_ =
+        new net::TestURLRequestContextGetter(message_loop_.task_runner());
+
+    fetcher_factory_.reset(new net::TestURLFetcherFactory());

[Bernhard Bauer, 2016/06/20 13:36:16]

You could make this a direct member of the fixture.

[markusheintz_, 2016/06/20 14:30:17]

Done.

+
+    num_callbacks_run_ = 0;

[Bernhard Bauer, 2016/06/20 13:36:15]

You don't need this -- you get a fresh test fixture instance for every test.

[markusheintz_, 2016/06/20 14:30:17]

Removed

+  }
+
+  // Callback that is called by the FetchImageData test.
+  void OnImageDataFetched(const std::string& image_data) {
+    EXPECT_EQ(image_data, kURLResponseData);

[Bernhard Bauer, 2016/06/20 13:36:15]

The expected value should go first (for nicer error messages on failure).

[markusheintz_, 2016/06/20 14:30:17]

Done.

+  }
+
+  // Callback that is called by the FetchImageData_FailedRequest test.
+  void OnImageDataFetchedFailedRequest(const std::string& image_data) {
+    EXPECT_EQ(image_data, std::string(""));

[Bernhard Bauer, 2016/06/20 13:36:16]

The empty string literal in the constructor is unnecessary.

[markusheintz_, 2016/06/20 14:30:17]

Done.

+  }
+
+  // Callback that is called by the FetchImageData_MultipleRequests test.
+  void OnImageDataFetchedMultipleRequests(const std::string& image_data) {
+    ++num_callbacks_run_;

[Bernhard Bauer, 2016/06/20 13:36:16]

Aren't you going to verify that data that comes back?

[markusheintz_, 2016/06/20 14:30:17]

That's what the other two tests are for. 

Here I only want to test that multiple calls to FetchImageData result in
multiple calls to callbacks. There is no batching implemented like in other
places.

+  }
+
+ protected:
+  base::MessageLoop message_loop_;
+
+  scoped_refptr<net::URLRequestContextGetter> test_request_context_getter_;
+
+  std::unique_ptr<net::TestURLFetcherFactory> fetcher_factory_;
+
+  // The number of times the OnImageDataFetchedMultipleRequests callback was
+  // called during a testrun. This counter is reset duing the SetUp method.

[Bernhard Bauer, 2016/06/20 13:36:16]

"during the SetUp method" (but really, the comment is moot; see my comment
above).

[markusheintz_, 2016/06/20 14:30:17]

removed

+  int num_callbacks_run_;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ImageDataFetcherTest);
+};
+
+TEST_F(ImageDataFetcherTest, FetchImageData) {
+  image_fetcher::ImageDataFetcher image_data_fetcher(

[Bernhard Bauer, 2016/06/20 13:36:16]

You could make this part of the fixture, as you create one in every test.

[markusheintz_, 2016/06/20 14:30:17]

Done. 

[Marc Treib, 2016/06/20 14:47:29]

You could put this whole file into the image_fetcher namespace.

[markusheintz_, 2016/06/20 15:06:52]

Done.

+      test_request_context_getter_.get());
+  image_fetcher::ImageDataFetcher::ImageDataFetcherCallback callback =
+      base::Bind(&ImageDataFetcherTest::OnImageDataFetched,
+                 base::Unretained(this));
+
+  image_data_fetcher.FetchImageData(kImageURL, callback);
+
+  // Get and configure the TestURLFetcher.
+  net::TestURLFetcher* test_url_fetcher = fetcher_factory_->GetFetcherByID(0);
+  ASSERT_NE(test_url_fetcher, nullptr);

[Bernhard Bauer, 2016/06/20 13:36:16]

Expected value first.

[markusheintz_, 2016/06/20 14:30:17]

Done. Here and below

+  test_url_fetcher->set_status(
+      net::URLRequestStatus(net::URLRequestStatus::SUCCESS, net::OK));
+  test_url_fetcher->SetResponseString(kURLResponseData);
+
+  // Call the URLFetcher delegate to continue the test.

[Bernhard Bauer, 2016/06/20 13:36:16]

And then what? Don't you want to verify that the fetcher actually finished?

[markusheintz_, 2016/06/20 14:30:17]

You mean whether I want to test the TestURLFetcher? ;-)

When the callbacks above are called, then the fetcher completed one or the other
way.
The comment in the TestURLFetcher code does not mention any further things that
need to be run in order to make URLFetcher run to completion.

[Bernhard Bauer, 2016/06/20 17:28:38]

Yes, but how do you know the callbacks above are actually called? :)

+  test_url_fetcher->delegate()->OnURLFetchComplete(test_url_fetcher);
+}
+
+TEST_F(ImageDataFetcherTest, FetchImageData_FailedRequest) {
+  image_fetcher::ImageDataFetcher image_data_fetcher(
+      test_request_context_getter_.get());
+  image_fetcher::ImageDataFetcher::ImageDataFetcherCallback callback =
+      base::Bind(&ImageDataFetcherTest::OnImageDataFetchedFailedRequest,
+                 base::Unretained(this));
+
+  image_data_fetcher.FetchImageData(kImageURL, callback);
+
+  // Get and configure the TestURLFetcher.
+  net::TestURLFetcher* test_url_fetcher = fetcher_factory_->GetFetcherByID(0);
+  ASSERT_NE(test_url_fetcher, nullptr);
+  test_url_fetcher->set_status(
+      net::URLRequestStatus(net::URLRequestStatus::FAILED,
+                            net::ERR_INVALID_URL));
+
+  // Call the URLFetcher delegate to continue the test.
+  test_url_fetcher->delegate()->OnURLFetchComplete(test_url_fetcher);
+}
+
+TEST_F(ImageDataFetcherTest, FetchImageData_MultipleRequests) {
+  image_fetcher::ImageDataFetcher image_data_fetcher(
+      test_request_context_getter_.get());
+
+  image_fetcher::ImageDataFetcher::ImageDataFetcherCallback callback1 =
+      base::Bind(&ImageDataFetcherTest::OnImageDataFetchedMultipleRequests,
+                 base::Unretained(this));
+  image_fetcher::ImageDataFetcher::ImageDataFetcherCallback callback2 =
+      base::Bind(&ImageDataFetcherTest::OnImageDataFetchedMultipleRequests,
+                 base::Unretained(this));
+
+  image_data_fetcher.FetchImageData(kImageURL, callback1);
+  image_data_fetcher.FetchImageData(kImageURL, callback2);
+
+  net::TestURLFetcher* test_url_fetcher = fetcher_factory_->GetFetcherByID(0);
+  ASSERT_NE(test_url_fetcher, nullptr);
+  test_url_fetcher->delegate()->OnURLFetchComplete(test_url_fetcher);
+
+  test_url_fetcher = fetcher_factory_->GetFetcherByID(1);
+  ASSERT_NE(test_url_fetcher, nullptr);
+  test_url_fetcher->delegate()->OnURLFetchComplete(test_url_fetcher);
+
+  EXPECT_EQ(num_callbacks_run_, 2);
+}