Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1087883002:
Clean up URLFetcher unit tests, part 5. (Closed)
DescriptionClean up URLFetcher unit tests, part 5.
Convert URLFetcherFile tests to use a shared test fixture.
BUG=471069
Committed: https://crrev.com/8eeb3bf4a02be612cacb3cdbb5c223d909939527
Cr-Commit-Position: refs/heads/master@{#325291}
Patch Set 1 #Patch Set 2 : Update comments, remove test fixture #Patch Set 3 : Merge #Patch Set 4 : Fix merge #
Total comments: 5
Patch Set 5 : Reponse to comments, remove unused variable #Patch Set 6 : Fix comment #
Total comments: 2
Patch Set 7 : Response to comments, fix tests #Messages
Total messages: 21 (8 generated)
Patchset #2 (id:20001) has been deleted
mmenke@chromium.org changed reviewers: + davidben@chromium.org
By the way, David: There's absolutely no rush on these. I'm deliberately doling them out slowly. Don't feel you have to review anything at 8:00 at night (Though I know you normally work fairly late, anyways).
https://codereview.chromium.org/1087883002/diff/80001/net/url_request/url_fet... File net/url_request/url_fetcher_impl_unittest.cc (right): https://codereview.chromium.org/1087883002/diff/80001/net/url_request/url_fet... net/url_request/url_fetcher_impl_unittest.cc:246: void SaveTempFileTest(const char* file_to_fetch, bool take_ownership) { These two functions are almost completely identical. Do you think it'd be better to have a shared helper. Sort of weird to pass in an optional FilePath, but you could do something like... SaveFileTestImpl(const char* file_to_fetch, bool use_temporary, const base::FilePath& out_path, // ignored if use_temporary bool take_ownership); https://codereview.chromium.org/1087883002/diff/80001/net/url_request/url_fet... net/url_request/url_fetcher_impl_unittest.cc:1268: SaveFileTest(kFileToFetch, out_path, true); Not from this CL, but take_ownership = false + existing file seems a very weird combination. Seems a better API would have been for take_ownership to require SaveResponseToTemporaryFile rather than SaveResponseToFilePathAtPath.
https://codereview.chromium.org/1087883002/diff/80001/net/url_request/url_fet... File net/url_request/url_fetcher_impl_unittest.cc (right): https://codereview.chromium.org/1087883002/diff/80001/net/url_request/url_fet... net/url_request/url_fetcher_impl_unittest.cc:246: void SaveTempFileTest(const char* file_to_fetch, bool take_ownership) { On 2015/04/15 18:21:10, David Benjamin wrote: > These two functions are almost completely identical. Do you think it'd be better > to have a shared helper. Sort of weird to pass in an optional FilePath, but you > could do something like... > > SaveFileTestImpl(const char* file_to_fetch, > bool use_temporary, > const base::FilePath& out_path, // ignored if use_temporary > bool take_ownership); Done. Considered this, but was thinking they were a little more different than they actually were. https://codereview.chromium.org/1087883002/diff/80001/net/url_request/url_fet... net/url_request/url_fetcher_impl_unittest.cc:1268: SaveFileTest(kFileToFetch, out_path, true); On 2015/04/15 18:21:10, David Benjamin wrote: > Not from this CL, but take_ownership = false + existing file seems a very weird > combination. Seems a better API would have been for take_ownership to require > SaveResponseToTemporaryFile rather than SaveResponseToFilePathAtPath. Yea, it does seem weird. Not sure it's worth fixing, though.
lgtm https://codereview.chromium.org/1087883002/diff/80001/net/url_request/url_fet... File net/url_request/url_fetcher_impl_unittest.cc (right): https://codereview.chromium.org/1087883002/diff/80001/net/url_request/url_fet... net/url_request/url_fetcher_impl_unittest.cc:1268: SaveFileTest(kFileToFetch, out_path, true); On 2015/04/15 18:43:41, mmenke wrote: > On 2015/04/15 18:21:10, David Benjamin wrote: > > Not from this CL, but take_ownership = false + existing file seems a very > weird > > combination. Seems a better API would have been for take_ownership to require > > SaveResponseToTemporaryFile rather than SaveResponseToFilePathAtPath. > > Yea, it does seem weird. Not sure it's worth fixing, though. Yeah, certainly not in this CL. https://codereview.chromium.org/1087883002/diff/120001/net/url_request/url_fe... File net/url_request/url_fetcher_impl_unittest.cc (right): https://codereview.chromium.org/1087883002/diff/120001/net/url_request/url_fe... net/url_request/url_fetcher_impl_unittest.cc:218: if (!save_to_temporary_file) { Nit: May as well swap the order of these so you can save a !.
I also flipped my trues/falses in the tests. Fixed now. Trybots would have caught it, anyways, for the non-temp file tests. https://codereview.chromium.org/1087883002/diff/120001/net/url_request/url_fe... File net/url_request/url_fetcher_impl_unittest.cc (right): https://codereview.chromium.org/1087883002/diff/120001/net/url_request/url_fe... net/url_request/url_fetcher_impl_unittest.cc:218: if (!save_to_temporary_file) { On 2015/04/15 18:49:28, David Benjamin wrote: > Nit: May as well swap the order of these so you can save a !. Done. Don't care so much about the !, but it matches the comment order.
The CQ bit was checked by mmenke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from davidben@chromium.org Link to the patchset: https://codereview.chromium.org/1087883002/#ps140001 (title: "Response to comments, fix tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1087883002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by mmenke@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1087883002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by mmenke@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1087883002/140001
Message was sent while issue was closed.
Committed patchset #7 (id:140001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/8eeb3bf4a02be612cacb3cdbb5c223d909939527 Cr-Commit-Position: refs/heads/master@{#325291} |
