Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2285133004:
Overhaul ntp_snippets_service_unittest.cc. (Closed)
DescriptionOverhaul ntp_snippets_service_unittest.cc.
A partial list of things done here:
* Move tests that verified NTPSnippet::best_source() behavior into
ntp_snippet_test.cc; they don't belong here.
* Switch it to use the chromecontentsuggestions-pa. Testing new
features like server-provided categories is not possible without
this.
* Replace MockContentSuggestionsProviderObserver with a fake. It
shouldn't have been a mock, since we weren't using it to test
behavior, only state.
* In one case, test using what the observer sees, instead of peeking
in through debugging hooks.
* Get rid of special test base class and Mock::VerifyAndClear calls; I
consider these anti-patterns.
* Initialize the service better; give it an empty list of snippets for
its first fetch, and wait for it to complete the fetch before
returning (I think this is what was most strongly implicated by the
failures with the other CL).
* Create the service and expect the scheduling directly from each
case, so they are more readable individually.
Committed: https://crrev.com/e88d16809c5f7a0d5e8d59caa451dc4359fa7b06
Cr-Commit-Position: refs/heads/master@{#415358}
Patch Set 1 #
Total comments: 66
Patch Set 2 : Address review comments. #Patch Set 3 : Fix histogram test. #Patch Set 4 : merge #
Total comments: 6
Patch Set 5 : Address review comments. #Patch Set 6 : No brackets. #
Dependent Patchsets: Messages
Total messages: 31 (18 generated)
sfiera@chromium.org changed reviewers: + treib@chromium.org
With this, I'll be able to write proper tests for the other CL, instead of just having tests mysteriously fail!
tschumann@chromium.org changed reviewers: + tschumann@chromium.org
i like the overall change a lot. I left some comments, not sure about our team practice with CHECK()s in tests but I'm personally not opposed to them. https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippet_unittest.cc (right): https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippet_unittest.cc:19: if (!json_value->GetAsDictionary(&json_dict)) { as you don't expect this to ever fail, you can also just CHECK(json_value->...); [IMO, it's fine to CHECK-fail in case of bugs inside the test code itself.] https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippet_unittest.cc:58: if (!dict) { i don't think you need to handle this case. None of the callers relies on this. https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippet_unittest.cc:93: if (!json_value->GetAsDictionary(&json_dict)) { CHECK()? https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippet_unittest.cc:209: if (!json_value->GetAsDictionary(&json_dict)) { CHECK()? https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_service_unittest.cc (right): https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service_unittest.cc:72: "https://chromecontentsuggestions-pa.googleapis.com/v1/suggestions/" reading this test, you might think that we're actually fetching data from service (which we hopefully don't as we are using a fake fetcher). What requirements to the to-be-fake-fetched URLs do we need to meet? Would be nice if we could have a string that makes it very clear that this is not a real URL to be fetched, like "fake-url-to-suggestions-api" https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service_unittest.cc:120: std::string ids_string; ids_string = base::JoinStrings(ids, "\",\n \""); https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service_unittest.cc:193: if (pos == std::string::npos) { couldn't we just CHECK() then? https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service_unittest.cc:254: class FakeContentSuggestionsProviderObserver nice! https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service_unittest.cc:323: class NTPSnippetsServiceTest : public ::testing::Test { yay! https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service_unittest.cc:437: MockScheduler scheduler_; consider making this a ::testing::NiceMock<MockScheduler> The nice thing is that if you don't define an expectation for a function call, it won't trigger a warning. So test cases which don't care about whether or not the scheduler get's called don't need to care about the EXPECT_CALL() statements. But if you add them, then they get verified like with normal (or StrictMock<>s). https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service_unittest.cc:472: // EXPECT_EQ(GURL(kSnippetSalientImage), snippet.salient_image_url()); why is this commented out? https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service_unittest.cc:623: EXPECT_CALL(callback, MockRun(_, _)) not now, but this mock usage also has a pretty bad smell. The problem is that we are not setting up a proper expectation of the call itself but verify the parameters as part of the action. Parameters should be verified with parameter matchers. So either, we follow this pattern, but then I'd rather have a free function that simply captures the state (as done with CaptureDismissedSuggestions in https://codereview.chromium.org/2291593002/). Or we provide proper matchers to verify the parameter. In this case (and because of the trickiness with mocking move-only parameters), I'd be in favor of just binding a normal function that captures the state to be verified after the call. But like I said, this can totally happen in another CL. https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_test_utils.h (right): https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_test_utils.h:42: class NTPSnippetsTestUtils { very nice! I feel we're making not enough use of composition patterns (yet) ;-)
Very nice! Bunch of comments below, but nothing major. https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippet_unittest.cc (right): https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippet_unittest.cc:19: if (!json_value->GetAsDictionary(&json_dict)) { On 2016/08/30 06:57:21, tschumann wrote: > as you don't expect this to ever fail, you can also just CHECK(json_value->...); > > [IMO, it's fine to CHECK-fail in case of bugs inside the test code itself.] I find it a bit nicer to ASSERT_*, so the test fails "properly" rather than crashing. https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippet_unittest.cc:65: std::string kJsonStr = nit: If you use kConstantNaming, you should also make it constant https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippet_unittest.cc:103: // Expect the first source to be chosen Why? AFAICT, they're equivalent. Do we really want to test that the first one gets chosen in that case? nit: Comments should end with a ".", also below. https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_service_unittest.cc (right): https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service_unittest.cc:113: std::string GetSnippetWithUrlAndTimesAndSources( "Sources" isn't accurate anymore, there's only one. https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service_unittest.cc:128: "{\n" nit: indentation? https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service_unittest.cc:150: {kSnippetUrl}, source_urls[0], GetDefaultCreationTime(), This seems wrong - why pass in a vector of source_urls but only use one? https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service_unittest.cc:264: suggestions_[category].insert(suggestions_[category].begin(), Shouldn't you clear suggestions_[category_] first? That's what you would expect of a ProviderObserver - OnNewSuggestions replaces any that were there before. https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service_unittest.cc:332: google_apis::GetAPIKey().c_str())), I don't think the actual API key matters here? We can probably just hardcode some arbitrary value into our URL constant? https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service_unittest.cc:358: utils_.ResetSigninManager(); So if you call MakeSnippetsService twice in a row, the first service will be in a bad state, right? Can we somehow ensure that that doesn't happen? https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service_unittest.cc:466: const ContentSuggestion& snippet = Can we rename this to |suggestion| please? https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service_unittest.cc:668: ASSERT_THAT(service->GetSnippetsForTesting(), SizeIs(0)); Why has the behavior changed here? Also, this should be EXPECT rather than ASSERT. https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service_unittest.cc:703: {GetSnippetWithSources({"aaaa"}, "Source 1", "http://source1.amp.com")})); nit: s/aaaa/not a url/ ? https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service_unittest.cc:772: // Recreating the service and loading from prefs shouldn't count as fetched Yay outdated comments - "loading from prefs" isn't correct anymore; we're loading from the DB. https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service_unittest.cc:773: // articles. However, recreating the service does trigger one fetch. Why? If we get suggestions from the DB, we shouldn't fetch again. https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service_unittest.cc:791: const std::vector<std::string> publishers = {"Mashable", "AOL", "Mashable"}; The third entry is never used. https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service_unittest.cc:792: const std::vector<std::string> amp_urls = { Only amp_urls[1] is ever used. https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service_unittest.cc:801: publishers[0], amp_urls[1])})); Should this be amp_urls[0]? https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service_unittest.cc:855: .WillOnce(testing::WithArgs<0, 2>(Invoke(ServeOneByOneImage))); optional: You could add "using testing::..." statements, there's already a bunch of them.
Patchset #3 (id:40001) has been deleted
The CQ bit was checked by sfiera@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...
https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippet_unittest.cc (right): https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippet_unittest.cc:19: if (!json_value->GetAsDictionary(&json_dict)) { On 2016/08/30 09:29:28, Marc Treib wrote: > On 2016/08/30 06:57:21, tschumann wrote: > > as you don't expect this to ever fail, you can also just > CHECK(json_value->...); > > > > [IMO, it's fine to CHECK-fail in case of bugs inside the test code itself.] > > I find it a bit nicer to ASSERT_*, so the test fails "properly" rather than > crashing. My feeling is that ASSERTing is better but often not worth the effort. Here, since I've already made the effort, I'd keep it. https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippet_unittest.cc:65: std::string kJsonStr = On 2016/08/30 09:29:28, Marc Treib wrote: > nit: If you use kConstantNaming, you should also make it constant Done. https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippet_unittest.cc:103: // Expect the first source to be chosen On 2016/08/30 09:29:29, Marc Treib wrote: > Why? AFAICT, they're equivalent. Do we really want to test that the first one > gets chosen in that case? I don't know; this was moved verbatim from the other test. I could change the comment but keep the asserts as is, or change the asserts to merely expect that the best_source has values, not what they are. > nit: Comments should end with a ".", also below. Done. https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_service_unittest.cc (right): https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service_unittest.cc:72: "https://chromecontentsuggestions-pa.googleapis.com/v1/suggestions/" On 2016/08/30 06:57:21, tschumann wrote: > reading this test, you might think that we're actually fetching data from > service (which we hopefully don't as we are using a fake fetcher). > What requirements to the to-be-fake-fetched URLs do we need to meet? > Would be nice if we could have a string that makes it very clear that this is > not a real URL to be fetched, like "fake-url-to-suggestions-api" Done. https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service_unittest.cc:113: std::string GetSnippetWithUrlAndTimesAndSources( On 2016/08/30 09:29:29, Marc Treib wrote: > "Sources" isn't accurate anymore, there's only one. Done. https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service_unittest.cc:120: std::string ids_string; On 2016/08/30 06:57:21, tschumann wrote: > ids_string = base::JoinStrings(ids, "\",\n \""); Done. https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service_unittest.cc:128: "{\n" On 2016/08/30 09:29:29, Marc Treib wrote: > nit: indentation? Do you mean in the JSON? It's indented so that it will look right when it's slotted into GetTestJson() above. https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service_unittest.cc:150: {kSnippetUrl}, source_urls[0], GetDefaultCreationTime(), On 2016/08/30 09:29:29, Marc Treib wrote: > This seems wrong - why pass in a vector of source_urls but only use one? Not sure. I think this was used for the tests that were moved to NTPSnippetTest, but the whole multiple-source thing doesn't exist here anymore now that we use the new-style API. It's only ever called with one source anyway, so I've changed that. https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service_unittest.cc:264: suggestions_[category].insert(suggestions_[category].begin(), On 2016/08/30 09:29:29, Marc Treib wrote: > Shouldn't you clear suggestions_[category_] first? That's what you would expect > of a ProviderObserver - OnNewSuggestions replaces any that were there before. Oh, apparently. Done. https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service_unittest.cc:332: google_apis::GetAPIKey().c_str())), On 2016/08/30 09:29:29, Marc Treib wrote: > I don't think the actual API key matters here? We can probably just hardcode > some arbitrary value into our URL constant? We're using a real fetcher, which will use a real API key. Actually, we probably *should* inject the API key into the fetcher, instead of is_stable_channel, but I think that change is separate from this one. https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service_unittest.cc:437: MockScheduler scheduler_; On 2016/08/30 06:57:21, tschumann wrote: > consider making this a ::testing::NiceMock<MockScheduler> > The nice thing is that if you don't define an expectation for a function call, > it won't trigger a warning. So test cases which don't care about whether or not > the scheduler get's called don't need to care about the EXPECT_CALL() > statements. But if you add them, then they get verified like with normal (or > StrictMock<>s). I see your NiceMock<> and raise you a StrictMock<>. https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service_unittest.cc:466: const ContentSuggestion& snippet = On 2016/08/30 09:29:29, Marc Treib wrote: > Can we rename this to |suggestion| please? Done. https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service_unittest.cc:472: // EXPECT_EQ(GURL(kSnippetSalientImage), snippet.salient_image_url()); On 2016/08/30 06:57:21, tschumann wrote: > why is this commented out? Right now ContentSuggestion::salient_image_url_ has no accessor or mutator (?). https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service_unittest.cc:623: EXPECT_CALL(callback, MockRun(_, _)) On 2016/08/30 06:57:21, tschumann wrote: > not now, but this mock usage also has a pretty bad smell. > The problem is that we are not setting up a proper expectation of the call > itself but verify the parameters as part of the action. Parameters should be > verified with parameter matchers. > > So either, we follow this pattern, but then I'd rather have a free function that > simply captures the state (as done with CaptureDismissedSuggestions in > https://codereview.chromium.org/2291593002/). > > Or we provide proper matchers to verify the parameter. In this case (and because > of the trickiness with mocking move-only parameters), I'd be in favor of just > binding a normal function that captures the state to be verified after the call. > > But like I said, this can totally happen in another CL. This is actually doable just with a lambda, which reads better in all ways. https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service_unittest.cc:668: ASSERT_THAT(service->GetSnippetsForTesting(), SizeIs(0)); On 2016/08/30 09:29:29, Marc Treib wrote: > Why has the behavior changed here? > Also, this should be EXPECT rather than ASSERT. NTPSnippet::CreateFromContentSuggestionsDictionary() is stricter about this. Before, we'd succeed at creating the snippet, but it would be !is_complete(). https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service_unittest.cc:703: {GetSnippetWithSources({"aaaa"}, "Source 1", "http://source1.amp.com")})); On 2016/08/30 09:29:29, Marc Treib wrote: > nit: s/aaaa/not a url/ ? Done (for locale fr) https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service_unittest.cc:773: // articles. However, recreating the service does trigger one fetch. On 2016/08/30 09:29:29, Marc Treib wrote: > Why? If we get suggestions from the DB, we shouldn't fetch again. We don't get suggestions from the DB. There's one snippet there, but it's dismissed. I've fixed this up to test what you thought it did :) https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service_unittest.cc:791: const std::vector<std::string> publishers = {"Mashable", "AOL", "Mashable"}; On 2016/08/30 09:29:29, Marc Treib wrote: > The third entry is never used. Removed. https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service_unittest.cc:792: const std::vector<std::string> amp_urls = { On 2016/08/30 09:29:29, Marc Treib wrote: > Only amp_urls[1] is ever used. Removed third entry. https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service_unittest.cc:801: publishers[0], amp_urls[1])})); On 2016/08/30 09:29:29, Marc Treib wrote: > Should this be amp_urls[0]? I suppose so. Done. https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service_unittest.cc:855: .WillOnce(testing::WithArgs<0, 2>(Invoke(ServeOneByOneImage))); On 2016/08/30 09:29:29, Marc Treib wrote: > optional: You could add "using testing::..." statements, there's already a bunch > of them. Done (for several testing::)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Exceeded global retry quota
The CQ bit was checked by sfiera@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_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
LGTM with a few more nits/suggestions https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippet_unittest.cc (right): https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippet_unittest.cc:103: // Expect the first source to be chosen On 2016/08/30 13:16:43, sfiera wrote: > On 2016/08/30 09:29:29, Marc Treib wrote: > > Why? AFAICT, they're equivalent. Do we really want to test that the first one > > gets chosen in that case? > > I don't know; this was moved verbatim from the other test. I could change the > comment but keep the asserts as is, or change the asserts to merely expect that > the best_source has values, not what they are. Hm, so if we can't figure out why we'd expect this exact behavior, then my preference would be not testing for it. If you're trying to move on with things, just adding a comment (TODO) is fine. > > nit: Comments should end with a ".", also below. > > Done. https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_service_unittest.cc (right): https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service_unittest.cc:332: google_apis::GetAPIKey().c_str())), On 2016/08/30 13:16:43, sfiera wrote: > On 2016/08/30 09:29:29, Marc Treib wrote: > > I don't think the actual API key matters here? We can probably just hardcode > > some arbitrary value into our URL constant? > > We're using a real fetcher, which will use a real API key. > > Actually, we probably *should* inject the API key into the fetcher, instead of > is_stable_channel, but I think that change is separate from this one. Aah, I remember now. Carry on then :) https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service_unittest.cc:358: utils_.ResetSigninManager(); On 2016/08/30 09:29:29, Marc Treib wrote: > So if you call MakeSnippetsService twice in a row, the first service will be in > a bad state, right? Can we somehow ensure that that doesn't happen? Not addressed yet. https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service_unittest.cc:472: // EXPECT_EQ(GURL(kSnippetSalientImage), snippet.salient_image_url()); On 2016/08/30 13:16:43, sfiera wrote: > On 2016/08/30 06:57:21, tschumann wrote: > > why is this commented out? > > Right now ContentSuggestion::salient_image_url_ has no accessor or mutator (?). Because it shouldn't be there :) Mind removing it? https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service_unittest.cc:773: // articles. However, recreating the service does trigger one fetch. On 2016/08/30 13:16:44, sfiera wrote: > On 2016/08/30 09:29:29, Marc Treib wrote: > > Why? If we get suggestions from the DB, we shouldn't fetch again. > > We don't get suggestions from the DB. There's one snippet there, but it's > dismissed. > > I've fixed this up to test what you thought it did :) Yay! Thanks! https://codereview.chromium.org/2285133004/diff/80001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service_unittest.cc (right): https://codereview.chromium.org/2285133004/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service_unittest.cc:644: EXPECT_THAT(service->GetSnippetsForTesting(), SizeIs(0)); nit: IsEmpty() rather than SizeIs(0) https://codereview.chromium.org/2285133004/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service_unittest.cc:697: EXPECT_CALL(mock_scheduler(), Schedule(_, _, _, _)).Times(3); ? I think this is evidence in favor of Tim's argument for a NiceMock: I don't think this test particularly cares if/how often Schedule gets called. https://codereview.chromium.org/2285133004/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service_unittest.cc:803: ASSERT_THAT(service->GetSnippetsForTesting(), IsEmpty()); This should be EXPECT_THAT
lgtm https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippet_unittest.cc (right): https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippet_unittest.cc:19: if (!json_value->GetAsDictionary(&json_dict)) { On 2016/08/30 13:16:43, sfiera wrote: > On 2016/08/30 09:29:28, Marc Treib wrote: > > On 2016/08/30 06:57:21, tschumann wrote: > > > as you don't expect this to ever fail, you can also just > > CHECK(json_value->...); > > > > > > [IMO, it's fine to CHECK-fail in case of bugs inside the test code itself.] > > > > I find it a bit nicer to ASSERT_*, so the test fails "properly" rather than > > crashing. > > My feeling is that ASSERTing is better but often not worth the effort. Here, > since I've already made the effort, I'd keep it. Ok. My motivation is that the test verifies the system under test and uses EXPECT and ASSERT to accomplish that. But to verify behavior of the test itself -- invariants -- IMO check-failing is fine as it a)should never happen (-->less branches to follow + simpler semantics) and b) is easy to spot. That aside, can you actually ASSERT within a non-void function? ASSERT (at least in google3) has the really annoying property of including a return statement which completely screws things up if not called within the TEST(){} block. maybe we should discuss this in a larger context -- not necessarily in this CL. https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_service_unittest.cc (right): https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service_unittest.cc:193: if (pos == std::string::npos) { On 2016/08/30 06:57:21, tschumann wrote: > couldn't we just CHECK() then? In this particular case, CHECK() would be cleaner though as your program would crash 2 lines later anyways, no? (plus you don't have to deal with returning and empty string or the like). https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service_unittest.cc:332: google_apis::GetAPIKey().c_str())), On 2016/08/30 13:43:38, Marc Treib wrote: > On 2016/08/30 13:16:43, sfiera wrote: > > On 2016/08/30 09:29:29, Marc Treib wrote: > > > I don't think the actual API key matters here? We can probably just hardcode > > > some arbitrary value into our URL constant? > > > > We're using a real fetcher, which will use a real API key. > > > > Actually, we probably *should* inject the API key into the fetcher, instead of > > is_stable_channel, but I think that change is separate from this one. > > Aah, I remember now. Carry on then :) but please tell me we're not making an actual fetch from some system requiring the key, right? https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service_unittest.cc:437: MockScheduler scheduler_; On 2016/08/30 13:16:44, sfiera wrote: > On 2016/08/30 06:57:21, tschumann wrote: > > consider making this a ::testing::NiceMock<MockScheduler> > > The nice thing is that if you don't define an expectation for a function call, > > it won't trigger a warning. So test cases which don't care about whether or > not > > the scheduler get's called don't need to care about the EXPECT_CALL() > > statements. But if you add them, then they get verified like with normal (or > > StrictMock<>s). > > I see your NiceMock<> and raise you a StrictMock<>. but please make sure the test doesn't get too brittle with that. It's easy to end-up with noise and copy-past expectations all over the place. https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service_unittest.cc:472: // EXPECT_EQ(GURL(kSnippetSalientImage), snippet.salient_image_url()); On 2016/08/30 13:43:39, Marc Treib wrote: > On 2016/08/30 13:16:43, sfiera wrote: > > On 2016/08/30 06:57:21, tschumann wrote: > > > why is this commented out? > > > > Right now ContentSuggestion::salient_image_url_ has no accessor or mutator > (?). > > Because it shouldn't be there :) Mind removing it? +1 for removing it. Commented out code tends to rot. It's easy to write once it's there. https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service_unittest.cc:623: EXPECT_CALL(callback, MockRun(_, _)) On 2016/08/30 13:16:43, sfiera wrote: > On 2016/08/30 06:57:21, tschumann wrote: > > not now, but this mock usage also has a pretty bad smell. > > The problem is that we are not setting up a proper expectation of the call > > itself but verify the parameters as part of the action. Parameters should be > > verified with parameter matchers. > > > > So either, we follow this pattern, but then I'd rather have a free function > that > > simply captures the state (as done with CaptureDismissedSuggestions in > > https://codereview.chromium.org/2291593002/). > > > > Or we provide proper matchers to verify the parameter. In this case (and > because > > of the trickiness with mocking move-only parameters), I'd be in favor of just > > binding a normal function that captures the state to be verified after the > call. > > > > But like I said, this can totally happen in another CL. > > This is actually doable just with a lambda, which reads better in all ways. Actually, it would be even nice as a standalone function like ExpectDismissedSuggestions() to avoid duplication and easier to read code (lambdas at this length start cluttering the code and as you can't give them a name, tend to have hard-to-guess semantics). Still okay though.
lgtm
https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_service_unittest.cc (right): https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service_unittest.cc:472: // EXPECT_EQ(GURL(kSnippetSalientImage), snippet.salient_image_url()); On 2016/08/30 14:01:39, tschumann wrote: > On 2016/08/30 13:43:39, Marc Treib wrote: > > On 2016/08/30 13:16:43, sfiera wrote: > > > On 2016/08/30 06:57:21, tschumann wrote: > > > > why is this commented out? > > > > > > Right now ContentSuggestion::salient_image_url_ has no accessor or mutator > > (?). > > > > Because it shouldn't be there :) Mind removing it? > > +1 for removing it. Commented out code tends to rot. It's easy to write once > it's there. I mostly meant removing that member, which is completely unused. The commented-out test code should also go of course :)
Two outstanding action items which will I'll handle separately: * Remove image from ContentSuggestion * Inject API key into fetcher https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippet_unittest.cc (right): https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippet_unittest.cc:103: // Expect the first source to be chosen On 2016/08/30 13:43:38, Marc Treib wrote: > On 2016/08/30 13:16:43, sfiera wrote: > > On 2016/08/30 09:29:29, Marc Treib wrote: > > > Why? AFAICT, they're equivalent. Do we really want to test that the first > one > > > gets chosen in that case? > > > > I don't know; this was moved verbatim from the other test. I could change the > > comment but keep the asserts as is, or change the asserts to merely expect > that > > the best_source has values, not what they are. > > Hm, so if we can't figure out why we'd expect this exact behavior, then my > preference would be not testing for it. If you're trying to move on with things, > just adding a comment (TODO) is fine. I've loosened the tests; there are things we care about, even if not the specific rule about the first source being chosen. https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_service_unittest.cc (right): https://codereview.chromium.org/2285133004/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service_unittest.cc:358: utils_.ResetSigninManager(); On 2016/08/30 13:43:39, Marc Treib wrote: > On 2016/08/30 09:29:29, Marc Treib wrote: > > So if you call MakeSnippetsService twice in a row, the first service will be > in > > a bad state, right? Can we somehow ensure that that doesn't happen? > > Not addressed yet. Checked !observer.Loaded() above (which is reset first when resetting the service). https://codereview.chromium.org/2285133004/diff/80001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service_unittest.cc (right): https://codereview.chromium.org/2285133004/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service_unittest.cc:644: EXPECT_THAT(service->GetSnippetsForTesting(), SizeIs(0)); On 2016/08/30 13:43:39, Marc Treib wrote: > nit: IsEmpty() rather than SizeIs(0) Done. https://codereview.chromium.org/2285133004/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service_unittest.cc:697: EXPECT_CALL(mock_scheduler(), Schedule(_, _, _, _)).Times(3); On 2016/08/30 13:43:39, Marc Treib wrote: > ? > > I think this is evidence in favor of Tim's argument for a NiceMock: I don't > think this test particularly cares if/how often Schedule gets called. Done. https://codereview.chromium.org/2285133004/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service_unittest.cc:803: ASSERT_THAT(service->GetSnippetsForTesting(), IsEmpty()); On 2016/08/30 13:43:39, Marc Treib wrote: > This should be EXPECT_THAT Done.
The CQ bit was checked by sfiera@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...
LGTM++, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sfiera@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tschumann@chromium.org, treib@chromium.org Link to the patchset: https://codereview.chromium.org/2285133004/#ps110001 (title: "No brackets.")
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 #6 (id:110001)
Message was sent while issue was closed.
Description was changed from
==========
Overhaul ntp_snippets_service_unittest.cc.
A partial list of things done here:
* Move tests that verified NTPSnippet::best_source() behavior into
ntp_snippet_test.cc; they don't belong here.
* Switch it to use the chromecontentsuggestions-pa. Testing new
features like server-provided categories is not possible without
this.
* Replace MockContentSuggestionsProviderObserver with a fake. It
shouldn't have been a mock, since we weren't using it to test
behavior, only state.
* In one case, test using what the observer sees, instead of peeking
in through debugging hooks.
* Get rid of special test base class and Mock::VerifyAndClear calls; I
consider these anti-patterns.
* Initialize the service better; give it an empty list of snippets for
its first fetch, and wait for it to complete the fetch before
returning (I think this is what was most strongly implicated by the
failures with the other CL).
* Create the service and expect the scheduling directly from each
case, so they are more readable individually.
==========
to
==========
Overhaul ntp_snippets_service_unittest.cc.
A partial list of things done here:
* Move tests that verified NTPSnippet::best_source() behavior into
ntp_snippet_test.cc; they don't belong here.
* Switch it to use the chromecontentsuggestions-pa. Testing new
features like server-provided categories is not possible without
this.
* Replace MockContentSuggestionsProviderObserver with a fake. It
shouldn't have been a mock, since we weren't using it to test
behavior, only state.
* In one case, test using what the observer sees, instead of peeking
in through debugging hooks.
* Get rid of special test base class and Mock::VerifyAndClear calls; I
consider these anti-patterns.
* Initialize the service better; give it an empty list of snippets for
its first fetch, and wait for it to complete the fetch before
returning (I think this is what was most strongly implicated by the
failures with the other CL).
* Create the service and expect the scheduling directly from each
case, so they are more readable individually.
Committed: https://crrev.com/e88d16809c5f7a0d5e8d59caa451dc4359fa7b06
Cr-Commit-Position: refs/heads/master@{#415358}
==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/e88d16809c5f7a0d5e8d59caa451dc4359fa7b06 Cr-Commit-Position: refs/heads/master@{#415358} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
