Fix for flakiness in pyauto automation hook WaitForDownloadsToComplete.

Re-write pyauto automation hook WaitForAllDownloadsToComplete.

Now that the Chrome downloads system is more stable since when this hook was originally written, the hook is now updated to work for all downloads (it previously did not work well for dangerous downloads, or downloads from an incognito window).

A new input to the automation hook also requires changes to several pyauto files that invoke the hook.

BUG=69738, 91263
TEST=None

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=96193

[Nirnimesh, 2011-08-01 21:29:28]

Awesome.

LGTM

[Paweł Hajdan Jr., 2011-08-01 22:42:10]

NAK, doesn't seems to be the right solution. And this is chrome/test/OWNERS
here. Please do a proper fix, and thank you for fixing flakiness.

http://codereview.chromium.org/7544026/diff/1/chrome/browser/automation/testi...
File chrome/browser/automation/testing_automation_provider.cc (right):

http://codereview.chromium.org/7544026/diff/1/chrome/browser/automation/testi...
chrome/browser/automation/testing_automation_provider.cc:2948: // It's possible
that pending downloads may have completed before the observer
Seems hacky. How about starting the new download only after the observer is
ready? Also, isn't the new version still racy?

[Nirnimesh, 2011-08-01 22:49:23]

http://codereview.chromium.org/7544026/diff/1/chrome/browser/automation/testi...
File chrome/browser/automation/testing_automation_provider.cc (right):

http://codereview.chromium.org/7544026/diff/1/chrome/browser/automation/testi...
chrome/browser/automation/testing_automation_provider.cc:2948: // It's possible
that pending downloads may have completed before the observer
On 2011/08/01 22:42:10, Paweł Hajdan Jr. wrote:
> Seems hacky. How about starting the new download only after the observer is
> ready?

In a perfect world, yes. But the flakes in the download system aren't fully
solved, and I see crashes every few days. What happens when and how isn't quite
clear. This CL patches up this automation to make it at least more robust.

[dennis_jeffrey, 2011-08-02 00:04:53]

Thanks for looking over this, Pawel.  Let me know your thoughts.

http://codereview.chromium.org/7544026/diff/1/chrome/browser/automation/testi...
File chrome/browser/automation/testing_automation_provider.cc (right):

http://codereview.chromium.org/7544026/diff/1/chrome/browser/automation/testi...
chrome/browser/automation/testing_automation_provider.cc:2948: // It's possible
that pending downloads may have completed before the observer
On 2011/08/01 22:42:10, Paweł Hajdan Jr. wrote:
> Seems hacky. How about starting the new download only after the observer is
> ready? Also, isn't the new version still racy?

The suggested approach works well in the case where we simply want to trigger a
download in a particular way and wait for it to finish, as these actions can be
encapsulated in a single automation hook.  However, this use case doesn't
satisfy all existing pyauto needs: e.g., see testMultipleDownloadsInfobar in
infobars.py, which separates the action of triggering a zip download from the
action of waiting for downloads to finish.  A complete solution for fixing
download flakiness in the automation is not clear to me, but the current change
at least significantly improves robustness of this hook and should improve
robustness of several existing pyauto tests.  I needed this change to unblock me
in other work I'm doing to investigate performance of downloads in
chrome/chromeos, and I don't have the bandwidth to tackle a complete solution
anytime soon.  Given this, how should we proceed with this CL?

[Paweł Hajdan Jr., 2011-08-02 16:53:20]

Feel free to ping me on chat or come to my desk about this. I don't want to
block you, just care a lot about quality of this code.

To make it clear: my NAK still holds.

http://codereview.chromium.org/7544026/diff/1/chrome/browser/automation/testi...
File chrome/browser/automation/testing_automation_provider.cc (right):

http://codereview.chromium.org/7544026/diff/1/chrome/browser/automation/testi...
chrome/browser/automation/testing_automation_provider.cc:2948: // It's possible
that pending downloads may have completed before the observer
On 2011/08/01 22:49:24, Nirnimesh wrote:
> In a perfect world, yes. But the flakes in the download system aren't fully
> solved

