Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2244123004:
Added CategoryFactoryTest. (Closed)
DescriptionAdded CategoryFactoryTest.
BUG=638189
Committed: https://crrev.com/2a14fc09be905556a6a3b123a3e550f00932c3ba
Cr-Commit-Position: refs/heads/master@{#412777}
Patch Set 1 #
Total comments: 18
Patch Set 2 : Adressed Marc's comments. #
Total comments: 4
Patch Set 3 : Addressed Marc's comments. #Patch Set 4 : The last comment. #
Messages
Total messages: 18 (7 generated)
vitaliii@chromium.org changed reviewers: + treib@chromium.org
pke@google.com changed reviewers: + pke@google.com
lgtm https://codereview.chromium.org/2244123004/diff/1/components/ntp_snippets/cat... File components/ntp_snippets/category_factory_unittest.cc (right): https://codereview.chromium.org/2244123004/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_factory_unittest.cc:24: ++unused_remote_category_id_; Isn't this whole function equivalent to "return unused_remote_category_id++"? https://codereview.chromium.org/2244123004/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_factory_unittest.cc:106: for (size_t i = 0; i < 4; ++i) { Include stddef.h because of size_t? Let Marc decide.
Thanks for adding these! A few comments below. https://codereview.chromium.org/2244123004/diff/1/components/ntp_snippets/cat... File components/ntp_snippets/category_factory_unittest.cc (right): https://codereview.chromium.org/2244123004/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_factory_unittest.cc:20: void SetUp() override { unused_remote_category_id_ = 1; } Put this in the constructor and remove SetUp https://codereview.chromium.org/2244123004/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_factory_unittest.cc:37: const CategoryFactory* factory() const { return &factory_; } I think the const version isn't required. https://codereview.chromium.org/2244123004/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_factory_unittest.cc:48: const KnownCategories known_category = KnownCategoryByID(0); Just use any actual known category here and get rid of KnownCategoryByID? https://codereview.chromium.org/2244123004/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_factory_unittest.cc:104: TEST_F(CategoryFactoryTest, CompareCategoriesSortContract) { This name is not very expressive - it really tests that remote categories are ordered by order of registering. https://codereview.chromium.org/2244123004/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_factory_unittest.cc:106: for (size_t i = 0; i < 4; ++i) { On 2016/08/16 13:22:47, Philipp Keck wrote: > Include stddef.h because of size_t? Let Marc decide. Eh.. it's technically required, but I think most files in Chrome don't include it, and I've never seen any problems caused by that. Since the counter isn't used for anything, might as well just make it an int. https://codereview.chromium.org/2244123004/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_factory_unittest.cc:124: CompareCategories(categories[first_i], categories[second_i])); Hm, I find this a bit hard to read - it's not really clear from reading the code what the expected behavior is. Would the test lose anything if there were just two categories, and we hardcoded the expected ordering?
Description was changed from ========== added CategoryFactoryTest. BUG=638189 ========== to ========== Added CategoryFactoryTest. BUG=638189 ==========
https://codereview.chromium.org/2244123004/diff/1/components/ntp_snippets/cat... File components/ntp_snippets/category_factory_unittest.cc (right): https://codereview.chromium.org/2244123004/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_factory_unittest.cc:20: void SetUp() override { unused_remote_category_id_ = 1; } On 2016/08/16 13:44:21, Marc Treib wrote: > Put this in the constructor and remove SetUp Done. https://codereview.chromium.org/2244123004/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_factory_unittest.cc:37: const CategoryFactory* factory() const { return &factory_; } On 2016/08/16 13:44:21, Marc Treib wrote: > I think the const version isn't required. Done. https://codereview.chromium.org/2244123004/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_factory_unittest.cc:48: const KnownCategories known_category = KnownCategoryByID(0); On 2016/08/16 13:44:21, Marc Treib wrote: > Just use any actual known category here and get rid of KnownCategoryByID? Done. https://codereview.chromium.org/2244123004/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_factory_unittest.cc:104: TEST_F(CategoryFactoryTest, CompareCategoriesSortContract) { On 2016/08/16 13:44:21, Marc Treib wrote: > This name is not very expressive - it really tests that remote categories are > ordered by order of registering. Acknowledged. https://codereview.chromium.org/2244123004/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_factory_unittest.cc:106: for (size_t i = 0; i < 4; ++i) { On 2016/08/16 13:44:21, Marc Treib wrote: > On 2016/08/16 13:22:47, Philipp Keck wrote: > > Include stddef.h because of size_t? Let Marc decide. > > Eh.. it's technically required, but I think most files in Chrome don't include > it, and I've never seen any problems caused by that. > Since the counter isn't used for anything, might as well just make it an int. Acknowledged. https://codereview.chromium.org/2244123004/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_factory_unittest.cc:124: CompareCategories(categories[first_i], categories[second_i])); On 2016/08/16 13:44:21, Marc Treib wrote: > Hm, I find this a bit hard to read - it's not really clear from reading the code > what the expected behavior is. > > Would the test lose anything if there were just two categories, and we hardcoded > the expected ordering? It is already done just for two categories in FromRemoteCategoryOrder. I rewrote this test to test two categories when more than two have been added.
https://codereview.chromium.org/2244123004/diff/1/components/ntp_snippets/cat... File components/ntp_snippets/category_factory_unittest.cc (right): https://codereview.chromium.org/2244123004/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_factory_unittest.cc:24: ++unused_remote_category_id_; On 2016/08/16 13:22:47, Philipp Keck wrote: > Isn't this whole function equivalent to "return unused_remote_category_id++"? +1 - I'd just do return unused_remote_category_id++; https://codereview.chromium.org/2244123004/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_factory_unittest.cc:104: TEST_F(CategoryFactoryTest, CompareCategoriesSortContract) { On 2016/08/17 13:45:07, vitaliii wrote: > On 2016/08/16 13:44:21, Marc Treib wrote: > > This name is not very expressive - it really tests that remote categories are > > ordered by order of registering. > > Acknowledged. CL tool usage nit: "Acknowledged" means "I read your comment but didn't do anything about it". Usually you want "Done" instead :) https://codereview.chromium.org/2244123004/diff/20001/components/ntp_snippets... File components/ntp_snippets/category_factory_unittest.cc (right): https://codereview.chromium.org/2244123004/diff/20001/components/ntp_snippets... components/ntp_snippets/category_factory_unittest.cc:18: CategoryFactoryTest() { unused_remote_category_id_ = 1; } nit: Do this in the initializer list instead of in the ctor body. https://codereview.chromium.org/2244123004/diff/20001/components/ntp_snippets... components/ntp_snippets/category_factory_unittest.cc:64: Category added_first = factory()->FromRemoteCategory(second_id); Add a comment here, saying that you add second_id first on purpose? I missed that on the first read, and if I hadn't, I would probably have been confused. Maybe give the id vars different names? (small_id, large_id?)
https://codereview.chromium.org/2244123004/diff/20001/components/ntp_snippets... File components/ntp_snippets/category_factory_unittest.cc (right): https://codereview.chromium.org/2244123004/diff/20001/components/ntp_snippets... components/ntp_snippets/category_factory_unittest.cc:18: CategoryFactoryTest() { unused_remote_category_id_ = 1; } On 2016/08/17 14:41:34, Marc Treib wrote: > nit: Do this in the initializer list instead of in the ctor body. Done. https://codereview.chromium.org/2244123004/diff/20001/components/ntp_snippets... components/ntp_snippets/category_factory_unittest.cc:64: Category added_first = factory()->FromRemoteCategory(second_id); On 2016/08/17 14:41:34, Marc Treib wrote: > Add a comment here, saying that you add second_id first on purpose? I missed > that on the first read, and if I hadn't, I would probably have been confused. > Maybe give the id vars different names? (small_id, large_id?) Done.
LGTM with one more still-open comment (the review tool is a bit weird - it's not enough to look at comments on the latest patch set, you actually do have to read everything in the message) https://codereview.chromium.org/2244123004/diff/1/components/ntp_snippets/cat... File components/ntp_snippets/category_factory_unittest.cc (right): https://codereview.chromium.org/2244123004/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_factory_unittest.cc:24: ++unused_remote_category_id_; On 2016/08/17 14:41:34, Marc Treib wrote: > On 2016/08/16 13:22:47, Philipp Keck wrote: > > Isn't this whole function equivalent to "return unused_remote_category_id++"? > > +1 - I'd just do > return unused_remote_category_id++; This is still open.
https://codereview.chromium.org/2244123004/diff/1/components/ntp_snippets/cat... File components/ntp_snippets/category_factory_unittest.cc (right): https://codereview.chromium.org/2244123004/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_factory_unittest.cc:24: ++unused_remote_category_id_; On 2016/08/17 14:41:34, Marc Treib wrote: > On 2016/08/16 13:22:47, Philipp Keck wrote: > > Isn't this whole function equivalent to "return unused_remote_category_id++"? > > +1 - I'd just do > return unused_remote_category_id++; Done.
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, treib@chromium.org Link to the patchset: https://codereview.chromium.org/2244123004/#ps60001 (title: "The last comment.")
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 ========== Added CategoryFactoryTest. BUG=638189 ========== to ========== Added CategoryFactoryTest. BUG=638189 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Added CategoryFactoryTest. BUG=638189 ========== to ========== Added CategoryFactoryTest. BUG=638189 Committed: https://crrev.com/2a14fc09be905556a6a3b123a3e550f00932c3ba Cr-Commit-Position: refs/heads/master@{#412777} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/2a14fc09be905556a6a3b123a3e550f00932c3ba Cr-Commit-Position: refs/heads/master@{#412777} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
