Improve test coverage for ContentFaviconDriver

components/favicon/content/content_favicon_driver_unittest.cc

Index: components/favicon/content/content_favicon_driver_unittest.cc
diff --git a/components/favicon/content/content_favicon_driver_unittest.cc b/components/favicon/content/content_favicon_driver_unittest.cc
index 0c75c52a22d38e76bbd200648c50ee2be985e97e..2288d3dd06954f65e685c8110eaa0b77c06ca7c8 100644
--- a/components/favicon/content/content_favicon_driver_unittest.cc
+++ b/components/favicon/content/content_favicon_driver_unittest.cc
@@ -8,11 +8,14 @@
 #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"
@@ -21,12 +24,28 @@
 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 {}
 
@@ -38,53 +57,79 @@ class ContentFaviconDriverTest : public content::RenderViewHostTestHarness {
         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