Please show me the bugs and I can fix them. Let's at least make sure they're on
file and not mythical.

Also note that doing *this* in a hacky way contributes to overall flakiness and
hackiness.

> and I see crashes every few days.

Agreed, we have crashes, and I think pyauto is very helpful in detecting them (I
remember it was pyauto that detected most of my download code regressions in
2010). However, I think this is not relevant to doing this in a hack or
non-hacky way.

> This CL patches up this automation to make it at least more robust.

Agreed that for short term it improves things, but the fix seems fragile and can
break with the next change in the downloads system. Let's make sure we
understand the root cause of the problem and fix it once and for good.

http://codereview.chromium.org/7544026/diff/1/chrome/browser/automation/testi...
chrome/browser/automation/testing_automation_provider.cc:2948: // It's possible
that pending downloads may have completed before the observer
On 2011/08/02 00:04:53, dennis_jeffrey wrote:
> The suggested approach works well in the case where we simply want to trigger
a
> download in a particular way and wait for it to finish, as these actions can
be
> encapsulated in a single automation hook.  However, this use case doesn't
> satisfy all existing pyauto needs: e.g., see testMultipleDownloadsInfobar in
> infobars.py, which separates the action of triggering a zip download from the
> action of waiting for downloads to finish.

This is a known problem with pyauto. I see the following ways to handle it:

1) Declare it's out of scope for pyauto and test it with a browser test instead.
You probably don't want that, but I included it for completeness.

2) Have a windowed notification observer, i.e. set up the observers first with
one automation message, then (possibly indirectly) trigger the download, and
finally wait for that observer to observe the wanted event. This is what we do
in browser_tests and it *works*.

> I needed this change to unblock me
> in other work I'm doing to investigate performance of downloads in
> chrome/chromeos, and I don't have the bandwidth to tackle a complete solution
> anytime soon.  Given this, how should we proceed with this CL?

"I have no time to do it properly" is no argument. Sorry, but I have no time to
fix all the bad hacks after you guys, and this is why I'm doing those drive-by
reviews. The hacks cause us a lot of problems long term, and we should really
avoid doing them in the first place.

Please let me know though if I can help you with writing a more correct
solution. My only goal here is to help you make this change in a solid way, so
that it wouldn't need frequent fixes in the future.

[dennis_jeffrey, 2011-08-02 17:09:10]

Thanks for the detailed reply, Pawel.  I agree that a complete solution to this
flakiness is the best course of action, and with your help in discussing ideas,
maybe we can come up with a solution in a timely manner.  I'd like to loop in
Nirnimesh with any discussions we have about how to tackle this, because of his
pyauto experience.  Let me talk to him and see what we can do.  I'll stop by or
ping you if I want to bounce some ideas off of you.  Thank you!

[Nirnimesh, 2011-08-02 17:22:53]

This is turning out to be another one of those tussles.

All your suggestions are invalid for the things Dennis wants to fix.

1. browser_tests: the concept of browser_tests doesn't gel well with chromeos.
Remember, chromeos has the concept of primary browser, you can't replace that
with browser_tests. This is Dennis's primary motivation.
2. setup observers and wait later: yes, this would work IFF downloads system
didn't have inherent unsolved problems to begin with. Look for bugs with 'pyauto
feature=Downloads' that have been flaky for a long time because the downloads
system doesn't do what it's supposed to -- like provide right notifications at
the right time. Fixing those is out of our scope. Do you mean that until then,
none of our work that depends on downloads will get done? This is ridiculous.

[dennis_jeffrey, 2011-08-03 16:01:07]

On 2011/08/02 17:22:53, Nirnimesh wrote:
> This is turning out to be another one of those tussles.
> 
> All your suggestions are invalid for the things Dennis wants to fix.
> 
> 1. browser_tests: the concept of browser_tests doesn't gel well with chromeos.
> Remember, chromeos has the concept of primary browser, you can't replace that
> with browser_tests. This is Dennis's primary motivation.
> 2. setup observers and wait later: yes, this would work IFF downloads system
> didn't have inherent unsolved problems to begin with. Look for bugs with
'pyauto
> feature=Downloads' that have been flaky for a long time because the downloads
> system doesn't do what it's supposed to -- like provide right notifications at
> the right time. Fixing those is out of our scope. Do you mean that until then,
> none of our work that depends on downloads will get done? This is ridiculous.

