Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2765793004:
[Doodle] Cleanups in DoodleFetcherImplTest (Closed)
|
Created:
3 years, 9 months ago by Marc Treib Modified:
3 years, 9 months ago Reviewers:
fhorschig CC:
chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[Doodle] Cleanups in DoodleFetcherImplTest
BUG=690467
Review-Url: https://codereview.chromium.org/2765793004
Cr-Commit-Position: refs/heads/master@{#459028}
Committed: https://chromium.googlesource.com/chromium/src/+/9a17ccc9dee66e65bef17be5a0d6fe6c5ee79767
Patch Set 1 #
Total comments: 13
Patch Set 2 : review #Messages
Total messages: 16 (8 generated)
treib@chromium.org changed reviewers: + fhorschig@chromium.org
Not ready for submission, but some questions below. PTAL! https://codereview.chromium.org/2765793004/diff/1/components/doodle/doodle_fe... File components/doodle/doodle_fetcher_impl_unittest.cc (right): https://codereview.chromium.org/2765793004/diff/1/components/doodle/doodle_fe... components/doodle/doodle_fetcher_impl_unittest.cc:55: GURL Resolve(const std::string& relative_url) { Should this be a member of the test class and use GetGoogleBaseURL? https://codereview.chromium.org/2765793004/diff/1/components/doodle/doodle_fe... components/doodle/doodle_fetcher_impl_unittest.cc:108: // TODO(treib): Replace with a MockCallback plus testing::SaveArgs? I'm not quite sure about this one. I've converted one test to the MockCallback/SaveArg style as an example, see below. I think that's a bit more explicit/less magic. WDYT? If you agree, I'll convert the rest too. https://codereview.chromium.org/2765793004/diff/1/components/doodle/doodle_fe... components/doodle/doodle_fetcher_impl_unittest.cc:416: Eq(GetGoogleBaseURL().Resolve(kDoodleConfigPath))); This doesn't really test anything, does it? The Google base URL has the default value anyway.
Good ideas! https://codereview.chromium.org/2765793004/diff/1/components/doodle/doodle_fe... File components/doodle/doodle_fetcher_impl_unittest.cc (right): https://codereview.chromium.org/2765793004/diff/1/components/doodle/doodle_fe... components/doodle/doodle_fetcher_impl_unittest.cc:55: GURL Resolve(const std::string& relative_url) { On 2017/03/22 13:44:07, Marc Treib wrote: > Should this be a member of the test class and use GetGoogleBaseURL? Sounds reasonable. https://codereview.chromium.org/2765793004/diff/1/components/doodle/doodle_fe... components/doodle/doodle_fetcher_impl_unittest.cc:108: // TODO(treib): Replace with a MockCallback plus testing::SaveArgs? On 2017/03/22 13:44:07, Marc Treib wrote: > I'm not quite sure about this one. I've converted one test to the > MockCallback/SaveArg style as an example, see below. I think that's a bit more > explicit/less magic. WDYT? If you agree, I'll convert the rest too. Less magic is relative but it's certainly easier to debug. So I would be in favor of the SaveArg-style. https://codereview.chromium.org/2765793004/diff/1/components/doodle/doodle_fe... components/doodle/doodle_fetcher_impl_unittest.cc:148: EXPECT_CALL(callback, Run(_, _, _)) It's somewhat confusing to see the expectation after the call, but of course it makes sense given that the callback isn't triggered until the server responds .... https://codereview.chromium.org/2765793004/diff/1/components/doodle/doodle_fe... components/doodle/doodle_fetcher_impl_unittest.cc:416: Eq(GetGoogleBaseURL().Resolve(kDoodleConfigPath))); On 2017/03/22 13:44:07, Marc Treib wrote: > This doesn't really test anything, does it? The Google base URL has the default > value anyway. Yes. Meanwhile, this case is covered by pretty much every other test.
The CQ bit was checked by treib@chromium.org to run a CQ dry run
PTAL again! https://codereview.chromium.org/2765793004/diff/1/components/doodle/doodle_fe... File components/doodle/doodle_fetcher_impl_unittest.cc (right): https://codereview.chromium.org/2765793004/diff/1/components/doodle/doodle_fe... components/doodle/doodle_fetcher_impl_unittest.cc:55: GURL Resolve(const std::string& relative_url) { On 2017/03/22 14:27:41, fhorschig wrote: > On 2017/03/22 13:44:07, Marc Treib wrote: > > Should this be a member of the test class and use GetGoogleBaseURL? > > Sounds reasonable. Done. https://codereview.chromium.org/2765793004/diff/1/components/doodle/doodle_fe... components/doodle/doodle_fetcher_impl_unittest.cc:108: // TODO(treib): Replace with a MockCallback plus testing::SaveArgs? On 2017/03/22 14:27:41, fhorschig wrote: > On 2017/03/22 13:44:07, Marc Treib wrote: > > I'm not quite sure about this one. I've converted one test to the > > MockCallback/SaveArg style as an example, see below. I think that's a bit more > > explicit/less magic. WDYT? If you agree, I'll convert the rest too. > > Less magic is relative but it's certainly easier to debug. > So I would be in favor of the SaveArg-style. Done. https://codereview.chromium.org/2765793004/diff/1/components/doodle/doodle_fe... components/doodle/doodle_fetcher_impl_unittest.cc:148: EXPECT_CALL(callback, Run(_, _, _)) On 2017/03/22 14:27:41, fhorschig wrote: > It's somewhat confusing to see the expectation after the call, but of > course it makes sense given that the callback isn't triggered until the > server responds .... I read this as: I expect the call when I send the response. But if you prefer, I can move the expectation up. https://codereview.chromium.org/2765793004/diff/1/components/doodle/doodle_fe... components/doodle/doodle_fetcher_impl_unittest.cc:416: Eq(GetGoogleBaseURL().Resolve(kDoodleConfigPath))); On 2017/03/22 14:27:41, fhorschig wrote: > On 2017/03/22 13:44:07, Marc Treib wrote: > > This doesn't really test anything, does it? The Google base URL has the > default > > value anyway. > > Yes. Meanwhile, this case is covered by pretty much every other test. GoogleBaseURLTracker doesn't seem to have an easy way to override the URL, unfortunately :-/ I've added a TODO for now.
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: This issue passed the CQ dry run.
https://codereview.chromium.org/2765793004/diff/1/components/doodle/doodle_fe... File components/doodle/doodle_fetcher_impl_unittest.cc (right): https://codereview.chromium.org/2765793004/diff/1/components/doodle/doodle_fe... components/doodle/doodle_fetcher_impl_unittest.cc:148: EXPECT_CALL(callback, Run(_, _, _)) On 2017/03/22 15:35:37, Marc Treib wrote: > On 2017/03/22 14:27:41, fhorschig wrote: > > It's somewhat confusing to see the expectation after the call, but of > > course it makes sense given that the callback isn't triggered until the > > server responds .... > > I read this as: I expect the call when I send the response. But if you prefer, I > can move the expectation up. No, don't move it up. It also makes sense that the expectation can never be triggered before we send the response. It's just unusual because we are usually forced to have expectations earlier. I would prefer it to always be near the place where the call happens. Like here :D https://codereview.chromium.org/2765793004/diff/1/components/doodle/doodle_fe... components/doodle/doodle_fetcher_impl_unittest.cc:416: Eq(GetGoogleBaseURL().Resolve(kDoodleConfigPath))); On 2017/03/22 15:35:37, Marc Treib wrote: > On 2017/03/22 14:27:41, fhorschig wrote: > > On 2017/03/22 13:44:07, Marc Treib wrote: > > > This doesn't really test anything, does it? The Google base URL has the > > default > > > value anyway. > > > > Yes. Meanwhile, this case is covered by pretty much every other test. > > GoogleBaseURLTracker doesn't seem to have an easy way to override the URL, > unfortunately :-/ I've added a TODO for now. sgtm (I can remotely remember to have stumbled upon the same issue.)
Sooo... is this good to submit then? (I can haz lgtm?)
On 2017/03/23 08:33:45, Marc Treib wrote: > Sooo... is this good to submit then? (I can haz lgtm?) sure, lgtm!
The CQ bit was checked by treib@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1490259167386140,
"parent_rev": "ebc65fb85f272e802722a53ea28c0714a5ab7e30", "commit_rev":
"9a17ccc9dee66e65bef17be5a0d6fe6c5ee79767"}
Message was sent while issue was closed.
Description was changed from ========== [Doodle] Cleanups in DoodleFetcherImplTest BUG=690467 ========== to ========== [Doodle] Cleanups in DoodleFetcherImplTest BUG=690467 Review-Url: https://codereview.chromium.org/2765793004 Cr-Commit-Position: refs/heads/master@{#459028} Committed: https://chromium.googlesource.com/chromium/src/+/9a17ccc9dee66e65bef17be5a0d6... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/9a17ccc9dee66e65bef17be5a0d6... |
