Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2579813002:
ntp_tiles: Add tests covering tile-fetching logic (Closed)
Descriptionntp_tiles: Add tests covering tile-fetching logic
The previous tests only covered a tiny part of the component, which is
the merging logic, but not the actual combinations of events that can
happen during the fetching of tiles from the backends (top sites,
suggestions service, popular sites).
Mocks are used extensively due to the lack of fakes, and because the
interfaces are really big. It's somewhat painful to read, but
alternatives are not better.
The tests surface one bug which is not addressed here, left more a
follow-up patch and represented with a TODO.
Popular Sites fetching is still not covered by these tests, and
another TODO is added for this purpose.
BUG=619584
Committed: https://crrev.com/451ce6c31e4a2643271bc29e68a103109069fb38
Cr-Commit-Position: refs/heads/master@{#439335}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Minor improvements. #Patch Set 3 : Various improvements. #Patch Set 4 : Various improvements. #
Total comments: 12
Patch Set 5 : Addressed comments. #Patch Set 6 : Addressed more comments and reverted to fixtures. #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 28 (16 generated)
mastiz@chromium.org changed reviewers: + treib@chromium.org
Quite painful, but better than no tests :-)
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 ========== ntp_tiles: Add tests covering tile-fetching logic The previous tests only covered a tiny part of the component, which is the merging logic, but not the actual combinations of events that can happen during the fetching of tiles from the backends (top sites, suggestions service, popular sites). Mocks are used extensively due to the lack of fakes, and because the interfaces are really big. It's somewhat painful to read, but alternatives are not better. The tests surface one bug which is not addressed here, left more a follow-up patch and represented with a TODO. Popular Sites fetching is still not covered by these tests, and another TODO is added for this purpose. BUG=619584 ========== to ========== ntp_tiles: Add tests covering tile-fetching logic The previous tests only covered a tiny part of the component, which is the merging logic, but not the actual combinations of events that can happen during the fetching of tiles from the backends (top sites, suggestions service, popular sites). Mocks are used extensively due to the lack of fakes, and because the interfaces are really big. It's somewhat painful to read, but alternatives are not better. The tests surface one bug which is not addressed here, left more a follow-up patch and represented with a TODO. Popular Sites fetching is still not covered by these tests, and another TODO is added for this purpose. BUG=619584 ==========
mastiz@chromium.org changed reviewers: + sfiera@chromium.org - treib@chromium.org
Redirecting to sfiera@ since treib@ is OOO.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) 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...
Overall I think this is a great improvement. https://codereview.chromium.org/2579813002/diff/60001/components/ntp_tiles/mo... File components/ntp_tiles/most_visited_sites.h (right): https://codereview.chromium.org/2579813002/diff/60001/components/ntp_tiles/mo... components/ntp_tiles/most_visited_sites.h:118: static void RegisterProfilePrefs(PrefRegistrySimple* registry); Was this changed for the sake of being able to use TestingPrefRegistrySimple? There's also TestingPrefRegistrySyncable, in a different component: https://cs.chromium.org/chromium/src/components/sync_preferences/testing_pref... I don't know enough about preferences in Chromium to LGTM a change from PrefRegistrySyncable to PrefRegistrySimple in confidence. https://codereview.chromium.org/2579813002/diff/60001/components/ntp_tiles/mo... File components/ntp_tiles/most_visited_sites_unittest.cc (right): https://codereview.chromium.org/2579813002/diff/60001/components/ntp_tiles/mo... components/ntp_tiles/most_visited_sites_unittest.cc:142: ~MockTopSites() override = default; Is this protected for the sake of disallowing non-StrictMock usage? I'm not sure I'd bother. https://codereview.chromium.org/2579813002/diff/60001/components/ntp_tiles/mo... components/ntp_tiles/most_visited_sites_unittest.cc:215: /*icon_cacher=*/{}, nullptr https://codereview.chromium.org/2579813002/diff/60001/components/ntp_tiles/mo... components/ntp_tiles/most_visited_sites_unittest.cc:258: class MostVisitedSitesWithCacheHitTest : public MostVisitedSitesTest { Why is this a class, as opposed to a method of MostVisitedSitesTest?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL, thanks! https://codereview.chromium.org/2579813002/diff/60001/components/ntp_tiles/mo... File components/ntp_tiles/most_visited_sites.h (right): https://codereview.chromium.org/2579813002/diff/60001/components/ntp_tiles/mo... components/ntp_tiles/most_visited_sites.h:118: static void RegisterProfilePrefs(PrefRegistrySimple* registry); On 2016/12/16 11:01:21, sfiera wrote: > Was this changed for the sake of being able to use TestingPrefRegistrySimple? > There's also TestingPrefRegistrySyncable, in a different component: > https://cs.chromium.org/chromium/src/components/sync_preferences/testing_pref... > > I don't know enough about preferences in Chromium to LGTM a change from > PrefRegistrySyncable to PrefRegistrySimple in confidence. This should be a no-op because PrefRegistrySimple is a base class of PrefRegistrySyncable, and we don't use any API extension in PrefRegistrySyncable. This simplifies tests considerably. https://codereview.chromium.org/2579813002/diff/60001/components/ntp_tiles/mo... File components/ntp_tiles/most_visited_sites_unittest.cc (right): https://codereview.chromium.org/2579813002/diff/60001/components/ntp_tiles/mo... components/ntp_tiles/most_visited_sites_unittest.cc:142: ~MockTopSites() override = default; On 2016/12/16 11:01:21, sfiera wrote: > Is this protected for the sake of disallowing non-StrictMock usage? I'm not sure > I'd bother. This is required because TopSites inherits from RefcountedKeyedService: [chromium-style] Classes that are ref-counted should have explicit destructors that are declared protected or private. https://codereview.chromium.org/2579813002/diff/60001/components/ntp_tiles/mo... components/ntp_tiles/most_visited_sites_unittest.cc:215: /*icon_cacher=*/{}, On 2016/12/16 11:01:21, sfiera wrote: > nullptr Done. https://codereview.chromium.org/2579813002/diff/60001/components/ntp_tiles/mo... components/ntp_tiles/most_visited_sites_unittest.cc:258: class MostVisitedSitesWithCacheHitTest : public MostVisitedSitesTest { On 2016/12/16 11:01:21, sfiera wrote: > Why is this a class, as opposed to a method of MostVisitedSitesTest? I've addressed your suggestion but I'd actually like to push back and revert the change to the previous version. Rationale: 1. Many tests will call these functions first to set up the initial/baseline state for the test: this is by definition a test fixture. 2. Test names are now too long and harder to parse, as opposed to keeping three fixture names in mind.
https://codereview.chromium.org/2579813002/diff/60001/components/ntp_tiles/mo... File components/ntp_tiles/most_visited_sites.h (right): https://codereview.chromium.org/2579813002/diff/60001/components/ntp_tiles/mo... components/ntp_tiles/most_visited_sites.h:118: static void RegisterProfilePrefs(PrefRegistrySimple* registry); On 2016/12/16 12:01:04, mastiz wrote: > On 2016/12/16 11:01:21, sfiera wrote: > > Was this changed for the sake of being able to use TestingPrefRegistrySimple? > > There's also TestingPrefRegistrySyncable, in a different component: > > > https://cs.chromium.org/chromium/src/components/sync_preferences/testing_pref... > > > > I don't know enough about preferences in Chromium to LGTM a change from > > PrefRegistrySyncable to PrefRegistrySimple in confidence. > > This should be a no-op because PrefRegistrySimple is a base class of > PrefRegistrySyncable, and we don't use any API extension in > PrefRegistrySyncable. Right, but we would allow a non-syncable registry to be passed, and we want these preferences to be synced. > This simplifies tests considerably. How so? TestingPrefServiceSyncable is a drop-in replacement for TestingPrefServiceSimple, isn't it? I'm not saying this change is wrong, but if you think it's correct, you should loop in someone more familiar with Chrome preferences to make a call. https://codereview.chromium.org/2579813002/diff/60001/components/ntp_tiles/mo... File components/ntp_tiles/most_visited_sites_unittest.cc (right): https://codereview.chromium.org/2579813002/diff/60001/components/ntp_tiles/mo... components/ntp_tiles/most_visited_sites_unittest.cc:258: class MostVisitedSitesWithCacheHitTest : public MostVisitedSitesTest { On 2016/12/16 12:01:04, mastiz wrote: > On 2016/12/16 11:01:21, sfiera wrote: > > Why is this a class, as opposed to a method of MostVisitedSitesTest? > > I've addressed your suggestion but I'd actually like to push back and revert the > change to the previous version. Rationale: > > 1. Many tests will call these functions first to set up the initial/baseline > state for the test: this is by definition a test fixture. > > 2. Test names are now too long and harder to parse, as opposed to keeping three > fixture names in mind. I do find the revised version easier to read but I'm fine with going back to the previous.
Thanks! https://codereview.chromium.org/2579813002/diff/60001/components/ntp_tiles/mo... File components/ntp_tiles/most_visited_sites.h (right): https://codereview.chromium.org/2579813002/diff/60001/components/ntp_tiles/mo... components/ntp_tiles/most_visited_sites.h:118: static void RegisterProfilePrefs(PrefRegistrySimple* registry); On 2016/12/16 13:05:29, sfiera wrote: > On 2016/12/16 12:01:04, mastiz wrote: > > On 2016/12/16 11:01:21, sfiera wrote: > > > Was this changed for the sake of being able to use > TestingPrefRegistrySimple? > > > There's also TestingPrefRegistrySyncable, in a different component: > > > > > > https://cs.chromium.org/chromium/src/components/sync_preferences/testing_pref... > > > > > > I don't know enough about preferences in Chromium to LGTM a change from > > > PrefRegistrySyncable to PrefRegistrySimple in confidence. > > > > This should be a no-op because PrefRegistrySimple is a base class of > > PrefRegistrySyncable, and we don't use any API extension in > > PrefRegistrySyncable. > > Right, but we would allow a non-syncable registry to be passed, and we want > these preferences to be synced. > > > This simplifies tests considerably. > > How so? TestingPrefServiceSyncable is a drop-in replacement for > TestingPrefServiceSimple, isn't it? > > I'm not saying this change is wrong, but if you think it's correct, you should > loop in someone more familiar with Chrome preferences to make a call. You're right, I take back my test-complexity argument. I apparently got confused at some point, but it is indeed a drop-in replacement, thanks for pointing out. In doubt, I'm going to leave the constructor signature unmodified, so reverted this change. https://codereview.chromium.org/2579813002/diff/60001/components/ntp_tiles/mo... File components/ntp_tiles/most_visited_sites_unittest.cc (right): https://codereview.chromium.org/2579813002/diff/60001/components/ntp_tiles/mo... components/ntp_tiles/most_visited_sites_unittest.cc:258: class MostVisitedSitesWithCacheHitTest : public MostVisitedSitesTest { On 2016/12/16 13:05:29, sfiera wrote: > On 2016/12/16 12:01:04, mastiz wrote: > > On 2016/12/16 11:01:21, sfiera wrote: > > > Why is this a class, as opposed to a method of MostVisitedSitesTest? > > > > I've addressed your suggestion but I'd actually like to push back and revert > the > > change to the previous version. Rationale: > > > > 1. Many tests will call these functions first to set up the initial/baseline > > state for the test: this is by definition a test fixture. > > > > 2. Test names are now too long and harder to parse, as opposed to keeping > three > > fixture names in mind. > > I do find the revised version easier to read but I'm fine with going back to the > previous. Since you're not feeling strongly, reverting to previous version.
LGTM, thanks!
The CQ bit was checked by mastiz@chromium.org
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": 100001, "attempt_start_ts": 1481959521401140,
"parent_rev": "9e8be9e71d07181854df15fa721532cc8a25ef5c", "commit_rev":
"0ef02718e6be839bf3870a9b76d288ef99b65f8b"}
Message was sent while issue was closed.
Description was changed from ========== ntp_tiles: Add tests covering tile-fetching logic The previous tests only covered a tiny part of the component, which is the merging logic, but not the actual combinations of events that can happen during the fetching of tiles from the backends (top sites, suggestions service, popular sites). Mocks are used extensively due to the lack of fakes, and because the interfaces are really big. It's somewhat painful to read, but alternatives are not better. The tests surface one bug which is not addressed here, left more a follow-up patch and represented with a TODO. Popular Sites fetching is still not covered by these tests, and another TODO is added for this purpose. BUG=619584 ========== to ========== ntp_tiles: Add tests covering tile-fetching logic The previous tests only covered a tiny part of the component, which is the merging logic, but not the actual combinations of events that can happen during the fetching of tiles from the backends (top sites, suggestions service, popular sites). Mocks are used extensively due to the lack of fakes, and because the interfaces are really big. It's somewhat painful to read, but alternatives are not better. The tests surface one bug which is not addressed here, left more a follow-up patch and represented with a TODO. Popular Sites fetching is still not covered by these tests, and another TODO is added for this purpose. BUG=619584 Review-Url: https://codereview.chromium.org/2579813002 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== ntp_tiles: Add tests covering tile-fetching logic The previous tests only covered a tiny part of the component, which is the merging logic, but not the actual combinations of events that can happen during the fetching of tiles from the backends (top sites, suggestions service, popular sites). Mocks are used extensively due to the lack of fakes, and because the interfaces are really big. It's somewhat painful to read, but alternatives are not better. The tests surface one bug which is not addressed here, left more a follow-up patch and represented with a TODO. Popular Sites fetching is still not covered by these tests, and another TODO is added for this purpose. BUG=619584 Review-Url: https://codereview.chromium.org/2579813002 ========== to ========== ntp_tiles: Add tests covering tile-fetching logic The previous tests only covered a tiny part of the component, which is the merging logic, but not the actual combinations of events that can happen during the fetching of tiles from the backends (top sites, suggestions service, popular sites). Mocks are used extensively due to the lack of fakes, and because the interfaces are really big. It's somewhat painful to read, but alternatives are not better. The tests surface one bug which is not addressed here, left more a follow-up patch and represented with a TODO. Popular Sites fetching is still not covered by these tests, and another TODO is added for this purpose. BUG=619584 Committed: https://crrev.com/451ce6c31e4a2643271bc29e68a103109069fb38 Cr-Commit-Position: refs/heads/master@{#439335} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/451ce6c31e4a2643271bc29e68a103109069fb38 Cr-Commit-Position: refs/heads/master@{#439335}
Message was sent while issue was closed.
treib@chromium.org changed reviewers: + treib@chromium.org
Message was sent while issue was closed.
I had started to review this last Thu; sending out the comments for completeness' sake. Probably most are obsolete now. https://codereview.chromium.org/2579813002/diff/1/components/ntp_tiles/most_v... File components/ntp_tiles/most_visited_sites.cc (right): https://codereview.chromium.org/2579813002/diff/1/components/ntp_tiles/most_v... components/ntp_tiles/most_visited_sites.cc:21: #include "components/prefs/pref_registry_simple.h" Nice! Once we do the same for PopularSites, we can probably remove the dependency on components/pref_registry :) https://codereview.chromium.org/2579813002/diff/1/components/ntp_tiles/most_v... File components/ntp_tiles/most_visited_sites_unittest.cc (right): https://codereview.chromium.org/2579813002/diff/1/components/ntp_tiles/most_v... components/ntp_tiles/most_visited_sites_unittest.cc:203: base::WrapUnique(mock_popular_sites_), Hm, IMO this makes ownership kinda hard to understand. I usually prefer the pattern: auto ptr = MakeUnique<..>(); Class* raw_ptr = ptr.get(); DoTheThing(std::move(ptr)); WDYT? Could that work here? https://codereview.chromium.org/2579813002/diff/1/components/ntp_tiles/most_v... components/ntp_tiles/most_visited_sites_unittest.cc:205: /*supervisor=*/{}) { nullptr? https://codereview.chromium.org/2579813002/diff/1/components/ntp_tiles/most_v... components/ntp_tiles/most_visited_sites_unittest.cc:206: MostVisitedSites::RegisterProfilePrefs(pref_service_.registry()); I think it'd be best to instantiate MostVisitedSites *after* we have registered its prefs. https://codereview.chromium.org/2579813002/diff/1/components/ntp_tiles/most_v... components/ntp_tiles/most_visited_sites_unittest.cc:214: .WillRepeatedly(ReturnRef(empty_popular_sites_vector_)); Is this a case for ON_CALL(...).WillByDefault? https://codereview.chromium.org/2579813002/diff/1/components/ntp_tiles/most_v... components/ntp_tiles/most_visited_sites_unittest.cc:255: EXPECT_CALL(mock_suggestions_service_, AddCallback(_)); Here the InSequence might be a bit too strong: It doesn't really matter which of the two above is called first. Or am I missing something?
Message was sent while issue was closed.
Just for completeness' sake. https://codereview.chromium.org/2579813002/diff/1/components/ntp_tiles/most_v... File components/ntp_tiles/most_visited_sites.cc (right): https://codereview.chromium.org/2579813002/diff/1/components/ntp_tiles/most_v... components/ntp_tiles/most_visited_sites.cc:21: #include "components/prefs/pref_registry_simple.h" On 2016/12/19 09:11:50, Marc Treib (OOO until Jan 12) wrote: > Nice! Once we do the same for PopularSites, we can probably remove the > dependency on components/pref_registry :) sfiera@ raised some concerns so I reverted this change :-/ https://codereview.chromium.org/2579813002/diff/1/components/ntp_tiles/most_v... File components/ntp_tiles/most_visited_sites_unittest.cc (right): https://codereview.chromium.org/2579813002/diff/1/components/ntp_tiles/most_v... components/ntp_tiles/most_visited_sites_unittest.cc:203: base::WrapUnique(mock_popular_sites_), On 2016/12/19 09:11:50, Marc Treib (OOO until Jan 12) wrote: > Hm, IMO this makes ownership kinda hard to understand. I usually prefer the > pattern: > auto ptr = MakeUnique<..>(); > Class* raw_ptr = ptr.get(); > DoTheThing(std::move(ptr)); > > WDYT? > Could that work here? The main reason for this was to avoid the need for a unique_ptr with MostVisitedSites. This will change soon anyway, so I'll do as you suggest in a future patch. https://codereview.chromium.org/2579813002/diff/1/components/ntp_tiles/most_v... components/ntp_tiles/most_visited_sites_unittest.cc:205: /*supervisor=*/{}) { On 2016/12/19 09:11:50, Marc Treib (OOO until Jan 12) wrote: > nullptr? Obsolete (done). https://codereview.chromium.org/2579813002/diff/1/components/ntp_tiles/most_v... components/ntp_tiles/most_visited_sites_unittest.cc:206: MostVisitedSites::RegisterProfilePrefs(pref_service_.registry()); On 2016/12/19 09:11:50, Marc Treib (OOO until Jan 12) wrote: > I think it'd be best to instantiate MostVisitedSites *after* we have registered > its prefs. Will address in a future patch, probably in the form of SetUpTestCase(). https://codereview.chromium.org/2579813002/diff/1/components/ntp_tiles/most_v... components/ntp_tiles/most_visited_sites_unittest.cc:214: .WillRepeatedly(ReturnRef(empty_popular_sites_vector_)); On 2016/12/19 09:11:50, Marc Treib (OOO until Jan 12) wrote: > Is this a case for ON_CALL(...).WillByDefault? I think not: setting a default could (in the future) interfere with other expectations with PopularSites::sites(). IIUC WillByDefault is used to avoid boilerplate when there's multiple expectations. See http://stackoverflow.com/questions/13933475/gmock-setting-default-actions-on-... https://codereview.chromium.org/2579813002/diff/1/components/ntp_tiles/most_v... components/ntp_tiles/most_visited_sites_unittest.cc:255: EXPECT_CALL(mock_suggestions_service_, AddCallback(_)); On 2016/12/19 09:11:50, Marc Treib (OOO until Jan 12) wrote: > Here the InSequence might be a bit too strong: It doesn't really matter which of > the two above is called first. Or am I missing something? Obsolete. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