Hi guys - how about this: we change the automation hook
WaitForDownloadsToComplete into a hook that simply returns the number of current
downloads.  Then, all pyauto tests that invoke this hook now use WaitUntil() the
current number of downloads is 0.

Pawel - while this doesn't completely address download flakiness, I believe it
does remove flakiness from the hook involved in this CL.

Nirnimesh - this idea adds a little more complexity to the pyauto tests, but I
believe still fixes the original problem that this CL was meant to address: the
flakiness in the WaitForDownloadsToComplete hook.

I'd like to try to find a way to get ACKs from both of you on this CL ;-)

[Paweł Hajdan Jr., 2011-08-03 16:42:13]

On 2011/08/03 16:01:07, dennis_jeffrey wrote:
> Hi guys - how about this: we change the automation hook
> WaitForDownloadsToComplete into a hook that simply returns the number of
current
> downloads.  Then, all pyauto tests that invoke this hook now use WaitUntil()
the
> current number of downloads is 0.
> 
> Pawel - while this doesn't completely address download flakiness, I believe it
> does remove flakiness from the hook involved in this CL.

Thanks for finding a solution that could work here. I think I'm fine with that,
it keeps the automation side reasonably clean.

[krisr, 2011-08-03 17:04:43]

In the future I prefer to not have automation hook changes require updates
to multiple existing tests.  The right thing to do is to build a new method
that new tests use and put a deprecation warning in the old method.

Please remember that there are multiple consumers of this code and snap
changes like this can break workflows for them.

Kris

On Wed, Aug 3, 2011 at 9:42 AM, <phajdan.jr@chromium.org> wrote:

> On 2011/08/03 16:01:07, dennis_jeffrey wrote:
>
>> Hi guys - how about this: we change the automation hook
>> WaitForDownloadsToComplete into a hook that simply returns the number of
>>
> current
>
>> downloads.  Then, all pyauto tests that invoke this hook now use
>> WaitUntil()
>>
> the
>
>> current number of downloads is 0.
>>
>
>  Pawel - while this doesn't completely address download flakiness, I
>> believe it
>> does remove flakiness from the hook involved in this CL.
>>
>
> Thanks for finding a solution that could work here. I think I'm fine with
> that,
> it keeps the automation side reasonably clean.
>
>
>
http://codereview.chromium.**org/7544026/<http://codereview.chromium.org/7544...
>

[dennisjeffrey, 2011-08-03 17:18:37]

In this case the current version of the automation hook was flaky and we
could not find a way to completely eliminate that flakiness without changing
the nature of the hook itself, so I think such a change was needed here.
 But I agree that in general, hook changes should try to minimize disruption
to existing tests whenever possible.

-Dennis

On Wed, Aug 3, 2011 at 10:04 AM, Kris Rambish <krisr@chromium.org> wrote:

> In the future I prefer to not have automation hook changes require updates
> to multiple existing tests.  The right thing to do is to build a new method
> that new tests use and put a deprecation warning in the old method.
>
> Please remember that there are multiple consumers of this code and snap
> changes like this can break workflows for them.
>
> Kris
>
>
> On Wed, Aug 3, 2011 at 9:42 AM, <phajdan.jr@chromium.org> wrote:
>
>> On 2011/08/03 16:01:07, dennis_jeffrey wrote:
>>
>>> Hi guys - how about this: we change the automation hook
>>> WaitForDownloadsToComplete into a hook that simply returns the number of
>>>
>> current
>>
>>> downloads.  Then, all pyauto tests that invoke this hook now use
>>> WaitUntil()
>>>
>> the
>>
>>> current number of downloads is 0.
>>>
>>
>>  Pawel - while this doesn't completely address download flakiness, I
>>> believe it
>>> does remove flakiness from the hook involved in this CL.
>>>
>>
>> Thanks for finding a solution that could work here. I think I'm fine with
>> that,
>> it keeps the automation side reasonably clean.
>>
>>
>>
http://codereview.chromium.**org/7544026/<http://codereview.chromium.org/7544...
>>
>
>

