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..e614d5fe1c9bba4d14f66a71bebc6fd30e4dbffc
--- /dev/null
+++ b/components/image_fetcher/image_data_fetcher_unittest.cc
@@ -0,0 +1,123 @@
+// 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");
+const char kURLResponseData[] = "EncodedImageData";
+
+}  // namespace
+
+class ImageDataFetcherTest : public testing::Test {
+ public:
+  ImageDataFetcherTest()
+      : message_loop_(),

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

Not necessary (the default ctor will be called anyway)

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

I know but it also does no harm. On the upside I find it more readable when you
have all the fields initialized here. In particular since this on is used to
initialize the next.

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

That's not really an argument for adding unnecessary bytes to the file.
The compiler will automatically generate a call to the empty constructor in the
exact same spot if you don't specify an initializer list. You can know that the
field is initialized, because the compiler will warn you if you would try to
initialize fields out of order.

[markusheintz_, 2016/06/21 15:56:08]

Removed

+        test_request_context_getter_(
+            new net::TestURLRequestContextGetter(message_loop_.task_runner())),
+        image_data_fetcher_(test_request_context_getter_.get()),
+        num_callbacks_run_(0) {}
+  ~ImageDataFetcherTest() override {}
+
+  // Callback that is called by the FetchImageData test.
+  void OnImageDataFetched(const std::string& image_data) {

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

I think it'd be more idiomatic (and easier to read) to have a
MOCK_METHOD1(OnImageDataFetched, void(const std::string&))
and then use EXPECT_CALL below.

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

Sorry I'm a bit rusty when it comes to gtest. Wouldn't this require some virtual
method I can mock?

[Marc Treib, 2016/06/20 15:21:42]

No, the mock method doesn't need to override anything. You can just define a
method with name and signature, and then EXPECT on it.
The upside is that then, the expectations would be in the test body, rather than
in some "random" method up here.

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

These callbacks could be protected.

[markusheintz_, 2016/06/21 15:56:08]

Using MOCK_METHODS now

[Bernhard Bauer, 2016/06/21 16:24:19]

They still could be protected :-D

+    EXPECT_EQ(std::string(kURLResponseData), image_data);
+  }
+
+  // Callback that is called by the FetchImageData_FailedRequest test.
+  void OnImageDataFetchedFailedRequest(const std::string& image_data) {
+    EXPECT_EQ(std::string(), image_data);
+  }
+
+  // Callback that is called by the FetchImageData_MultipleRequests test.
+  void OnImageDataFetchedMultipleRequests(const std::string& image_data) {
+    ++num_callbacks_run_;
+  }
+
+ protected:
+  base::MessageLoop message_loop_;
+
+  scoped_refptr<net::URLRequestContextGetter> test_request_context_getter_;
+
+  image_fetcher::ImageDataFetcher image_data_fetcher_;
+
+  net::TestURLFetcherFactory fetcher_factory_;
+
+  // The number of times the OnImageDataFetchedMultipleRequests callback was
+  // called during a test run.
+  int num_callbacks_run_;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ImageDataFetcherTest);
+};
+
+TEST_F(ImageDataFetcherTest, FetchImageData) {
+  image_fetcher::ImageDataFetcher::ImageDataFetcherCallback callback =

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

optional: If you don't use this again, just do the Bind directly where you need
it?

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

Done.

+      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(nullptr, test_url_fetcher);
+  test_url_fetcher->set_status(
+      net::URLRequestStatus(net::URLRequestStatus::SUCCESS, net::OK));
+  test_url_fetcher->SetResponseString(std::string(kURLResponseData));
+
+  // Call the URLFetcher delegate to continue the test.
+  test_url_fetcher->delegate()->OnURLFetchComplete(test_url_fetcher);
+}
+
+TEST_F(ImageDataFetcherTest, FetchImageData_FailedRequest) {
+  image_fetcher::ImageDataFetcher::ImageDataFetcherCallback callback =

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

Same here.

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

Done.

+      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(nullptr, test_url_fetcher);
+  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::ImageDataFetcherCallback callback1 =
+      base::Bind(&ImageDataFetcherTest::OnImageDataFetchedMultipleRequests,
+                 base::Unretained(this));
+  image_fetcher::ImageDataFetcher::ImageDataFetcherCallback callback2 =

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

You can use the same callback twice.

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

I want to simulate two individual calls. Therefore I wanted to have to separate
objects. May I'm over thinking it.

[Marc Treib, 2016/06/20 15:21:42]

You'd still check the two calls with the EXPECT_EQ(num_callbacks_run_, 2).
Anyway, not important.

[With the mock method, this would become something like:
EXPECT_CALL(*this, OnImageDataFetched(_)).Times(2);
with no need for manual tracking :)]

+      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(nullptr, test_url_fetcher);
+  test_url_fetcher->delegate()->OnURLFetchComplete(test_url_fetcher);
+
+  test_url_fetcher = fetcher_factory_.GetFetcherByID(1);
+  ASSERT_NE(nullptr, test_url_fetcher);
+  test_url_fetcher->delegate()->OnURLFetchComplete(test_url_fetcher);
+
+  EXPECT_EQ(num_callbacks_run_, 2);
+}