Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2151863002:
Add Unit-Test for ContentSuggestionsService (Closed)
|
Created:
4 years, 5 months ago by Philipp Keck Modified:
4 years, 5 months ago Reviewers:
Marc Treib CC:
chromium-reviews, ntp-dev+reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@offlinepagesprovider Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd Unit-Test for ContentSuggestionsService
Implement ContentSuggestionsServiceTest
BUG=619560
Committed: https://crrev.com/adf6d2f83a45d1f7435d2294e72f7f6b4ecf1396
Cr-Commit-Position: refs/heads/master@{#406240}
Patch Set 1 #
Total comments: 48
Patch Set 2 : Comments #
Total comments: 12
Patch Set 3 : More comments #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 13 (3 generated)
pke@google.com changed reviewers: + treib@chromium.org
PTAL Note that this CL depends on the offline-page-provider CL only because it uses the category enum value.
Nice! Bunch of comments below... https://codereview.chromium.org/2151863002/diff/1/components/ntp_snippets/con... File components/ntp_snippets/content_suggestions_service_unittest.cc (right): https://codereview.chromium.org/2151863002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service_unittest.cc:15: #include "components/ntp_snippets/switches.h" not needed I think? https://codereview.chromium.org/2151863002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service_unittest.cc:33: using State = ntp_snippets::ContentSuggestionsService::State; Hm, IMO these make things kinda harder to read. Are you very opposed to spelling out the types? Note that "ntp_snippets::" isn't required since everything is in that namespace. https://codereview.chromium.org/2151863002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service_unittest.cc:40: ContentSuggestion makeSuggestion(int number) { MakeSuggestions (capitalize), or maybe CreateSuggestion? https://codereview.chromium.org/2151863002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service_unittest.cc:42: std::to_string(number), base::IntToString https://codereview.chromium.org/2151863002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service_unittest.cc:57: std::map<CSCategory, CSStatus> statuses_; These should go at the end, and be private. https://codereview.chromium.org/2151863002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service_unittest.cc:76: observer_->OnNewSuggestions(category, std::move(suggestions)); Might as well just inline the MakeSuggestions here (also then the std::move won't be needed anymore) https://codereview.chromium.org/2151863002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service_unittest.cc:97: MOCK_METHOD2(Run, void(const std::string& suggestion_id, const gfx::Image&)); I'd just make this an OnImageFetched method in the MockProvider class; no need for a separate class. https://codereview.chromium.org/2151863002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service_unittest.cc:106: ~MockServiceObserver() {} Should probably be override? https://codereview.chromium.org/2151863002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service_unittest.cc:119: void CreateContentSuggestionsService(State enabled) { Make this protected? https://codereview.chromium.org/2151863002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service_unittest.cc:131: return; I think this isn't required - if the category doesn't exist, the service will return an empty list of suggestions anyway? https://codereview.chromium.org/2151863002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service_unittest.cc:140: auto colon_index = combined_id.find(":"); Errr wait, where does the colon come from here? We create the suggestions with just stringified numbers as IDs, no? https://codereview.chromium.org/2151863002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service_unittest.cc:161: void ServiceShutdown() { nit: empty line between methods https://codereview.chromium.org/2151863002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service_unittest.cc:165: ((MockProvider*)providers().begin()->second)->FireShutdown(); static_cast But is this required? Shouldn't each test shutdown all the providers it creates? If you do want a helper for this, I'd at least make it explicit by calling it ShutdownAllProviders() https://codereview.chromium.org/2151863002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service_unittest.cc:170: service_.reset(); This could go into the if as well. https://codereview.chromium.org/2151863002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service_unittest.cc:195: service()->RegisterProvider(&provider2); Maybe register just provider1 at first and check the category statuses etc? https://codereview.chromium.org/2151863002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service_unittest.cc:219: TEST_F(ContentSuggestionsServiceDisabledTest, DisabledTest) { nit: "DisabledTest" isn't a very useful name. "ShouldNotServeSuggestions" or something like that maybe? ("Disabled" is already in the class name, no need to repeat) https://codereview.chromium.org/2151863002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service_unittest.cc:225: CSStatus::ALL_SUGGESTIONS_EXPLICITLY_DISABLED); You're missing an Eq() here, right? (Also in a bunch of other places) https://codereview.chromium.org/2151863002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service_unittest.cc:288: service()->AddObserver(&observer); You should probably remove the observer again at the end. https://codereview.chromium.org/2151863002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service_unittest.cc:355: ServiceShutdown(); // Early shutdown because providers will go out of scope Not required here, since the providers are already unregistered, right?
https://codereview.chromium.org/2151863002/diff/1/components/ntp_snippets/con... File components/ntp_snippets/content_suggestions_service_unittest.cc (right): https://codereview.chromium.org/2151863002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service_unittest.cc:15: #include "components/ntp_snippets/switches.h" On 2016/07/14 15:21:11, Marc Treib wrote: > not needed I think? Done. https://codereview.chromium.org/2151863002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service_unittest.cc:33: using State = ntp_snippets::ContentSuggestionsService::State; On 2016/07/14 15:21:11, Marc Treib wrote: > Hm, IMO these make things kinda harder to read. Are you very opposed to spelling > out the types? Note that "ntp_snippets::" isn't required since everything is in > that namespace. I'm not "very" opposed. I changed it -- introduced 49 new line breaks. https://codereview.chromium.org/2151863002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service_unittest.cc:40: ContentSuggestion makeSuggestion(int number) { On 2016/07/14 15:21:11, Marc Treib wrote: > MakeSuggestions (capitalize), or maybe CreateSuggestion? Done. https://codereview.chromium.org/2151863002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service_unittest.cc:42: std::to_string(number), On 2016/07/14 15:21:11, Marc Treib wrote: > base::IntToString Done. https://codereview.chromium.org/2151863002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service_unittest.cc:57: std::map<CSCategory, CSStatus> statuses_; On 2016/07/14 15:21:10, Marc Treib wrote: > These should go at the end, and be private. Done. https://codereview.chromium.org/2151863002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service_unittest.cc:76: observer_->OnNewSuggestions(category, std::move(suggestions)); On 2016/07/14 15:21:10, Marc Treib wrote: > Might as well just inline the MakeSuggestions here (also then the std::move > won't be needed anymore) Not sure about that. The separate MakeSuggestions might prove useful when extending the unit test later. Also, it keeps the test data separate (even if it's not in a separate file in this case) from the test logic. https://codereview.chromium.org/2151863002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service_unittest.cc:97: MOCK_METHOD2(Run, void(const std::string& suggestion_id, const gfx::Image&)); On 2016/07/14 15:21:11, Marc Treib wrote: > I'd just make this an OnImageFetched method in the MockProvider class; no need > for a separate class. Do you mean the ContentSuggestionsServiceTest class? This is the callback that the UI would normally pass in. So while it would certainly compile, it doesn't make sense for this to live in the MockProvider. From the viewpoint of the UI-facing interface to ContentSuggestionsService, you make a request for an image, pass in a callback, and then someone (possibly even the service, but in reality the provider) calls that callback. https://codereview.chromium.org/2151863002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service_unittest.cc:106: ~MockServiceObserver() {} On 2016/07/14 15:21:11, Marc Treib wrote: > Should probably be override? Done. https://codereview.chromium.org/2151863002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service_unittest.cc:119: void CreateContentSuggestionsService(State enabled) { On 2016/07/14 15:21:10, Marc Treib wrote: > Make this protected? Done. https://codereview.chromium.org/2151863002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service_unittest.cc:131: return; On 2016/07/14 15:21:10, Marc Treib wrote: > I think this isn't required - if the category doesn't exist, the service will > return an empty list of suggestions anyway? Done. https://codereview.chromium.org/2151863002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service_unittest.cc:140: auto colon_index = combined_id.find(":"); On 2016/07/14 15:21:10, Marc Treib wrote: > Errr wait, where does the colon come from here? We create the suggestions with > just stringified numbers as IDs, no? Oh yes, of course! Amazing that the tests still pass even with this in here ... :-) https://codereview.chromium.org/2151863002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service_unittest.cc:161: void ServiceShutdown() { On 2016/07/14 15:21:10, Marc Treib wrote: > nit: empty line between methods Done. https://codereview.chromium.org/2151863002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service_unittest.cc:165: ((MockProvider*)providers().begin()->second)->FireShutdown(); On 2016/07/14 15:21:11, Marc Treib wrote: > static_cast > But is this required? Shouldn't each test shutdown all the providers it creates? > If you do want a helper for this, I'd at least make it explicit by calling it > ShutdownAllProviders() Ok. At this point, it makes sense. When I first wrote this helper method, the service was still in charge of shutting the providers down and then this helper method gradually changed. I changed the whole shutdown as you proposed: every test shuts down its providers. The test class now has a TearDown() to shutdown the service. For the last test method, it will call the service's Shutdown() method a second time, but that's fine at least with the current service implementation. https://codereview.chromium.org/2151863002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service_unittest.cc:170: service_.reset(); On 2016/07/14 15:21:10, Marc Treib wrote: > This could go into the if as well. Right. Now it's gone anyway. https://codereview.chromium.org/2151863002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service_unittest.cc:195: service()->RegisterProvider(&provider2); On 2016/07/14 15:21:10, Marc Treib wrote: > Maybe register just provider1 at first and check the category statuses etc? Done. https://codereview.chromium.org/2151863002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service_unittest.cc:219: TEST_F(ContentSuggestionsServiceDisabledTest, DisabledTest) { On 2016/07/14 15:21:10, Marc Treib wrote: > nit: "DisabledTest" isn't a very useful name. "ShouldNotServeSuggestions" or > something like that maybe? ("Disabled" is already in the class name, no need to > repeat) I could rename all tests to use the "should ..." format? https://codereview.chromium.org/2151863002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service_unittest.cc:225: CSStatus::ALL_SUGGESTIONS_EXPLICITLY_DISABLED); On 2016/07/14 15:21:10, Marc Treib wrote: > You're missing an Eq() here, right? (Also in a bunch of other places) Done. https://codereview.chromium.org/2151863002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service_unittest.cc:288: service()->AddObserver(&observer); On 2016/07/14 15:21:10, Marc Treib wrote: > You should probably remove the observer again at the end. Done. https://codereview.chromium.org/2151863002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service_unittest.cc:355: ServiceShutdown(); // Early shutdown because providers will go out of scope On 2016/07/14 15:21:10, Marc Treib wrote: > Not required here, since the providers are already unregistered, right? It wasn't required above in the first test, but is required here because I want to check that ContentSuggestionsServiceShutdown() is called. I removed the comment and added another one (because Shutdown() is called twice).
https://codereview.chromium.org/2151863002/diff/1/components/ntp_snippets/con... File components/ntp_snippets/content_suggestions_service_unittest.cc (right): https://codereview.chromium.org/2151863002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service_unittest.cc:7: #include <memory> This one is technically required for unique_ptr https://codereview.chromium.org/2151863002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service_unittest.cc:76: observer_->OnNewSuggestions(category, std::move(suggestions)); On 2016/07/14 17:06:27, Philipp Keck wrote: > On 2016/07/14 15:21:10, Marc Treib wrote: > > Might as well just inline the MakeSuggestions here (also then the std::move > > won't be needed anymore) > > Not sure about that. The separate MakeSuggestions might prove useful when > extending the unit test later. Also, it keeps the test data separate (even if > it's not in a separate file in this case) from the test logic. Ah, I meant just to merge these two lines into one, i.e. move the MakeSuggestions call into OnNewSuggestions directly. https://codereview.chromium.org/2151863002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service_unittest.cc:97: MOCK_METHOD2(Run, void(const std::string& suggestion_id, const gfx::Image&)); On 2016/07/14 17:06:28, Philipp Keck wrote: > On 2016/07/14 15:21:11, Marc Treib wrote: > > I'd just make this an OnImageFetched method in the MockProvider class; no need > > for a separate class. > > Do you mean the ContentSuggestionsServiceTest class? Ah yes, sorry. Mostly I was saying that there's no need to have a separate class just for a mock method. If you prefer to keep it, that's fine too, but I'd give the "Run" method a different name - I find that kinda confusing, since it's not actually a callback. > This is the callback that the UI would normally pass in. So while it would > certainly compile, it doesn't make sense for this to live in the MockProvider. > From the viewpoint of the UI-facing interface to ContentSuggestionsService, you > make a request for an image, pass in a callback, and then someone (possibly even > the service, but in reality the provider) calls that callback. https://codereview.chromium.org/2151863002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service_unittest.cc:140: auto colon_index = combined_id.find(":"); On 2016/07/14 17:06:28, Philipp Keck wrote: > On 2016/07/14 15:21:10, Marc Treib wrote: > > Errr wait, where does the colon come from here? We create the suggestions with > > just stringified numbers as IDs, no? > > Oh yes, of course! Amazing that the tests still pass even with this in here ... > :-) My theory: You get string::npos as the colon_index, which happens to be the max value of a size_t. Then you add one and get zero again. (Note that that is almost certainly not well-defined behavior though) https://codereview.chromium.org/2151863002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service_unittest.cc:219: TEST_F(ContentSuggestionsServiceDisabledTest, DisabledTest) { On 2016/07/14 17:06:28, Philipp Keck wrote: > On 2016/07/14 15:21:10, Marc Treib wrote: > > nit: "DisabledTest" isn't a very useful name. "ShouldNotServeSuggestions" or > > something like that maybe? ("Disabled" is already in the class name, no need > to > > repeat) > > I could rename all tests to use the "should ..." format? I know that Mikel prefers that format :) Personally, I don't mind too much, but I'm certainly not opposed. https://codereview.chromium.org/2151863002/diff/20001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service_unittest.cc (right): https://codereview.chromium.org/2151863002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service_unittest.cc:53: std::vector<ContentSuggestionsCategory>({provided_category})){}; Out of curiosity: Is the explicit "std::vector<...>" required here, or would the initializer list get converted automatically? https://codereview.chromium.org/2151863002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service_unittest.cc:145: DCHECK(base::StringToInt(suggestion.id(), &id)); This should probably be EXPECT_TRUE or ASSERT_TRUE, to make the test fail properly rather than crash. https://codereview.chromium.org/2151863002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service_unittest.cc:167: DCHECK(!service_); Here too https://codereview.chromium.org/2151863002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service_unittest.cc:428: // The service will receive two Shutdown() calls. Alternatively, you could reset the service here, and add a null check in TearDown.
https://codereview.chromium.org/2151863002/diff/1/components/ntp_snippets/con... File components/ntp_snippets/content_suggestions_service_unittest.cc (right): https://codereview.chromium.org/2151863002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service_unittest.cc:7: #include <memory> On 2016/07/15 08:31:56, Marc Treib wrote: > This one is technically required for unique_ptr Done. https://codereview.chromium.org/2151863002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service_unittest.cc:76: observer_->OnNewSuggestions(category, std::move(suggestions)); On 2016/07/15 08:31:56, Marc Treib wrote: > On 2016/07/14 17:06:27, Philipp Keck wrote: > > On 2016/07/14 15:21:10, Marc Treib wrote: > > > Might as well just inline the MakeSuggestions here (also then the std::move > > > won't be needed anymore) > > > > Not sure about that. The separate MakeSuggestions might prove useful when > > extending the unit test later. Also, it keeps the test data separate (even if > > it's not in a separate file in this case) from the test logic. > > Ah, I meant just to merge these two lines into one, i.e. move the > MakeSuggestions call into OnNewSuggestions directly. :-) that makes sense. Done. https://codereview.chromium.org/2151863002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service_unittest.cc:97: MOCK_METHOD2(Run, void(const std::string& suggestion_id, const gfx::Image&)); On 2016/07/15 08:31:56, Marc Treib wrote: > On 2016/07/14 17:06:28, Philipp Keck wrote: > > On 2016/07/14 15:21:11, Marc Treib wrote: > > > I'd just make this an OnImageFetched method in the MockProvider class; no > need > > > for a separate class. > > > > Do you mean the ContentSuggestionsServiceTest class? > > Ah yes, sorry. > Mostly I was saying that there's no need to have a separate class just for a > mock method. If you prefer to keep it, that's fine too, but I'd give the "Run" > method a different name - I find that kinda confusing, since it's not actually a > callback. > > > This is the callback that the UI would normally pass in. So while it would > > certainly compile, it doesn't make sense for this to live in the MockProvider. > > From the viewpoint of the UI-facing interface to ContentSuggestionsService, > you > > make a request for an image, pass in a callback, and then someone (possibly > even > > the service, but in reality the provider) calls that callback. > Ok. I think it's okay if it's in the test class itself, moved it there. https://codereview.chromium.org/2151863002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service_unittest.cc:140: auto colon_index = combined_id.find(":"); On 2016/07/15 08:31:56, Marc Treib wrote: > On 2016/07/14 17:06:28, Philipp Keck wrote: > > On 2016/07/14 15:21:10, Marc Treib wrote: > > > Errr wait, where does the colon come from here? We create the suggestions > with > > > just stringified numbers as IDs, no? > > > > Oh yes, of course! Amazing that the tests still pass even with this in here > ... > > :-) > > My theory: You get string::npos as the colon_index, which happens to be the max > value of a size_t. Then you add one and get zero again. (Note that that is > almost certainly not well-defined behavior though) Acknowledged. https://codereview.chromium.org/2151863002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service_unittest.cc:219: TEST_F(ContentSuggestionsServiceDisabledTest, DisabledTest) { On 2016/07/15 08:31:56, Marc Treib wrote: > On 2016/07/14 17:06:28, Philipp Keck wrote: > > On 2016/07/14 15:21:10, Marc Treib wrote: > > > nit: "DisabledTest" isn't a very useful name. "ShouldNotServeSuggestions" or > > > something like that maybe? ("Disabled" is already in the class name, no need > > to > > > repeat) > > > > I could rename all tests to use the "should ..." format? > > I know that Mikel prefers that format :) Personally, I don't mind too much, but > I'm certainly not opposed. Done. https://codereview.chromium.org/2151863002/diff/20001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service_unittest.cc (right): https://codereview.chromium.org/2151863002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service_unittest.cc:53: std::vector<ContentSuggestionsCategory>({provided_category})){}; On 2016/07/15 08:31:56, Marc Treib wrote: > Out of curiosity: Is the explicit "std::vector<...>" required here, or would the > initializer list get converted automatically? It would get converted automatically, but the call with only one category (be it "category" or "{category}") is ambiguous between the two constructors. It will choose the upper one when you only provide one category -- even if you write "{category}", in which case you would get an additional "error: braces around scalar initializer". Then it complains that the constructor "MockProvider(oneCategory) : MockProvider({oneCategory}) {}" is calling it self. It's the same with "{{category}}". But to simplify the code up here, this constructor could be removed altogether and all calls below could use the curly braces. https://codereview.chromium.org/2151863002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service_unittest.cc:145: DCHECK(base::StringToInt(suggestion.id(), &id)); On 2016/07/15 08:31:56, Marc Treib wrote: > This should probably be EXPECT_TRUE or ASSERT_TRUE, to make the test fail > properly rather than crash. Done. https://codereview.chromium.org/2151863002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service_unittest.cc:167: DCHECK(!service_); On 2016/07/15 08:31:56, Marc Treib wrote: > Here too Done. https://codereview.chromium.org/2151863002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service_unittest.cc:428: // The service will receive two Shutdown() calls. On 2016/07/15 08:31:56, Marc Treib wrote: > Alternatively, you could reset the service here, and add a null check in > TearDown. Yes. That would require to expose a resetService() method or service_ itself.
LGTM, thanks! https://codereview.chromium.org/2151863002/diff/20001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service_unittest.cc (right): https://codereview.chromium.org/2151863002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service_unittest.cc:53: std::vector<ContentSuggestionsCategory>({provided_category})){}; On 2016/07/15 10:05:03, Philipp Keck wrote: > On 2016/07/15 08:31:56, Marc Treib wrote: > > Out of curiosity: Is the explicit "std::vector<...>" required here, or would > the > > initializer list get converted automatically? > > It would get converted automatically, but the call with only one category (be it > "category" or "{category}") is ambiguous between the two constructors. It will > choose the upper one when you only provide one category -- even if you write > "{category}", in which case you would get an additional "error: braces around > scalar initializer". Then it complains that the constructor > "MockProvider(oneCategory) : MockProvider({oneCategory}) {}" is calling it self. > It's the same with "{{category}}". Thanks for the explanation! Looks like initializer lists have their fair share of warts... > But to simplify the code up here, this constructor could be removed altogether > and all calls below could use the curly braces. Eh, either way's fine with me. Pick whatever you prefer :) https://codereview.chromium.org/2151863002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service_unittest.cc:428: // The service will receive two Shutdown() calls. On 2016/07/15 10:05:03, Philipp Keck wrote: > On 2016/07/15 08:31:56, Marc Treib wrote: > > Alternatively, you could reset the service here, and add a null check in > > TearDown. > > Yes. That would require to expose a resetService() method or service_ itself. A ResetService SGTM. (The Service might conceivably expect that Shutdown will only be called once, because that's guaranteed in "live" code. OTOH, while the service doesn't actually care about that, I guess it doesn't matter what the tests do, so.. eh. I'll leave this up to you.)
https://codereview.chromium.org/2151863002/diff/20001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service_unittest.cc (right): https://codereview.chromium.org/2151863002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service_unittest.cc:53: std::vector<ContentSuggestionsCategory>({provided_category})){}; On 2016/07/15 10:12:28, Marc Treib wrote: > On 2016/07/15 10:05:03, Philipp Keck wrote: > > On 2016/07/15 08:31:56, Marc Treib wrote: > > > Out of curiosity: Is the explicit "std::vector<...>" required here, or would > > the > > > initializer list get converted automatically? > > > > It would get converted automatically, but the call with only one category (be > it > > "category" or "{category}") is ambiguous between the two constructors. It will > > choose the upper one when you only provide one category -- even if you write > > "{category}", in which case you would get an additional "error: braces around > > scalar initializer". Then it complains that the constructor > > "MockProvider(oneCategory) : MockProvider({oneCategory}) {}" is calling it > self. > > It's the same with "{{category}}". > > Thanks for the explanation! Looks like initializer lists have their fair share > of warts... Yes, there are examples where it silently chooses a newly added list-initializer constructor if you called the one-argument constructor with "ctor({argument})" instead of "ctor(argument)" before, even though the calling code doesn't change. > > > But to simplify the code up here, this constructor could be removed altogether > > and all calls below could use the curly braces. > > Eh, either way's fine with me. Pick whatever you prefer :) I leave it as is. Helper methods and classes should be easy to use because that's their purpose. https://codereview.chromium.org/2151863002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service_unittest.cc:428: // The service will receive two Shutdown() calls. On 2016/07/15 10:12:28, Marc Treib wrote: > On 2016/07/15 10:05:03, Philipp Keck wrote: > > On 2016/07/15 08:31:56, Marc Treib wrote: > > > Alternatively, you could reset the service here, and add a null check in > > > TearDown. > > > > Yes. That would require to expose a resetService() method or service_ itself. > > A ResetService SGTM. > (The Service might conceivably expect that Shutdown will only be called once, > because that's guaranteed in "live" code. OTOH, while the service doesn't > actually care about that, I guess it doesn't matter what the tests do, so.. eh. > I'll leave this up to you.) Maybe the KeyedService interface should specify that the Shutdown() implementation must be idempotent. The class doc already says that it's supposed to "drop references", which sounds like an idempotent action. I looked at a couple of implementations, and they all can be called twice with no extra side effects except for logging. In that case, the unit test shouldn't do any extra efforts to avoid the double call. Or the interface could specify that it must not be called twice (but service's don't have to check against that), then the unit test would need the ResetService(). Either way, the unit test could then test the defined interface of the service rather than simulating "live" situations, which would be more of an integration test. But than, can't just change the KeyedService interface. So I'll leave that and the unit test as is.
The CQ bit was checked by pke@google.com
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.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Add Unit-Test for ContentSuggestionsService Implement ContentSuggestionsServiceTest BUG=619560 ========== to ========== Add Unit-Test for ContentSuggestionsService Implement ContentSuggestionsServiceTest BUG=619560 Committed: https://crrev.com/adf6d2f83a45d1f7435d2294e72f7f6b4ecf1396 Cr-Commit-Position: refs/heads/master@{#406240} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/adf6d2f83a45d1f7435d2294e72f7f6b4ecf1396 Cr-Commit-Position: refs/heads/master@{#406240} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