[dennisjeffrey, 2011-08-03 19:13:02]

>
> Hi guys - how about this: we change the automation hook
>> WaitForDownloadsToComplete into a hook that simply returns the number of
>>
> current
>
>> downloads.  Then, all pyauto tests that invoke this hook now use
>> WaitUntil()
>>
> the
>
>> current number of downloads is 0.
>>
>
> current_downloads == 0 does not imply that these downloads have been
> committed
> and ready to use. They might not be renamed yet. They might not be
> completely
> committed to disk yet.
>
>
Isn't the same problem already there in the current version of code?  The
WaitForDownloadsToComplete automation hook returns when all the pending
downloads have their states changed to "COMPLETE".  It does not explicitly
wait for data to be committed to disk (unless that is implied by having the
status of a download be "COMPLETE").  This may be another source of
flakiness, but I think it's a different source than what I was trying to
address in this CL.

[Nirnimesh, 2011-08-03 19:41:06]

On 2011/08/03 19:13:02, dennisjeffrey wrote:
> >
> > Hi guys - how about this: we change the automation hook
> >> WaitForDownloadsToComplete into a hook that simply returns the number of
> >>
> > current
> >
> >> downloads.  Then, all pyauto tests that invoke this hook now use
> >> WaitUntil()
> >>
> > the
> >
> >> current number of downloads is 0.
> >>
> >
> > current_downloads == 0 does not imply that these downloads have been
> > committed
> > and ready to use. They might not be renamed yet. They might not be
> > completely
> > committed to disk yet.
> >
> >
> Isn't the same problem already there in the current version of code?  The
> WaitForDownloadsToComplete automation hook returns when all the pending
> downloads have their states changed to "COMPLETE".  It does not explicitly
> wait for data to be committed to disk (unless that is implied by having the
> status of a download be "COMPLETE").  This may be another source of
> flakiness, but I think it's a different source than what I was trying to
> address in this CL.

Well, maybe 'complete' implies that it's been committed to disk, but does not
imply that it's renamed yet (or else you wouldn't have a problem to begin with).
Talking to someone you knows the downloads system better might help here.

[Nirnimesh, 2011-08-03 19:45:19]

I also see a ModelChanged() in DownloadManager::Observer that looks interesting.
// New or deleted download, observers should query us for the current set
// of downloads.

[dennis_jeffrey, 2011-08-06 02:17:26]

I re-wrote the automation hook based on ideas provided by the Downloads folks,
and modified the CL description accordingly.  Please take another look when you
have a moment.

[Paweł Hajdan Jr., 2011-08-08 16:36:13]

Code I commented in the drive-by LGTM.

http://codereview.chromium.org/7544026/diff/15001/chrome/browser/automation/a...
File chrome/browser/automation/automation_provider_observers.cc (right):

