Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2178723002:
Add more unit tests for RequestPicker (Closed)
|
Created:
4 years, 5 months ago by Pete Williamson Modified:
4 years, 4 months ago CC:
chromium-reviews, romax+watch_chromium.org, fgorski+watch_chromium.org, dewittj+watch_chromium.org, petewil+watch_chromium.org, chili+watch_chromium.org, dimich+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd more unit tests for RequestPicker
BUG=610521
Committed: https://crrev.com/e9e18f8b3f0d9214dd50ca89a8d47cda5078c2b9
Cr-Commit-Position: refs/heads/master@{#407901}
Patch Set 1 #
Total comments: 15
Patch Set 2 : CR feedback per FGorski and DougArnett #
Total comments: 5
Patch Set 3 : Rename helper function, improve comment #
Messages
Total messages: 18 (8 generated)
petewil@chromium.org changed reviewers: + dougarnett@chromium.org, fgorski@chromium.org
The CQ bit was checked by petewil@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: This issue passed the CQ dry run.
https://codereview.chromium.org/2178723002/diff/1/components/offline_pages/ba... File components/offline_pages/background/offliner_policy.h (right): https://codereview.chromium.org/2178723002/diff/1/components/offline_pages/ba... components/offline_pages/background/offliner_policy.h:22: : prefer_untried_requests_(false), please put that into the .cc file. https://codereview.chromium.org/2178723002/diff/1/components/offline_pages/ba... components/offline_pages/background/offliner_policy.h:26: OfflinerPolicy(bool prefer_untried, bool prefer_earlier, same here + git cl format. https://codereview.chromium.org/2178723002/diff/1/components/offline_pages/ba... components/offline_pages/background/offliner_policy.h:38: bool ShouldPreferUntriedRequests() { return prefer_untried_requests_; } Can this method be marked const? Same for the 2 that follow. https://codereview.chromium.org/2178723002/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_picker_unittest.cc (right): https://codereview.chromium.org/2178723002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_picker_unittest.cc:33: const bool kPreferUntried = false; nit: Did you intentionally set this to false? The name slightly indicates that the value of const is true, as in "Yes, prefer untried". Also this set of constants corresponds to the default settings of the policy, is that correct? Is that what it is meant to do? https://codereview.chromium.org/2178723002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_picker_unittest.cc:121: request2.set_attempt_count(kRetryCount2); nit: consider renaming kRetryCount2 to kAttemptCount.
https://codereview.chromium.org/2178723002/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_picker_unittest.cc (right): https://codereview.chromium.org/2178723002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_picker_unittest.cc:154: // Put some test requests on the Queue. "Put some" => "Add ... to ..." here and other cases below https://codereview.chromium.org/2178723002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_picker_unittest.cc:193: queue_->AddRequest(request1, base::Bind(&RequestPickerTest::AddRequestDone, This pattern of adding requests, pump loop, choose next, pump loop looks identical in many tests here so might help reduce clutter to factor out into method that takes the requests to add.
https://codereview.chromium.org/2178723002/diff/1/components/offline_pages/ba... File components/offline_pages/background/offliner_policy.h (right): https://codereview.chromium.org/2178723002/diff/1/components/offline_pages/ba... components/offline_pages/background/offliner_policy.h:22: : prefer_untried_requests_(false), On 2016/07/25 16:22:57, fgorski wrote: > please put that into the .cc file. There is no .cc file yet. It will be coming in a later update, to help us get values from Finch. Is it OK to wait until then before creating the .cc file? https://codereview.chromium.org/2178723002/diff/1/components/offline_pages/ba... components/offline_pages/background/offliner_policy.h:26: OfflinerPolicy(bool prefer_untried, bool prefer_earlier, On 2016/07/25 16:22:57, fgorski wrote: > same here + git cl format. Format done https://codereview.chromium.org/2178723002/diff/1/components/offline_pages/ba... components/offline_pages/background/offliner_policy.h:38: bool ShouldPreferUntriedRequests() { return prefer_untried_requests_; } On 2016/07/25 16:22:57, fgorski wrote: > Can this method be marked const? > Same for the 2 that follow. Done. https://codereview.chromium.org/2178723002/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_picker_unittest.cc (right): https://codereview.chromium.org/2178723002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_picker_unittest.cc:33: const bool kPreferUntried = false; On 2016/07/25 16:22:57, fgorski wrote: > nit: Did you intentionally set this to false? The name slightly indicates that > the value of const is true, as in "Yes, prefer untried". > > Also this set of constants corresponds to the default settings of the policy, is > that correct? Is that what it is meant to do? Your interpretation is correct, so setting kPreferUntried to false means that we prefer requests that have already been tried (but not exceeded the retry count) to untried requests. Yes, these values correspond to the defaults. When using them below, I use them with a "!" for places we are doing the opposite of the default. My code should have been clear enough that you didn't have to ask, so I beefed up the comments, see if it is clearer now. https://codereview.chromium.org/2178723002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_picker_unittest.cc:121: request2.set_attempt_count(kRetryCount2); On 2016/07/25 16:22:57, fgorski wrote: > nit: consider renaming kRetryCount2 to kAttemptCount. Done. https://codereview.chromium.org/2178723002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_picker_unittest.cc:154: // Put some test requests on the Queue. On 2016/07/25 18:32:10, dougarnett wrote: > "Put some" => "Add ... to ..." here and other cases below Done. https://codereview.chromium.org/2178723002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_picker_unittest.cc:193: queue_->AddRequest(request1, base::Bind(&RequestPickerTest::AddRequestDone, On 2016/07/25 18:32:10, dougarnett wrote: > This pattern of adding requests, pump loop, choose next, pump loop looks > identical in many tests here so might help reduce clutter to factor out into > method that takes the requests to add. Done.
LGTM with couple suggestions https://codereview.chromium.org/2178723002/diff/20001/components/offline_page... File components/offline_pages/background/request_picker_unittest.cc (right): https://codereview.chromium.org/2178723002/diff/20001/components/offline_page... components/offline_pages/background/request_picker_unittest.cc:102: // Test helper method do so the boilerplate operations for most tests. More descriptive of "what" it does rather than "why" might be better. // Test helper to queue the two given requests and then pick one of them per configured policy. https://codereview.chromium.org/2178723002/diff/20001/components/offline_page... components/offline_pages/background/request_picker_unittest.cc:103: void RequestPickerTest::AddRequestsAndChooseOne( QueueAndChoose()? but not sure
lgtm https://codereview.chromium.org/2178723002/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_picker_unittest.cc (right): https://codereview.chromium.org/2178723002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_picker_unittest.cc:33: const bool kPreferUntried = false; On 2016/07/25 19:54:21, Pete Williamson wrote: > On 2016/07/25 16:22:57, fgorski wrote: > > nit: Did you intentionally set this to false? The name slightly indicates that > > the value of const is true, as in "Yes, prefer untried". > > > > Also this set of constants corresponds to the default settings of the policy, > is > > that correct? Is that what it is meant to do? > > Your interpretation is correct, so setting kPreferUntried to false means that we > prefer requests that have already been tried (but not exceeded the retry count) > to untried requests. > > Yes, these values correspond to the defaults. When using them below, I use them > with a "!" for places we are doing the opposite of the default. > > My code should have been clear enough that you didn't have to ask, so I beefed > up the comments, see if it is clearer now. Ack. This is my OCD around positive sounding constants being set to false. I am sure it bothers me more than most of people, so I think this is safe to stay that way. I did see your usage below and have no problems with that... It is just how it sounds to me. Don't worry about that. https://codereview.chromium.org/2178723002/diff/20001/components/offline_page... File components/offline_pages/background/request_picker_unittest.cc (right): https://codereview.chromium.org/2178723002/diff/20001/components/offline_page... components/offline_pages/background/request_picker_unittest.cc:103: void RequestPickerTest::AddRequestsAndChooseOne( Neat refactoring btw.
https://codereview.chromium.org/2178723002/diff/20001/components/offline_page... File components/offline_pages/background/request_picker_unittest.cc (right): https://codereview.chromium.org/2178723002/diff/20001/components/offline_page... components/offline_pages/background/request_picker_unittest.cc:102: // Test helper method do so the boilerplate operations for most tests. On 2016/07/26 16:01:43, dougarnett wrote: > More descriptive of "what" it does rather than "why" might be better. > > // Test helper to queue the two given requests and then pick one of them per > configured policy. Done. https://codereview.chromium.org/2178723002/diff/20001/components/offline_page... components/offline_pages/background/request_picker_unittest.cc:103: void RequestPickerTest::AddRequestsAndChooseOne( On 2016/07/26 16:01:43, dougarnett wrote: > QueueAndChoose()? but not sure Done.
The CQ bit was checked by petewil@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dougarnett@chromium.org, fgorski@chromium.org Link to the patchset: https://codereview.chromium.org/2178723002/#ps40001 (title: "Rename helper function, improve 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.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Add more unit tests for RequestPicker BUG=610521 ========== to ========== Add more unit tests for RequestPicker BUG=610521 Committed: https://crrev.com/e9e18f8b3f0d9214dd50ca89a8d47cda5078c2b9 Cr-Commit-Position: refs/heads/master@{#407901} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/e9e18f8b3f0d9214dd50ca89a8d47cda5078c2b9 Cr-Commit-Position: refs/heads/master@{#407901} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
