Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2703603002:
Improve FaviconHandler tests by testing public API (Closed)
DescriptionImprove FaviconHandler tests by testing public API
No behavioral changes: instead of subclassing the class under test to
bypass/intercept internal calls, let's instead verify external
interactions by injecting test doubles and testing the public API.
The new ones are more concise by adopting mocks to verify call
arguments or fakes (mock-to-fake delegation) for the simplest cases.
This greatly reduces the size of tests to ~50%, ~800 LoC less!
The actual diffstat is smaller due to newly added tests, increasing the
total number of tests from 18 to 28, including basic test coverage for
incognito (DownloadUnknownFaviconInIncognito) or a corner case that is
prone to regressions (MultipleFaviconsLast404).
BUG=694312
Review-Url: https://codereview.chromium.org/2703603002
Cr-Commit-Position: refs/heads/master@{#458084}
Committed: https://chromium.googlesource.com/chromium/src/+/36c9189e71036d65209df554c9ddc96be6f78869
Patch Set 1 #
Total comments: 8
Patch Set 2 : WIP #Patch Set 3 : Rebased. #Patch Set 4 : Rebased. #Patch Set 5 : Remove comments. #Patch Set 6 : Shorter tests. #Patch Set 7 : Rebased. #Patch Set 8 : Rebased. #Patch Set 9 : Rebased. #Patch Set 10 : Minor changes. #Patch Set 11 : Updated missing tests. #Patch Set 12 : Fixed missing build dependency. #Patch Set 13 : Removed TODO. #Patch Set 14 : Reintroduce HasPendingTasksForTest. #Patch Set 15 : Helper functions to set fake images. #Patch Set 16 : Added comments. #Patch Set 17 : Adopt FakeImageDownloader. #
Total comments: 51
Patch Set 18 : Addressed more comments. #Patch Set 19 : Fixed Windows build. #Patch Set 20 : Conditioned test to OS_ANDROID #Patch Set 21 : Adopted fake downloads() and db_requests(). #Patch Set 22 : Minor changes. #Patch Set 23 : Reverted ifdef. #
Total comments: 132
Patch Set 24 : Addressed more comments. #Patch Set 25 : Inject scale factors. #Patch Set 26 : Inject scale factors to CreateFaviconImageSkia() #Patch Set 27 : Reverted injecting scale factors. #
Total comments: 83
Patch Set 28 : Addressed nits. #Patch Set 29 : Removed UpdateFaviconMappingsAndFetch() #
Dependent Patchsets: Messages
Total messages: 101 (68 generated)
pkotwicz@chromium.org changed reviewers: + pkotwicz@chromium.org
https://codereview.chromium.org/2703603002/diff/1/components/favicon/core/fav... File components/favicon/core/favicon_handler_unittest.cc (right): https://codereview.chromium.org/2703603002/diff/1/components/favicon/core/fav... components/favicon/core/favicon_handler_unittest.cc:284: GetFaviconForPageURL(page_url, favicon_base::FAVICON, _, _, _)); I don't think that which FaviconService method got called is interesting. It is useful to test that there was just one db request https://codereview.chromium.org/2703603002/diff/1/components/favicon/core/fav... components/favicon/core/favicon_handler_unittest.cc:312: icon_url, /*icon_url_changed=*/true, _)); This method should have been called twice https://codereview.chromium.org/2703603002/diff/1/components/favicon/core/fav... components/favicon/core/favicon_handler_unittest.cc:336: 200, icon_url, {CreateBitmap(16, 16)}, {gfx::Size(16, 16)})); Doesn't this duplicate the check on line 313?
I'll look at this some more on Friday morning
https://codereview.chromium.org/2703603002/diff/1/components/favicon/core/fav... File components/favicon/core/favicon_handler_unittest.cc (right): https://codereview.chromium.org/2703603002/diff/1/components/favicon/core/fav... components/favicon/core/favicon_handler_unittest.cc:295: icon_url, favicon_base::FAVICON, /*expired=*/false))); The test right now tests the order of the db request calls and download calls. In some tests we want to verify order. We want to ensure that we have FaviconHandler::OnFaviconDataForInitialURLFromFaviconService() getting called before and after FaviconHandler::OnUpdateFaviconURL() Right now the sequence of events if there are two icon URLs is FaviconHandler::FetchFavicon() FaviconHandler::OnUpdateFaviconURL() FaviconHandler::GetFaviconForURLFromFaviconService() FaviconHandler::UpdateFaviconMappingAndFetch() FaviconHandler::ScheduleDownload() FaviconHandler::UpdateFaviconMappingAndFetch() FaviconHandler::ScheduleDownload() In my ideal world, very few tests would need to change if the implementation changes the order to FaviconHandler::FetchFavicon() FaviconHandler::OnUpdateFaviconURL() FaviconHandler::GetFaviconForURLFromFaviconService() FaviconHandler::UpdateFaviconMappingAndFetch() FaviconHandler::UpdateFaviconMappingAndFetch() FaviconHandler::ScheduleDownload() FaviconHandler::ScheduleDownload() The tests are right now white box tests and I am hoping that more of them can be black box tests (or a fast integration tests). In order to make them black box tests I think that FaviconServiceTester needs to be slightly more complex FaviconServiceTester { public: void Store(const std::const GURL& page_url, const GURL& icon_url, const std::vector<FaviconRawBitmapResult>& results); void RunPendingCallbacks() int num_db_requests() const {} } I'd want tests to be able to call RunAllPending() the way I implemented it in https://codereview.chromium.org/2696423002/diff/1/a.c - I see how gmock is useful for checking calls to SetFavicons() OnFaviconUpdated() - Most tests don't care which FaviconService getters were invoked. We do care about the # of db accesses. That means that in most tests we should be able to remove EXPECT_CALL(favicon_service_, GetFaviconForPageURL(page_url, favicon_base::FAVICON, _, _, _)) statements I think that implies that MockFaviconService cannot be a StrictMock Random: One of my coworkers pointed me to this thread https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/gmock|... apparently the usage of GMock in Chromium is a subject of debate. There seems to be consensus that the GMock matchers are useful
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: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) 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...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Description was changed from ========== WIP improve FaviconHandler and tests. BUG= ========== to ========== Avoid cyclic dependency FaviconHandler<-->FaviconDriverImpl This patch reduces the exposed public API and introduces a delegate instead of a hardwired dependency such that FaviconHandler is more testable and has clear semantics. BUG=694312 ==========
Description was changed from ========== Avoid cyclic dependency FaviconHandler<-->FaviconDriverImpl This patch reduces the exposed public API and introduces a delegate instead of a hardwired dependency such that FaviconHandler is more testable and has clear semantics. BUG=694312 ========== to ========== Improve FaviconHandler tests by testing public API 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: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Note that this is WIP. In particular, two different techniques are implemented, leading to duplicate tests (some suffixed with _New). https://codereview.chromium.org/2703603002/diff/1/components/favicon/core/fav... File components/favicon/core/favicon_handler_unittest.cc (right): https://codereview.chromium.org/2703603002/diff/1/components/favicon/core/fav... components/favicon/core/favicon_handler_unittest.cc:284: GetFaviconForPageURL(page_url, favicon_base::FAVICON, _, _, _)); On 2017/02/16 16:36:17, pkotwicz wrote: > I don't think that which FaviconService method got called is interesting. > > It is useful to test that there was just one db request I propose as follows: I'll try to finalize these patch with overspecification of calls to FaviconService, and then we can trivially choose which ones get simplified (i.e. use _, don't consider order, etc.), prior to landing. https://codereview.chromium.org/2703603002/diff/1/components/favicon/core/fav... components/favicon/core/favicon_handler_unittest.cc:295: icon_url, favicon_base::FAVICON, /*expired=*/false))); On 2017/02/17 15:46:24, pkotwicz wrote: > The test right now tests the order of the db request calls and download calls. > In some tests we want to verify order. We want to ensure that we have > FaviconHandler::OnFaviconDataForInitialURLFromFaviconService() getting called > before and after FaviconHandler::OnUpdateFaviconURL() > > Right now the sequence of events if there are two icon URLs is > FaviconHandler::FetchFavicon() > FaviconHandler::OnUpdateFaviconURL() > FaviconHandler::GetFaviconForURLFromFaviconService() > FaviconHandler::UpdateFaviconMappingAndFetch() > FaviconHandler::ScheduleDownload() > FaviconHandler::UpdateFaviconMappingAndFetch() > FaviconHandler::ScheduleDownload() > > In my ideal world, very few tests would need to change if the implementation > changes the order to > FaviconHandler::FetchFavicon() > FaviconHandler::OnUpdateFaviconURL() > FaviconHandler::GetFaviconForURLFromFaviconService() > FaviconHandler::UpdateFaviconMappingAndFetch() > FaviconHandler::UpdateFaviconMappingAndFetch() > FaviconHandler::ScheduleDownload() > FaviconHandler::ScheduleDownload() I agree that this is a implementation detail. However, I'm not aware of a technique that ignores order for a particular pair of events. I believe the two options are: 1. Ignore order for all calls in some tests. 2. Always enforce order and accept the burden of trivially updating tests if such change occurs. > > The tests are right now white box tests and I am hoping that more of them can be > black box tests (or a fast integration tests). In order to make them black box > tests I think that FaviconServiceTester needs to be slightly more complex > > FaviconServiceTester { > public: > void Store(const std::const GURL& page_url, const GURL& icon_url, const > std::vector<FaviconRawBitmapResult>& results); > void RunPendingCallbacks() > int num_db_requests() const {} > } > > I'd want tests to be able to call RunAllPending() the way I implemented it in > https://codereview.chromium.org/2696423002/diff/1/a.c RunAllPending() essentially means we don't want to simulate a particular order of execution. For these, I would use the real FaviconService and then RunUntilIdle(). WDYT? I however think it'd be a bit confusing to use a different FaviconService depending on the test. Perhaps we can do this in FaviconDriverImplTest? > > - I see how gmock is useful for checking calls to SetFavicons() > OnFaviconUpdated() > > - Most tests don't care which FaviconService getters were invoked. We do care > about the # of db accesses. That means that in most tests we should be able to > remove > EXPECT_CALL(favicon_service_, GetFaviconForPageURL(page_url, > favicon_base::FAVICON, _, _, _)) statements > > I think that implies that MockFaviconService cannot be a StrictMock I would expect you actually want to be a bit more careful with *writes*. Hence, you probably want to whitelist (i.e. not be strict about) one or two methods? This can be easily achieved by: EXPECT_CALL(favicon_service_, GetFaviconForPageURL(_, _, _, _, _)).Times(AtLeast(1)); > > Random: One of my coworkers pointed me to this thread > https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/gmock%... > apparently the usage of GMock in Chromium is a subject of debate. There seems > to be consensus that the GMock matchers are useful I indeed followed this thread very closely. From what I understand, the general guidance is to avoid test doubles and use production implementations (FaviconService->HistoryService->HistoryBackend). I think we should do this in FaviconDriverImplTest, ContentFaviconDriverImplTest, or both. WDYT? https://codereview.chromium.org/2703603002/diff/1/components/favicon/core/fav... components/favicon/core/favicon_handler_unittest.cc:312: icon_url, /*icon_url_changed=*/true, _)); On 2017/02/16 16:36:17, pkotwicz wrote: > This method should have been called twice There's one more occurrence below, with a different value for icon_url_changed. https://codereview.chromium.org/2703603002/diff/1/components/favicon/core/fav... components/favicon/core/favicon_handler_unittest.cc:336: 200, icon_url, {CreateBitmap(16, 16)}, {gfx::Size(16, 16)})); On 2017/02/16 16:36:17, pkotwicz wrote: > Doesn't this duplicate the check on line 313? This is the price you pay for controlling when exactly each operation (in this case download) finishes. As discussed earlier, this seems desirable for quite some tests in this set. Since you seem to like RunUntilIdle-like approaches, I rewrote tests with this other technique, please see tests suffixed with _New.
I need to think about this CL some more. There are a lot of good things about your tests. I want to try rewriting some of the tests myself and see what they look like. I am going to try to figure things out by the end of the week my time. I will look at your CL again by the end of the week my time. I don't think that this CL blocks other CLs
As an update I have made progress but there are lots of tests. I have 7 tests in a shape that I kind of like so far
On 2017/02/26 18:15:58, pkotwicz wrote: > As an update I have made progress but there are lots of tests. I have 7 tests in > a shape that I kind of like so far Friendly ping :-)
Making more progress. Will try to be done by Monday night my time
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_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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 FaviconHandler tests by testing public API BUG=694312 ========== to ========== Improve FaviconHandler tests by testing public API Instead of subclassing the class under test to bypass/intercept internal calls, let's instead verify external interactions by injecting test doubles and testing the public API. The new are more concise by adopting mocks to verify call arguments, ordering, and counts, which is what the home-grown test doubles were doing anyway prior to this patch. This greatly reduces the size of tests to 50%, ~1000 LoC less! Tests otherwise verify the very same expectations. As bonus point, a few new tests are added, such as test coverage for incognito (DownloadUnknownFaviconInIncognito) or a corner case that is prone to regressions (MultipleFaviconsLast404). BUG=694312 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2017/03/06 06:48:06, pkotwicz wrote: > Making more progress. Will try to be done by Monday night my time Thanks! I finalized the remaining tests so I consider this patch ready for final review, modulo a FIXME I want to clarify with you first. Just as a reminder: because of the size of the patch, I would suggest incremental reviews, i.e. no need to go through all tests before we have a first round of comments (since there will duplicates anyway).
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 FaviconHandler tests by testing public API Instead of subclassing the class under test to bypass/intercept internal calls, let's instead verify external interactions by injecting test doubles and testing the public API. The new are more concise by adopting mocks to verify call arguments, ordering, and counts, which is what the home-grown test doubles were doing anyway prior to this patch. This greatly reduces the size of tests to 50%, ~1000 LoC less! Tests otherwise verify the very same expectations. As bonus point, a few new tests are added, such as test coverage for incognito (DownloadUnknownFaviconInIncognito) or a corner case that is prone to regressions (MultipleFaviconsLast404). BUG=694312 ========== to ========== Improve FaviconHandler tests by testing public API Instead of subclassing the class under test to bypass/intercept internal calls, let's instead verify external interactions by injecting test doubles and testing the public API. The new are more concise by adopting mocks to verify call arguments, ordering, and counts, which is what the home-grown test doubles were doing anyway prior to this patch. This greatly reduces the size of tests to 50%, ~900 LoC less! Tests otherwise verify the very same expectations. As bonus point, a few new tests are added, such as test coverage for incognito (DownloadUnknownFaviconInIncognito) or a corner case that is prone to regressions (MultipleFaviconsLast404). BUG=694312 ==========
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...)
Your CL looks better than I expected that it would. I did something stupid and spent a lot of time rewriting the tests myself. https://codereview.chromium.org/2738663002/ The annoying thing is that your version is not clearly better than my version and vice versa. One of the reasons that I wrote my version the way that I did is that I misunderstood how NiceMock works. I assumed that EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, kIconURL, _, _)) checks that Delegate::OnFaviconUpdated() is called once with |kIconUrl| as a parameter but not that Delegate::OnFaviconUpdated() is called only once. It looks like a single EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, kIconURL, _, _)) checks that Delegate::OnFaviconUpdated() is called only once (Assuming that there is just one EXPECT_CALL() statement) - My version has a lot of comments. At the very least you should steal the comments from my version. It also has 17 new tests but we can leave those for a follow up CL. - Calling TestFaviconService::Store(kPageUrl, kIconUrl, CreateRawBitmapResult(kIconURL16x16, FAVICON, /*expired=*/true)) is a lot nicer than ON_CALL(favicon_service_, GetFaviconForPageURL(kPageURL, _, _, _, _)) .WillByDefault(PostReply<5>(CreateRawBitmapResult(kIconURL16x16, FAVICON, /*expired=*/true))); ON_CALL(favicon_service_, UpdateFaviconMappingsAndFetch(kPageURL, URLVector{kNewIconURL}, FAVICON, _, _, _)) .WillByDefault(PostReply<6>(CreateRawBitmapResult(kIconURL, FAVICON, /*expired=*/false))); You could stick the ON_CALL statements into a function. However, when you are sticking ON_CALL() statements into functions, you might as well and write a fake for the GetFaviconForPageURL() and UpdateFaviconMappingsAndFetch() methods. I am unsure whether mixing GMock and non GMock is frowned upon. For instance, using GMock EXPECT_CALL() for Delegate::OnFaviconUpdated() but use EXPECT_THAT(TestDelegate::downloads(), ElementsAre()) for Delegate::downloads(). It is slightly confusing to check expectations before and after "the code which does stuff" In my version I set all of the database data and download data within the test itself. It does add a bit of boiler plate to each test. I am unsure whether it makes the tests more readable. In your version you save a LOT of bolier plate by doing the setup in FaviconHandlerTest. Doing EXPECT_THAT(delegate_->downloads(), ElementsAre(kIconUrl10x10, kIconUrl11x11)); is kind of addictive because it super short. I am unsure if it is less clear than the more verbose GMock EXPECT_CALL(delegate_, DownloadImage(kIconUrl10x10, _, _)); EXPECT_CALL(delegate_, DownloadImage(kIconUrl11x11, _, _)); I think the best way to proceed would be setup a meeting which is separate from Tuesday's sync to discuss
On 2017/03/07 07:00:46, pkotwicz wrote: > Your CL looks better than I expected that it would. > > I did something stupid and spent a lot of time rewriting the tests myself. > https://codereview.chromium.org/2738663002/ The annoying thing is that your > version is not clearly better than my version and vice versa. > > One of the reasons that I wrote my version the way that I did is that I > misunderstood how NiceMock works. I assumed that > EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, kIconURL, _, _)) checks that > Delegate::OnFaviconUpdated() is called once with |kIconUrl| as a parameter but > not that Delegate::OnFaviconUpdated() is called only once. > It looks like a single EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, kIconURL, > _, _)) checks that Delegate::OnFaviconUpdated() is called only once (Assuming > that there is just one EXPECT_CALL() statement) > > - My version has a lot of comments. At the very least you should steal the > comments from my version. Will do. > It also has 17 new tests but we can leave those for a > follow up CL. In general, I fear we could be putting too much effort into getting the ultimate tests in the first patch, while I believe any of the discussed variants is clearly superior to the former tests. > - Calling > TestFaviconService::Store(kPageUrl, kIconUrl, > CreateRawBitmapResult(kIconURL16x16, FAVICON, /*expired=*/true)) > is a lot nicer than > ON_CALL(favicon_service_, GetFaviconForPageURL(kPageURL, _, _, _, _)) > .WillByDefault(PostReply<5>(CreateRawBitmapResult(kIconURL16x16, FAVICON, > /*expired=*/true))); > ON_CALL(favicon_service_, > UpdateFaviconMappingsAndFetch(kPageURL, URLVector{kNewIconURL}, > FAVICON, _, _, _)) > .WillByDefault(PostReply<6>(CreateRawBitmapResult(kIconURL, FAVICON, > /*expired=*/false))); > > You could stick the ON_CALL statements into a function. However, when you are > sticking ON_CALL() statements into functions, you might as well and write a fake > for the GetFaviconForPageURL() and UpdateFaviconMappingsAndFetch() methods. I agree there are various variants, almost equally nice: 1. Introduce a helper function: FaviconHandlerTest::SetFakeFaviconServiceResponse(...) 2. Delegate getters to a fake. 3. Inline ON_CALL statements (current proposal), which is the least "magic" IMO. If you have a strong preference for any of these, I can go ahead and change the code. Otherwise, I propose being pragmatic and deferring modifications to future work. > > I am unsure whether mixing GMock and non GMock is frowned upon. For instance, > using GMock EXPECT_CALL() for Delegate::OnFaviconUpdated() but use > EXPECT_THAT(TestDelegate::downloads(), ElementsAre()) for Delegate::downloads(). > It is slightly confusing to check expectations before and after "the code which > does stuff" I think combining mock expectations with regular expectations is legit in general. The particular case above, however, is a bit weird, because downloads() is IMO a home-grown mock that verifies method calls, which I would rather not do. > > In my version I set all of the database data and download data within the test > itself. It does add a bit of boiler plate to each test. I am unsure whether it > makes the tests more readable. In your version you save a LOT of bolier plate by > doing the setup in FaviconHandlerTest. I also have mixed feelings about this decision but concluded that more concise tests were worth the tradeoff. > > Doing > EXPECT_THAT(delegate_->downloads(), ElementsAre(kIconUrl10x10, kIconUrl11x11)); > is kind of addictive because it super short. I am unsure if it is less clear > than the more verbose GMock > EXPECT_CALL(delegate_, DownloadImage(kIconUrl10x10, _, _)); > EXPECT_CALL(delegate_, DownloadImage(kIconUrl11x11, _, _)); > > I think the best way to proceed would be setup a meeting which is separate from > Tuesday's sync to discuss Good idea. Scheduled a VC, thanks!
Thanks, PTAL! Wrt to the points we discussed yesterday: 1. Camelcasing of URL: I took one more look and realized the favicon codebase uses uppercase URL all over the place (in type definitions and function names) so I left this unmodified for local consistency. Let me know if you still prefer otherwise. 2. Improved/fixed the two tests: TestSortFavicon and OnFaviconAvailableNotificationSentAfterIconURLChange. 3. Copied many comments and some new tests from https://codereview.chromium.org/2738663002/. I propose we leave the rest for follow-up CLs if needed. 4. Wrt to a fake image downloader, I exported two proposals, patchsets 16 and 17, to make the diff more obvious. The later has a FakeImageDownloader similar to what you proposed in https://codereview.chromium.org/2738663002/. I myself don't see a big benefit, but I'm fine either way. Besides, I added a line with DownloadImage().Times(0) to enforce tests to be explicit about expected downloads. I can revert this if you want, but I think it makes sense to be extra careful with downloads.
Some initial comments. I think that it would be nice to use EXPECT_THAT(Delegate::downloads(), ElementsAre()) EXPECT_THAT(FaviconService::db_requets(), ElementsAre()) Doing this saves quite a bit of typing and removes the need for several testing::InSequence blocks It also gets rid of the dreaded 5 argument EXPECT_CALL(favicon_service_, UpdateFaviconMappingAndFetch(_, kIconUrl, _, _, _) You already have a fake for Delegate::DownloadImage(). You might as well take full advantage https://codereview.chromium.org/2703603002/diff/320001/components/favicon/cor... File components/favicon/core/favicon_handler_unittest.cc (right): https://codereview.chromium.org/2703603002/diff/320001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:235: EXPECT_CALL(delegate_, DownloadImage(_, _, _)).Times(0); Each test which wants to check that there has been zero downloads should do this check explicitly https://codereview.chromium.org/2703603002/diff/320001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:239: &FakeImageDownloader::DownloadImage)); Can we move this ON_CALL to the MockDelegate constructor like MockDownloadManagerService in download_manager_service_unittest.cc does? https://codereview.chromium.org/2703603002/diff/320001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:428: CreateRawBitmapResult(kIconURL16x16, FAVICON, /*expired=*/true))); TestFaviconService::Store(kPageURL, kIconURL16x16, CreateRawBitmapResult(kIconURL16x16, FAVICON, /*expired=*/true)) would be a lot nicer than this https://codereview.chromium.org/2703603002/diff/320001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:429: We want to check that UpdateFaviconMappingsAndFetch() was not called for |kIconURL16x16| https://codereview.chromium.org/2703603002/diff/320001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:433: kIconURL16x16, /*icon_url_changed=*/true, _)); We don't care about the values of any of the parameters except for the icon URL https://codereview.chromium.org/2703603002/diff/320001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:439: kIconURL16x16, /*icon_url_changed=*/false, _)); It would be nice if we could check whether the two OnFaviconUpdated() calls are called with different gfx::Images (As opposed to calling OnFaviconUpdated() with the expired gfx::Image both times). You could do this by having the data in the database and the data which is downloaded have a different color. Checking for the size is unfortunately not helpful because FaviconHandlers of type FaviconDriverObserver::NON_TOUCH_16_DIP resize the gfx::Image to 16x16 prior to calling Delegate::OnFaviconUpdated() https://codereview.chromium.org/2703603002/diff/320001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:442: {FaviconURL(kIconURL16x16, FAVICON, kEmptySizes)}); Can you add RunHandlerWithGURLCandidates() which takes a std::vector<GURL> and constructs FaviconURLs with favicon_base::FAVICON and empty sizes https://codereview.chromium.org/2703603002/diff/320001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:560: FAVICON, _, _, _)) We don't need to check whether UpdateFaviconMappingsAndFetch() is called or not. If a download was not done, FaviconHandler must have gotten the image data from the database https://codereview.chromium.org/2703603002/diff/320001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:565: kNewIconURL, /*icon_url_changed=*/true, _)); For the sake of completeness we should check that FaviconService::SetFavicons() is not called https://codereview.chromium.org/2703603002/diff/320001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:610: TEST_F(FaviconHandlerTest, UpdateDuringDownloading) { We want to: - set initial state - test the initial state - run the test - test the final state In order for the testing of the final state to check things which did not occur as part of setting the initial state, you have to call VerifyAndClearExpectations() You can probably put VerifyAndClearExpectations(favicon_service_) and VerifyAndClearExpectations(delegate_) into a function called VerifyAndClearExpectCalls() https://codereview.chromium.org/2703603002/diff/320001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:622: EXPECT_CALL(favicon_service_, SetFavicons(_, kIconURL, _, _)).Times(0); We don't want SetFavicons() to be called at all for any URL. You can replace kIconURL with '_' https://codereview.chromium.org/2703603002/diff/320001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:632: /*expired=*/false))); We don't care about the UpdateFaviconMappingsAndFetch() calls in this test https://codereview.chromium.org/2703603002/diff/320001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:635: kLatestIconURL, /*icon_url_changed=*/true, _)); kLatestIconUrl is the only parameter that we care about in the OnFaviconUpdated() call https://codereview.chromium.org/2703603002/diff/320001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:637: std::unique_ptr<FaviconHandler> handler = RunHandlerWithCandidates( 1) Some of the old tests were testing too many things at once. UpdateDuringDownloading() was one such test. Make this test deal with FaviconURLs of type favicon_base::FAVICON In https://codereview.chromium.org/2738663002 I added FaviconHandlerTest.OnlyDownloadMatchingIconType FaviconHandlerTest.ShouldCallSetFaviconsWithCorrectIconType and to test the behavior of touch FaviconURLs https://codereview.chromium.org/2703603002/diff/320001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:642: }); For the "verify the final state" part of this test you should check EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, kLatestIconURL, _, _)); EXPECT_CALL(favicon_service_, SetFavicons(_, kLatestIconURL, _, _)) EXPECT_CALL(favicon_service_, UpdateFaviconMappingsAndFetch(_, URLVector{kLatestIconURL}, _, _, _, _)); EXPECT_CALL(delegate_, DownloadImage(_, _, _)).Times(0); https://codereview.chromium.org/2703603002/diff/320001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:715: You should call VerifyExpectCalls() and have new expectations for the "test final state" part of the test. In particular EXPECT_CALL(favicon_service_, UpdateFaviconMappingsAndFetch(_, _, _, _, _, _)).Times(0); EXPECT_CALL(delegate_, DownloadImage(_, _, _)).Times(0); EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, _, _, _)).Times(0); EXPECT_CALL(favicon_service_, SetFavicons(_, _, _, _)).Times(0); https://codereview.chromium.org/2703603002/diff/320001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:747: EXPECT_CALL(delegate_, DownloadImage(kIconURL2, _, _)).Times(2); - We only care about the downloads which occur in the "setup" part of the test. The reason that we care about the downloads in the "setup" part is that we want to ensure that FaviconHandler tried both candidates - For the setup of this test it is critical that the download of |kIconURL1| occurs before the download of |kIconURL2|. The GMock syntax would be I guess { InSequence seq; EXPECT_CALL(delegate_, DownloadImage(kIconURL1, _, _)); EXPECT_CALL(delegate_, DownloadImage(kIconURL2, _, _)); } When I am going through the CL I am comparing the assertions against those in my version of FaviconHandlerTest.ShouldCallOnFaviconAvailableAfterIconURLChange https://codereview.chromium.org/2703603002/diff/320001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:753: CreateRawBitmapResult(kIconURL1, FAVICON, /*expired=*/true))); It is unimportant for there to be data in the database for this test. The test works just fine without any data in the database (It also results in a less insane number of calls to Delegate::OnUpdateFaviconURL(). The insane number of calls to Delegate::OnUpdateFaviconURL() is a bug.) https://codereview.chromium.org/2703603002/diff/320001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:773: }); We want to: - set initial state - test the initial state - run the test - test the final state In order for the testing of the final state to check things which did not occur as part of setting the initial state, you have to call VerifyAndClearExpectations() https://codereview.chromium.org/2703603002/diff/320001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:963: { I think that it is easy to have bugs with this test. In particular, the order that the bitmap sizes are downloaded is non obvious (the order that the icon URLs are downloaded is obvious). I think that this would make the order that the bitmap sizes are downloaded clearer: const FaviconURL kSourceIconURLs[] = { ... }; struct ExpectedResult { ... } results[] = { ... }; { InSequence seq; for (const ExpectedResult& result : results) { FaviconURL expected_favicon = kSourceIconURLs[result.favicon_index]; if (result.edge_size > 0) { EXPECT_THAT(favicon.icon_sizes, testing::Contains(gfx::Size(result.edge_size, result.edge_size))); } EXPECT_CALL(delegate_, DownloadImage(expected_favicon.icon_url, _, _)); } } Alternatively, you could have different variables for each FaviconURL and name the FaviconURLs based on the sizes. For instance: FaviconURL favicon_1024_and_512 = FaviconURL(GURL("http://www.google.com/a"), FAVICON, {gfx::Size(1024, 1024), gfx::Size(512, 512)}); https://codereview.chromium.org/2703603002/diff/320001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:979: .Times(5); We don't care about calls to UpdateFaviconMappingsAndFetch() here https://codereview.chromium.org/2703603002/diff/320001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1058: } We don't care about the UpdateFaviconMappingsAndFetch() calls in the context of this test https://codereview.chromium.org/2703603002/diff/320001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1063: ImageSizeIs(12, 12))); In my CL I have made all of the tests check for the Delegate::OnFaviconUpdated() call. I understand that the old test calls FaviconService::SetFavicons() Checking for Delegate::OnFaviconUpdated() and FaviconService::SetFavicons() is somewhat interchangeable. FaviconService::SetFavicons() is called a subset of the times that Delegate::OnFaviconUpdated() is called. As usual, we only care about the selected icon URL. Thus EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, kIconURL12x12, _, _));
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...
Thanks, PTAL! Addressed most comments with two main pushbacks: 1. Addition of new tests: this CL being originally a refactoring of existing tests, I propose we avoid adding new ones now (except the ones that have already been added) and leave that for future CLs. 2. Adoption of a fake FaviconService: I'm not convinced about this idea and would like to push back and defer this discussion, since there are multiple alternatives in this space including using the production FaviconService. Besides, there's the high-level question of whether NiceMock should be used for FaviconService. I suspect you prefer it that way based on your comments, but I want to make sure this has been thought trough because I feel a bit uncomfortable in silently accepting writes to the DB (at the expense of some extra verbosity in various tests). https://codereview.chromium.org/2703603002/diff/320001/components/favicon/cor... File components/favicon/core/favicon_handler_unittest.cc (right): https://codereview.chromium.org/2703603002/diff/320001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:235: EXPECT_CALL(delegate_, DownloadImage(_, _, _)).Times(0); On 2017/03/09 02:15:09, pkotwicz wrote: > Each test which wants to check that there has been zero downloads should do this > check explicitly Done, reverted. https://codereview.chromium.org/2703603002/diff/320001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:239: &FakeImageDownloader::DownloadImage)); On 2017/03/09 02:15:08, pkotwicz wrote: > Can we move this ON_CALL to the MockDelegate constructor like > MockDownloadManagerService in download_manager_service_unittest.cc does? Done, although I see no benefit. https://codereview.chromium.org/2703603002/diff/320001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:428: CreateRawBitmapResult(kIconURL16x16, FAVICON, /*expired=*/true))); On 2017/03/09 02:15:08, pkotwicz wrote: > TestFaviconService::Store(kPageURL, kIconURL16x16, > CreateRawBitmapResult(kIconURL16x16, FAVICON, /*expired=*/true)) > > would be a lot nicer than this I personally disagree this is better because the backend is complex enough to make the Store() quite magic. On order to do progress with this CL, I hope we can defer the possibility of adopting a fake FaviconService to the future, because then I'd even argue a real backend should be adopted. https://codereview.chromium.org/2703603002/diff/320001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:429: On 2017/03/09 02:15:08, pkotwicz wrote: > We want to check that UpdateFaviconMappingsAndFetch() was not called for > |kIconURL16x16| Done. https://codereview.chromium.org/2703603002/diff/320001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:433: kIconURL16x16, /*icon_url_changed=*/true, _)); On 2017/03/09 02:15:08, pkotwicz wrote: > We don't care about the values of any of the parameters except for the icon URL Done, except for icon_url_changed which I think we do care about. https://codereview.chromium.org/2703603002/diff/320001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:439: kIconURL16x16, /*icon_url_changed=*/false, _)); On 2017/03/09 02:15:08, pkotwicz wrote: > It would be nice if we could check whether the two OnFaviconUpdated() calls are > called with different gfx::Images (As opposed to calling OnFaviconUpdated() with > the expired gfx::Image both times). > > You could do this by having the data in the database and the data which is > downloaded have a different color. Checking for the size is unfortunately not > helpful because FaviconHandlers of type FaviconDriverObserver::NON_TOUCH_16_DIP > resize the gfx::Image to 16x16 prior to calling Delegate::OnFaviconUpdated() Added TODO, since I believe the former tests didn't verify this either. I'll address this in a follow-up patch. https://codereview.chromium.org/2703603002/diff/320001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:442: {FaviconURL(kIconURL16x16, FAVICON, kEmptySizes)}); On 2017/03/09 02:15:08, pkotwicz wrote: > Can you add RunHandlerWithGURLCandidates() which takes a std::vector<GURL> > and constructs FaviconURLs with favicon_base::FAVICON and empty sizes Done. https://codereview.chromium.org/2703603002/diff/320001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:560: FAVICON, _, _, _)) On 2017/03/09 02:15:08, pkotwicz wrote: > We don't need to check whether UpdateFaviconMappingsAndFetch() is called or not. > If a download was not done, FaviconHandler must have gotten the image data from > the database Needed unless we adopt NiceMock for FaviconService. https://codereview.chromium.org/2703603002/diff/320001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:565: kNewIconURL, /*icon_url_changed=*/true, _)); On 2017/03/09 02:15:08, pkotwicz wrote: > For the sake of completeness we should check that FaviconService::SetFavicons() > is not called Done. https://codereview.chromium.org/2703603002/diff/320001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:610: TEST_F(FaviconHandlerTest, UpdateDuringDownloading) { On 2017/03/09 02:15:09, pkotwicz wrote: > We want to: > - set initial state > - test the initial state > - run the test > - test the final state > > In order for the testing of the final state to check things which did not occur > as part of setting the initial state, you have to call > VerifyAndClearExpectations() > > You can probably put > VerifyAndClearExpectations(favicon_service_) and > VerifyAndClearExpectations(delegate_) into a function called > VerifyAndClearExpectCalls() Done. FYI, I've used VerifyAndClearExpectations in the past (https://cs.chromium.org/chromium/src/components/ntp_tiles/most_visited_sites_...) and you should know that it was always perceived as smell, although gmock documents this is a technique: https://g3doc.corp.google.com/third_party/gmock/g3doc/guide.md#UsingCheckPoints In this case, I believe what you have in mind is to make "achieve downloading state" as part of the test fixture. In the ideal world, the text fixture should be set without exercising the class under test, which is not possible here. The ugliness here is about the EXPECT_CALL(favicon_service_, ...).Times(AnyNumber()) that the code does on some trivial functions, without actually adopting a NiceMock for FaviconService. I suspect you might instead want to indeed adopt NiceMock for FaviconService, please let me know. https://codereview.chromium.org/2703603002/diff/320001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:622: EXPECT_CALL(favicon_service_, SetFavicons(_, kIconURL, _, _)).Times(0); On 2017/03/09 02:15:08, pkotwicz wrote: > We don't want SetFavicons() to be called at all for any URL. You can replace > kIconURL with '_' Done. https://codereview.chromium.org/2703603002/diff/320001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:632: /*expired=*/false))); On 2017/03/09 02:15:08, pkotwicz wrote: > We don't care about the UpdateFaviconMappingsAndFetch() calls in this test Needed unless we adopt NiceMock for FaviconService. https://codereview.chromium.org/2703603002/diff/320001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:635: kLatestIconURL, /*icon_url_changed=*/true, _)); On 2017/03/09 02:15:09, pkotwicz wrote: > kLatestIconUrl is the only parameter that we care about in the > OnFaviconUpdated() call Done. Note that my tests we covering the same expectations as before. https://codereview.chromium.org/2703603002/diff/320001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:637: std::unique_ptr<FaviconHandler> handler = RunHandlerWithCandidates( On 2017/03/09 02:15:08, pkotwicz wrote: > 1) Some of the old tests were testing too many things at once. > UpdateDuringDownloading() was one such test. Make this test deal with > FaviconURLs of type favicon_base::FAVICON > > In https://codereview.chromium.org/2738663002 I added > FaviconHandlerTest.OnlyDownloadMatchingIconType > FaviconHandlerTest.ShouldCallSetFaviconsWithCorrectIconType and to test the > behavior of touch FaviconURLs Adopted FAVICON and added TODO for the rest. I propose we add new tests in follow-up CLs, this was originally a refactoring of existing tests and is already quite huge. https://codereview.chromium.org/2703603002/diff/320001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:642: }); On 2017/03/09 02:15:08, pkotwicz wrote: > For the "verify the final state" part of this test you should check > EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, kLatestIconURL, _, _)); > EXPECT_CALL(favicon_service_, SetFavicons(_, kLatestIconURL, _, _)) > EXPECT_CALL(favicon_service_, UpdateFaviconMappingsAndFetch(_, > URLVector{kLatestIconURL}, _, _, _, _)); > EXPECT_CALL(delegate_, DownloadImage(_, _, _)).Times(0); Done, I added similar expectations but not exactly the ones above, since kLatestIconURL is actually downloaded. https://codereview.chromium.org/2703603002/diff/320001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:715: On 2017/03/09 02:15:08, pkotwicz wrote: > You should call VerifyExpectCalls() and have new expectations for the "test > final state" part of the test. > > In particular > EXPECT_CALL(favicon_service_, UpdateFaviconMappingsAndFetch(_, _, _, _, _, > _)).Times(0); > EXPECT_CALL(delegate_, DownloadImage(_, _, _)).Times(0); > EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, _, _, _)).Times(0); > EXPECT_CALL(favicon_service_, SetFavicons(_, _, _, _)).Times(0); Done. https://codereview.chromium.org/2703603002/diff/320001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:747: EXPECT_CALL(delegate_, DownloadImage(kIconURL2, _, _)).Times(2); On 2017/03/09 02:15:08, pkotwicz wrote: > - We only care about the downloads which occur in the "setup" part of the test. > The reason that we care about the downloads in the "setup" part is that we want > to ensure that FaviconHandler tried both candidates > > - For the setup of this test it is critical that the download of |kIconURL1| > occurs before the download of |kIconURL2|. > > The GMock syntax would be I guess > { > InSequence seq; > EXPECT_CALL(delegate_, DownloadImage(kIconURL1, _, _)); > EXPECT_CALL(delegate_, DownloadImage(kIconURL2, _, _)); > } > > When I am going through the CL I am comparing the assertions against those in my > version of FaviconHandlerTest.ShouldCallOnFaviconAvailableAfterIconURLChange Done. https://codereview.chromium.org/2703603002/diff/320001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:753: CreateRawBitmapResult(kIconURL1, FAVICON, /*expired=*/true))); On 2017/03/09 02:15:08, pkotwicz wrote: > It is unimportant for there to be data in the database for this test. The test > works just fine without any data in the database > > (It also results in a less insane number of calls to > Delegate::OnUpdateFaviconURL(). The insane number of calls to > Delegate::OnUpdateFaviconURL() is a bug.) Done. Please file a bug that describes the repeated notifications and assign it to me. https://codereview.chromium.org/2703603002/diff/320001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:773: }); On 2017/03/09 02:15:08, pkotwicz wrote: > We want to: > - set initial state > - test the initial state > - run the test > - test the final state > > In order for the testing of the final state to check things which did not occur > as part of setting the initial state, you have to call > VerifyAndClearExpectations() Done. https://codereview.chromium.org/2703603002/diff/320001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:963: { On 2017/03/09 02:15:08, pkotwicz wrote: > I think that it is easy to have bugs with this test. In particular, the order > that the bitmap sizes are downloaded is non obvious (the order that the icon > URLs are downloaded is obvious). > > I think that this would make the order that the bitmap sizes are downloaded > clearer: > > const FaviconURL kSourceIconURLs[] = { > ... > }; > struct ExpectedResult { > ... > } results[] = { > ... > }; > > { > InSequence seq; > for (const ExpectedResult& result : results) { > FaviconURL expected_favicon = kSourceIconURLs[result.favicon_index]; > if (result.edge_size > 0) { > EXPECT_THAT(favicon.icon_sizes, > testing::Contains(gfx::Size(result.edge_size, result.edge_size))); > } > EXPECT_CALL(delegate_, DownloadImage(expected_favicon.icon_url, _, _)); > } > } > > Alternatively, you could have different variables for each FaviconURL and name > the FaviconURLs based on the sizes. > For instance: > FaviconURL favicon_1024_and_512 = > FaviconURL(GURL("http://www.google.com/a"), > FAVICON, > {gfx::Size(1024, 1024), gfx::Size(512, 512)}); Done, went ahead with the second. https://codereview.chromium.org/2703603002/diff/320001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:979: .Times(5); On 2017/03/09 02:15:08, pkotwicz wrote: > We don't care about calls to UpdateFaviconMappingsAndFetch() here Needed unless we adopt a NiceMock for FaviconService. https://codereview.chromium.org/2703603002/diff/320001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1058: } On 2017/03/09 02:15:08, pkotwicz wrote: > We don't care about the UpdateFaviconMappingsAndFetch() calls in the context of > this test Ditto. https://codereview.chromium.org/2703603002/diff/320001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1063: ImageSizeIs(12, 12))); On 2017/03/09 02:15:08, pkotwicz wrote: > In my CL I have made all of the tests check for the Delegate::OnFaviconUpdated() > call. I understand that the old test calls FaviconService::SetFavicons() > > Checking for Delegate::OnFaviconUpdated() and FaviconService::SetFavicons() is > somewhat interchangeable. FaviconService::SetFavicons() is called a subset of > the times that Delegate::OnFaviconUpdated() is called. > > As usual, we only care about the selected icon URL. Thus > EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, kIconURL12x12, _, _)); Ditto: while FaviconService uses StrictMock, it's more convenient to verify SetFavicons.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
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_...)
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...)
In the interest of time, I think it is most expedient to proceed with landing my CL: https://codereview.chromium.org/2738663002/ Can you please do a code review? I can foresee us spending at least another week over this particular CL with likely not much progress. In the case of the tests of the tests with this file I have a very specific idea of what they should look like. So I guess, this decision is mostly based on my quirky opinions. I know that you are eager to move on to follow up CLs. In my implementation, I only used GMock where we want to check non standard method parameters (e.g. to check the image parameter in Delegate::OnFaviconUpdated()). Yes, I do roll my own mock however I think it is warranted. It avoids having EXPECT_CALL()s for 5 parameter functions where we only care about a single parameter. Rolling my own mock is nicer than having a function and calling the function to set expectations Example: void ExpectDownload(const GURL& url) { EXPECT_CALL(delegate_, DownloadImage(url, _, _); } I don't mind changing my CL to using EXPECT_CALL() for Delegate::OnFaviconUpdated() and FaviconService::SetFavicons() as a compromise. I don't want to use EXPECT_CALL() for Delegate::DownloadImage() in order to avoid too many InSequence blocks. I will annotate my CL as to how it maps to old tests to make code review easier ******************************************************************************************************************************************************************* Shorter tests tend to be more readable. People tend to be more enthusiastic about adding tests if the tests they need to write are short. Having complex statements like this one that you need to copy and paste is depressing: EXPECT_CALL(favicon_service_, UpdateFaviconMappingsAndFetch(kPageURL, URLVector{kNewIconURL}, FAVICON, _, _, _)) .WillOnce(PostReply<6>( CreateRawBitmapResult(kNewIconURL, FAVICON, /*expired=*/false))); The purpose of TestFaviconService::Store() is to have a short and easy way of setting what the result is going to be for FaviconService::GetFaviconImage() and FaviconService::UpdateFaviconMappingsAndFetch(). UpdateFaviconMappingsAndFetch() does map page URLs to icon URLs but none of the current tests care about this and I don't expect that future tests will. I don't think that using a real FaviconService at this time is warranted. From my comments for the previous patch set: I think that it would be nice to use EXPECT_THAT(Delegate::downloads(), ElementsAre()) EXPECT_THAT(FaviconService::db_requets(), ElementsAre()) Doing this saves quite a bit of typing and removes the need for several testing::InSequence blocks It also gets rid of the dreaded 5 argument EXPECT_CALL(favicon_service_, UpdateFaviconMappingAndFetch(_, kIconUrl, _, _, _)
To answer your question about NiceMock, yes I prefer NiceMock.
Thanks, PTAL! I hope most controversial points have converged in this version, let me know otherwise. On 2017/03/10 05:07:34, pkotwicz wrote: > In the interest of time, I think it is most expedient to proceed with landing my > CL: https://codereview.chromium.org/2738663002/ Can you please do a code review? > I can foresee us spending at least another week over this particular CL with > likely not much progress. I appreciate your commitment to unblock this front. Really, I honestly didn't expect so much effort you put into this! Working across time zones on such a problem is tricky and it's good we're looking into ways to improve that. I've however spent three weeks in this CL and I think it's fair to claim authorship and credits for most of the contributions here, so it's important for me to get this landed (if you want, I can revert a few changes like the comments I borrowed from your CL). > In the case of the tests of the tests with this file I > have a very specific idea of what they should look like. So I guess, this > decision is mostly based on my quirky opinions. I know that you are eager to > move on to follow up CLs. In my implementation, I only used GMock where we want > to check non standard method parameters (e.g. to check the image parameter in > Delegate::OnFaviconUpdated()). Yes, I do roll my own mock however I think it is > warranted. It avoids having EXPECT_CALL()s for 5 parameter functions where we > only care about a single parameter. Rolling my own mock is nicer than having a > function and calling the function to set expectations > > Example: > void ExpectDownload(const GURL& url) { > EXPECT_CALL(delegate_, DownloadImage(url, _, _); > } > > I don't mind changing my CL to using EXPECT_CALL() for > Delegate::OnFaviconUpdated() and FaviconService::SetFavicons() as a compromise. Appreciated! > I don't want to use EXPECT_CALL() for Delegate::DownloadImage() in order to > avoid too many InSequence blocks. Done, adopted downloads() and db_requests(). The exact implementation in the test doubles differs slightly and follows a more orthodox mock-to-delegate technique, but this doesn't affect the code in the tests themselves. You'll also see that VerifyAndClearExpectations() clears both lists. I wasn't sure about this so let me know if you prefer otherwise. > > I will annotate my CL as to how it maps to old tests to make code review easier Thanks! > > ******************************************************************************************************************************************************************* > > Shorter tests tend to be more readable. People tend to be more enthusiastic > about adding tests if the tests they need to write are short. Having complex > statements like this one that you need to copy and paste is depressing: > EXPECT_CALL(favicon_service_, > UpdateFaviconMappingsAndFetch(kPageURL, URLVector{kNewIconURL}, > FAVICON, _, _, _)) > .WillOnce(PostReply<6>( > CreateRawBitmapResult(kNewIconURL, FAVICON, > /*expired=*/false))); Replaced with db_requests(). > > > The purpose of TestFaviconService::Store() is to have a short and easy way of > setting what the result is going to be for FaviconService::GetFaviconImage() and > FaviconService::UpdateFaviconMappingsAndFetch(). UpdateFaviconMappingsAndFetch() > does map page URLs to icon URLs but none of the current tests care about this > and I don't expect that future tests will. I don't think that using a real > FaviconService at this time is warranted. > > From my comments for the previous patch set: > I think that it would be nice to use > EXPECT_THAT(Delegate::downloads(), ElementsAre()) > EXPECT_THAT(FaviconService::db_requets(), ElementsAre()) Done. > > Doing this saves quite a bit of typing and removes the need for several > testing::InSequence blocks > > It also gets rid of the dreaded 5 argument > EXPECT_CALL(favicon_service_, UpdateFaviconMappingAndFetch(_, kIconUrl, _, _, _) Also adopted NiceMock for FaviconService.
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_...)
Thank you for making the changes. I will look over your CL by Monday
I've gotten through most of the CL. Nothing major :) https://codereview.chromium.org/2703603002/diff/320001/components/favicon/cor... File components/favicon/core/favicon_handler_unittest.cc (right): https://codereview.chromium.org/2703603002/diff/320001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:433: kIconURL16x16, /*icon_url_changed=*/true, _)); I'd rather have separate tests which test the |icon_url_changed| parameter. It is confusing to have one test test multiple things. For the purpose of this test I think that we only care about the "icon URL" https://codereview.chromium.org/2703603002/diff/320001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:439: kIconURL16x16, /*icon_url_changed=*/false, _)); Please file a bug to do this :) https://codereview.chromium.org/2703603002/diff/320001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:637: std::unique_ptr<FaviconHandler> handler = RunHandlerWithCandidates( Adding new tests in a follow up CL sounds good to me https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... File components/favicon/core/favicon_handler_unittest.cc (right): https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:237: void Store(const GURL& page_or_icon_url, This function should take both a page_url and an icon_url. This makes its use more intuitive (and prevents testing scenarios which can't occur) https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:278: Invoke(&fake_, &FakeFaviconService::GetFaviconForPageOrIconURL))); Nit: Add to FakeFaviconService FakeFaviconService::GetFavicon(), FakeFaviconService::GetFaviconForPageURL() and FakeFaviconService::UpdateFaviconMappingsAndFetch() methods so that you don't need to WithArgs(). The less arcane GTest stuff the better. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:287: FakeFaviconService& fake() { return fake_; } Returning a non const reference is kind of weird. Can this be a pointer instead? https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:290: FakeFaviconService fake_; DISALLOW_COPY_AND_ASSIGN(MockFaviconServiceWithFake) https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:296: const std::vector<SkBitmap> kEmptyBitmaps; You don't seem to use |kEmptyBitmaps| anywhere https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:297: const std::vector<FaviconRawBitmapResult> kEmptyRawBitmapResult; You don't seem to use |kEmptyRawBitmapResult| anywhere. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:311: delegate_.fake_downloader().Add(kIconURL64x64, IntVector{64}); Super Nit: Can you please add a new line? https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:334: bool flush_events_before_candidates = true) { In my opinion, tests which care about |flush_events_before_candidates| should construct the FaviconHandler manually. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:370: const IntVector& candidate_icon_sizes) { It looks like all of the tests which call this function: - construct the FaviconURLs with URLs we don't care out - use icon type FAVICON - use kEmptySizes Can you generate the FaviconURLs via base::StringPrintf() and a static int e.g. "https://www.google.com/generated/1" https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:381: .Times(AtMost(1)); Like the majority of tests this function does not care about calls to FaviconService::SetFavicons() https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:386: /*icon_url_changed=*/true, _)) The icon URL is the only parameter that we care about. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:419: For the comment, how about: Test that the FaviconHandler process finishes when: - There is no data in the database for neither the page URL nor the icon URL. AND - FaviconService::GetFaviconForPageURL() callback returns before FaviconHandler::OnUpdateFaviconURL() is called. Maybe rename the test to "DownloadUnknownFaviconIfCandidatesSlower" https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:429: kIconURL16x16, /*icon_url_changed=*/true, _)); For the sake of this test, can you call FaviconHandler::FetchFavicon() and FaviconHandler::OnUpdateFaviconURL() explicitly instead of via RunHandlerWithCandidates(). https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:434: For the comment, how about: "Test that the FaviconHandler process does not save anything to the database for incognito tabs." https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:443: InSequence seq; InSequence when there is just one EXPECT_CALL() in scope is kind of pointless https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:445: OnFaviconUpdated(_, _, _, /*icon_url_changed=*/true, _)); We care about the "icon URL" parameter but not the "icon_url_changed" parameter. If we want to test the "icon_url_changed" parameter we can add new tests which test just the parameter https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:450: For the comment, how about: Test that the FaviconHandler process finishes when: - There is no data in the database for neither the page URL nor the icon URL. AND - FaviconService::GetFaviconForPageURL() callback returns after FaviconHandler::OnUpdateFaviconURL() is called. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:463: /*flush_events_before_candidates=*/false); For the sake of this test, can you call FaviconHandler::FetchFavicon() and FaviconHandler::OnUpdateFaviconURL() explicitly instead of via RunHandlerWithCandidates(). I tested this via the magic of FakeFaviconService::SetRunCallbackManuallyForUrl(). That way I was able to test that FaviconHandler would not complete if the database for kPageURL is not fetched https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:466: Nits: How about this as the test comment: - "Test that the icon is redownloaded if the icon cached for the page URL expired." - Rename this test to RedownloadExpiredPageUrlFavicon (This makes the test name more specific. It is possible that the icon at kIconURL16x16 is expired but there is no mapping from kPageURL to kIconURL16x16) https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:473: .Times(0); This should be EXPECT_THAT(favicon_service_.fake().db_requests(), ElementsAre(kPageURL)); Can you please add this comment for this expectation: "We know from the |kPageUrl| database request that |kIconURL16x16| has expired. A second request for |kIconURL16x16| should not have been made because it is redundant." https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:477: OnFaviconUpdated(_, _, _, /*icon_url_changed=*/true, _)); kIconURL16x16 should be part of the EXPECT_CALL() statement https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:479: ImageSizeIs(16, 16))); kIconURL16x16 is the only parameter that we care about for the SetFavicons() call https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:529: _, _, _)); Nit: We don't care about whether UpdateFaviconMappingsAndFetch() is called https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:532: /*icon_url_changed=*/true, _)); We don't care about the |icon_url_changed| parameter in OnFaviconUpdated(). If we want to test the icon_url_changed parameter we can add extra tests which test just that https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:537: For the comment, how about: If there is data for the page URL in history which is invalid, test that: - the invalid data is not sent to the UI. - the icon is redownloaded. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:551: EXPECT_CALL(favicon_service_, SetFavicons(kPageURL, kIconURL16x16, _, _)); We don't care about the FaviconService::SetFavicons() call for the purpose of this test https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:554: kIconURL16x16, /*icon_url_changed=*/true, _)); We don't care about any of the parameters to Delegate::OnFaviconUpdated() except the icon URL. This would be a good test to check the color of the gfx::Image parameter. You can't truly test that the invalid data is not sent in the UI without checking the gfx::Image parameter. I believe that in a different comment your requested to do this as part of a separate CL https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:556: RunHandlerWithSimpleFaviconCandidates({kIconURL16x16}); You should also check that |kPageURL| was requested from the databse. That's kind of the entire point of the test (Perhaps a bit redundant because all tests request kPageURL from the database) https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:570: /*expired=*/false)); I think that the code would be more readable if Store() took both a page URL and an icon URL. This would better document: "How is it possible for there to be data cached for |kNewIconUrl|"? https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:577: kIconURL, /*icon_url_changed=*/true, _)); This test doesn't care about any of the parameters of OnFaviconUpdated() except of |kIconUrl|. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:580: FAVICON, _, _, _)); This should be replaced with EXPECT_THAT(favicon_service_.fake().db_requests(), ElementsAre(kNewIconURL)); I don't think that it is important to test that UpdateFaviconMappingsAndFetch() is called only after the Delegate::OnFaviconUpdated() https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:583: kNewIconURL, /*icon_url_changed=*/true, _)); This test doesn't care about any of the parameters of OnFaviconUpdated() except of |kIconUrl|. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:620: const GURL kNewIconURL = kIconURL16x16; I know that the old test used the name |kNewIconURL|. Can we use a different constant name? |kIconURL1| and |kIconURL2| perhaps? The constant name kIconURL does not make sense in the context of this test https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:626: delegate_.fake_downloader().Add(kIconURL, IntVector{16}); To stay true to the original test (and your previous patch set), there should be an entry in the database for kLatestIconURL https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:636: EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, kIconURL, _, _)).Times(0); Don't the EXPECT_CALL() statements on lines 638 and 639 implicitly test what's on lines 635 and 636? Perhaps we should move these EXPECT_CALL() statements above VerifyAndClearExpectations() but I am not particular https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:644: // Finalizes download, which should be thrown away as there is favicon update. Nit: "as there is favicon update." -> "as the favicon URLs were updated." https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:655: TEST_F(FaviconHandlerTest, UpdateSameIconURLs) { You should split this up into two tests: - One test which verifies that calling FaviconHandler::OnUpdateFaviconURL() after the FaviconHandler is done does not cause any database requests or downloads - One test which verifies that calling FaviconHandler::OnUpdateFaviconURL() when FaviconHandler is in the middle of processing is a no-op https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:670: Probably also worth checking: - that FaviconService::SetFavicons() and Delegate::OnFaviconUpdated() were not called - which downloads were done https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:675: We should verify that nothing did in fact happen. Thus: EXPECT_CALL(favicon_service_, SetFavicons(_, _, _, _)).Times(0); EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, _, _, _)).Times(0); EXPECT_THAT(favicon_service_.db_requests(), IsEmpty()); EXPECT_THAT(delegate_.downloads(), IsEmpty()); https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:680: We should VerifyAndClearExpectations() now. (Otherwise it is possible for the call to FaviconHandler::OnUpdateFaviconURL() on line 678 to have haad an effect https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:691: // finishes loading. This can occur several for pages with iframes. "several for pages with iframes." -> "several times for pages with iframes." https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:699: FaviconDriverObserver::NON_TOUCH_16_DIP, favicon_urls); Nit: You can use RunHandlerWithSimpleFaviconCandidates() here https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:708: base::RunLoop().RunUntilIdle(); Might as well check that there haven't been any database requests https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:729: // - The database doesn't know about |kIconURL1| or |kIconURl2|. Might as well delete this comment. Hopefully the shorter test should make the initial state clearer https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:737: // |kIconURL1|, one before and one after the download. This comment seems to be out of date. How about: "|kIconURL1| is the better match." I think that you can remove the comment on lines 731 - 732 https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:739: EXPECT_CALL(favicon_service_, SetFavicons(kPageURL, kIconURL1, _, _)); Calls to FaviconService::SetFavicons() are unimportant in this test https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:745: // occurs before the download of |kIconURL2|. How about: "Both |kIconURL1| and |kIconURL2| should have been requested from the database and downloaded. |kIconURL2| should have been fetched from the database and downloaded last." https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:747: // FaviconHandler should have asked for the mappings. I think that you can remove this comment in favor of my suggestion for the comment on lines 744 - 745 https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:755: /*icon_url_changed=*/true, _)); Nit: We only care about the icon URL parameter to OnFaviconUpdated() https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:793: // that the largest exact match is chosen. For the comment, how about: Tests that running FaviconHandler - on an OS which supports the 1x and 2x scale factor - on a page with <link rel="icon"> tags with no "sizes" information selects the largest exact match. Note that a 32x32 PNG image is not a "true exact match" on an OS which supports an 1x and 2x. A "true exact match" is a .ico file with 16x16 and 32x32 bitmaps. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:808: ChooseMinorResamplingOverHugeFavicons) { Rename this test to ChooseMinorDownsamplingOverHugeIcon https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:813: ChooseMinorResamplingOverHugeFavicons2) { Rename this test to ChooseMinorUpsamplingOverHugeIcon https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:830: IntVector{16, 64})); This test does not test the same functionality as the old test. This test will not try to download the 64x64 icon at all because the 16x16 icon is a perfect match on a 1x-density-only machine. In order to differentiate this test from MultipleFaviconsLast404, the 404 icon should not be last. Given that you have already set data to be downloaded for kIconURL16x16 and kIconURL64x64 in the test fixture I would recommend not using DownloadTillDoneIgnoringHistory(). https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:838: TEST_F(FaviconHandlerTest, MultipleFaviconsLast404) { - You can simplify this test case by providing no sizes to the FaviconURLs. This will force both icons to be downloaded. - Using DownloadTillDoneIgnoringHistory() isn't great because you are not testing that both icons got downloaded. It should be pretty easy to setup test without using DownloadTillDoneIgnoringHistory() https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:885: FaviconURL(GURL("http://www.google.com/b"), FAVICON, Might as well alphabetize the URLs with 'a' coming first (i.e. make this one 'a') https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:886: {gfx::Size(15, 15), gfx::Size(16, 16)}), I think that it would be more interesting if this favicon was 1x1 and 17x17 (This way the multi resolution icon has a bitmap which is both larger than 16x16 and smaller than 16x16) If the goal is to test that the order of the favicon URLs is not altered if the favicons are all the same size we should specifically call this out https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:946: // size. Please add to the comment: "... which allows checking the order that the favicons were downloaded. The favicons should have been requested in decreasing order of their sizes. Favicons without any <link sizes=""> attribute should have been downloaded last." https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:960: SetFavicons(kPageURL, kIconURL2, FAVICON, ImageSizeIs(16, 16))); Nit: Remove the check for SetFavicons(). I know that the old test has it but it is unnecessary https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:964: kIconURL2, /*icon_url_changed=*/true, _)); Nit: We don't care about any of the parameters except for |kIconURL2| https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:969: FaviconURL(kIconURL2, FAVICON, {gfx::Size(14, 14), gfx::Size(16, 16)})}); Can you test which images got downloaded. Otherwise, it is possible for the test to pass because downloading kIconURL1 causes a 404 A nice way of getting rid of some boiler plate is to add a method to the FakeDelegate which adds images for an array of FaviconURLs FakeDelegate::AddForFaviconURLs(const std::vector<FaviconURL>& favicons); https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:988: ImageSizeIs(kMaximalSize, kMaximalSize))); Nit: Make this test check Delegate::OnFaviconUpdated() As usual, we don't care about the page URL or the icon type passed to Delegate::OnFaviconUpdated(). Checking for the image size is OK https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:995: SizeVector{gfx::Size(kOriginalSize2, kOriginalSize2)})}); Nit: Can you also test what images were downloaded. (Otherwise, it is possible that the test passes because downloading kIconURL1 causes a 404) https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:997: For the comment, how about: "Test that if several icons are downloaded because the icons are smaller than expected that OnFaviconUpdated() is called with the largest downloaded bitmap." https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1000: ImageSizeIs(12, 12))); Now that FaviconService no longer users StrictMock we can make this test check OnFaviconUpdated() As usual, we only care about the icon URL in the call to Delegate::OnFaviconUpdated() We can add a test in a follow up CL to check that Delegate::OnFaviconUpdated() is called with an unresized image
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 FaviconHandler tests by testing public API Instead of subclassing the class under test to bypass/intercept internal calls, let's instead verify external interactions by injecting test doubles and testing the public API. The new are more concise by adopting mocks to verify call arguments, ordering, and counts, which is what the home-grown test doubles were doing anyway prior to this patch. This greatly reduces the size of tests to 50%, ~900 LoC less! Tests otherwise verify the very same expectations. As bonus point, a few new tests are added, such as test coverage for incognito (DownloadUnknownFaviconInIncognito) or a corner case that is prone to regressions (MultipleFaviconsLast404). BUG=694312 ========== to ========== Improve FaviconHandler tests by testing public API No behavioral changes: instead of subclassing the class under test to bypass/intercept internal calls, let's instead verify external interactions by injecting test doubles and testing the public API. The new are more concise by adopting mocks to verify call arguments or fakes (mock-to-fake delegation) for the simplest cases. This greatly reduces the size of tests to ~50%, ~1000 LoC less! The actual diffstat is smaller due to newly added tests, increasing the total number of tests from 18 to 28, including basic test coverage for incognito (DownloadUnknownFaviconInIncognito) or a corner case that is prone to regressions (MultipleFaviconsLast404). BUG=694312 ==========
Description was changed from ========== Improve FaviconHandler tests by testing public API No behavioral changes: instead of subclassing the class under test to bypass/intercept internal calls, let's instead verify external interactions by injecting test doubles and testing the public API. The new are more concise by adopting mocks to verify call arguments or fakes (mock-to-fake delegation) for the simplest cases. This greatly reduces the size of tests to ~50%, ~1000 LoC less! The actual diffstat is smaller due to newly added tests, increasing the total number of tests from 18 to 28, including basic test coverage for incognito (DownloadUnknownFaviconInIncognito) or a corner case that is prone to regressions (MultipleFaviconsLast404). BUG=694312 ========== to ========== Improve FaviconHandler tests by testing public API No behavioral changes: instead of subclassing the class under test to bypass/intercept internal calls, let's instead verify external interactions by injecting test doubles and testing the public API. The new ones are more concise by adopting mocks to verify call arguments or fakes (mock-to-fake delegation) for the simplest cases. This greatly reduces the size of tests to ~50%, ~1000 LoC less! The actual diffstat is smaller due to newly added tests, increasing the total number of tests from 18 to 28, including basic test coverage for incognito (DownloadUnknownFaviconInIncognito) or a corner case that is prone to regressions (MultipleFaviconsLast404). BUG=694312 ==========
Thanks, PTAL! Addressed all comments except a few that need clarification in UpdateSameIconURLs. https://codereview.chromium.org/2703603002/diff/320001/components/favicon/cor... File components/favicon/core/favicon_handler_unittest.cc (right): https://codereview.chromium.org/2703603002/diff/320001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:433: kIconURL16x16, /*icon_url_changed=*/true, _)); On 2017/03/13 05:16:31, pkotwicz wrote: > I'd rather have separate tests which test the |icon_url_changed| parameter. It > is confusing to have one test test multiple things. For the purpose of this test > I think that we only care about the "icon URL" Done. https://codereview.chromium.org/2703603002/diff/320001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:439: kIconURL16x16, /*icon_url_changed=*/false, _)); On 2017/03/13 05:16:31, pkotwicz wrote: > Please file a bug to do this :) Done. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... File components/favicon/core/favicon_handler_unittest.cc (right): https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:237: void Store(const GURL& page_or_icon_url, On 2017/03/13 05:16:32, pkotwicz wrote: > This function should take both a page_url and an icon_url. This makes its use > more intuitive (and prevents testing scenarios which can't occur) Done. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:278: Invoke(&fake_, &FakeFaviconService::GetFaviconForPageOrIconURL))); On 2017/03/13 05:16:32, pkotwicz wrote: > Nit: Add to FakeFaviconService FakeFaviconService::GetFavicon(), > FakeFaviconService::GetFaviconForPageURL() and > FakeFaviconService::UpdateFaviconMappingsAndFetch() methods so that you don't > need to WithArgs(). > > The less arcane GTest stuff the better. Done. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:287: FakeFaviconService& fake() { return fake_; } On 2017/03/13 05:16:32, pkotwicz wrote: > Returning a non const reference is kind of weird. Can this be a pointer instead? Done. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:290: FakeFaviconService fake_; On 2017/03/13 05:16:31, pkotwicz wrote: > DISALLOW_COPY_AND_ASSIGN(MockFaviconServiceWithFake) Done. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:296: const std::vector<SkBitmap> kEmptyBitmaps; On 2017/03/13 05:16:33, pkotwicz wrote: > You don't seem to use |kEmptyBitmaps| anywhere Done, thx. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:297: const std::vector<FaviconRawBitmapResult> kEmptyRawBitmapResult; On 2017/03/13 05:16:31, pkotwicz wrote: > You don't seem to use |kEmptyRawBitmapResult| anywhere. Done. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:311: delegate_.fake_downloader().Add(kIconURL64x64, IntVector{64}); On 2017/03/13 05:16:33, pkotwicz wrote: > Super Nit: Can you please add a new line? Done. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:334: bool flush_events_before_candidates = true) { On 2017/03/13 05:16:31, pkotwicz wrote: > In my opinion, tests which care about |flush_events_before_candidates| should > construct the FaviconHandler manually. Done. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:370: const IntVector& candidate_icon_sizes) { On 2017/03/13 05:16:33, pkotwicz wrote: > It looks like all of the tests which call this function: > - construct the FaviconURLs with URLs we don't care out > - use icon type FAVICON > - use kEmptySizes > > Can you generate the FaviconURLs via base::StringPrintf() and a static int > e.g. "https://www.google.com/generated/1" Done. For the URL, I used the size instead, e.g. 16x16. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:381: .Times(AtMost(1)); On 2017/03/13 05:16:32, pkotwicz wrote: > Like the majority of tests this function does not care about calls to > FaviconService::SetFavicons() Done. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:386: /*icon_url_changed=*/true, _)) On 2017/03/13 05:16:34, pkotwicz wrote: > The icon URL is the only parameter that we care about. Done. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:419: On 2017/03/13 05:16:33, pkotwicz wrote: > For the comment, how about: > Test that the FaviconHandler process finishes when: > - There is no data in the database for neither the page URL nor the icon URL. > AND > - FaviconService::GetFaviconForPageURL() callback returns before > FaviconHandler::OnUpdateFaviconURL() is called. > > Maybe rename the test to "DownloadUnknownFaviconIfCandidatesSlower" Done. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:429: kIconURL16x16, /*icon_url_changed=*/true, _)); On 2017/03/13 05:16:32, pkotwicz wrote: > For the sake of this test, can you call FaviconHandler::FetchFavicon() and > FaviconHandler::OnUpdateFaviconURL() explicitly instead of via > RunHandlerWithCandidates(). Done. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:434: On 2017/03/13 05:16:34, pkotwicz wrote: > For the comment, how about: > > "Test that the FaviconHandler process does not save anything to the database for > incognito tabs." Done. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:443: InSequence seq; On 2017/03/13 05:16:31, pkotwicz wrote: > InSequence when there is just one EXPECT_CALL() in scope is kind of pointless Done, this was a leftover. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:445: OnFaviconUpdated(_, _, _, /*icon_url_changed=*/true, _)); On 2017/03/13 05:16:32, pkotwicz wrote: > We care about the "icon URL" parameter but not the "icon_url_changed" parameter. > If we want to test the "icon_url_changed" parameter we can add new tests which > test just the parameter Done. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:450: On 2017/03/13 05:16:34, pkotwicz wrote: > For the comment, how about: > Test that the FaviconHandler process finishes when: > - There is no data in the database for neither the page URL nor the icon URL. > AND > - FaviconService::GetFaviconForPageURL() callback returns after > FaviconHandler::OnUpdateFaviconURL() is called. Done. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:463: /*flush_events_before_candidates=*/false); On 2017/03/13 05:16:31, pkotwicz wrote: > For the sake of this test, can you call FaviconHandler::FetchFavicon() and > FaviconHandler::OnUpdateFaviconURL() explicitly instead of via > RunHandlerWithCandidates(). > > I tested this via the magic of > FakeFaviconService::SetRunCallbackManuallyForUrl(). That way I was able to test > that FaviconHandler would not complete if the database for kPageURL is not > fetched Done. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:466: On 2017/03/13 05:16:33, pkotwicz wrote: > Nits: > How about this as the test comment: > - "Test that the icon is redownloaded if the icon cached for the page URL > expired." > - Rename this test to RedownloadExpiredPageUrlFavicon > (This makes the test name more specific. It is possible that the icon at > kIconURL16x16 is expired but there is no mapping from kPageURL to kIconURL16x16) Done. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:473: .Times(0); On 2017/03/13 05:16:31, pkotwicz wrote: > This should be EXPECT_THAT(favicon_service_.fake().db_requests(), > ElementsAre(kPageURL)); > > Can you please add this comment for this expectation: > "We know from the |kPageUrl| database request that |kIconURL16x16| has expired. > A second request for |kIconURL16x16| should not have been made because it is > redundant." Done. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:477: OnFaviconUpdated(_, _, _, /*icon_url_changed=*/true, _)); On 2017/03/13 05:16:33, pkotwicz wrote: > kIconURL16x16 should be part of the EXPECT_CALL() statement Done. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:479: ImageSizeIs(16, 16))); On 2017/03/13 05:16:31, pkotwicz wrote: > kIconURL16x16 is the only parameter that we care about for the SetFavicons() > call Done. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:529: _, _, _)); On 2017/03/13 05:16:32, pkotwicz wrote: > Nit: We don't care about whether UpdateFaviconMappingsAndFetch() is called Done. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:532: /*icon_url_changed=*/true, _)); On 2017/03/13 05:16:33, pkotwicz wrote: > We don't care about the |icon_url_changed| parameter in OnFaviconUpdated(). > > If we want to test the icon_url_changed parameter we can add extra tests which > test just that Done. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:537: On 2017/03/13 05:16:32, pkotwicz wrote: > For the comment, how about: > > If there is data for the page URL in history which is invalid, test that: > - the invalid data is not sent to the UI. > - the icon is redownloaded. Done. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:551: EXPECT_CALL(favicon_service_, SetFavicons(kPageURL, kIconURL16x16, _, _)); On 2017/03/13 05:16:31, pkotwicz wrote: > We don't care about the FaviconService::SetFavicons() call for the purpose of > this test Done. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:554: kIconURL16x16, /*icon_url_changed=*/true, _)); On 2017/03/13 05:16:31, pkotwicz wrote: > We don't care about any of the parameters to Delegate::OnFaviconUpdated() except > the icon URL. > > This would be a good test to check the color of the gfx::Image parameter. You > can't truly test that the invalid data is not sent in the UI without checking > the gfx::Image parameter. I believe that in a different comment your requested > to do this as part of a separate CL Done and added TODO. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:556: RunHandlerWithSimpleFaviconCandidates({kIconURL16x16}); On 2017/03/13 05:16:32, pkotwicz wrote: > You should also check that |kPageURL| was requested from the databse. That's > kind of the entire point of the test > > (Perhaps a bit redundant because all tests request kPageURL from the database) Done. You might want to replace ElementsAre(kPageURL, kIconURL) with Contains(kIconURL) in other tests. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:570: /*expired=*/false)); On 2017/03/13 05:16:32, pkotwicz wrote: > I think that the code would be more readable if Store() took both a page URL and > an icon URL. > > This would better document: "How is it possible for there to be data cached for > |kNewIconUrl|"? Done. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:577: kIconURL, /*icon_url_changed=*/true, _)); On 2017/03/13 05:16:32, pkotwicz wrote: > This test doesn't care about any of the parameters of OnFaviconUpdated() except > of |kIconUrl|. Done. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:580: FAVICON, _, _, _)); On 2017/03/13 05:16:31, pkotwicz wrote: > This should be replaced with EXPECT_THAT(favicon_service_.fake().db_requests(), > ElementsAre(kNewIconURL)); > > I don't think that it is important to test that UpdateFaviconMappingsAndFetch() > is called only after the Delegate::OnFaviconUpdated() Done. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:583: kNewIconURL, /*icon_url_changed=*/true, _)); On 2017/03/13 05:16:33, pkotwicz wrote: > This test doesn't care about any of the parameters of OnFaviconUpdated() except > of |kIconUrl|. Done. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:620: const GURL kNewIconURL = kIconURL16x16; On 2017/03/13 05:16:34, pkotwicz wrote: > I know that the old test used the name |kNewIconURL|. Can we use a different > constant name? |kIconURL1| and |kIconURL2| perhaps? > > The constant name kIconURL does not make sense in the context of this test Done. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:626: delegate_.fake_downloader().Add(kIconURL, IntVector{16}); On 2017/03/13 05:16:33, pkotwicz wrote: > To stay true to the original test (and your previous patch set), there should be > an entry in the database for kLatestIconURL Done. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:636: EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, kIconURL, _, _)).Times(0); On 2017/03/13 05:16:33, pkotwicz wrote: > Don't the EXPECT_CALL() statements on lines 638 and 639 implicitly test what's > on lines 635 and 636? > > Perhaps we should move these EXPECT_CALL() statements above > VerifyAndClearExpectations() but I am not particular They implicitly do, but I wanted to be more explicit here. Anyway, removed. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:644: // Finalizes download, which should be thrown away as there is favicon update. On 2017/03/13 05:16:34, pkotwicz wrote: > Nit: "as there is favicon update." -> "as the favicon URLs were updated." Done. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:655: TEST_F(FaviconHandlerTest, UpdateSameIconURLs) { On 2017/03/13 05:16:32, pkotwicz wrote: > You should split this up into two tests: > - One test which verifies that calling FaviconHandler::OnUpdateFaviconURL() > after the FaviconHandler is done does not cause any database requests or > downloads > - One test which verifies that calling FaviconHandler::OnUpdateFaviconURL() when > FaviconHandler is in the middle of processing is a no-op Isn't the first precisely covered by UpdateSameIconURLsAfterFinished? Renamed tests to make this even more obvious, and I suspect some of the following comments could be moot points. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:670: On 2017/03/13 05:16:33, pkotwicz wrote: > Probably also worth checking: > - that FaviconService::SetFavicons() and Delegate::OnFaviconUpdated() were not > called > - which downloads were done These are all verified later below. Are you suggesting we should verify the correctness of the test fixture? https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:675: On 2017/03/13 05:16:33, pkotwicz wrote: > We should verify that nothing did in fact happen. > > Thus: > EXPECT_CALL(favicon_service_, SetFavicons(_, _, _, _)).Times(0); > EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, _, _, _)).Times(0); > EXPECT_THAT(favicon_service_.db_requests(), IsEmpty()); > EXPECT_THAT(delegate_.downloads(), IsEmpty()); One was missing in UpdateSameIconURLsAfterFinishedShouldBeNoop(), added there. I'll assume this is what you had in mind, let me know otherwise. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:680: On 2017/03/13 05:16:31, pkotwicz wrote: > We should VerifyAndClearExpectations() now. (Otherwise it is possible for the > call to FaviconHandler::OnUpdateFaviconURL() on line 678 to have haad an effect 678 is precisely the code under test and should follow VerifyAndClearExpectations(). This was perhaps misleading with the old test name. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:691: // finishes loading. This can occur several for pages with iframes. On 2017/03/13 05:16:32, pkotwicz wrote: > "several for pages with iframes." -> "several times for pages with iframes." Done. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:699: FaviconDriverObserver::NON_TOUCH_16_DIP, favicon_urls); On 2017/03/13 05:16:31, pkotwicz wrote: > Nit: You can use RunHandlerWithSimpleFaviconCandidates() here Done. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:708: base::RunLoop().RunUntilIdle(); On 2017/03/13 05:16:32, pkotwicz wrote: > Might as well check that there haven't been any database requests Done. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:729: // - The database doesn't know about |kIconURL1| or |kIconURl2|. On 2017/03/13 05:16:32, pkotwicz wrote: > Might as well delete this comment. Hopefully the shorter test should make the > initial state clearer Done. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:737: // |kIconURL1|, one before and one after the download. On 2017/03/13 05:16:32, pkotwicz wrote: > This comment seems to be out of date. > > How about: "|kIconURL1| is the better match." > > I think that you can remove the comment on lines 731 - 732 Done. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:739: EXPECT_CALL(favicon_service_, SetFavicons(kPageURL, kIconURL1, _, _)); On 2017/03/13 05:16:33, pkotwicz wrote: > Calls to FaviconService::SetFavicons() are unimportant in this test Done. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:745: // occurs before the download of |kIconURL2|. On 2017/03/13 05:16:32, pkotwicz wrote: > How about: "Both |kIconURL1| and |kIconURL2| should have been requested from the > database and downloaded. |kIconURL2| should have been fetched from the database > and downloaded last." Done. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:747: // FaviconHandler should have asked for the mappings. On 2017/03/13 05:16:33, pkotwicz wrote: > I think that you can remove this comment in favor of my suggestion for the > comment on lines 744 - 745 Done. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:755: /*icon_url_changed=*/true, _)); On 2017/03/13 05:16:31, pkotwicz wrote: > Nit: We only care about the icon URL parameter to OnFaviconUpdated() Done. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:793: // that the largest exact match is chosen. On 2017/03/13 05:16:33, pkotwicz wrote: > For the comment, how about: > > Tests that running FaviconHandler > - on an OS which supports the 1x and 2x scale factor > - on a page with <link rel="icon"> tags with no "sizes" information selects the > largest exact match. Note that a 32x32 PNG image is not a "true > exact match" on an OS which supports an 1x and 2x. A "true exact match" is a > .ico file with 16x16 and 32x32 bitmaps. Done. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:808: ChooseMinorResamplingOverHugeFavicons) { On 2017/03/13 05:16:32, pkotwicz wrote: > Rename this test to ChooseMinorDownsamplingOverHugeIcon Done. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:813: ChooseMinorResamplingOverHugeFavicons2) { On 2017/03/13 05:16:33, pkotwicz wrote: > Rename this test to ChooseMinorUpsamplingOverHugeIcon Done. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:830: IntVector{16, 64})); On 2017/03/13 05:16:31, pkotwicz wrote: > This test does not test the same functionality as the old test. This test will > not try to download the 64x64 icon at all because the 16x16 icon is a perfect > match on a 1x-density-only machine. > > In order to differentiate this test from MultipleFaviconsLast404, the 404 icon > should not be last. > > Given that you have already set data to be downloaded for kIconURL16x16 and > kIconURL64x64 in the test fixture I would recommend not using > DownloadTillDoneIgnoringHistory(). Done. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:838: TEST_F(FaviconHandlerTest, MultipleFaviconsLast404) { On 2017/03/13 05:16:31, pkotwicz wrote: > - You can simplify this test case by providing no sizes to the FaviconURLs. This > will force both icons to be downloaded. > > - Using DownloadTillDoneIgnoringHistory() isn't great because you are not > testing that both icons got downloaded. It should be pretty easy to setup test > without using DownloadTillDoneIgnoringHistory() Done. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:885: FaviconURL(GURL("http://www.google.com/b"), FAVICON, On 2017/03/13 05:16:32, pkotwicz wrote: > Might as well alphabetize the URLs with 'a' coming first (i.e. make this one > 'a') Done. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:886: {gfx::Size(15, 15), gfx::Size(16, 16)}), On 2017/03/13 05:16:33, pkotwicz wrote: > I think that it would be more interesting if this favicon was > 1x1 and 17x17 (This way the multi resolution icon has a bitmap which is both > larger than 16x16 and smaller than 16x16) > > If the goal is to test that the order of the favicon URLs is not altered if the > favicons are all the same size we should specifically call this out Done. The original goal was to stick to previous tests as much as possible. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:946: // size. On 2017/03/13 05:16:33, pkotwicz wrote: > Please add to the comment: > "... which allows checking the order that the favicons were downloaded. The > favicons should have been requested in decreasing order of their sizes. Favicons > without any <link sizes=""> attribute should have been downloaded last." Done. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:960: SetFavicons(kPageURL, kIconURL2, FAVICON, ImageSizeIs(16, 16))); On 2017/03/13 05:16:31, pkotwicz wrote: > Nit: Remove the check for SetFavicons(). I know that the old test has it but it > is unnecessary Done. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:964: kIconURL2, /*icon_url_changed=*/true, _)); On 2017/03/13 05:16:34, pkotwicz wrote: > Nit: We don't care about any of the parameters except for |kIconURL2| Done, but leaving NON_TOUCH_LARGEST because I believe this is the only test where it's verified, and it's quite relevant for the test name. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:969: FaviconURL(kIconURL2, FAVICON, {gfx::Size(14, 14), gfx::Size(16, 16)})}); On 2017/03/13 05:16:32, pkotwicz wrote: > Can you test which images got downloaded. Otherwise, it is possible for the test > to pass because downloading kIconURL1 causes a 404 > > A nice way of getting rid of some boiler plate is to add a method to the > FakeDelegate which adds images for an array of FaviconURLs > > FakeDelegate::AddForFaviconURLs(const std::vector<FaviconURL>& favicons); Done, I registered kIconURL1 as available download and verified downloads(). Since I suspect AddForFaviconURLs() would be used in this single test, I'm leaning towards making the test self contained and not adding yet another convenience function. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:988: ImageSizeIs(kMaximalSize, kMaximalSize))); On 2017/03/13 05:16:33, pkotwicz wrote: > Nit: Make this test check Delegate::OnFaviconUpdated() > > As usual, we don't care about the page URL or the icon type passed to > Delegate::OnFaviconUpdated(). Checking for the image size is OK Done. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:995: SizeVector{gfx::Size(kOriginalSize2, kOriginalSize2)})}); On 2017/03/13 05:16:33, pkotwicz wrote: > Nit: Can you also test what images were downloaded. (Otherwise, it is possible > that the test passes because downloading kIconURL1 causes a 404) Done. This however seems wrong because it's trying to detect bugs in test code. I registered kIconURL1 as available download, which should've been enough IMO. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:997: On 2017/03/13 05:16:31, pkotwicz wrote: > For the comment, how about: > "Test that if several icons are downloaded because the icons are smaller than > expected that OnFaviconUpdated() is called with the largest downloaded bitmap." Done. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1000: ImageSizeIs(12, 12))); On 2017/03/13 05:16:34, pkotwicz wrote: > Now that FaviconService no longer users StrictMock we can make this test check > OnFaviconUpdated() > > As usual, we only care about the icon URL in the call to > Delegate::OnFaviconUpdated() > > We can add a test in a follow up CL to check that Delegate::OnFaviconUpdated() > is called with an unresized image Done.
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_...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
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...
FYI, you'll see additional changes to inject scale factors. As it turns out, ui::test::ScopedSetSupportedScaleFactors doesn't work on Android, and the tests were failing. I think this is anyway a desirable change and in line with my future plans.
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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
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...
On 2017/03/13 15:01:41, mastiz wrote: > FYI, you'll see additional changes to inject scale factors. As it turns out, > ui::test::ScopedSetSupportedScaleFactors doesn't work on Android, and the tests > were failing. > > I think this is anyway a desirable change and in line with my future plans. Sorry, I need to take this back, so I reverted the changes. It seems the bug was in my test fixture, presumably fixed now.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Just nits! https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... File components/favicon/core/favicon_handler_unittest.cc (right): https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:655: TEST_F(FaviconHandlerTest, UpdateSameIconURLs) { My bad. Yes the first one is the same. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:670: I was mostly thinking of testing that FaviconHandler has not completed yet and sent FaviconService::SetFavicons() and Delegate::OnFaviconUpdated() already For instance, if kIconURL64x64 was a 16x16 icon, FaviconHandler would have finished. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:675: This test has several parts: Part 1 - Setup std::unique_ptr<FaviconHandler> handler = RunHandlerWithCandidates( FaviconDriverObserver::NON_TOUCH_16_DIP, favicon_urls); Part 2 - Check that running FaviconHandler::OnUpdateFaviconURL() does nothing handler->OnUpdateFaviconURL(kPageURL, favicon_urls); base::RunLoop().RunUntilIdle(); Part 3 - Check that FaviconHandler process completes and nothing wonky happened EXPECT_TRUE(delegate_.fake_downloader().RunCallbackManually()); base::RunLoop().RunUntilIdle(); In part 2 of this test there are currently no assertions. Thus, this test does not really test that "nothing happens when OnUpdateFaviconURL() is called". In my opinion, it should. Yes, I know that you could test this by moving the current EXPECT_CALL()s to the top of the file. That would be confusing in my opinion For the sake clarity, the test should be like this in my opinion: ... ASSERT_TRUE(delegate_.fake_downloader().HasPendingManualCallback()); // Calling OnUpdateFaviconURL() with the same icon URLs should have no effect, // despite the ongoing download. EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, _, _, _)).Times(0); EXPECT_CALL(favicon_service_, SetFavicons(_, _, _, _)).Times(0); handler->OnUpdateFaviconURL(kPageURL, favicon_urls); base::RunLoop().RunUntilIdle(); EXPECT_THAT(favicon_service_.fake()->db_requests(), IsEmpty()); EXPECT_THAT(delegate_.downloads(), IsEmpty()); VerifyAndClearExpectations(); // Complete the download. ... https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:995: SizeVector{gfx::Size(kOriginalSize2, kOriginalSize2)})}); Thank you for making the change. https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... File components/favicon/core/favicon_handler_unittest.cc (right): https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:18: #include "components/favicon/core/favicon_client.h" I don't think that you need this import? https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:39: using testing::AtMost; This doesn't seem to be used. :) https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:79: std::vector<gfx::Size> CreateSquareSizes(const IntVector& sizes) { There are two callers of this method. There is only one caller of CreateBitmaps() Can you: - Rename CreateBitmaps() to CreateBitmapsWithEdgeSizes() which takes in a std::vector<int>. That reduces the number of callers of CreateSquareSizes() to 1. - Can you inline CreateSquareSizes() into AddWithOriginalSizes() https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:129: int download_id = static_cast<int>(downloads_.size()); The download id is affected by ClearDownloads() :(. That's a bug. We should be able to not care about this parameter soon thanks to your follow up CL though https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:162: // download taking a long time. The callback will for DownloadImage() will be Nit: "callback will for" -> "callback for" https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:177: manual_callback_.Run(); In order for HasPendingManualCallback() to return the correct value, this function should reset |manual_callback_|. I don't think that HasPendingManualCallback() is necessary (Crashing when trying to run a Callback::is_null() callback is fine), but HasPendingManualCallback() does not hurt either. https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:193: // Callback for DownloadImage() request for |manual_callback_url_|. Nit: Group |manual_callback_url_| and |manual_callback_| one after the other https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:196: // Registered images. How about: "Registered responses." https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:197: std::map<GURL, Response> responses_; DISALLOW_COPY_AND_ASSIGN(FakeImageDownloader) https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:292: URLVector db_requests_; DISALLOW_COPY_AND_ASSIGN(FakeFaviconService) https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:370: // no sizes are provided. You should document in the comment that a FaviconHandler of type NON_TOUCH_16_DIP is created https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:394: // Shouldn't request to download icon. Remove this comment. (Or move it above line 400) https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:404: // - There is no data in the database for neither the page URL nor the icon URL. Sorry my fault. This should be: "There is data in the database for neither the page URL nor the icon URL." https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:412: FAVICON, _, _, _)); I don't think that it is useful to test that UpdateFaviconMappingsAndFetch() is called (as opposed to GetFavicon() in the context of this test. You can test the database request via EXPECT_THAT(favicon_service_.fake()->db_requests(), ElementsAre()) This enables you to remove the InSequence https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:417: kIconURL16x16, /*icon_url_changed=*/true, _)); Only the icon URL parameters are intersting in the SetFavicons() and OnFaviconUpdated() calls https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:420: &favicon_service_, &delegate_, FaviconDriverObserver::NON_TOUCH_16_DIP); Can you create FaviconHandler on the stack instead of wrapped in a std::unique_ptr<> ? https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:444: EXPECT_THAT(delegate_.downloads(), ElementsAre(kIconURL16x16)); We should check that |kIconURL16x16| is requested from the database (though not via UpdateFaviconMappingsAndFetch()) EXPECT_THAT(favicon_service_.fake()->db_requests(), ElementsAre(...)); https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:452: TEST_F(FaviconHandlerTest, DownloadUnknownFaviconIfCandidatesFaster) { Nit: Might as well make DownloadUnknownFaviconIfCandidatesSlower and DownloadUnknownFaviconIfCandidatesFaster tests consecutive https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:456: FAVICON, _, _, _)); You can check this by checking EXPECT_THAT(favicon_service_.fake()->db_requests(), ElementsAre(kPageUrl, kIconURL16x16)); Getting rid of this EXPECT_CALL() enables removing the InSequence https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:458: ImageSizeIs(16, 16))); We only care about the icon URL parameter. We can ignore all of the other parameters. Thus: EXPECT_CALL(favicon_service_, SetFavicons(_, kIconURL16x16, _, _); https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:459: EXPECT_CALL(delegate_, We should check the icon URL. I don't think that it is necessary to test the value of the |icon_url_changed| parameter in this test https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:463: &favicon_service_, &delegate_, FaviconDriverObserver::NON_TOUCH_16_DIP); Can FaviconHandler be on the stack as opposed to being wrapped in a std::unique_ptr<> ? https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:465: ASSERT_THAT(favicon_service_.fake()->db_requests(), ElementsAre(kPageURL)); Nit: Can you please add a new line? https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:485: .Times(AtLeast(1)); Nit: Times(2) https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:492: ASSERT_THAT(favicon_service_.fake()->db_requests(), ElementsAre(kPageURL)); ASSERT_THAT() -> EXPECT_THAT() https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:509: Nit: Remove new line https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:522: // FaviconHandler::OnUpdateFaviconURL() is called. I think that this comment better summarizes the test: Test that FaviconHandler requests the new data when: - There is valid data in the database for the page URL. AND - The icon URL used by the page has changed. AND - There is no data in database for the new icon URL. https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:618: TEST_F(FaviconHandlerTest, UpdateDuringDownloading) { For the commment, how about: "Test that download data for icon URLs other than the current favicon candidate URLs is ignored. This test tests the scenario where a download is in flight when FaviconHandler::OnUpdateFaviconURL() is called." https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:619: const GURL kIconURL("http://www.google.com/favicon"); Nit: make these |kIconURL1| - |kIconURL3| https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:625: delegate_.fake_downloader().SetRunCallbackManuallyForUrl(kIconURL); Nit: Can you please add a new line? https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:800: // - Pn an OS which supports the 1x and 2x scale factor Pn -> On https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:801: // - On a page with <link rel="icon"> tags with no "sizes" information selects Nit: new line before "selects" ("selects" is not part of the bullet point) https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:826: Nit: Move the other 404 related tests here Report404 and NotReport503 https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:844: // - Size hints are misleading and cause the 404 icon to be processed last. Can you please change the last point. The test tests the case of there being no sizes information at all. Perhaps: - All of the icons are downloaded because none of the icons have the ideal size. - The 404 icon is last. https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:850: RunHandlerWithSimpleFaviconCandidates({kIconURL64x64, kIconURL16x16}); Can you use a differently sized URL than |kIconURL16x16| in this test and MultipleFavicons404 Using 16x16 icon as "random data" for FaviconHandlers created for NON_TOUCH_16_DIP and FaviconURLs with no sizes information is confusing because: - 16x16 icons are a perfect match and cause downloading to stop - non 16x16 icons are not perfect data and don't stop downloading https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:907: // The 16x16 is largest. This comment is no longer accurate. https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:909: // The rest of bitmaps come in order, there is no sizes attribute. Nit: Add quotes around "sizes" https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:944: // size, which allows checking the order that the favicons were downloaded. Nit: Remove "which allows checking whether the icons were requested according to their size,"
Just nits! https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... File components/favicon/core/favicon_handler_unittest.cc (right): https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:655: TEST_F(FaviconHandlerTest, UpdateSameIconURLs) { My bad. Yes the first one is the same. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:670: I was mostly thinking of testing that FaviconHandler has not completed yet and sent FaviconService::SetFavicons() and Delegate::OnFaviconUpdated() already For instance, if kIconURL64x64 was a 16x16 icon, FaviconHandler would have finished. https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:675: This test has several parts: Part 1 - Setup std::unique_ptr<FaviconHandler> handler = RunHandlerWithCandidates( FaviconDriverObserver::NON_TOUCH_16_DIP, favicon_urls); Part 2 - Check that running FaviconHandler::OnUpdateFaviconURL() does nothing handler->OnUpdateFaviconURL(kPageURL, favicon_urls); base::RunLoop().RunUntilIdle(); Part 3 - Check that FaviconHandler process completes and nothing wonky happened EXPECT_TRUE(delegate_.fake_downloader().RunCallbackManually()); base::RunLoop().RunUntilIdle(); In part 2 of this test there are currently no assertions. Thus, this test does not really test that "nothing happens when OnUpdateFaviconURL() is called". In my opinion, it should. Yes, I know that you could test this by moving the current EXPECT_CALL()s to the top of the file. That would be confusing in my opinion For the sake clarity, the test should be like this in my opinion: ... ASSERT_TRUE(delegate_.fake_downloader().HasPendingManualCallback()); // Calling OnUpdateFaviconURL() with the same icon URLs should have no effect, // despite the ongoing download. EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, _, _, _)).Times(0); EXPECT_CALL(favicon_service_, SetFavicons(_, _, _, _)).Times(0); handler->OnUpdateFaviconURL(kPageURL, favicon_urls); base::RunLoop().RunUntilIdle(); EXPECT_THAT(favicon_service_.fake()->db_requests(), IsEmpty()); EXPECT_THAT(delegate_.downloads(), IsEmpty()); VerifyAndClearExpectations(); // Complete the download. ... https://codereview.chromium.org/2703603002/diff/440001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:995: SizeVector{gfx::Size(kOriginalSize2, kOriginalSize2)})}); Thank you for making the change. https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... File components/favicon/core/favicon_handler_unittest.cc (right): https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:18: #include "components/favicon/core/favicon_client.h" I don't think that you need this import? https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:39: using testing::AtMost; This doesn't seem to be used. :) https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:79: std::vector<gfx::Size> CreateSquareSizes(const IntVector& sizes) { There are two callers of this method. There is only one caller of CreateBitmaps() Can you: - Rename CreateBitmaps() to CreateBitmapsWithEdgeSizes() which takes in a std::vector<int>. That reduces the number of callers of CreateSquareSizes() to 1. - Can you inline CreateSquareSizes() into AddWithOriginalSizes() https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:129: int download_id = static_cast<int>(downloads_.size()); The download id is affected by ClearDownloads() :(. That's a bug. We should be able to not care about this parameter soon thanks to your follow up CL though https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:162: // download taking a long time. The callback will for DownloadImage() will be Nit: "callback will for" -> "callback for" https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:177: manual_callback_.Run(); In order for HasPendingManualCallback() to return the correct value, this function should reset |manual_callback_|. I don't think that HasPendingManualCallback() is necessary (Crashing when trying to run a Callback::is_null() callback is fine), but HasPendingManualCallback() does not hurt either. https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:193: // Callback for DownloadImage() request for |manual_callback_url_|. Nit: Group |manual_callback_url_| and |manual_callback_| one after the other https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:196: // Registered images. How about: "Registered responses." https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:197: std::map<GURL, Response> responses_; DISALLOW_COPY_AND_ASSIGN(FakeImageDownloader) https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:292: URLVector db_requests_; DISALLOW_COPY_AND_ASSIGN(FakeFaviconService) https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:370: // no sizes are provided. You should document in the comment that a FaviconHandler of type NON_TOUCH_16_DIP is created https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:394: // Shouldn't request to download icon. Remove this comment. (Or move it above line 400) https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:404: // - There is no data in the database for neither the page URL nor the icon URL. Sorry my fault. This should be: "There is data in the database for neither the page URL nor the icon URL." https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:412: FAVICON, _, _, _)); I don't think that it is useful to test that UpdateFaviconMappingsAndFetch() is called (as opposed to GetFavicon() in the context of this test. You can test the database request via EXPECT_THAT(favicon_service_.fake()->db_requests(), ElementsAre()) This enables you to remove the InSequence https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:417: kIconURL16x16, /*icon_url_changed=*/true, _)); Only the icon URL parameters are intersting in the SetFavicons() and OnFaviconUpdated() calls https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:420: &favicon_service_, &delegate_, FaviconDriverObserver::NON_TOUCH_16_DIP); Can you create FaviconHandler on the stack instead of wrapped in a std::unique_ptr<> ? https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:444: EXPECT_THAT(delegate_.downloads(), ElementsAre(kIconURL16x16)); We should check that |kIconURL16x16| is requested from the database (though not via UpdateFaviconMappingsAndFetch()) EXPECT_THAT(favicon_service_.fake()->db_requests(), ElementsAre(...)); https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:452: TEST_F(FaviconHandlerTest, DownloadUnknownFaviconIfCandidatesFaster) { Nit: Might as well make DownloadUnknownFaviconIfCandidatesSlower and DownloadUnknownFaviconIfCandidatesFaster tests consecutive https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:456: FAVICON, _, _, _)); You can check this by checking EXPECT_THAT(favicon_service_.fake()->db_requests(), ElementsAre(kPageUrl, kIconURL16x16)); Getting rid of this EXPECT_CALL() enables removing the InSequence https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:458: ImageSizeIs(16, 16))); We only care about the icon URL parameter. We can ignore all of the other parameters. Thus: EXPECT_CALL(favicon_service_, SetFavicons(_, kIconURL16x16, _, _); https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:459: EXPECT_CALL(delegate_, We should check the icon URL. I don't think that it is necessary to test the value of the |icon_url_changed| parameter in this test https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:463: &favicon_service_, &delegate_, FaviconDriverObserver::NON_TOUCH_16_DIP); Can FaviconHandler be on the stack as opposed to being wrapped in a std::unique_ptr<> ? https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:465: ASSERT_THAT(favicon_service_.fake()->db_requests(), ElementsAre(kPageURL)); Nit: Can you please add a new line? https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:485: .Times(AtLeast(1)); Nit: Times(2) https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:492: ASSERT_THAT(favicon_service_.fake()->db_requests(), ElementsAre(kPageURL)); ASSERT_THAT() -> EXPECT_THAT() https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:509: Nit: Remove new line https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:522: // FaviconHandler::OnUpdateFaviconURL() is called. I think that this comment better summarizes the test: Test that FaviconHandler requests the new data when: - There is valid data in the database for the page URL. AND - The icon URL used by the page has changed. AND - There is no data in database for the new icon URL. https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:618: TEST_F(FaviconHandlerTest, UpdateDuringDownloading) { For the commment, how about: "Test that download data for icon URLs other than the current favicon candidate URLs is ignored. This test tests the scenario where a download is in flight when FaviconHandler::OnUpdateFaviconURL() is called." https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:619: const GURL kIconURL("http://www.google.com/favicon"); Nit: make these |kIconURL1| - |kIconURL3| https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:625: delegate_.fake_downloader().SetRunCallbackManuallyForUrl(kIconURL); Nit: Can you please add a new line? https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:800: // - Pn an OS which supports the 1x and 2x scale factor Pn -> On https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:801: // - On a page with <link rel="icon"> tags with no "sizes" information selects Nit: new line before "selects" ("selects" is not part of the bullet point) https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:826: Nit: Move the other 404 related tests here Report404 and NotReport503 https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:844: // - Size hints are misleading and cause the 404 icon to be processed last. Can you please change the last point. The test tests the case of there being no sizes information at all. Perhaps: - All of the icons are downloaded because none of the icons have the ideal size. - The 404 icon is last. https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:850: RunHandlerWithSimpleFaviconCandidates({kIconURL64x64, kIconURL16x16}); Can you use a differently sized URL than |kIconURL16x16| in this test and MultipleFavicons404 Using 16x16 icon as "random data" for FaviconHandlers created for NON_TOUCH_16_DIP and FaviconURLs with no sizes information is confusing because: - 16x16 icons are a perfect match and cause downloading to stop - non 16x16 icons are not perfect data and don't stop downloading https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:907: // The 16x16 is largest. This comment is no longer accurate. https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:909: // The rest of bitmaps come in order, there is no sizes attribute. Nit: Add quotes around "sizes" https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:944: // size, which allows checking the order that the favicons were downloaded. Nit: Remove "which allows checking whether the icons were requested according to their size,"
Just nits!
Thanks, PTAL! Addressed all comments except one group that requires clarification. https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... File components/favicon/core/favicon_handler_unittest.cc (right): https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:18: #include "components/favicon/core/favicon_client.h" On 2017/03/17 05:43:11, pkotwicz wrote: > I don't think that you need this import? Removed. https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:39: using testing::AtMost; On 2017/03/17 05:43:12, pkotwicz wrote: > This doesn't seem to be used. :) Done. https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:79: std::vector<gfx::Size> CreateSquareSizes(const IntVector& sizes) { On 2017/03/17 05:43:11, pkotwicz wrote: > There are two callers of this method. There is only one caller of > CreateBitmaps() > > Can you: > - Rename CreateBitmaps() to CreateBitmapsWithEdgeSizes() which takes in a > std::vector<int>. That reduces the number of callers of CreateSquareSizes() to > 1. > - Can you inline CreateSquareSizes() into AddWithOriginalSizes() Done. Since you seem to be concerned about the number of callers, I inlined CreateBitmaps() too, which has one single caller. This leaves CreateBitmapWithEdgeSize(), following your naming suggestion. https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:129: int download_id = static_cast<int>(downloads_.size()); On 2017/03/17 05:43:12, pkotwicz wrote: > The download id is affected by ClearDownloads() :(. That's a bug. We should be > able to not care about this parameter soon thanks to your follow up CL though Done. https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:162: // download taking a long time. The callback will for DownloadImage() will be On 2017/03/17 05:43:11, pkotwicz wrote: > Nit: "callback will for" -> "callback for" Done. https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:177: manual_callback_.Run(); On 2017/03/17 05:43:12, pkotwicz wrote: > In order for HasPendingManualCallback() to return the correct value, this > function should reset |manual_callback_|. > > I don't think that HasPendingManualCallback() is necessary (Crashing when trying > to run a Callback::is_null() callback is fine), but HasPendingManualCallback() > does not hurt either. Done. Crashing tests in failed expectations is not great so leaving HasPendingManualCallback() there. https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:193: // Callback for DownloadImage() request for |manual_callback_url_|. On 2017/03/17 05:43:12, pkotwicz wrote: > Nit: Group |manual_callback_url_| and |manual_callback_| one after the other Done. https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:196: // Registered images. On 2017/03/17 05:43:12, pkotwicz wrote: > How about: "Registered responses." Done. https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:197: std::map<GURL, Response> responses_; On 2017/03/17 05:43:11, pkotwicz wrote: > DISALLOW_COPY_AND_ASSIGN(FakeImageDownloader) Done, although I don't think this buys anything on test code. https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:292: URLVector db_requests_; On 2017/03/17 05:43:12, pkotwicz wrote: > DISALLOW_COPY_AND_ASSIGN(FakeFaviconService) Done, same here. https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:370: // no sizes are provided. On 2017/03/17 05:43:12, pkotwicz wrote: > You should document in the comment that a FaviconHandler of type > NON_TOUCH_16_DIP is created Done. https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:394: // Shouldn't request to download icon. On 2017/03/17 05:43:12, pkotwicz wrote: > Remove this comment. (Or move it above line 400) Removed, since I'm not a big fan of comments that state the obvious. https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:404: // - There is no data in the database for neither the page URL nor the icon URL. On 2017/03/17 05:43:12, pkotwicz wrote: > Sorry my fault. This should be: > "There is data in the database for neither the page URL nor the icon URL." Done. https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:412: FAVICON, _, _, _)); On 2017/03/17 05:43:13, pkotwicz wrote: > I don't think that it is useful to test that UpdateFaviconMappingsAndFetch() is > called (as opposed to GetFavicon() in the context of this test. > > You can test the database request via > EXPECT_THAT(favicon_service_.fake()->db_requests(), ElementsAre()) > > This enables you to remove the InSequence InSequence can be removed either way, the question is whether you want to verify the order. Which test do you think is responsible for testing the calls to UpdateFaviconMappingsAndFetch? It looks to me that this one *and* DownloadUnknownFaviconIfCandidatesFaster are a good fit. https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:417: kIconURL16x16, /*icon_url_changed=*/true, _)); On 2017/03/17 05:43:13, pkotwicz wrote: > Only the icon URL parameters are intersting in the SetFavicons() and > OnFaviconUpdated() calls Same here: parameters for OnFaviconUpdated() must be verified somewhere, and this particular test differs in codepath from GetFaviconFromHistory(), so I don't think we should relax expectations, unless you have some other test in mind. https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:420: &favicon_service_, &delegate_, FaviconDriverObserver::NON_TOUCH_16_DIP); On 2017/03/17 05:43:12, pkotwicz wrote: > Can you create FaviconHandler on the stack instead of wrapped in a > std::unique_ptr<> ? Done. https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:444: EXPECT_THAT(delegate_.downloads(), ElementsAre(kIconURL16x16)); On 2017/03/17 05:43:13, pkotwicz wrote: > We should check that |kIconURL16x16| is requested from the database (though not > via UpdateFaviconMappingsAndFetch()) > > EXPECT_THAT(favicon_service_.fake()->db_requests(), ElementsAre(...)); Done. https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:452: TEST_F(FaviconHandlerTest, DownloadUnknownFaviconIfCandidatesFaster) { On 2017/03/17 05:43:12, pkotwicz wrote: > Nit: Might as well make DownloadUnknownFaviconIfCandidatesSlower and > DownloadUnknownFaviconIfCandidatesFaster tests consecutive Done. https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:456: FAVICON, _, _, _)); On 2017/03/17 05:43:12, pkotwicz wrote: > You can check this by checking > EXPECT_THAT(favicon_service_.fake()->db_requests(), ElementsAre(kPageUrl, > kIconURL16x16)); > > Getting rid of this EXPECT_CALL() enables removing the InSequence Clarification needed in earlier comment, in DownloadUnknownFaviconIfCandidatesSlower, as per where the parameters for UpdateFaviconMappingsAndFetch should be verified. https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:458: ImageSizeIs(16, 16))); On 2017/03/17 05:43:11, pkotwicz wrote: > We only care about the icon URL parameter. We can ignore all of the other > parameters. > Thus: > EXPECT_CALL(favicon_service_, SetFavicons(_, kIconURL16x16, _, _); Ditto. https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:459: EXPECT_CALL(delegate_, On 2017/03/17 05:43:11, pkotwicz wrote: > We should check the icon URL. I don't think that it is necessary to test the > value of the |icon_url_changed| parameter in this test Done. https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:463: &favicon_service_, &delegate_, FaviconDriverObserver::NON_TOUCH_16_DIP); On 2017/03/17 05:43:11, pkotwicz wrote: > Can FaviconHandler be on the stack as opposed to being wrapped in a > std::unique_ptr<> ? Done. https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:465: ASSERT_THAT(favicon_service_.fake()->db_requests(), ElementsAre(kPageURL)); On 2017/03/17 05:43:12, pkotwicz wrote: > Nit: Can you please add a new line? Done. https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:485: .Times(AtLeast(1)); On 2017/03/17 05:43:12, pkotwicz wrote: > Nit: Times(2) Done. https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:492: ASSERT_THAT(favicon_service_.fake()->db_requests(), ElementsAre(kPageURL)); On 2017/03/17 05:43:13, pkotwicz wrote: > ASSERT_THAT() -> EXPECT_THAT() Done. https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:509: On 2017/03/17 05:43:11, pkotwicz wrote: > Nit: Remove new line All tests have a newline after URLs, I'm not following why you want to change this one (there was one exception which I now made consistent). https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:522: // FaviconHandler::OnUpdateFaviconURL() is called. On 2017/03/17 05:43:12, pkotwicz wrote: > I think that this comment better summarizes the test: > > Test that FaviconHandler requests the new data when: > - There is valid data in the database for the page URL. > AND > - The icon URL used by the page has changed. > AND > - There is no data in database for the new icon URL. Done. https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:618: TEST_F(FaviconHandlerTest, UpdateDuringDownloading) { On 2017/03/17 05:43:11, pkotwicz wrote: > For the commment, how about: > > "Test that download data for icon URLs other than the current favicon > candidate URLs is ignored. This test tests the scenario where a download is in > flight when FaviconHandler::OnUpdateFaviconURL() is called." Done. https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:619: const GURL kIconURL("http://www.google.com/favicon"); On 2017/03/17 05:43:12, pkotwicz wrote: > Nit: make these |kIconURL1| - |kIconURL3| Done. https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:625: delegate_.fake_downloader().SetRunCallbackManuallyForUrl(kIconURL); On 2017/03/17 05:43:12, pkotwicz wrote: > Nit: Can you please add a new line? Done. https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:800: // - Pn an OS which supports the 1x and 2x scale factor On 2017/03/17 05:43:13, pkotwicz wrote: > Pn -> On Done. https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:801: // - On a page with <link rel="icon"> tags with no "sizes" information selects On 2017/03/17 05:43:12, pkotwicz wrote: > Nit: new line before "selects" ("selects" is not part of the bullet point) Done. https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:826: On 2017/03/17 05:43:12, pkotwicz wrote: > Nit: Move the other 404 related tests here Report404 and NotReport503 Done. https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:844: // - Size hints are misleading and cause the 404 icon to be processed last. On 2017/03/17 05:43:13, pkotwicz wrote: > Can you please change the last point. The test tests the case of there being no > sizes information at all. Perhaps: > > - All of the icons are downloaded because none of the icons have the ideal size. > - The 404 icon is last. Done. https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:850: RunHandlerWithSimpleFaviconCandidates({kIconURL64x64, kIconURL16x16}); On 2017/03/17 05:43:12, pkotwicz wrote: > Can you use a differently sized URL than |kIconURL16x16| in this test and > MultipleFavicons404 > > Using 16x16 icon as "random data" for FaviconHandlers created for > NON_TOUCH_16_DIP and FaviconURLs with no sizes information is confusing because: > - 16x16 icons are a perfect match and cause downloading to stop > - non 16x16 icons are not perfect data and don't stop downloading Done, also for MultipleFaviconsAll404. The truth is it doesn't matter because the icon doesn't get downloaded and tests specifically verify that's the case. https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:907: // The 16x16 is largest. On 2017/03/17 05:43:11, pkotwicz wrote: > This comment is no longer accurate. Done. https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:909: // The rest of bitmaps come in order, there is no sizes attribute. On 2017/03/17 05:43:12, pkotwicz wrote: > Nit: Add quotes around "sizes" Done. https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:944: // size, which allows checking the order that the favicons were downloaded. On 2017/03/17 05:43:13, pkotwicz wrote: > Nit: Remove "which allows checking whether the icons were requested according to > their size," Done.
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: This issue passed the CQ dry run.
LGTM with nits. Thank you for bearing with me. Can you please update the CL description. - favicon_handler_unittests.cc is no longer 1000 lines shorter - There are now 24 tests not 28 https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... File components/favicon/core/favicon_handler_unittest.cc (right): https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:412: FAVICON, _, _, _)); We currently don't have a test which tests that UpdateFaviconMappingsAndFetch() is called. In my opinion, tests should test one thing only. I am fine with adding a test which tests that UpdateFaviconMappingsAndFetch() is called in none incognito mode https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:417: kIconURL16x16, /*icon_url_changed=*/true, _)); Yes, they should be verified somewhere. I suggest that they these expectation be verified in new tests In my CL I added FaviconHandler.ShouldCallSetFaviconsWithCorrectIconType() to test this As we have discussed previously, we can add new tests in a separate CL https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:509: Ok, it is fine to stay as is https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:850: RunHandlerWithSimpleFaviconCandidates({kIconURL64x64, kIconURL16x16}); I know that it doesn't matter. However, using 16x16 icons does make the tests more confusing in my opinion
https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... File components/favicon/core/favicon_handler_unittest.cc (right): https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:412: FAVICON, _, _, _)); On 2017/03/17 15:22:31, pkotwicz wrote: > We currently don't have a test which tests that UpdateFaviconMappingsAndFetch() > is called. In my opinion, tests should test one thing only. I am fine with > adding a test which tests that UpdateFaviconMappingsAndFetch() is called in none > incognito mode Done, and added a TODO to introduce a new test. https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:417: kIconURL16x16, /*icon_url_changed=*/true, _)); On 2017/03/17 15:22:31, pkotwicz wrote: > Yes, they should be verified somewhere. I suggest that they these expectation be > verified in new tests > > In my CL I added FaviconHandler.ShouldCallSetFaviconsWithCorrectIconType() to > test this > > As we have discussed previously, we can add new tests in a separate CL While I see this argument for UpdateFaviconMappingsAndFetch(), I think splitting the verification of SetFavicons and OnFaviconUpdated, in this case, is overdoing it. You could also split the verification of image downloads. WDYT?
On 2017/03/17 15:49:28, mastiz wrote: > https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... > File components/favicon/core/favicon_handler_unittest.cc (right): > > https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... > components/favicon/core/favicon_handler_unittest.cc:412: FAVICON, _, _, _)); > On 2017/03/17 15:22:31, pkotwicz wrote: > > We currently don't have a test which tests that > UpdateFaviconMappingsAndFetch() > > is called. In my opinion, tests should test one thing only. I am fine with > > adding a test which tests that UpdateFaviconMappingsAndFetch() is called in > none > > incognito mode > > Done, and added a TODO to introduce a new test. > > https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... > components/favicon/core/favicon_handler_unittest.cc:417: kIconURL16x16, > /*icon_url_changed=*/true, _)); > On 2017/03/17 15:22:31, pkotwicz wrote: > > Yes, they should be verified somewhere. I suggest that they these expectation > be > > verified in new tests > > > > In my CL I added FaviconHandler.ShouldCallSetFaviconsWithCorrectIconType() to > > test this > > > > As we have discussed previously, we can add new tests in a separate CL > > While I see this argument for UpdateFaviconMappingsAndFetch(), I think splitting > the verification of SetFavicons and OnFaviconUpdated, in this case, is overdoing > it. You could also split the verification of image downloads. WDYT? Friendly ping, thanks!
Description was changed from ========== Improve FaviconHandler tests by testing public API No behavioral changes: instead of subclassing the class under test to bypass/intercept internal calls, let's instead verify external interactions by injecting test doubles and testing the public API. The new ones are more concise by adopting mocks to verify call arguments or fakes (mock-to-fake delegation) for the simplest cases. This greatly reduces the size of tests to ~50%, ~1000 LoC less! The actual diffstat is smaller due to newly added tests, increasing the total number of tests from 18 to 28, including basic test coverage for incognito (DownloadUnknownFaviconInIncognito) or a corner case that is prone to regressions (MultipleFaviconsLast404). BUG=694312 ========== to ========== Improve FaviconHandler tests by testing public API No behavioral changes: instead of subclassing the class under test to bypass/intercept internal calls, let's instead verify external interactions by injecting test doubles and testing the public API. The new ones are more concise by adopting mocks to verify call arguments or fakes (mock-to-fake delegation) for the simplest cases. This greatly reduces the size of tests to ~50%, ~800 LoC less! The actual diffstat is smaller due to newly added tests, increasing the total number of tests from 18 to 28, including basic test coverage for incognito (DownloadUnknownFaviconInIncognito) or a corner case that is prone to regressions (MultipleFaviconsLast404). BUG=694312 ==========
https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... File components/favicon/core/favicon_handler_unittest.cc (right): https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:417: kIconURL16x16, /*icon_url_changed=*/true, _)); Fair enough. You can test all of the arguments for SetFavicons() and OnFaviconUpdated() here.
On 2017/03/20 15:01:17, pkotwicz wrote: > https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... > File components/favicon/core/favicon_handler_unittest.cc (right): > > https://codereview.chromium.org/2703603002/diff/520001/components/favicon/cor... > components/favicon/core/favicon_handler_unittest.cc:417: kIconURL16x16, > /*icon_url_changed=*/true, _)); > Fair enough. You can test all of the arguments for SetFavicons() and > OnFaviconUpdated() here. Great, thanks! I think all comments have been addressed so proceeding with landing, thanks for the thorough review.
The CQ bit was checked by mastiz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkotwicz@chromium.org Link to the patchset: https://codereview.chromium.org/2703603002/#ps560001 (title: "Removed UpdateFaviconMappingsAndFetch()")
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": 560001, "attempt_start_ts": 1490023364869860,
"parent_rev": "822d58e6b22506cbd4787b58d8ccb421409b5b65", "commit_rev":
"36c9189e71036d65209df554c9ddc96be6f78869"}
Message was sent while issue was closed.
Description was changed from ========== Improve FaviconHandler tests by testing public API No behavioral changes: instead of subclassing the class under test to bypass/intercept internal calls, let's instead verify external interactions by injecting test doubles and testing the public API. The new ones are more concise by adopting mocks to verify call arguments or fakes (mock-to-fake delegation) for the simplest cases. This greatly reduces the size of tests to ~50%, ~800 LoC less! The actual diffstat is smaller due to newly added tests, increasing the total number of tests from 18 to 28, including basic test coverage for incognito (DownloadUnknownFaviconInIncognito) or a corner case that is prone to regressions (MultipleFaviconsLast404). BUG=694312 ========== to ========== Improve FaviconHandler tests by testing public API No behavioral changes: instead of subclassing the class under test to bypass/intercept internal calls, let's instead verify external interactions by injecting test doubles and testing the public API. The new ones are more concise by adopting mocks to verify call arguments or fakes (mock-to-fake delegation) for the simplest cases. This greatly reduces the size of tests to ~50%, ~800 LoC less! The actual diffstat is smaller due to newly added tests, increasing the total number of tests from 18 to 28, including basic test coverage for incognito (DownloadUnknownFaviconInIncognito) or a corner case that is prone to regressions (MultipleFaviconsLast404). BUG=694312 Review-Url: https://codereview.chromium.org/2703603002 Cr-Commit-Position: refs/heads/master@{#458084} Committed: https://chromium.googlesource.com/chromium/src/+/36c9189e71036d65209df554c9dd... ==========
Message was sent while issue was closed.
Committed patchset #29 (id:560001) as https://chromium.googlesource.com/chromium/src/+/36c9189e71036d65209df554c9dd... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