http://codereview.chromium.org/7544026/diff/15001/chrome/browser/automation/a...
chrome/browser/automation/automation_provider_observers.cc:1522: if
(pending_downloads_.empty()) {
nit: Preferred would be:

if (!pending_downloads_.empty())
  return;

http://codereview.chromium.org/7544026/diff/15001/chrome/browser/automation/a...
chrome/browser/automation/automation_provider_observers.cc:1524: if (provider_)
nit: Add braces {} here.

[Nirnimesh, 2011-08-08 17:26:16]

http://codereview.chromium.org/7544026/diff/15001/chrome/browser/automation/a...
File chrome/browser/automation/automation_provider_observers.cc (right):

http://codereview.chromium.org/7544026/diff/15001/chrome/browser/automation/a...
chrome/browser/automation/automation_provider_observers.cc:1487:
(*it)->GetAsInteger(&val);
wrap this inside if()

http://codereview.chromium.org/7544026/diff/15001/chrome/browser/automation/a...
chrome/browser/automation/automation_provider_observers.cc:1514: if
(download->state() != DownloadItem::IN_PROGRESS) {
shouldn't you check for status COMPLETED?

http://codereview.chromium.org/7544026/diff/15001/chrome/test/functional/down...
File chrome/test/functional/downloads.py (right):

http://codereview.chromium.org/7544026/diff/15001/chrome/test/functional/down...
chrome/test/functional/downloads.py:103: self.WaitForAllDownloadsToComplete([])
See my notes in pyauto.py
This should default to [], so changes to this file (and other test scripts)
should not be necessary.

http://codereview.chromium.org/7544026/diff/15001/chrome/test/pyautolib/pyaut...
File chrome/test/pyautolib/pyauto.py (right):

http://codereview.chromium.org/7544026/diff/15001/chrome/test/pyautolib/pyaut...
chrome/test/pyautolib/pyauto.py:1049: def WaitForAllDownloadsToComplete(self,
pre_download_ids, windex=0,
pre_download_ids should default to [] so that most callers need not worry with
it.

[dennis_jeffrey, 2011-08-08 22:29:51]

http://codereview.chromium.org/7544026/diff/15001/chrome/browser/automation/a...
File chrome/browser/automation/automation_provider_observers.cc (right):

http://codereview.chromium.org/7544026/diff/15001/chrome/browser/automation/a...
chrome/browser/automation/automation_provider_observers.cc:1487:
(*it)->GetAsInteger(&val);
On 2011/08/08 17:26:17, Nirnimesh wrote:
> wrap this inside if()

Done.

http://codereview.chromium.org/7544026/diff/15001/chrome/browser/automation/a...
chrome/browser/automation/automation_provider_observers.cc:1514: if
(download->state() != DownloadItem::IN_PROGRESS) {
On 2011/08/08 17:26:17, Nirnimesh wrote:
> shouldn't you check for status COMPLETED?

No, I want the hook to return as soon as we don't need to wait for any downloads
to further progress.  The "IN_PROGRESS" state is the only state in which a
download is still actively progressing.  The other states -- COMPLETE,
CANCELLED, REMOVING, INTERRUPTED -- occur when a download will not make any
further progression.  I'm leaving it up to the test to check/decide what to do
with the end result of a download; this hook merely waits until all downloads
stop progressing.

If we only wait for the COMPLETE state, this hook may never return.

http://codereview.chromium.org/7544026/diff/15001/chrome/browser/automation/a...
chrome/browser/automation/automation_provider_observers.cc:1522: if
(pending_downloads_.empty()) {
On 2011/08/08 16:36:13, Paweł Hajdan Jr. wrote:
> nit: Preferred would be:
> 
> if (!pending_downloads_.empty())
>   return;

Done.

http://codereview.chromium.org/7544026/diff/15001/chrome/browser/automation/a...
chrome/browser/automation/automation_provider_observers.cc:1524: if (provider_)
On 2011/08/08 16:36:13, Paweł Hajdan Jr. wrote:
> nit: Add braces {} here.

I assume this is to improve readability because lines 1525-1526 below is on 2
lines?  If so, then with the latest changes, those 2 lines can now fit on 1
line, so I think it's ok to leave out the {} now.

http://codereview.chromium.org/7544026/diff/15001/chrome/test/functional/down...
File chrome/test/functional/downloads.py (right):

http://codereview.chromium.org/7544026/diff/15001/chrome/test/functional/down...
chrome/test/functional/downloads.py:103: self.WaitForAllDownloadsToComplete([])
On 2011/08/08 17:26:17, Nirnimesh wrote:
> See my notes in pyauto.py
> This should default to [], so changes to this file (and other test scripts)
> should not be necessary.

Done.

http://codereview.chromium.org/7544026/diff/15001/chrome/test/pyautolib/pyaut...
File chrome/test/pyautolib/pyauto.py (right):

http://codereview.chromium.org/7544026/diff/15001/chrome/test/pyautolib/pyaut...
chrome/test/pyautolib/pyauto.py:1049: def WaitForAllDownloadsToComplete(self,
pre_download_ids, windex=0,
On 2011/08/08 17:26:17, Nirnimesh wrote:
> pre_download_ids should default to [] so that most callers need not worry with
> it.

Great idea.

[Nirnimesh, 2011-08-09 17:25:27]

LGTM