Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2259873002:
Add ContentSuggestionsService tests. (Closed)
DescriptionAdd ContentSuggestionsService tests.
This CL adds more tests for the ContentSuggestionsService, namely:
- for |GetCategoryInfo(category)|;
- for a case when category status changes;
BUG=638917
Committed: https://crrev.com/183c628941ee645e25d155ca5e4edffaa5872049
Cr-Commit-Position: refs/heads/master@{#413084}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Added a test. #
Total comments: 14
Patch Set 3 : Marc's comments. #Patch Set 4 : Rebase. #Messages
Total messages: 27 (12 generated)
Description was changed from ========== Added ContentSuggestionsService tests: - for |GetCategoryInfo(category)|; - for a case when category status changes; BUG=638917 ========== to ========== Added ContentSuggestionsService tests: - for |GetCategoryInfo(category)|; - for a case when category status changes; BUG=638917 ==========
vitaliii@chromium.org changed reviewers: + pke@google.com
https://codereview.chromium.org/2259873002/diff/1/components/ntp_snippets/con... File components/ntp_snippets/content_suggestions_service_unittest.cc (right): https://codereview.chromium.org/2259873002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service_unittest.cc:391: ShouldRegisterNewCategoryOnNewSuggestions) { OnCategoryStatusChanged has the same ability of adding a category that didn't exist before. Testing that when the status becomes AVAILABLE is trivial, it's the same as this test case here, so we probably won't need another one. But there is the case where a category comes into existence without becoming AVAILABLE. So maybe could add a ShouldRegisterNewCategoryOnCategoryStatusChanged and use SIGNED_OUT or INITIALIZING there.
Description was changed from ========== Added ContentSuggestionsService tests: - for |GetCategoryInfo(category)|; - for a case when category status changes; BUG=638917 ========== to ========== Adds ContentSuggestionsService tests. This CL adds more tests for the ContentSuggestionsService, namely: - for |GetCategoryInfo(category)|; - for a case when category status changes; BUG=638917 ==========
Description was changed from ========== Adds ContentSuggestionsService tests. This CL adds more tests for the ContentSuggestionsService, namely: - for |GetCategoryInfo(category)|; - for a case when category status changes; BUG=638917 ========== to ========== Add ContentSuggestionsService tests. This CL adds more tests for the ContentSuggestionsService, namely: - for |GetCategoryInfo(category)|; - for a case when category status changes; BUG=638917 ==========
https://codereview.chromium.org/2259873002/diff/1/components/ntp_snippets/con... File components/ntp_snippets/content_suggestions_service_unittest.cc (right): https://codereview.chromium.org/2259873002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service_unittest.cc:391: ShouldRegisterNewCategoryOnNewSuggestions) { On 2016/08/18 12:51:58, Philipp Keck wrote: > OnCategoryStatusChanged has the same ability of adding a category that didn't > exist before. Testing that when the status becomes AVAILABLE is trivial, it's > the same as this test case here, so we probably won't need another one. But > there is the case where a category comes into existence without becoming > AVAILABLE. > So maybe could add a ShouldRegisterNewCategoryOnCategoryStatusChanged and use > SIGNED_OUT or INITIALIZING there. Done.
lgtm
vitaliii@chromium.org changed reviewers: + treib@chromium.org
Protip: Write some message when you add a reviewer, saying what you expect from them. The empty email is very easy to miss otherwise. https://codereview.chromium.org/2259873002/diff/20001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service_unittest.cc (right): https://codereview.chromium.org/2259873002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service_unittest.cc:65: void SetProvidedCategories(std::vector<Category> provided_categories) { nit: const std::vector<..>& ? (Also in the ctor above) https://codereview.chromium.org/2259873002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service_unittest.cc:170: MockProvider* MakeProvider(Category provided_category) { Not your doing, but would you mind renaming this to (maybe) RegisterProvider? As it is, the test code looks like it's leaking memory. https://codereview.chromium.org/2259873002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service_unittest.cc:382: EXPECT_TRUE(result.has_value()); nit: ASSERT_TRUE so that the test stops if it's null. Otherwise it'll crash later. https://codereview.chromium.org/2259873002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service_unittest.cc:398: // |OnCategoryStatusChanged|. [Mostly to pke:] Is this something we want to support? Shouldn't we require that the provider calls OnCategoryStatusChanged first? https://codereview.chromium.org/2259873002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service_unittest.cc:403: EXPECT_CALL(observer, OnNewSuggestions(new_category)).Times(1); .Times(1) isn't necessary, it doesn't do anything (applies everywhere) https://codereview.chromium.org/2259873002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service_unittest.cc:410: EXPECT_THAT(providers().at(category), Eq(provider)); EXPECT_EQ or EXPECT_THAT(.., Eq(..))? Either's fine, but please be consistent within the file.
https://codereview.chromium.org/2259873002/diff/20001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service_unittest.cc (right): https://codereview.chromium.org/2259873002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service_unittest.cc:398: // |OnCategoryStatusChanged|. On 2016/08/18 15:27:42, Marc Treib wrote: > [Mostly to pke:] Is this something we want to support? Shouldn't we require that > the provider calls OnCategoryStatusChanged first? Last time we talked about this with Tim, we said we support this without thinking about it further. I have an item on my todo list to merge these two methods into one. If that goes well and leads to nicer code, this question is gone anyway. Otherwise, we can reconsider then.
https://codereview.chromium.org/2259873002/diff/20001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service_unittest.cc (right): https://codereview.chromium.org/2259873002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service_unittest.cc:398: // |OnCategoryStatusChanged|. On 2016/08/18 15:29:56, Philipp Keck wrote: > On 2016/08/18 15:27:42, Marc Treib wrote: > > [Mostly to pke:] Is this something we want to support? Shouldn't we require > that > > the provider calls OnCategoryStatusChanged first? > > Last time we talked about this with Tim, we said we support this without > thinking about it further. > > I have an item on my todo list to merge these two methods into one. If that goes > well and leads to nicer code, this question is gone anyway. Otherwise, we can > reconsider then. Alright, thanks. Then I'm okay with leaving this as-is for now. Is it worth adding a comment here?
Hello trieb, Please review a changelist. https://codereview.chromium.org/2259873002/diff/20001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service_unittest.cc (right): https://codereview.chromium.org/2259873002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service_unittest.cc:65: void SetProvidedCategories(std::vector<Category> provided_categories) { On 2016/08/18 15:27:42, Marc Treib wrote: > nit: const std::vector<..>& ? (Also in the ctor above) Done. https://codereview.chromium.org/2259873002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service_unittest.cc:170: MockProvider* MakeProvider(Category provided_category) { On 2016/08/18 15:27:42, Marc Treib wrote: > Not your doing, but would you mind renaming this to (maybe) RegisterProvider? As > it is, the test code looks like it's leaking memory. Done. https://codereview.chromium.org/2259873002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service_unittest.cc:382: EXPECT_TRUE(result.has_value()); On 2016/08/18 15:27:42, Marc Treib wrote: > nit: ASSERT_TRUE so that the test stops if it's null. Otherwise it'll crash > later. Done. https://codereview.chromium.org/2259873002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service_unittest.cc:398: // |OnCategoryStatusChanged|. On 2016/08/18 15:33:55, Marc Treib wrote: > On 2016/08/18 15:29:56, Philipp Keck wrote: > > On 2016/08/18 15:27:42, Marc Treib wrote: > > > [Mostly to pke:] Is this something we want to support? Shouldn't we require > > that > > > the provider calls OnCategoryStatusChanged first? > > > > Last time we talked about this with Tim, we said we support this without > > thinking about it further. > > > > I have an item on my todo list to merge these two methods into one. If that > goes > > well and leads to nicer code, this question is gone anyway. Otherwise, we can > > reconsider then. > > Alright, thanks. Then I'm okay with leaving this as-is for now. Is it worth > adding a comment here? Done. https://codereview.chromium.org/2259873002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service_unittest.cc:403: EXPECT_CALL(observer, OnNewSuggestions(new_category)).Times(1); On 2016/08/18 15:27:42, Marc Treib wrote: > .Times(1) isn't necessary, it doesn't do anything (applies everywhere) Done. https://codereview.chromium.org/2259873002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service_unittest.cc:410: EXPECT_THAT(providers().at(category), Eq(provider)); On 2016/08/18 15:27:43, Marc Treib wrote: > EXPECT_EQ or EXPECT_THAT(.., Eq(..))? Either's fine, but please be consistent > within the file. Done.
LGTM, thanks!
The CQ bit was checked by vitaliii@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pke@google.com Link to the patchset: https://codereview.chromium.org/2259873002/#ps40001 (title: "Marc's comments.")
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
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by vitaliii@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org, pke@google.com Link to the patchset: https://codereview.chromium.org/2259873002/#ps60001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add ContentSuggestionsService tests. This CL adds more tests for the ContentSuggestionsService, namely: - for |GetCategoryInfo(category)|; - for a case when category status changes; BUG=638917 ========== to ========== Add ContentSuggestionsService tests. This CL adds more tests for the ContentSuggestionsService, namely: - for |GetCategoryInfo(category)|; - for a case when category status changes; BUG=638917 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Add ContentSuggestionsService tests. This CL adds more tests for the ContentSuggestionsService, namely: - for |GetCategoryInfo(category)|; - for a case when category status changes; BUG=638917 ========== to ========== Add ContentSuggestionsService tests. This CL adds more tests for the ContentSuggestionsService, namely: - for |GetCategoryInfo(category)|; - for a case when category status changes; BUG=638917 Committed: https://crrev.com/183c628941ee645e25d155ca5e4edffaa5872049 Cr-Commit-Position: refs/heads/master@{#413084} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/183c628941ee645e25d155ca5e4edffaa5872049 Cr-Commit-Position: refs/heads/master@{#413084} |
