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..a3bad83568e47f2a164aa7262f0085ae3a16edc3 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,23 @@
 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(_, _, _, _, _, _))
+        .WillByDefault(PostReply<6>(kEmptyRawBitmapResult));
+    ON_CALL(favicon_service_, GetFaviconForPageURL(_, _, _, _, _))
+        .WillByDefault(PostReply<5>(kEmptyRawBitmapResult));
+  }
 
   ~ContentFaviconDriverTest() override {}
 
@@ -38,53 +52,74 @@ class ContentFaviconDriverTest : public content::RenderViewHostTestHarness {
         web_contents(), &favicon_service_, nullptr, nullptr);
   }
 
-  testing::StrictMock<MockFaviconService> favicon_service_;
+  content::WebContentsTester* web_contents_tester() {
+    return content::WebContentsTester::For(web_contents());
+  }
+
+  void TestFetchFaviconForPage(
+      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::NiceMock<MockFaviconService> favicon_service_;
 };
 
+TEST_F(ContentFaviconDriverTest, ShouldCacheAvailableFavicon) {

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

How about for the test comment:
"Test that UnableToDownloadFavicon() is not called as a result of a favicon
download with 200 status."

Maybe rename the test to "AbleToDownload200Status"
I have suggested new names for all of the tests. I think that the name of this
test is particularly confusing

Can you please explictly test
EXPECT_CALL(favicon_service_, UnableToDownloadFavicon(kIconURL)).Times(0);
here too?

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

Done.

See comment below about naming convention.

Besides, although the former tests didn't do this, I suspect we should rather
extend the tests to actually return bitmaps and make sure some key
FaviconService functions are exercised like SetFavicon.

[pkotwicz, 2017/02/27 15:29:07]

It is weird that FaviconService::WasUnableToDownloadFavicon() is called by
FaviconHandler but FaviconService::UnableToDownloadFavicon() is called by
ContentFaviconDriver. I think we can get a better test once FaviconHandler does
both

[mastiz, 2017/02/27 21:05:57]

Acknowledged.

+  EXPECT_CALL(favicon_service_, UpdateFaviconMappingsAndFetch(
+                                    kPageURL, std::vector<GURL>{kIconURL},
+                                    content::FaviconURL::FAVICON, _, _, _));
+  // Mimic a page load.
+  TestFetchFaviconForPage(
+      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));
+}
+

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

How about for the test comment:
"Test that UnableToDownload() is called as result of a favicon download with 404
status."

Maybe rename the test to "UnableToDownload404Status"

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

Done. However, I must say that I like the ShouldXXX notation much better, since
it puts the author in a mood to focus on actual expectations/requirements. This
convention is quite popular out there but unfortunately less so within chromium.
WDYT?

[mastiz, 2017/02/27 21:05:56]

Ping? Should I rename tests back to ShouldXXX form or is it fine in the current
form?

[pkotwicz, 2017/02/27 23:58:21]

The current form of this particular test is
ShouldCallUnableToDownloadFaviconFor404() which I think is fine

AbleToDownload200Status should probably be
ShouldNotCallUnableToDownloadFaviconFor200() to match the other tests

The name NotRequestRepeatedlyIfUnavailable() is fine (though I don't mind if you
change it back to ShouldNotRequestRepeatedlyIfUnavailable())

+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.
+  TestFetchFaviconForPage(
+      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));
+}
+

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

How about for the test comment:
"Test that UnableToDownloadFavicon() is not called as a result of a favicon
download with 503 status."

Maybe rename the test to "AbleToDownload503Status"

You can remove the comment on line 101 :)

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

Done. Ditto about the naming convention.

[pkotwicz, 2017/02/27 15:29:07]

I don't mind the test names:
- ShouldNotCallUnableToDownloadFaviconFor503()
- ShouldNotReportUnableToDownloadFor503()

ShouldNotCacheMissingFaviconFor503() is confusing. The fact that
FaviconService::UnableToDownloadFavicon() caches the fact that the favicon could
not be downloaded for a specific URL is an implementation detail of
FaviconService::UnableToDownloadFavicon()

[mastiz, 2017/02/27 21:05:57]

Done, renamed.

+TEST_F(ContentFaviconDriverTest, ShouldNotCacheMissingFaviconFor503) {
+  // No calls expected to UnableToDownloadFavicon() because of HTTP 503 status.
+  EXPECT_CALL(favicon_service_, UnableToDownloadFavicon(kIconURL)).Times(0);
+  // Mimic a page load.
+  TestFetchFaviconForPage(
+      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))
-      .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_);
+// the favicon is known to be unavailable (e.g. due to HTTP 404 status).
+TEST_F(ContentFaviconDriverTest, ShouldNotRepequestRepeatedlyIfUnavailable) {
+  ON_CALL(favicon_service_, WasUnableToDownloadFavicon(kIconURL))
+      .WillByDefault(Return(true));
+  // Mimic a page load.
+  TestFetchFaviconForPage(
+      kPageURL, {content::FaviconURL(kIconURL, content::FaviconURL::FAVICON,
+                                     kEmptyIconSizes)});
+  // Verify that no download request is pending for the image.
+  EXPECT_FALSE(web_contents_tester()->HasPendingDownloadImage(kIconURL));
 }
 
 // Test that ContentFaviconDriver ignores updated favicon URLs if there is no