Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2697803003:
Improve test coverage for ContentFaviconDriver (Closed)
DescriptionImprove test coverage for ContentFaviconDriver
We need this prior to adding new features to the class hierarchy, most
notably the support for multiple favicons of the same type (i.e. for
different resolutions).
The main issue with the former tests is that they don't exercise the
public API, but instead parts of the implementation.
In order to do this, TestWebContents must be extended with basic
support for testing DownloadImage().
BUG=694312
Review-Url: https://codereview.chromium.org/2697803003
Cr-Commit-Position: refs/heads/master@{#453680}
Committed: https://chromium.googlesource.com/chromium/src/+/ba3a4d8ad97649db9ce47979a18f93fd07b0499e
Patch Set 1 #Patch Set 2 : WIP iOS #Patch Set 3 : Formatting. #
Total comments: 7
Patch Set 4 : Rebased #Patch Set 5 : Rebased. #Patch Set 6 : Rebased. #
Total comments: 1
Patch Set 7 : Rebased. #Patch Set 8 : Use forward declarations #Patch Set 9 : Rebased. #Patch Set 10 : Adopt PostReply. #Patch Set 11 : Simplify tests. #Patch Set 12 : Minor revert. #
Total comments: 17
Patch Set 13 : Addressed comments. #Patch Set 14 : Addressed comments. #Patch Set 15 : Trivial rename. #Patch Set 16 : Trivial rename. #
Total comments: 12
Patch Set 17 : Addressed comments. #Patch Set 18 : Addressed comments. #
Total comments: 6
Patch Set 19 : Addressed more comments. #Patch Set 20 : Minor test renames. #
Messages
Total messages: 69 (49 generated)
The CQ bit was checked by mastiz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
The CQ bit was checked by mastiz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
mastiz@chromium.org changed reviewers: + pkotwicz@chromium.org
pkotwicz@: some iOS tests need to be updated but otherwise this looks ready for review. I can also split the CL into two or three patches, but providing the big picture seems useful to me.
At a high level: - Making FaviconService an interface sounds good to me - It is not my call to make whether adding TestWebContents::TestDidDownloadImage() is OK. I suspect that it is - Can you rewrite a couple of the tests in FaviconHandlerTest? Being able to rewrite a few of the tests (and eventually all of them) in a way which is clearer would convince me that this refactoring is helpful. If you want you can put these tests in favicon_handler_unittest2.cc for now. Doing this exercise will shape what the interface should be for MockFaviconService. Sorry for making you do all of this work ahead of time https://codereview.chromium.org/2697803003/diff/40001/components/favicon/cont... File components/favicon/content/content_favicon_driver_unittest.cc (right): https://codereview.chromium.org/2697803003/diff/40001/components/favicon/cont... components/favicon/content/content_favicon_driver_unittest.cc:77: .WillOnce(DoAll(SaveArg<3>(&get_favicon_callback), Return(1))); Testing the returned base::CancelableTaskTracker::TaskId from FaviconService::GetFaviconForPageURL() is uninteresting in my opinion https://codereview.chromium.org/2697803003/diff/40001/components/favicon/cont... components/favicon/content/content_favicon_driver_unittest.cc:96: update_mappings_callback.Run(kEmptyRawBitmapResult); Random Comment #1: ContentFaviconDriver is not used on iOS. So we unfortunately cannot move all of the tests from FaviconHandler here Random comment #2: In an ideal world, your mock FaviconService would be more complicated so that this call and the one on line 92 would be unnecessary in the test case. Lines 92 and 96 are testing the implementation. I am mostly envisioning this for the FaviconHandler tests I am envisioning something like this: (This is very rough. There is bound to be something that I am missing) MockFaviconService { public: void Store(const GURL& page_url, const Icon& icon); void AddMapping(const GURL& page_url, const GURL& icon_url); Icon* GetIcon(const GURL& icon_url); std::vector<GURL> GetPageMappingsForIconURL(const GURL& icon_url); // The number of icons in the database int num_icons_stored() const; // The number of icon mappings in the database int num_icon_mappings() const; // Whether to run the callbacks for FaviconService::GetFavicon() right away or to queue them. // This is necessary for testing race conditions between FaviconService::GetFaviconForURLFromFaviconService() and FaviconHandler::OnUpdateFaviconURL() void SetQueueCallbacks(bool queue) // Runs all pending callbacks which were queued after call to SetQueueCallbacks(true) void RunPendingCallbacks() // FaviconService overrides: ... } class Icon { public: Icon* Create(const GURL& icon_url) {} Icon* CreateWithEdgeSize(const GURL& icon_url, int edge_size) {} GURL icon_url; int edge_size; bool expired; scoped_refptr<base::RefCountedMemory> bitmap_data; }
The CQ bit was checked by mastiz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Improve test coverage for FaviconDriver class hierarchy We need this prior to adding new features to the class hierarchy, most notably the support for multiple favicons of the same type (i.e. for different resolutions). There's two main issues with the former tests: 1. They don't exercise the public API, but instead parts of the implementation. 2. They don't inject any FaviconService, but just nullptr, which gets special-cased in the class implementation. We address these two which requires: A. Splitting FaviconService (interface) and FaviconServiceImpl, so tests can use mocks without subclassing from the service or special-casing null. B. Extending TestWebContents with support for image downloads. The result is most interesting in ContentFaviconDriver, where the new tests document more accurately the actual scenarios that we want to support. MockFaviconService is adopted in two unittest files for now, but there's a few leftovers (e.g. FaviconHandler) that will be migrated in follow-up patches. BUG= ========== to ========== Improve test coverage for FaviconDriver class hierarchy We need this prior to adding new features to the class hierarchy, most notably the support for multiple favicons of the same type (i.e. for different resolutions). The main issue with the former tests is that they don't exercise the public API, but instead parts of the implementation. In order to do this, TestWebContents must be extended with basic support for testing DownloadImage(). BUG= ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
PTAL! On 2017/02/15 04:43:00, pkotwicz wrote: > At a high level: > - Making FaviconService an interface sounds good to me As an attempt to do progress, I split this to https://codereview.chromium.org/2698473004/ > - It is not my call to make whether adding > TestWebContents::TestDidDownloadImage() is OK. I suspect that it is > - Can you rewrite a couple of the tests in FaviconHandlerTest? Being able to > rewrite a few of the tests (and eventually all of them) in a way which is > clearer would convince me that this refactoring is helpful. I'm not sure if you're referring to the refactoring of FaviconHandler in https://codereview.chromium.org/2691933004/? I have the impression the test coverage proposed in these tests is clearly an improvement, regardless of what happens with FaviconHandler. Are you not convinced about this? > If you want you can put these tests in favicon_handler_unittest2.cc for now. Doing this exercise > will shape what the interface should be for MockFaviconService. Sorry for making > you do all of this work ahead of time Unfortunately, updating FaviconHandler tests is quite painful and hence I'd prefer leaving outside the scope of this CL, mainly because the class needs a rewrite, at least the removal of virtual functions overriden in TestFaviconHandler (to avoid exercising the injected dependencies). If you agree, I'd rather stack test updates after the refactoring of the class on top of https://codereview.chromium.org/2691933004/, but I'll do progress on this without waiting for the reviews of the dependent patches. https://codereview.chromium.org/2697803003/diff/40001/components/favicon/cont... File components/favicon/content/content_favicon_driver_unittest.cc (right): https://codereview.chromium.org/2697803003/diff/40001/components/favicon/cont... components/favicon/content/content_favicon_driver_unittest.cc:77: .WillOnce(DoAll(SaveArg<3>(&get_favicon_callback), Return(1))); On 2017/02/15 04:43:00, pkotwicz wrote: > Testing the returned base::CancelableTaskTracker::TaskId from > FaviconService::GetFaviconForPageURL() is uninteresting in my opinion Agreed. However, the mock does need to return *some* task ID that is non-zero, so this is the simplest option. https://codereview.chromium.org/2697803003/diff/40001/components/favicon/cont... components/favicon/content/content_favicon_driver_unittest.cc:96: update_mappings_callback.Run(kEmptyRawBitmapResult); On 2017/02/15 04:43:00, pkotwicz wrote: > Random Comment #1: ContentFaviconDriver is not used on iOS. So we unfortunately > cannot move all of the tests from FaviconHandler here > > Random comment #2: In an ideal world, your mock FaviconService would be more > complicated so that this call and the one on line 92 would be unnecessary in the > test case. Lines 92 and 96 are testing the implementation. I am mostly > envisioning this for the FaviconHandler tests > > I am envisioning something like this: (This is very rough. There is bound to be > something that I am missing) > MockFaviconService { > public: > void Store(const GURL& page_url, const Icon& icon); > void AddMapping(const GURL& page_url, const GURL& icon_url); > Icon* GetIcon(const GURL& icon_url); > std::vector<GURL> GetPageMappingsForIconURL(const GURL& icon_url); > > // The number of icons in the database > int num_icons_stored() const; > > // The number of icon mappings in the database > int num_icon_mappings() const; > > // Whether to run the callbacks for FaviconService::GetFavicon() right away or > to queue them. > // This is necessary for testing race conditions between > FaviconService::GetFaviconForURLFromFaviconService() and > FaviconHandler::OnUpdateFaviconURL() > void SetQueueCallbacks(bool queue) > > // Runs all pending callbacks which were queued after call to > SetQueueCallbacks(true) > void RunPendingCallbacks() > > // FaviconService overrides: > ... > } > > class Icon { > public: > Icon* Create(const GURL& icon_url) {} > Icon* CreateWithEdgeSize(const GURL& icon_url, int edge_size) {} > > GURL icon_url; > int edge_size; > bool expired; > scoped_refptr<base::RefCountedMemory> bitmap_data; > } Wrt Random Comment #1: it's not my plan to reduce test coverage for FaviconHandler. These are just integration tests that cover comparable requirements in place prior to this CL. https://codereview.chromium.org/2697803003/diff/40001/components/favicon/cont... components/favicon/content/content_favicon_driver_unittest.cc:97: Wrt Random comment #2 above: I don't think what you propose makes the test simpler, since lines 92 and 96 would need to be replaced by RunPendingCallbacks(). I think a pure mock as proposed here fits well here, and eventually, if there's the need, we could introduce FakeFaviconService which would be similar to your proposal (but I would focus on the SetQueueCallbacks(false) case).
> I have the impression the test coverage proposed in these tests is clearly an > improvement, regardless of what happens with FaviconHandler. Are you not > convinced about this? I am not entirely convinced. The new tests test the implementation of FaviconHandler. Ideally the tests should not care about what FaviconService methods are called > Unfortunately, updating FaviconHandler tests is quite painful and hence I'd > prefer leaving outside the scope of this CL, mainly because the class needs a > rewrite, at least the removal of virtual functions overriden in > TestFaviconHandler (to avoid exercising the injected dependencies). - Yes, I appreciate that updating the FaviconHandler tests is hard. However, this is where the biggest gains are. I'd love it if we can significantly decrease the number of: - lines per test - ASSERT & EXPECT calls per test - I understand why you want to remove the virtual functions overridden in TestFaviconHandler. However, doing this is unnecessary to simplify the tests. You would just be moving the overridden methods from TestFaviconHandler to your partial mock of FaviconService https://codereview.chromium.org/2697803003/diff/40001/components/favicon/cont... File components/favicon/content/content_favicon_driver_unittest.cc (right): https://codereview.chromium.org/2697803003/diff/40001/components/favicon/cont... components/favicon/content/content_favicon_driver_unittest.cc:77: .WillOnce(DoAll(SaveArg<3>(&get_favicon_callback), Return(1))); I see. WillOnce() needs to have a Return() statement https://codereview.chromium.org/2697803003/diff/40001/components/favicon/cont... components/favicon/content/content_favicon_driver_unittest.cc:97: Calling RunPendingCallbacks() is cleaner than the setup in lines 71 - 77 + lines 92 and 96 I don't like that the test currently tests the implementation of FaviconHandler. Which FaviconService methods are called by FaviconHandler are part of the implementation details of FaviconHandler A partial mock would make the tests more readable here too.
The CQ bit was checked by mastiz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by mastiz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by mastiz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
mastiz@chromium.org changed reviewers: + phajdan.jr@chromium.org
phajdan.jr@chromium.org: Please review changes in content/ I'm in the process of discussing with pkotwicz@ whether we want to follow this path, but I'd be great to have some preliminary feedback as per such an extension to TestWebContents would be welcome.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Description was changed from ========== Improve test coverage for FaviconDriver class hierarchy We need this prior to adding new features to the class hierarchy, most notably the support for multiple favicons of the same type (i.e. for different resolutions). The main issue with the former tests is that they don't exercise the public API, but instead parts of the implementation. In order to do this, TestWebContents must be extended with basic support for testing DownloadImage(). BUG= ========== to ========== Improve test coverage for FaviconDriver class hierarchy We need this prior to adding new features to the class hierarchy, most notably the support for multiple favicons of the same type (i.e. for different resolutions). The main issue with the former tests is that they don't exercise the public API, but instead parts of the implementation. In order to do this, TestWebContents must be extended with basic support for testing DownloadImage(). BUG=694312 ==========
LGTM https://codereview.chromium.org/2697803003/diff/100001/content/public/test/we... File content/public/test/web_contents_tester.h (right): https://codereview.chromium.org/2697803003/diff/100001/content/public/test/we... content/public/test/web_contents_tester.h:12: #include "third_party/skia/include/core/SkBitmap.h" nit: Is it possible to forward declare needed types instead of adding more #includes?
The CQ bit was checked by mastiz@chromium.org to run a CQ dry run
On 2017/02/21 09:44:23, Paweł Hajdan Jr. wrote: > LGTM > > https://codereview.chromium.org/2697803003/diff/100001/content/public/test/we... > File content/public/test/web_contents_tester.h (right): > > https://codereview.chromium.org/2697803003/diff/100001/content/public/test/we... > content/public/test/web_contents_tester.h:12: #include > "third_party/skia/include/core/SkBitmap.h" > nit: Is it possible to forward declare needed types instead of adding more > #includes? Done, also in content/test/test_web_contents.h
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
Description was changed from ========== Improve test coverage for FaviconDriver class hierarchy We need this prior to adding new features to the class hierarchy, most notably the support for multiple favicons of the same type (i.e. for different resolutions). The main issue with the former tests is that they don't exercise the public API, but instead parts of the implementation. In order to do this, TestWebContents must be extended with basic support for testing DownloadImage(). BUG=694312 ========== to ========== Improve test coverage for ContentFaviconDriver We need this prior to adding new features to the class hierarchy, most notably the support for multiple favicons of the same type (i.e. for different resolutions). The main issue with the former tests is that they don't exercise the public API, but instead parts of the implementation. In order to do this, TestWebContents must be extended with basic support for testing DownloadImage(). BUG=694312 ==========
The CQ bit was checked by mastiz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by mastiz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
PTAL! On 2017/02/16 00:34:09, pkotwicz wrote: > > I have the impression the test coverage proposed in these tests is clearly an > > improvement, regardless of what happens with FaviconHandler. Are you not > > convinced about this? > > I am not entirely convinced. The new tests test the implementation of > FaviconHandler. Ideally the tests should not care about what FaviconService > methods are called The only way to not test the FaviconHandler would be to inject a test instance of FaviconHandler to verify what gets called. This would likely be cleanest by adopting a Builder for the FaviconHandler (will investigate). Perhaps what you have in mind is to verify *less* pieces of the FaviconHandler logic. In this case, there are two good options: 1. Write less expectations or adopt a NiceMock. 2. Adopt an actual FaviconService and HistoryBackend. The first is trivial. The second is implemented in https://codereview.chromium.org/2703603002 for FaviconHandlerIntegrationTest, which we could adopt here as well if you prefer. This all depends on what you'd like to get tested here (unit test vs integration tests). In this particular CL, I roughly cover similar expectations as before, only using the public API. > > > Unfortunately, updating FaviconHandler tests is quite painful and hence I'd > > prefer leaving outside the scope of this CL, mainly because the class needs a > > rewrite, at least the removal of virtual functions overriden in > > TestFaviconHandler (to avoid exercising the injected dependencies). > > - Yes, I appreciate that updating the FaviconHandler tests is hard. However, > this is where the biggest gains are. I'd love it if we can significantly > decrease the number of: > - lines per test > - ASSERT & EXPECT calls per test Let's move FaviconHandlerTest-specific discussions to https://codereview.chromium.org/2703603002 I think I've implemented the various techniques that fit in this scenario, we now need to choose one :-) Based on some of your comments, I updated this CL to adopt PostReply and RunUntilIdle. > > - I understand why you want to remove the virtual functions overridden in > TestFaviconHandler. However, doing this is unnecessary to simplify the tests. > You would just be moving the overridden methods from TestFaviconHandler to your > partial mock of FaviconService You're right that the LoC might not decrease, but the complexity is nevertheless lower. I think most people would agree that overriding class functions in tests is inferior to injecting test dependencies. > > https://codereview.chromium.org/2697803003/diff/40001/components/favicon/cont... > File components/favicon/content/content_favicon_driver_unittest.cc (right): > > https://codereview.chromium.org/2697803003/diff/40001/components/favicon/cont... > components/favicon/content/content_favicon_driver_unittest.cc:77: > .WillOnce(DoAll(SaveArg<3>(&get_favicon_callback), Return(1))); > I see. WillOnce() needs to have a Return() statement > > https://codereview.chromium.org/2697803003/diff/40001/components/favicon/cont... > components/favicon/content/content_favicon_driver_unittest.cc:97: > Calling RunPendingCallbacks() is cleaner than the setup in lines 71 - 77 + lines > 92 and 96 > > I don't like that the test currently tests the implementation of FaviconHandler. > Which FaviconService methods are called by FaviconHandler are part of the > implementation details of FaviconHandler > > A partial mock would make the tests more readable here too. I think I've replied to this point earlier: if you don't want to test any logic within FaviconHandler, then we'd need to inject one. Otherwise, we can trivially relax expectations.
The CQ bit was checked by mastiz@chromium.org to run a CQ dry run
pkotwicz@: this is ready for review, thanks!
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Looks mostly good. Sorry for the delay https://codereview.chromium.org/2697803003/diff/220001/components/favicon/con... File components/favicon/content/content_favicon_driver_unittest.cc (right): https://codereview.chromium.org/2697803003/diff/220001/components/favicon/con... components/favicon/content/content_favicon_driver_unittest.cc:41: _, _, _, _)) Might as well make all of the parameters testing::_ https://codereview.chromium.org/2697803003/diff/220001/components/favicon/con... components/favicon/content/content_favicon_driver_unittest.cc:46: EXPECT_CALL(favicon_service_, GetFaviconForPageURL(kPageUrl, _, _, _, _)) Might as well make all of the parameters testing::_ https://codereview.chromium.org/2697803003/diff/220001/components/favicon/con... components/favicon/content/content_favicon_driver_unittest.cc:47: .WillRepeatedly(PostReply<5>(kEmptyRawBitmapResult)); 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. https://codereview.chromium.org/2697803003/diff/220001/components/favicon/con... components/favicon/content/content_favicon_driver_unittest.cc:63: Maybe rename this function to FetchFaviconForPage() https://codereview.chromium.org/2697803003/diff/220001/components/favicon/con... components/favicon/content/content_favicon_driver_unittest.cc:108: content::FaviconURL::FAVICON, _, _, _)); 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 https://codereview.chromium.org/2697803003/diff/220001/components/favicon/con... components/favicon/content/content_favicon_driver_unittest.cc:121: EXPECT_CALL(favicon_service_, WasUnableToDownloadFavicon(kIconUrl)) This should be ON_CALL() https://codereview.chromium.org/2697803003/diff/220001/components/favicon/con... components/favicon/content/content_favicon_driver_unittest.cc:132: kIconUrl, 200, kEmptyIcons, kEmptyIconSizes)); 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)
Thx, PTAL! phajdan.jr@: can you also take another look after I added one more function as per peter's comment? https://codereview.chromium.org/2697803003/diff/220001/components/favicon/con... File components/favicon/content/content_favicon_driver_unittest.cc (right): https://codereview.chromium.org/2697803003/diff/220001/components/favicon/con... components/favicon/content/content_favicon_driver_unittest.cc:41: _, _, _, _)) On 2017/02/22 19:34:16, pkotwicz wrote: > Might as well make all of the parameters testing::_ Done. https://codereview.chromium.org/2697803003/diff/220001/components/favicon/con... components/favicon/content/content_favicon_driver_unittest.cc:46: EXPECT_CALL(favicon_service_, GetFaviconForPageURL(kPageUrl, _, _, _, _)) On 2017/02/22 19:34:16, pkotwicz wrote: > Might as well make all of the parameters testing::_ Done. https://codereview.chromium.org/2697803003/diff/220001/components/favicon/con... components/favicon/content/content_favicon_driver_unittest.cc:47: .WillRepeatedly(PostReply<5>(kEmptyRawBitmapResult)); On 2017/02/22 19:34:16, pkotwicz wrote: > 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. Done, adopted NiceMock. https://codereview.chromium.org/2697803003/diff/220001/components/favicon/con... components/favicon/content/content_favicon_driver_unittest.cc:63: On 2017/02/22 19:34:16, pkotwicz wrote: > Maybe rename this function to FetchFaviconForPage() Renamed to TestFetchFaviconForPage because otherwise I feel it can be harder to distinguish from methods in non-test classes. WDYT? https://codereview.chromium.org/2697803003/diff/220001/components/favicon/con... components/favicon/content/content_favicon_driver_unittest.cc:108: content::FaviconURL::FAVICON, _, _, _)); On 2017/02/22 19:34:16, pkotwicz wrote: > 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 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. https://codereview.chromium.org/2697803003/diff/220001/components/favicon/con... components/favicon/content/content_favicon_driver_unittest.cc:121: EXPECT_CALL(favicon_service_, WasUnableToDownloadFavicon(kIconUrl)) On 2017/02/22 19:34:16, pkotwicz wrote: > This should be ON_CALL() Done. https://codereview.chromium.org/2697803003/diff/220001/components/favicon/con... components/favicon/content/content_favicon_driver_unittest.cc:132: kIconUrl, 200, kEmptyIcons, kEmptyIconSizes)); On 2017/02/22 19:34:16, pkotwicz wrote: > 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) Done.
LGTM
Just nits. L-G-T-M when you have addressed them :) https://codereview.chromium.org/2697803003/diff/220001/components/favicon/con... File components/favicon/content/content_favicon_driver_unittest.cc (right): https://codereview.chromium.org/2697803003/diff/220001/components/favicon/con... components/favicon/content/content_favicon_driver_unittest.cc:63: I had not thought about making it obvious which methods are test methods and which are not in the past. TestFetchFaviconForPage() is great :) https://codereview.chromium.org/2697803003/diff/220001/components/favicon/con... components/favicon/content/content_favicon_driver_unittest.cc:108: content::FaviconURL::FAVICON, _, _, _)); 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 https://codereview.chromium.org/2697803003/diff/290001/components/favicon/con... File components/favicon/content/content_favicon_driver_unittest.cc (right): https://codereview.chromium.org/2697803003/diff/290001/components/favicon/con... components/favicon/content/content_favicon_driver_unittest.cc:73: TEST_F(ContentFaviconDriverTest, ShouldCacheAvailableFavicon) { 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? https://codereview.chromium.org/2697803003/diff/290001/components/favicon/con... components/favicon/content/content_favicon_driver_unittest.cc:85: 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" https://codereview.chromium.org/2697803003/diff/290001/components/favicon/con... components/favicon/content/content_favicon_driver_unittest.cc:99: 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 :)
PTAL. https://codereview.chromium.org/2697803003/diff/220001/components/favicon/con... File components/favicon/content/content_favicon_driver_unittest.cc (right): https://codereview.chromium.org/2697803003/diff/220001/components/favicon/con... components/favicon/content/content_favicon_driver_unittest.cc:108: content::FaviconURL::FAVICON, _, _, _)); On 2017/02/23 16:20:49, pkotwicz wrote: > 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 > Done. https://codereview.chromium.org/2697803003/diff/290001/components/favicon/con... File components/favicon/content/content_favicon_driver_unittest.cc (right): https://codereview.chromium.org/2697803003/diff/290001/components/favicon/con... components/favicon/content/content_favicon_driver_unittest.cc:73: TEST_F(ContentFaviconDriverTest, ShouldCacheAvailableFavicon) { On 2017/02/23 16:20:49, pkotwicz wrote: > 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? 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. https://codereview.chromium.org/2697803003/diff/290001/components/favicon/con... components/favicon/content/content_favicon_driver_unittest.cc:85: On 2017/02/23 16:20:49, pkotwicz wrote: > 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" 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? https://codereview.chromium.org/2697803003/diff/290001/components/favicon/con... components/favicon/content/content_favicon_driver_unittest.cc:99: On 2017/02/23 16:20:49, pkotwicz wrote: > 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 :) Done. Ditto about the naming convention.
LGTM Thank you for bearing with me https://codereview.chromium.org/2697803003/diff/290001/components/favicon/con... File components/favicon/content/content_favicon_driver_unittest.cc (right): https://codereview.chromium.org/2697803003/diff/290001/components/favicon/con... components/favicon/content/content_favicon_driver_unittest.cc:73: TEST_F(ContentFaviconDriverTest, ShouldCacheAvailableFavicon) { 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 https://codereview.chromium.org/2697803003/diff/290001/components/favicon/con... components/favicon/content/content_favicon_driver_unittest.cc:99: 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() https://codereview.chromium.org/2697803003/diff/330001/components/favicon/con... File components/favicon/content/content_favicon_driver_unittest.cc (right): https://codereview.chromium.org/2697803003/diff/330001/components/favicon/con... components/favicon/content/content_favicon_driver_unittest.cc:84: kIconURL, 200, kEmptyIcons, kEmptyIconSizes)); How about: "Completing the download should not cause a call to UnableToDownloadFavicon()." I think that the comment makes it clearer as to why TestWebContents::TestDidDownloadImage() is called (instead of TestWebContents::HasPendingDownloadImage()) https://codereview.chromium.org/2697803003/diff/330001/components/favicon/con... components/favicon/content/content_favicon_driver_unittest.cc:88: // download with 200 status. Can you please update your comment? 200 != 404 https://codereview.chromium.org/2697803003/diff/330001/components/favicon/con... components/favicon/content/content_favicon_driver_unittest.cc:110: // Mimic the completion of an image download. How about: "Completing the download should not cause a call to UnableToDownloadFavicon()." I think that the comment makes it clearer as to why TestWebContents::TestDidDownloadImage() is called
Thanks! Will land after a grace period, to see if you'd rather have tests with a consistent naming convention or leave them unmodified. https://codereview.chromium.org/2697803003/diff/290001/components/favicon/con... File components/favicon/content/content_favicon_driver_unittest.cc (right): https://codereview.chromium.org/2697803003/diff/290001/components/favicon/con... components/favicon/content/content_favicon_driver_unittest.cc:73: TEST_F(ContentFaviconDriverTest, ShouldCacheAvailableFavicon) { On 2017/02/27 15:29:07, pkotwicz wrote: > 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 Acknowledged. https://codereview.chromium.org/2697803003/diff/290001/components/favicon/con... components/favicon/content/content_favicon_driver_unittest.cc:85: On 2017/02/24 21:15:38, mastiz wrote: > On 2017/02/23 16:20:49, pkotwicz wrote: > > 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" > > 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? Ping? Should I rename tests back to ShouldXXX form or is it fine in the current form? https://codereview.chromium.org/2697803003/diff/290001/components/favicon/con... components/favicon/content/content_favicon_driver_unittest.cc:99: On 2017/02/27 15:29:07, pkotwicz wrote: > 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() Done, renamed. https://codereview.chromium.org/2697803003/diff/330001/components/favicon/con... File components/favicon/content/content_favicon_driver_unittest.cc (right): https://codereview.chromium.org/2697803003/diff/330001/components/favicon/con... components/favicon/content/content_favicon_driver_unittest.cc:84: kIconURL, 200, kEmptyIcons, kEmptyIconSizes)); On 2017/02/27 15:29:07, pkotwicz wrote: > How about: "Completing the download should not cause a call to > UnableToDownloadFavicon()." > > I think that the comment makes it clearer as to why > TestWebContents::TestDidDownloadImage() is called (instead of > TestWebContents::HasPendingDownloadImage()) Done. https://codereview.chromium.org/2697803003/diff/330001/components/favicon/con... components/favicon/content/content_favicon_driver_unittest.cc:88: // download with 200 status. On 2017/02/27 15:29:07, pkotwicz wrote: > Can you please update your comment? 200 != 404 Done. https://codereview.chromium.org/2697803003/diff/330001/components/favicon/con... components/favicon/content/content_favicon_driver_unittest.cc:110: // Mimic the completion of an image download. On 2017/02/27 15:29:07, pkotwicz wrote: > How about: "Completing the download should not cause a call to > UnableToDownloadFavicon()." > > I think that the comment makes it clearer as to why > TestWebContents::TestDidDownloadImage() is called Done.
https://codereview.chromium.org/2697803003/diff/290001/components/favicon/con... File components/favicon/content/content_favicon_driver_unittest.cc (right): https://codereview.chromium.org/2697803003/diff/290001/components/favicon/con... components/favicon/content/content_favicon_driver_unittest.cc:85: 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())
On 2017/02/27 23:58:21, pkotwicz wrote: > https://codereview.chromium.org/2697803003/diff/290001/components/favicon/con... > File components/favicon/content/content_favicon_driver_unittest.cc (right): > > https://codereview.chromium.org/2697803003/diff/290001/components/favicon/con... > components/favicon/content/content_favicon_driver_unittest.cc:85: > 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()) Renamed both, landing...
The CQ bit was checked by mastiz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from phajdan.jr@chromium.org, pkotwicz@chromium.org Link to the patchset: https://codereview.chromium.org/2697803003/#ps370001 (title: "Minor test renames.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 370001, "attempt_start_ts": 1488307745918920,
"parent_rev": "a2fe96582d0daf599ddcb5c4a655018de5c87755", "commit_rev":
"ba3a4d8ad97649db9ce47979a18f93fd07b0499e"}
Message was sent while issue was closed.
Description was changed from ========== Improve test coverage for ContentFaviconDriver We need this prior to adding new features to the class hierarchy, most notably the support for multiple favicons of the same type (i.e. for different resolutions). The main issue with the former tests is that they don't exercise the public API, but instead parts of the implementation. In order to do this, TestWebContents must be extended with basic support for testing DownloadImage(). BUG=694312 ========== to ========== Improve test coverage for ContentFaviconDriver We need this prior to adding new features to the class hierarchy, most notably the support for multiple favicons of the same type (i.e. for different resolutions). The main issue with the former tests is that they don't exercise the public API, but instead parts of the implementation. In order to do this, TestWebContents must be extended with basic support for testing DownloadImage(). BUG=694312 Review-Url: https://codereview.chromium.org/2697803003 Cr-Commit-Position: refs/heads/master@{#453680} Committed: https://chromium.googlesource.com/chromium/src/+/ba3a4d8ad97649db9ce47979a18f... ==========
Message was sent while issue was closed.
Committed patchset #20 (id:370001) as https://chromium.googlesource.com/chromium/src/+/ba3a4d8ad97649db9ce47979a18f... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
