Issue metadata
Sign in to add a comment
|
PageLoadMetricsBrowserTest.IgnoreDownloads failing without obvious cause on Win7 (32) Tests bot |
||||||||||||||||||||||
Issue descriptionThe test started failing on the bot in the following build: https://build.chromium.org/p/chromium.win/builders/Win7%20%2832%29%20Tests/builds/10984 Surprisingly, I can repro the test timeout on my windows machine at a revision earlier than the build above (at 6e09a67eb013cde927f50ac7931018a15bfdb2ba). The test hangs when a "Save As" dialog appears (for "download-test3-attachment.gif" file). Maybe the appearance of "Save As" dialog is triggered by some external factors (not by changes of Chromium product/test code)? Note that another test (NavigatingExtensionPopupBrowserTest.DownloadViaPost) started failing ( issue 653856 ) on this bot at a similar (but not quite the same time): https://build.chromium.org/p/chromium.win/builders/Win7%20%2832%29%20Tests/builds/10981.
,
Oct 7 2016
I'm going to guess that the test is writing to the current users' download directory and not cleaning up after itself. The correct way to test downloads is to set the default downloads directory to a scoped temp directory. If the test is triggering a download with a fixed base name, they will end up uniquified with a numeric suffix (i.e. foo.txt, "foo (1).txt", "foo (2).txt" ...). Once this index exceeds 100, the downloads logic starts prompting the user for the filename.
,
Oct 7 2016
Many thanks asanka@ - I think this is it! I'll try to fix my test (tracked in issue 653856 ).
,
Oct 7 2016
bmcquade@ - can you verify whether asanka@'s hypothesis (from #c2 above) applies also to the test you've recently added - PageLoadMetricsBrowserTest.IgnoreDownloads?
,
Oct 7 2016
Thank you for investigating, and thank you Asanka for pointing out the need for using a scoped temp directory. I'll investigate that change now and try to land it asap.
,
Oct 7 2016
In //chrome you can do something like this: https://cs.chromium.org/chromium/src/chrome/browser/download/download_browsertest.cc?rcl=1475839200&l=498 The CreateAndSetDownloadsDirectory() function at the above CS link should show you how we set up the download path in our tests in //chrome. The setup for //content looks like : https://cs.chromium.org/chromium/src/content/browser/download/download_browsertest.cc?rcl=0&l=563 .
,
Oct 7 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/405e0b424390e530491f050f1721cbfe8aa37697 commit 405e0b424390e530491f050f1721cbfe8aa37697 Author: bmcquade <bmcquade@chromium.org> Date: Fri Oct 07 21:04:14 2016 Use a scoped temp dir for page load metrics download test BUG= 653982 Review-Url: https://codereview.chromium.org/2405583002 Cr-Commit-Position: refs/heads/master@{#423962} [modify] https://crrev.com/405e0b424390e530491f050f1721cbfe8aa37697/chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc
,
Oct 7 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/405e0b424390e530491f050f1721cbfe8aa37697 commit 405e0b424390e530491f050f1721cbfe8aa37697 Author: bmcquade <bmcquade@chromium.org> Date: Fri Oct 07 21:04:14 2016 Use a scoped temp dir for page load metrics download test BUG= 653982 Review-Url: https://codereview.chromium.org/2405583002 Cr-Commit-Position: refs/heads/master@{#423962} [modify] https://crrev.com/405e0b424390e530491f050f1721cbfe8aa37697/chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc
,
Oct 7 2016
Just landed what I think should be a fix. I'll keep an eye on the build at https://build.chromium.org/p/chromium.win/builders/Win7%20%2832%29%20Tests/ to make sure it goes green.
,
Oct 8 2016
Tests started passing after this change landed. Thanks! |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by lukasza@chromium.org
, Oct 7 2016