Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12422012:
Fix DownloadExtensionTest_OnDeterminingFilename_InterruptedResume (Closed)
|
Created:
7 years, 9 months ago by benjhayden Modified:
7 years, 8 months ago Reviewers:
Randy Smith (Not in Mondays) CC:
chromium-reviews, benjhayden+dwatch_chromium.org, Aaron Boodman, chromium-apps-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFix DownloadExtensionTest_OnDeterminingFilename_InterruptedResume
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=192563
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=192816
Patch Set 1 : @r191608 #
Total comments: 5
Patch Set 2 : @r192071 #Patch Set 3 : @r192364 #Patch Set 4 : @r192655 #Messages
Total messages: 17 (0 generated)
This CL has not yet fixed the test due to a bug in
DownloadManagerImpl::StartDownload calling OnDownloadCreated.
[652:652:0321/155601:1316649884366:FATAL:observer_list.h(122)] Check failed:
false. Observers can only be added once!
[0x7f19c522b48e] base::debug::StackTrace::StackTrace()
[0x7f19c526b49f] logging::LogMessage::~LogMessage()
[0x7f19bca6184a] ObserverListBase<>::AddObserver()
[0x7f19bca584de] content::DownloadItemImpl::AddObserver()
[0x000000fd827a] AllDownloadItemNotifier::OnDownloadCreated()
[0x7f19bca6b3e6] content::DownloadManagerImpl::StartDownload()
[0x7f19bca7b291] content::(anonymous namespace)::StartOnUIThread()
On 2013/03/21 20:01:53, benjhayden_chromium wrote: > This CL has not yet fixed the test due to a bug in > DownloadManagerImpl::StartDownload calling OnDownloadCreated. > > [652:652:0321/155601:1316649884366:FATAL:observer_list.h(122)] Check failed: > false. Observers can only be added once! > > [0x7f19c522b48e] base::debug::StackTrace::StackTrace() > > > [0x7f19c526b49f] logging::LogMessage::~LogMessage() > [0x7f19bca6184a] ObserverListBase<>::AddObserver() > > > [0x7f19bca584de] content::DownloadItemImpl::AddObserver() > [0x000000fd827a] AllDownloadItemNotifier::OnDownloadCreated() > > > [0x7f19bca6b3e6] content::DownloadManagerImpl::StartDownload() > > > [0x7f19bca7b291] content::(anonymous namespace)::StartOnUIThread() Could you give a bit of detail about what the CL is actually doing to fix that test? Also, include a BUG= line. It's probably also worthwhile including a pointer in the CL messages to the issue that, when it's fixed, will result in the test being completely fixed (which I thought you had filed and sent me to fix, but I can't find it--were you planning to fix that, or is that mine to fix?)
https://codereview.chromium.org/12422012/diff/6001/chrome/browser/extensions/... File chrome/browser/extensions/api/downloads/downloads_api_unittest.cc (right): https://codereview.chromium.org/12422012/diff/6001/chrome/browser/extensions/... chrome/browser/extensions/api/downloads/downloads_api_unittest.cc:181: LOG(INFO) << "occam caught " << new_event->Debug(); In general, I think it's good practice to only ask for review for code you'd like to check in. If you want me to show you the git magic to have these kind of things in your checkout without them showing up in Rietveld, let me know. https://codereview.chromium.org/12422012/diff/6001/content/browser/download/d... File content/browser/download/download_manager_impl.h (right): https://codereview.chromium.org/12422012/diff/6001/content/browser/download/d... content/browser/download/download_manager_impl.h:98: int32 id, DownloadInterruptReason reason) OVERRIDE; I'm uncomfortable with this additional breaking of the DMI interface for testing. Why can't you use one of the more standard ways to inject errors? URLRequestSlowDownloadJob allows this kind of control.
PTAL
Looks basically good. https://codereview.chromium.org/12422012/diff/12007/chrome/browser/extensions... File chrome/browser/extensions/api/downloads/downloads_api_unittest.cc (right): https://codereview.chromium.org/12422012/diff/12007/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api_unittest.cc:3109: : public content::DownloadTestObserverInProgress { It probably won't surprise you to hear that I don't like duplicate classes that have *almost* the same functionality. Have you checked to see what breaks if you remove the filename check from DownloadTestObserverInProgress? https://codereview.chromium.org/12422012/diff/12007/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api_unittest.cc:3148: CURRENT_TAB, Please comment this innocuous looking enum :-}.
https://codereview.chromium.org/12422012/diff/12007/chrome/browser/extensions... File chrome/browser/extensions/api/downloads/downloads_api_unittest.cc (right): https://codereview.chromium.org/12422012/diff/12007/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api_unittest.cc:3109: : public content::DownloadTestObserverInProgress { On 2013/04/02 21:52:34, rdsmith wrote: > It probably won't surprise you to hear that I don't like duplicate classes that > have *almost* the same functionality. Have you checked to see what breaks if > you remove the filename check from DownloadTestObserverInProgress? Even if the tests all pass, wouldn't that cause races and flaky tests? Howbout a TODO to make DownloadUpdatedObserver more powerful and merge this with that? https://codereview.chromium.org/12422012/diff/12007/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api_unittest.cc:3148: CURRENT_TAB, On 2013/04/02 21:52:34, rdsmith wrote: > Please comment this innocuous looking enum :-}. Done.
https://codereview.chromium.org/12422012/diff/12007/chrome/browser/extensions... File chrome/browser/extensions/api/downloads/downloads_api_unittest.cc (right): https://codereview.chromium.org/12422012/diff/12007/chrome/browser/extensions... chrome/browser/extensions/api/downloads/downloads_api_unittest.cc:3109: : public content::DownloadTestObserverInProgress { On 2013/04/03 15:43:00, benjhayden_chromium wrote: > On 2013/04/02 21:52:34, rdsmith wrote: > > It probably won't surprise you to hear that I don't like duplicate classes > that > > have *almost* the same functionality. Have you checked to see what breaks if > > you remove the filename check from DownloadTestObserverInProgress? > > Even if the tests all pass, wouldn't that cause races and flaky tests? > Howbout a TODO to make DownloadUpdatedObserver more powerful and merge this with > that? Good point; you'd have to check the tests that use it individually to see if it was safe. But my larger point is that that extra test may be obsolete and you could spend a bit of time seeing if it's safe to get rid of it. And my even larger point is that it's worth a bit of time trying to find *some* way to avoid adding YATO that's really similar to ones we already have. If there's no easy way to do that, I'll accept the new class. But I do think it's worth some time trying to confirm there's no easy way to do that.
On 2013/04/03 15:56:47, rdsmith wrote: > https://codereview.chromium.org/12422012/diff/12007/chrome/browser/extensions... > File chrome/browser/extensions/api/downloads/downloads_api_unittest.cc (right): > > https://codereview.chromium.org/12422012/diff/12007/chrome/browser/extensions... > chrome/browser/extensions/api/downloads/downloads_api_unittest.cc:3109: : public > content::DownloadTestObserverInProgress { > On 2013/04/03 15:43:00, benjhayden_chromium wrote: > > On 2013/04/02 21:52:34, rdsmith wrote: > > > It probably won't surprise you to hear that I don't like duplicate classes > > that > > > have *almost* the same functionality. Have you checked to see what breaks > if > > > you remove the filename check from DownloadTestObserverInProgress? > > > > Even if the tests all pass, wouldn't that cause races and flaky tests? > > Howbout a TODO to make DownloadUpdatedObserver more powerful and merge this > with > > that? > > Good point; you'd have to check the tests that use it individually to see if it > was safe. But my larger point is that that extra test may be obsolete and you > could spend a bit of time seeing if it's safe to get rid of it. And my even > larger point is that it's worth a bit of time trying to find *some* way to avoid > adding YATO that's really similar to ones we already have. If there's no easy > way to do that, I'll accept the new class. But I do think it's worth some time > trying to confirm there's no easy way to do that. These instances don't appear to need the filename: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/dow... https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... These instances do appear to need the filename: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/dow... https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... I could pass an optional flag to DTOIP...
On 2013/04/03 16:58:34, benjhayden_chromium wrote: > On 2013/04/03 15:56:47, rdsmith wrote: > > > https://codereview.chromium.org/12422012/diff/12007/chrome/browser/extensions... > > File chrome/browser/extensions/api/downloads/downloads_api_unittest.cc > (right): > > > > > https://codereview.chromium.org/12422012/diff/12007/chrome/browser/extensions... > > chrome/browser/extensions/api/downloads/downloads_api_unittest.cc:3109: : > public > > content::DownloadTestObserverInProgress { > > On 2013/04/03 15:43:00, benjhayden_chromium wrote: > > > On 2013/04/02 21:52:34, rdsmith wrote: > > > > It probably won't surprise you to hear that I don't like duplicate classes > > > that > > > > have *almost* the same functionality. Have you checked to see what breaks > > if > > > > you remove the filename check from DownloadTestObserverInProgress? > > > > > > Even if the tests all pass, wouldn't that cause races and flaky tests? > > > Howbout a TODO to make DownloadUpdatedObserver more powerful and merge this > > with > > > that? > > > > Good point; you'd have to check the tests that use it individually to see if > it > > was safe. But my larger point is that that extra test may be obsolete and you > > could spend a bit of time seeing if it's safe to get rid of it. And my even > > larger point is that it's worth a bit of time trying to find *some* way to > avoid > > adding YATO that's really similar to ones we already have. If there's no easy > > way to do that, I'll accept the new class. But I do think it's worth some > time > > trying to confirm there's no easy way to do that. > > These instances don't appear to need the filename: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/dow... > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > > These instances do appear to need the filename: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/dow... > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... > > I could pass an optional flag to DTOIP... Ok, I'll accept due diligence as having been done. I'm ok with either the extra class with a TODO or the extra flag. But we should cleanup this whole testing infrastructure mess at some point.
TODOne. LGTY?
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/benjhayden@chromium.org/12422012/58001
Retried try job too often on win_x64_rel for step(s) compile http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...
Message was sent while issue was closed.
Committed patchset #3 manually as r192563 (presubmit successful).
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/benjhayden@chromium.org/12422012/78001
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_chromeos_clang. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
Message was sent while issue was closed.
Committed patchset #4 manually as r192816 (presubmit successful). |
