Improve test coverage for ContentFaviconDriver

components/favicon/content/content_favicon_driver_unittest.cc

 // Copyright 2015 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/favicon/content/content_favicon_driver.h"
 
 #include <memory>
 #include <vector>
 
 #include "base/macros.h"
+#include "base/run_loop.h"
 #include "components/favicon/core/favicon_client.h"
+#include "components/favicon/core/favicon_handler.h"
 #include "components/favicon/core/test/mock_favicon_service.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "content/public/common/favicon_url.h"
 #include "content/public/test/test_renderer_host.h"
+#include "content/public/test/web_contents_tester.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "ui/gfx/favicon_size.h"
 
 namespace favicon {
 namespace {
 
-using testing::Mock;
 using testing::Return;
+using testing::_;
 
 class ContentFaviconDriverTest : public content::RenderViewHostTestHarness {
  protected:
-  ContentFaviconDriverTest() {}
+  const std::vector<gfx::Size> kEmptyIconSizes;
+  const std::vector<SkBitmap> kEmptyIcons;
+  const std::vector<favicon_base::FaviconRawBitmapResult> kEmptyRawBitmapResult;
+  const GURL kPageUrl = GURL("http://www.google.com/");
+  const GURL kIconUrl = GURL("http://www.google.com/favicon.ico");
+
+  ContentFaviconDriverTest() {
+    ON_CALL(favicon_service_,
+            UpdateFaviconMappingsAndFetch(kPageUrl, std::vector<GURL>{kIconUrl},
+                                          _, _, _, _))

[pkotwicz, 2017/02/22 19:34:16]

Might as well make all of the parameters testing::_

[mastiz, 2017/02/22 20:53:13]

Done.

+        .WillByDefault(PostReply<6>(kEmptyRawBitmapResult));
+    // Let's be "nice" about reads, to avoid boilerplate in tests.
+    EXPECT_CALL(favicon_service_, WasUnableToDownloadFavicon(kIconUrl))
+        .WillRepeatedly(Return(false));
+    EXPECT_CALL(favicon_service_, GetFaviconForPageURL(kPageUrl, _, _, _, _))

[pkotwicz, 2017/02/22 19:34:16]

Might as well make all of the parameters testing::_

[mastiz, 2017/02/22 20:53:13]

Done.

+        .WillRepeatedly(PostReply<5>(kEmptyRawBitmapResult));

[pkotwicz, 2017/02/22 19:34:16]

Can't these be all ON_CALL() ?

I think that the EXPECT_CALL()s are required by StrictMock. FaviconService
should be a NiceMock not a StrictMock.

[mastiz, 2017/02/22 20:53:13]

Done, adopted NiceMock.

+  }
 
   ~ContentFaviconDriverTest() override {}
 
   // content::RenderViewHostTestHarness:
   void SetUp() override {
     RenderViewHostTestHarness::SetUp();
 
     ContentFaviconDriver::CreateForWebContents(
         web_contents(), &favicon_service_, nullptr, nullptr);
   }
 
+  content::WebContentsTester* web_contents_tester() {
+    return content::WebContentsTester::For(web_contents());
+  }
+

[pkotwicz, 2017/02/22 19:34:16]

Maybe rename this function to FetchFaviconForPage()

[mastiz, 2017/02/22 20:53:13]

Renamed to TestFetchFaviconForPage because otherwise I feel it can be harder to
distinguish from methods in non-test classes. WDYT?

[pkotwicz, 2017/02/23 16:20:49]

I had not thought about making it obvious which methods are test methods and
which are not in the past. TestFetchFaviconForPage() is great :)

+  void TestDidLoadPage(const GURL& page_url,
+                       const std::vector<content::FaviconURL>& candidates) {
+    ContentFaviconDriver* favicon_driver =
+        ContentFaviconDriver::FromWebContents(web_contents());
+    web_contents_tester()->NavigateAndCommit(page_url);
+    static_cast<content::WebContentsObserver*>(favicon_driver)
+        ->DidUpdateFaviconURL(candidates);
+    base::RunLoop().RunUntilIdle();
+  }
+
   testing::StrictMock<MockFaviconService> favicon_service_;
 };
 
+TEST_F(ContentFaviconDriverTest, ShouldCacheAvailableFavicon) {
+  EXPECT_CALL(favicon_service_, UpdateFaviconMappingsAndFetch(
+                                    kPageUrl, std::vector<GURL>{kIconUrl},
+                                    content::FaviconURL::FAVICON, _, _, _));
+  // Mimic a page load.
+  TestDidLoadPage(kPageUrl,
+                  {content::FaviconURL(kIconUrl, content::FaviconURL::FAVICON,
+                                       kEmptyIconSizes)});
+  // Mimic the completion of an image download.
+  EXPECT_TRUE(web_contents_tester()->TestDidDownloadImage(
+      kIconUrl, 200, kEmptyIcons, kEmptyIconSizes));
+}
+
+TEST_F(ContentFaviconDriverTest, ShouldCacheMissingFaviconFor404) {
+  EXPECT_CALL(favicon_service_, UpdateFaviconMappingsAndFetch(
+                                    kPageUrl, std::vector<GURL>{kIconUrl},
+                                    content::FaviconURL::FAVICON, _, _, _));
+  EXPECT_CALL(favicon_service_, UnableToDownloadFavicon(kIconUrl));
+  // Mimic a page load.
+  TestDidLoadPage(kPageUrl,
+                  {content::FaviconURL(kIconUrl, content::FaviconURL::FAVICON,
+                                       kEmptyIconSizes)});
+  // Mimic the completion of an image download.
+  EXPECT_TRUE(web_contents_tester()->TestDidDownloadImage(
+      kIconUrl, 404, kEmptyIcons, kEmptyIconSizes));
+}
+
+TEST_F(ContentFaviconDriverTest, ShouldNotCacheMissingFaviconFor503) {
+  // No calls expected to UnableToDownloadFavicon() because of HTTP 503 status.
+  EXPECT_CALL(favicon_service_, UpdateFaviconMappingsAndFetch(
+                                    kPageUrl, std::vector<GURL>{kIconUrl},
+                                    content::FaviconURL::FAVICON, _, _, _));

[pkotwicz, 2017/02/22 19:34:16]

This is not testing for calls for UnableToDownloadFavicon(). This test should
test that UnableToDownloadFavicon() is not called.

Testing that UpdateFaviconMappingsAndFetch() is called is not interesting from
the perspective of this test
I think that this is here because it is required by StrictMock. FaviconService
should be a NiceMock not a StrictMock. I was confused why this test was checking
that FaviconService::UpdateFaviconMappingsAndFetch() is called 

[mastiz, 2017/02/22 20:53:13]

The test was indeed testing that no calls were made to UnableToDownloadFavicon()
via StrictMock, but it's now more explicit. 

Removed UpdateFaviconMappingsAndFetch(), let me know if you want it removed from
other tests as well.

[pkotwicz, 2017/02/23 16:20:49]

Thank you for making the check for UnableToDownloadFavicon() not being called
more explicit

Yes please remove the UpdateFaviconMappingsAndFetch() check from the other tests
as well 

[mastiz, 2017/02/24 21:15:38]

Done.

+  // Mimic a page load.
+  TestDidLoadPage(kPageUrl,
+                  {content::FaviconURL(kIconUrl, content::FaviconURL::FAVICON,
+                                       kEmptyIconSizes)});
+  // Mimic the completion of an image download.
+  EXPECT_TRUE(web_contents_tester()->TestDidDownloadImage(
+      kIconUrl, 503, kEmptyIcons, kEmptyIconSizes));
+}
+
 // Test that Favicon is not requested repeatedly during the same session if
-// server returns HTTP 404 status.
-TEST_F(ContentFaviconDriverTest, UnableToDownloadFavicon) {
-  const GURL missing_icon_url("http://www.google.com/favicon.ico");
-
-  ContentFaviconDriver* content_favicon_driver =
-      ContentFaviconDriver::FromWebContents(web_contents());
-
-  std::vector<SkBitmap> empty_icons;
-  std::vector<gfx::Size> empty_icon_sizes;
-  int download_id = 0;
-
-  // Try to download missing icon.
-  EXPECT_CALL(favicon_service_, WasUnableToDownloadFavicon(missing_icon_url))
-      .WillOnce(Return(false));
-  download_id = content_favicon_driver->StartDownload(missing_icon_url, 0);
-  EXPECT_NE(0, download_id);
-
-  // Report download failure with HTTP 503 status.
-  content_favicon_driver->DidDownloadFavicon(download_id, 503, missing_icon_url,
-                                             empty_icons, empty_icon_sizes);
-  Mock::VerifyAndClearExpectations(&favicon_service_);
-
-  // Try to download again.
-  EXPECT_CALL(favicon_service_, WasUnableToDownloadFavicon(missing_icon_url))
-      .WillOnce(Return(false));
-  download_id = content_favicon_driver->StartDownload(missing_icon_url, 0);
-  EXPECT_NE(0, download_id);
-  Mock::VerifyAndClearExpectations(&favicon_service_);
-
-  // Report download failure with HTTP 404 status, which causes the icon to be
-  // marked as UnableToDownload.
-  EXPECT_CALL(favicon_service_, UnableToDownloadFavicon(missing_icon_url));
-  content_favicon_driver->DidDownloadFavicon(download_id, 404, missing_icon_url,
-                                             empty_icons, empty_icon_sizes);
-  Mock::VerifyAndClearExpectations(&favicon_service_);
-
-  // Try to download again.
-  EXPECT_CALL(favicon_service_, WasUnableToDownloadFavicon(missing_icon_url))
+// the favicon is known to be unavailable (e.g. due to HTTP 404 status).
+TEST_F(ContentFaviconDriverTest, ShouldNotRepequestRepeatedlyIfUnavailable) {
+  EXPECT_CALL(favicon_service_, WasUnableToDownloadFavicon(kIconUrl))

[pkotwicz, 2017/02/22 19:34:16]

This should be ON_CALL()

[mastiz, 2017/02/22 20:53:13]

Done.

       .WillOnce(Return(true));
-  download_id = content_favicon_driver->StartDownload(missing_icon_url, 0);
-  // Download is not started and Icon is still marked as UnableToDownload.
-  EXPECT_EQ(0, download_id);
-  Mock::VerifyAndClearExpectations(&favicon_service_);
+  EXPECT_CALL(favicon_service_, UpdateFaviconMappingsAndFetch(
+                                    kPageUrl, std::vector<GURL>{kIconUrl},
+                                    content::FaviconURL::FAVICON, _, _, _));
+  // Mimic a page load.
+  TestDidLoadPage(kPageUrl,
+                  {content::FaviconURL(kIconUrl, content::FaviconURL::FAVICON,
+                                       kEmptyIconSizes)});
+  // Verify that no download request is pending for the image.
+  EXPECT_FALSE(web_contents_tester()->TestDidDownloadImage(
+      kIconUrl, 200, kEmptyIcons, kEmptyIconSizes));

[pkotwicz, 2017/02/22 19:34:16]

Can we have TestWebContents::DidRequestDownload() and call that instead?

The parameters are not useful in any of the tests in this file (Yes, I know that
you are planning on using them in tests in another CL)

[mastiz, 2017/02/22 20:53:13]

Done.

 }
 
 // Test that ContentFaviconDriver ignores updated favicon URLs if there is no
 // last committed entry. This occurs when script is injected in about:blank.
 // See crbug.com/520759 for more details
 TEST_F(ContentFaviconDriverTest, FaviconUpdateNoLastCommittedEntry) {
   ASSERT_EQ(nullptr, web_contents()->GetController().GetLastCommittedEntry());
 
   std::vector<content::FaviconURL> favicon_urls;
   favicon_urls.push_back(content::FaviconURL(
       GURL("http://www.google.ca/favicon.ico"), content::FaviconURL::FAVICON,
       std::vector<gfx::Size>()));
   favicon::ContentFaviconDriver* driver =
       favicon::ContentFaviconDriver::FromWebContents(web_contents());
   static_cast<content::WebContentsObserver*>(driver)
       ->DidUpdateFaviconURL(favicon_urls);
 
   // Test that ContentFaviconDriver ignored the favicon url update.
   EXPECT_TRUE(driver->favicon_urls().empty());
 }
 
 }  // namespace
 }  // namespace favicon