Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2733823005:
Add tests for the JobController's observer. (Closed)
|
Created:
3 years, 9 months ago by harkness Modified:
3 years, 9 months ago Reviewers:
Peter Beverloo CC:
chromium-reviews, Peter Beverloo, darin-cc_chromium.org, jam, harkness+watch_chromium.org, awdf+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd tests for the JobController's observer.
Update the mock DownloadManager to keep track of DownloadItems which
have been created and allow the test to update those items. Also
added tests which explicitly update the DownloadItems and trigger the
DownloadItem::Observer to check that the JobController is doing the right
thing when successful updates happen.
BUG=692621
Review-Url: https://codereview.chromium.org/2733823005
Cr-Commit-Position: refs/heads/master@{#457116}
Committed: https://chromium.googlesource.com/chromium/src/+/e816138fb520f5fc8a2207212845554226af8226
Patch Set 1 #Patch Set 2 : More comments #
Total comments: 8
Patch Set 3 : Code review comments #
Total comments: 4
Patch Set 4 : Comments & emplace usage #Patch Set 5 : Rebase fixes and added RunLoop to DownloadUrl. #Patch Set 6 : Rebase #Patch Set 7 : Replace MockDownloadItemWithValues with FakeDownloadItem #
Total comments: 2
Patch Set 8 : naming changes #Messages
Total messages: 19 (9 generated)
harkness@chromium.org changed reviewers: + peter@chromium.org
https://codereview.chromium.org/2733823005/diff/20001/content/browser/backgro... File content/browser/background_fetch/background_fetch_job_controller_unittest.cc (right): https://codereview.chromium.org/2733823005/diff/20001/content/browser/backgro... content/browser/background_fetch/background_fetch_job_controller_unittest.cc:39: class MockDownloadItemWithValues : public MockDownloadItem { How about moving //chrome/browser/ntp_snippets/fake_download_item.h to a location where this test can access it? That seems like a more scalable approach that we can extend upon in the future. https://codereview.chromium.org/2733823005/diff/20001/content/browser/backgro... content/browser/background_fetch/background_fetch_job_controller_unittest.cc:64: DOWNLOAD_INTERRUPT_REASON_NONE); The `.back()` is a bit odd. What about: std::unique_ptr<MockDownloadItemWithValues> item = base::MakeUnique<MockDownloadItemWithValues>(base::GenerateGUID()); params->callback().Run(item.get(), DOWNLOAD_INTERRUPT_REASON_NONE); download_items_.push_back(std::move(item)); https://codereview.chromium.org/2733823005/diff/20001/content/browser/backgro... content/browser/background_fetch/background_fetch_job_controller_unittest.cc:76: std::vector<std::unique_ptr<MockDownloadItemWithValues>>& download_items() { Two thoughts: (1) This is a test, so it'd be OK to expose |download_items_| as a protected property. (2) If we decide to keep this, it should return a const reference and the method itself should be const too. https://codereview.chromium.org/2733823005/diff/20001/content/browser/backgro... content/browser/background_fetch/background_fetch_job_controller_unittest.cc:158: MockDownloadItemWithValues* item = download_items[0].get(); Since we'd be accessing an invalid entry here if |download_items.size()| is not one (or more), line 157 should be an ASSERT_EQ. Same on line 215.
https://codereview.chromium.org/2733823005/diff/20001/content/browser/backgro... File content/browser/background_fetch/background_fetch_job_controller_unittest.cc (right): https://codereview.chromium.org/2733823005/diff/20001/content/browser/backgro... content/browser/background_fetch/background_fetch_job_controller_unittest.cc:39: class MockDownloadItemWithValues : public MockDownloadItem { On 2017/03/10 00:55:14, Peter Beverloo wrote: > How about moving //chrome/browser/ntp_snippets/fake_download_item.h to a > location where this test can access it? That seems like a more scalable approach > that we can extend upon in the future. Ah, good find, I hadn't seen that. I'll move it to somewhere publicly accessible. https://codereview.chromium.org/2733823005/diff/20001/content/browser/backgro... content/browser/background_fetch/background_fetch_job_controller_unittest.cc:64: DOWNLOAD_INTERRUPT_REASON_NONE); On 2017/03/10 00:55:14, Peter Beverloo wrote: > The `.back()` is a bit odd. What about: > > std::unique_ptr<MockDownloadItemWithValues> item = > base::MakeUnique<MockDownloadItemWithValues>(base::GenerateGUID()); > > params->callback().Run(item.get(), DOWNLOAD_INTERRUPT_REASON_NONE); > download_items_.push_back(std::move(item)); Done. https://codereview.chromium.org/2733823005/diff/20001/content/browser/backgro... content/browser/background_fetch/background_fetch_job_controller_unittest.cc:76: std::vector<std::unique_ptr<MockDownloadItemWithValues>>& download_items() { On 2017/03/10 00:55:14, Peter Beverloo wrote: > Two thoughts: > > (1) This is a test, so it'd be OK to expose |download_items_| as a protected > property. > (2) If we decide to keep this, it should return a const reference and the > method itself should be const too. If the property was on BackgroundFetchJObControllerTest, I'd go with protected, but I decided to go with const instead. https://codereview.chromium.org/2733823005/diff/20001/content/browser/backgro... content/browser/background_fetch/background_fetch_job_controller_unittest.cc:158: MockDownloadItemWithValues* item = download_items[0].get(); On 2017/03/10 00:55:14, Peter Beverloo wrote: > Since we'd be accessing an invalid entry here if |download_items.size()| is not > one (or more), line 157 should be an ASSERT_EQ. Same on line 215. Done.
lgtm % comment https://codereview.chromium.org/2733823005/diff/40001/content/browser/backgro... File content/browser/background_fetch/background_fetch_job_controller_unittest.cc (right): https://codereview.chromium.org/2733823005/diff/40001/content/browser/backgro... content/browser/background_fetch/background_fetch_job_controller_unittest.cc:67: // This is very inefficient, but is only called during shutdown. nit: it would be good to clarify why. O(n) is not something inherently inefficient - the fact that it may become O(n^2) depending on the user is. The reader might also assume that GetGuid() may do something involved or something. https://codereview.chromium.org/2733823005/diff/40001/content/browser/backgro... content/browser/background_fetch/background_fetch_job_controller_unittest.cc:178: BackgroundFetchRequestInfo(GURL(kTestUrl), base::IntToString(i))); This is not how you'd use emplace_back, right? Shouldn't this read: request_infos.emplace_back(GURL(kTestUrl), base::IntToString(i)); (You're now constructing an instance and then calling the copy constructor with it.)
Updated with code review changes and rebase updates. PTAL I had to add a RunLoop to the DownloadUrl code because the test code calls DownloadUrl then immediately updates the observer, which assumes that the DownloadStarted callback has been invoked. This somewhat negates the earlier change to make the call to the DownloadStarted callback be a PostTask instead of an explicit call, but I don't see another way to make the test work short of making every call a PostTask and adding a lot of RunUntilIdle. What's your opinion? https://codereview.chromium.org/2733823005/diff/40001/content/browser/backgro... File content/browser/background_fetch/background_fetch_job_controller_unittest.cc (right): https://codereview.chromium.org/2733823005/diff/40001/content/browser/backgro... content/browser/background_fetch/background_fetch_job_controller_unittest.cc:67: // This is very inefficient, but is only called during shutdown. On 2017/03/13 14:05:27, Peter Beverloo wrote: > nit: it would be good to clarify why. O(n) is not something inherently > inefficient - the fact that it may become O(n^2) depending on the user is. The > reader might also assume that GetGuid() may do something involved or something. Updated. Based on our discussion last week, if we're converting everything to flat_set, we'll be encoding the size assumption other places as well. https://codereview.chromium.org/2733823005/diff/40001/content/browser/backgro... content/browser/background_fetch/background_fetch_job_controller_unittest.cc:178: BackgroundFetchRequestInfo(GURL(kTestUrl), base::IntToString(i))); On 2017/03/13 14:05:27, Peter Beverloo wrote: > This is not how you'd use emplace_back, right? Shouldn't this read: > > request_infos.emplace_back(GURL(kTestUrl), base::IntToString(i)); > > (You're now constructing an instance and then calling the copy constructor with > it.) Ack, of course you're right. Updated.
The CQ bit was checked by harkness@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.
PTAL - ready for review! Converted tests to use FakeDownloadItem (now in content!) instead of MockDownloadItemWithValues.
lgtm again Note that CLs that I've already lgtm'd tend to fall of my radar, please do ping me :). https://codereview.chromium.org/2733823005/diff/120001/content/browser/backgr... File content/browser/background_fetch/background_fetch_job_controller_unittest.cc (right): https://codereview.chromium.org/2733823005/diff/120001/content/browser/backgr... content/browser/background_fetch/background_fetch_job_controller_unittest.cc:139: BackgroundFetchJobData* job_data = job_data_ptr.get(); nit: it's the other way around: line 139 defines the pointer, 137 the owned object. Same elsewhere.
https://codereview.chromium.org/2733823005/diff/120001/content/browser/backgr... File content/browser/background_fetch/background_fetch_job_controller_unittest.cc (right): https://codereview.chromium.org/2733823005/diff/120001/content/browser/backgr... content/browser/background_fetch/background_fetch_job_controller_unittest.cc:139: BackgroundFetchJobData* job_data = job_data_ptr.get(); On 2017/03/15 16:39:47, Peter Beverloo wrote: > nit: it's the other way around: line 139 defines the pointer, 137 the owned > object. Same elsewhere. updated as per discussion.
The CQ bit was checked by harkness@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org Link to the patchset: https://codereview.chromium.org/2733823005/#ps140001 (title: "naming changes")
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": 140001, "attempt_start_ts": 1489596672000570,
"parent_rev": "919e21975bc29fb882f82a656920d9264752d4fa", "commit_rev":
"e816138fb520f5fc8a2207212845554226af8226"}
Message was sent while issue was closed.
Description was changed from ========== Add tests for the JobController's observer. Update the mock DownloadManager to keep track of DownloadItems which have been created and allow the test to update those items. Also added tests which explicitly update the DownloadItems and trigger the DownloadItem::Observer to check that the JobController is doing the right thing when successful updates happen. BUG=692621 ========== to ========== Add tests for the JobController's observer. Update the mock DownloadManager to keep track of DownloadItems which have been created and allow the test to update those items. Also added tests which explicitly update the DownloadItems and trigger the DownloadItem::Observer to check that the JobController is doing the right thing when successful updates happen. BUG=692621 Review-Url: https://codereview.chromium.org/2733823005 Cr-Commit-Position: refs/heads/master@{#457116} Committed: https://chromium.googlesource.com/chromium/src/+/e816138fb520f5fc8a2207212845... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/e816138fb520f5fc8a2207212845... |
