Removing command_execution_timeout_ms in favor of action_max_timeout_ms.

Removing command_execution_timeout_ms in favor of action_max_timeout_ms.

R=jcivelli@chromium.org,sky@chromium.org,phajdan.jr@chromium.org,nirnimesh@chromium.org
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=81164

[Paweł Hajdan Jr., 2011-03-19 10:21:34]

Thank you for working on this, looks like a step in right direction.

I think that you should use trybots to test this if you have access (otherwise
ask for provisional committer status), and you can always run ui_tests and
pyauto tests locally.

http://codereview.chromium.org/6685099/diff/1/chrome/test/automation/automati...
File chrome/test/automation/automation_proxy.cc (right):

http://codereview.chromium.org/6685099/diff/1/chrome/test/automation/automati...
chrome/test/automation/automation_proxy.cc:452:
AutomationProxy::command_execution_timeout_.InMilliseconds());
nit: We're in AutomationProxy, there is no need for "AutomationProxy::" here
when accessing a member variable.

http://codereview.chromium.org/6685099/diff/1/chrome/test/pyautolib/pyautolib.i
File chrome/test/pyautolib/pyautolib.i (right):

http://codereview.chromium.org/6685099/diff/1/chrome/test/pyautolib/pyautolib...
chrome/test/pyautolib/pyautolib.i:201: void set_command_execution_timeout_ms(int
timeout);
This should also be removed. Instead, we should be able to pass a custom timeout
for the automation calls near current places where this is used.

Example:
http://codesearch.google.com/codesearch/p#OAMlx_jo-ck/src/chrome/test/pyautol...

[Nirnimesh, 2011-03-20 07:36:14]

http://codereview.chromium.org/6685099/diff/1/chrome/test/pyautolib/pyautolib.i
File chrome/test/pyautolib/pyautolib.i (left):

http://codereview.chromium.org/6685099/diff/1/chrome/test/pyautolib/pyautolib...
chrome/test/pyautolib/pyautolib.i:197: %feature("docstring", "Get the timeout
(in milli secs) for an automation "
Please look into all instances of command_execution_timeout_ms in pyauto, and
make sure the tests pass.

[hnguyen, 2011-03-23 01:58:18]

On Sat, Mar 19, 2011 at 3:21 AM, <phajdan.jr@chromium.org> wrote:

> Thank you for working on this, looks like a step in right direction.
>
> I think that you should use trybots to test this if you have access
> (otherwise
> ask for provisional committer status), and you can always run ui_tests and
> pyauto tests locally.
>
>
>
>
http://codereview.chromium.org/6685099/diff/1/chrome/test/automation/automati...
> File chrome/test/automation/automation_proxy.cc (right):
>
>
>
http://codereview.chromium.org/6685099/diff/1/chrome/test/automation/automati...
> chrome/test/automation/automation_proxy.cc:452:
> AutomationProxy::command_execution_timeout_.InMilliseconds());
> nit: We're in AutomationProxy, there is no need for "AutomationProxy::"
> here when accessing a member variable.
>
>
>
http://codereview.chromium.org/6685099/diff/1/chrome/test/pyautolib/pyautolib.i
> File chrome/test/pyautolib/pyautolib.i (right):
>
>
>
http://codereview.chromium.org/6685099/diff/1/chrome/test/pyautolib/pyautolib...
> chrome/test/pyautolib/pyautolib.i:201: void
> set_command_execution_timeout_ms(int timeout);
> This should also be removed. Instead, we should be able to pass a custom
> timeout for the automation calls near current places where this is used.
>

Can you elaborate on this? It looks like the only place this function is
being used is pyauto.CmdExecutionTimeoutChanger.
Which in turns is used by test_utils.CallFunctionWithNewTimeout() which is
used in src/chrome/functional/downloads.py:220:

    test_utils.CallFunctionWithNewTimeout(self, 4 * 60 * 1000,  # 4 min.

 self.WaitForAllDownloadsToComplete)

Are you suggesting that WaitForAllDownloadsToComplete() should take a
timeout param?

Thanks!


>
> Example:
>
>
http://codesearch.google.com/codesearch/p#OAMlx_jo-ck/src/chrome/test/pyautol...
>
> http://codereview.chromium.org/6685099/
>

[Nirnimesh, 2011-03-23 06:05:07]

> 
> Can you elaborate on this? It looks like the only place this function is
> being used is pyauto.CmdExecutionTimeoutChanger.
> Which in turns is used by test_utils.CallFunctionWithNewTimeout() which is
> used in src/chrome/functional/downloads.py:220:
> 
>     test_utils.CallFunctionWithNewTimeout(self, 4 * 60 * 1000,  # 4 min.
> 
>  self.WaitForAllDownloadsToComplete)
> 
> Are you suggesting that WaitForAllDownloadsToComplete() should take a
> timeout param?

command_execution_timeout_ms currently controls the timeout for *all* automation
calls.
It's currently set to 25 secs, while the proposed replacement
action_max_timeout_ms is 20 secs.
Therefore, apart from fixing the above usage, please make sure that all pyauto
tests pass.

[Paweł Hajdan Jr., 2011-03-23 18:09:36]

On 2011/03/23 01:58:18, hnguyen wrote:
>
http://codereview.chromium.org/6685099/diff/1/chrome/test/pyautolib/pyautolib...
> > chrome/test/pyautolib/pyautolib.i:201: void
> > set_command_execution_timeout_ms(int timeout);
> > This should also be removed. Instead, we should be able to pass a custom
> > timeout for the automation calls near current places where this is used.
> >
> 
> Can you elaborate on this? It looks like the only place this function is
> being used is pyauto.CmdExecutionTimeoutChanger.
> Which in turns is used by test_utils.CallFunctionWithNewTimeout() which is
> used in src/chrome/functional/downloads.py:220:
> 
>     test_utils.CallFunctionWithNewTimeout(self, 4 * 60 * 1000,  # 4 min.
> 
>  self.WaitForAllDownloadsToComplete)
> 
> Are you suggesting that WaitForAllDownloadsToComplete() should take a
> timeout param?

That's a good question, and the answer is "yes". This is in Python code, so I'd
also suggest exposing the TestTimeouts values, so that you can pass one of the
larger ones (I'm pretty sure we have something greater than or equal 4 minutes).

On 2011/03/23 06:05:07, Nirnimesh wrote:
> command_execution_timeout_ms currently controls the timeout for *all*
automation
> calls.
> It's currently set to 25 secs, while the proposed replacement
> action_max_timeout_ms is 20 secs.
> Therefore, apart from fixing the above usage, please make sure that all pyauto
> tests pass.

I'm fine with just setting action_max_timeout to 25 seconds for simplicity.
Thanks for watching!

[Huyen, 2011-03-25 22:03:28]

Paweł, Please take another look.
I've send the CL to try bot as well, will address any errors when results are
mailed out.

Thanks!

[Paweł Hajdan Jr., 2011-03-26 10:55:12]

Excellent, set_command_execution_timeout is removed!

Now let's just polish it a little bit more. Feel free to ping me by e-mail, irc
or chat if some of the comments seem unclear or you're not sure how to implement
them.

http://codereview.chromium.org/6685099/diff/15001/base/test/test_timeouts.cc
File base/test/test_timeouts.cc (left):

http://codereview.chromium.org/6685099/diff/15001/base/test/test_timeouts.cc#...
base/test/test_timeouts.cc:86:
InitializeTimeout(switches::kUiTestCommandExecutionTimeout,
Oh, please also remove kUiTestCommandExecutionTimeout from
base/test/test_switches.

http://codereview.chromium.org/6685099/diff/15001/chrome/test/automation/auto...
File chrome/test/automation/automation_json_requests.h (right):

http://codereview.chromium.org/6685099/diff/15001/chrome/test/automation/auto...
chrome/test/automation/automation_json_requests.h:43: bool
SendAutomationJSONRequest(AutomationMessageSender* sender,
nit: SendAutomationJSONRequest seems to be used in less than ~10 places. Could
you make the version with timeout argument mandatory (i.e. remove the version
above), and use some timeout from TestTimeouts in the callers?

http://codereview.chromium.org/6685099/diff/15001/chrome/test/automation/auto...
File chrome/test/automation/automation_proxy.h (right):

http://codereview.chromium.org/6685099/diff/15001/chrome/test/automation/auto...
chrome/test/automation/automation_proxy.h:223: int timeout) WARN_UNUSED_RESULT;
nit: Similarly here, please remove the version that doesn't have the |timeout|
parameter.

http://codereview.chromium.org/6685099/diff/15001/chrome/test/automation/auto...
chrome/test/automation/automation_proxy.h:238: virtual bool Send(IPC::Message*
message, int timeout_ms) WARN_UNUSED_RESULT;
Just in case you ask, IMHO it's fine and even preferred to have both versions of
this method. Send is used much more often.

http://codereview.chromium.org/6685099/diff/15001/chrome/test/automation/brow...
File chrome/test/automation/browser_proxy.h (right):

http://codereview.chromium.org/6685099/diff/15001/chrome/test/automation/brow...
chrome/test/automation/browser_proxy.h:228: bool SendJSONRequest(const
std::string& request,
nit: Please remove the version that doesn't take a timeout.

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

http://codereview.chromium.org/6685099/diff/15001/chrome/test/functional/down...
chrome/test/functional/downloads.py:220:
self.WaitForAllDownloadsToComplete(timeout=240000);
Actually you want to get this value from TestTimeouts and not hardcode it.

http://codereview.chromium.org/6685099/diff/15001/chrome/test/functional/down...
chrome/test/functional/downloads.py:364:
self.WaitForAllDownloadsToComplete(timeout=120000);
Same here.

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

http://codereview.chromium.org/6685099/diff/15001/chrome/test/pyautolib/pyaut...
chrome/test/pyautolib/pyauto.py:481: timeout = self.action_max_timeout_ms()
Similarly to action_max_timeout, you can add accessors for other timeout values
from TestTimeouts to get them in Python scripts.

[Huyen, 2011-03-29 00:11:39]

Thanks for the review. I've address all comments. Please take a look when you
have time.

http://codereview.chromium.org/6685099/diff/15001/base/test/test_timeouts.cc
File base/test/test_timeouts.cc (left):

http://codereview.chromium.org/6685099/diff/15001/base/test/test_timeouts.cc#...
base/test/test_timeouts.cc:86:
InitializeTimeout(switches::kUiTestCommandExecutionTimeout,
On 2011/03/26 10:55:12, Paweł Hajdan Jr. wrote:
> Oh, please also remove kUiTestCommandExecutionTimeout from
> base/test/test_switches.

Done.

http://codereview.chromium.org/6685099/diff/15001/chrome/test/automation/auto...
File chrome/test/automation/automation_json_requests.h (right):

http://codereview.chromium.org/6685099/diff/15001/chrome/test/automation/auto...
chrome/test/automation/automation_json_requests.h:43: bool
SendAutomationJSONRequest(AutomationMessageSender* sender,
On 2011/03/26 10:55:12, Paweł Hajdan Jr. wrote:
> nit: SendAutomationJSONRequest seems to be used in less than ~10 places. Could
> you make the version with timeout argument mandatory (i.e. remove the version
> above), and use some timeout from TestTimeouts in the callers?

Done.

http://codereview.chromium.org/6685099/diff/15001/chrome/test/automation/auto...
File chrome/test/automation/automation_proxy.h (right):

http://codereview.chromium.org/6685099/diff/15001/chrome/test/automation/auto...
chrome/test/automation/automation_proxy.h:223: int timeout) WARN_UNUSED_RESULT;
On 2011/03/26 10:55:12, Paweł Hajdan Jr. wrote:
> nit: Similarly here, please remove the version that doesn't have the |timeout|
> parameter.

Done.

http://codereview.chromium.org/6685099/diff/15001/chrome/test/automation/brow...
File chrome/test/automation/browser_proxy.h (right):

http://codereview.chromium.org/6685099/diff/15001/chrome/test/automation/brow...
chrome/test/automation/browser_proxy.h:228: bool SendJSONRequest(const
std::string& request,
On 2011/03/26 10:55:12, Paweł Hajdan Jr. wrote:
> nit: Please remove the version that doesn't take a timeout.

Done.

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

http://codereview.chromium.org/6685099/diff/15001/chrome/test/functional/down...
chrome/test/functional/downloads.py:220:
self.WaitForAllDownloadsToComplete(timeout=240000);
On 2011/03/26 10:55:12, Paweł Hajdan Jr. wrote:
> Actually you want to get this value from TestTimeouts and not hardcode it.

Done.

http://codereview.chromium.org/6685099/diff/15001/chrome/test/functional/down...
chrome/test/functional/downloads.py:364:
self.WaitForAllDownloadsToComplete(timeout=120000);
On 2011/03/26 10:55:12, Paweł Hajdan Jr. wrote:
> Same here.

Done.

[Paweł Hajdan Jr., 2011-03-29 18:57:54]

LGTM with comments.

Thank you, this change fixes one of the really ugly hacks in the automation
system.

http://codereview.chromium.org/6685099/diff/23001/base/test/test_switches.cc
File base/test/test_switches.cc (left):

http://codereview.chromium.org/6685099/diff/23001/base/test/test_switches.cc#...
base/test/test_switches.cc:16: const char
switches::kUiTestCommandExecutionTimeout[] = "ui-test-timeout";
Please also remove kUiTestCommandExecutionTimeout from the header file.

http://codereview.chromium.org/6685099/diff/23001/chrome/test/automation/auto...
File chrome/test/automation/automation_json_requests.h (right):

http://codereview.chromium.org/6685099/diff/23001/chrome/test/automation/auto...
chrome/test/automation/automation_json_requests.h:42: int timeout)
WARN_UNUSED_RESULT;
nit: Change timeout to timeout_ms (in all places where this CL adds a "timeout"
parameter).

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

http://codereview.chromium.org/6685099/diff/23001/chrome/test/functional/down...
chrome/test/functional/downloads.py:218: # Waiting for big file to download
might exceed automation timeout.
nit: IMHO those two comment lines can now be removed, but it's really up to you
and Nirnimesh what to do with them (same below).

[Nirnimesh, 2011-03-29 20:27:28]

This is looking great. I've added my comments.
Thanks for doing this.

http://codereview.chromium.org/6685099/diff/23001/chrome/test/automation/auto...
File chrome/test/automation/automation_proxy.cc (right):

http://codereview.chromium.org/6685099/diff/23001/chrome/test/automation/auto...
chrome/test/automation/automation_proxy.cc:453:
command_execution_timeout_.InMilliseconds());
still command_execution_timeout_?
or action_max_timeout_?

http://codereview.chromium.org/6685099/diff/23001/chrome/test/automation/auto...
chrome/test/automation/automation_proxy.cc:564: if
(!SendAutomationJSONRequest(this, request, response, &result, timeout))
all out params (response, result) should go *after* all in params (request,
timeout). Please change the SendAutomationJSONRequest signature accordingly

http://codereview.chromium.org/6685099/diff/23001/chrome/test/automation/auto...
File chrome/test/automation/automation_proxy.h (left):

http://codereview.chromium.org/6685099/diff/23001/chrome/test/automation/auto...
chrome/test/automation/automation_proxy.h:247: int
command_execution_timeout_ms() const {
Please add corresponding getter/setter for action_max_timeout_ms.

We sometimes need to temporarily change these from pyauto.

http://codereview.chromium.org/6685099/diff/23001/chrome/test/automation/brow...
File chrome/test/automation/browser_proxy.h (right):

http://codereview.chromium.org/6685099/diff/23001/chrome/test/automation/brow...
chrome/test/automation/browser_proxy.h:227: int timeout) WARN_UNUSED_RESULT;
timeout should go before response

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

http://codereview.chromium.org/6685099/diff/23001/chrome/test/functional/down...
chrome/test/functional/downloads.py:218: # Waiting for big file to download
might exceed automation timeout.
On 2011/03/29 18:57:54, Paweł Hajdan Jr. wrote:
> nit: IMHO those two comment lines can now be removed, but it's really up to
you
> and Nirnimesh what to do with them (same below).
Yes, it'd be fine to remove these comments

http://codereview.chromium.org/6685099/diff/23001/chrome/test/functional/down...
chrome/test/functional/downloads.py:220:
self.WaitForAllDownloadsToComplete(timeout=self.large_test_timeout_ms());
What's the value of large_test_timeout_ms?

http://codereview.chromium.org/6685099/diff/23001/chrome/test/functional/test...
File chrome/test/functional/test_utils.py (left):

http://codereview.chromium.org/6685099/diff/23001/chrome/test/functional/test...
chrome/test/functional/test_utils.py:222: def CallFunctionWithNewTimeout(self,
new_timeout, function):
Please let this be. And fix CmdExecutionTimeoutChanger to use action_max_timeout

http://codereview.chromium.org/6685099/diff/23001/chrome/test/pyautolib/pyaut...
File chrome/test/pyautolib/pyauto.py (left):

http://codereview.chromium.org/6685099/diff/23001/chrome/test/pyautolib/pyaut...
chrome/test/pyautolib/pyauto.py:425: class CmdExecutionTimeoutChanger(object):
Please let this be. Just change it to use action_timeout_ms

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

http://codereview.chromium.org/6685099/diff/23001/chrome/test/pyautolib/pyaut...
chrome/test/pyautolib/pyauto.py:426: def _GetResultFromJSONRequest(self,
cmd_dict, windex=0):
Since the _GetResultFromJSONRequest below has a default timeout arg, do we still
need this version? Remove it?

http://codereview.chromium.org/6685099/diff/23001/chrome/test/pyautolib/pyaut...
chrome/test/pyautolib/pyauto.py:482: ret_dict =
json.loads(self._SendJSONRequest(
move self._SendJSONRequest to the next line.
It'll look prettier

http://codereview.chromium.org/6685099/diff/23001/chrome/test/pyautolib/pyaut...
chrome/test/pyautolib/pyauto.py:766: def WaitForAllDownloadsToComplete(self,
windex=0):
The WaitForAllDownloadsToComplete below has a default timeout param, so this
version might not be needed.

http://codereview.chromium.org/6685099/diff/23001/chrome/test/pyautolib/pyaut...
chrome/test/pyautolib/pyauto.py:776: if timeout == -1:  # Default
Why not just pass timeout ( = -1 ).
_GetResultFromJSONRequest will replace timeout=-1 with
timeout=action_max_timeout_ms() anyway

http://codereview.chromium.org/6685099/diff/23001/chrome/test/pyautolib/pyaut...
File chrome/test/pyautolib/pyautolib.h (right):

http://codereview.chromium.org/6685099/diff/23001/chrome/test/pyautolib/pyaut...
chrome/test/pyautolib/pyautolib.h:170: std::string _SendJSONRequest(int
window_index, const std::string& request);
Do we need this version anymore?

http://codereview.chromium.org/6685099/diff/23001/chrome/test/pyautolib/pyaut...
File chrome/test/pyautolib/pyautolib.i (left):

http://codereview.chromium.org/6685099/diff/23001/chrome/test/pyautolib/pyaut...
chrome/test/pyautolib/pyautolib.i:197: %feature("docstring", "Get the timeout
(in milli secs) for an automation "
Please retain these. Just replace command_execution_timout_ms with
action_max_timeout_ms

http://codereview.chromium.org/6685099/diff/23001/chrome/test/pyautolib/pyaut...
File chrome/test/pyautolib/pyautolib.i (right):

http://codereview.chromium.org/6685099/diff/23001/chrome/test/pyautolib/pyaut...
chrome/test/pyautolib/pyautolib.i:374: "Returns a JSON dict as a response.  "
Add some description about the timeout in the docstring

http://codereview.chromium.org/6685099/diff/23001/chrome/test/ui/ui_test.cc
File chrome/test/ui/ui_test.cc (left):

http://codereview.chromium.org/6685099/diff/23001/chrome/test/ui/ui_test.cc#o...
chrome/test/ui/ui_test.cc:162: // TODO(phajdan.jr): get rid of
set_command_execution_timeout_ms.
We'll need this

[Paweł Hajdan Jr., 2011-03-30 07:15:27]

http://codereview.chromium.org/6685099/diff/23001/chrome/test/ui/ui_test.cc
File chrome/test/ui/ui_test.cc (left):

http://codereview.chromium.org/6685099/diff/23001/chrome/test/ui/ui_test.cc#o...
chrome/test/ui/ui_test.cc:162: // TODO(phajdan.jr): get rid of
set_command_execution_timeout_ms.
On 2011/03/29 20:27:28, Nirnimesh wrote:
> We'll need this

Well, the entire point of this change is to get rid of it. Could you explain in
more detail?

[Nirnimesh, 2011-03-30 10:26:29]

http://codereview.chromium.org/6685099/diff/23001/chrome/test/ui/ui_test.cc
File chrome/test/ui/ui_test.cc (left):

http://codereview.chromium.org/6685099/diff/23001/chrome/test/ui/ui_test.cc#o...
chrome/test/ui/ui_test.cc:162: // TODO(phajdan.jr): get rid of
set_command_execution_timeout_ms.
On 2011/03/30 07:15:27, Paweł Hajdan Jr. wrote:
> On 2011/03/29 20:27:28, Nirnimesh wrote:
> > We'll need this
> 
> Well, the entire point of this change is to get rid of it. Could you explain
in
> more detail?

I thought this CL was about getting rid of command_execution_timeout.

We need this for situations where we know that 25 secs is just not going to cut
it. Example: wifi network selection use cases on chromeos; wifi features have a
timeout of 40 secs, and it makes sense for the automation to have a timeout
greater than that.

By using CmdExecutionTimeoutChanger (in pyauto.py) I ensure that once changed,
this timeout will be reset back to its original value.

[Paweł Hajdan Jr., 2011-03-30 16:36:38]

http://codereview.chromium.org/6685099/diff/23001/chrome/test/ui/ui_test.cc
File chrome/test/ui/ui_test.cc (left):

http://codereview.chromium.org/6685099/diff/23001/chrome/test/ui/ui_test.cc#o...
chrome/test/ui/ui_test.cc:162: // TODO(phajdan.jr): get rid of
set_command_execution_timeout_ms.
On 2011/03/30 10:26:30, Nirnimesh wrote:
> We need this for situations where we know that 25 secs is just not going to
cut
> it. Example: wifi network selection use cases on chromeos; wifi features have
a
> timeout of 40 secs, and it makes sense for the automation to have a timeout
> greater than that.
> 
> By using CmdExecutionTimeoutChanger (in pyauto.py) I ensure that once changed,
> this timeout will be reset back to its original value.

In this situation, a timeout_ms parameter should be added to the automation call
instead of having a somewhat global CmdExecutionTimeoutChanger. How does that
sound to you?

In other words, see how the downloads case was handled: instead of
test_utils.CallFunctionWithNewTimeout(..., self.WaitForAllDownloadsToComplete)
we call
self.WaitForAllDownloadsToComplete(timeout=self.large_test_timeout_ms());

[Nirnimesh, 2011-03-30 18:43:06]

http://codereview.chromium.org/6685099/diff/23001/chrome/test/ui/ui_test.cc
File chrome/test/ui/ui_test.cc (left):

http://codereview.chromium.org/6685099/diff/23001/chrome/test/ui/ui_test.cc#o...
chrome/test/ui/ui_test.cc:162: // TODO(phajdan.jr): get rid of
set_command_execution_timeout_ms.
On 2011/03/30 16:36:39, Paweł Hajdan Jr. wrote:
> On 2011/03/30 10:26:30, Nirnimesh wrote:
> > We need this for situations where we know that 25 secs is just not going to
> cut
> > it. Example: wifi network selection use cases on chromeos; wifi features
have
> a
> > timeout of 40 secs, and it makes sense for the automation to have a timeout
> > greater than that.
> > 
> > By using CmdExecutionTimeoutChanger (in pyauto.py) I ensure that once
changed,
> > this timeout will be reset back to its original value.
> 
> In this situation, a timeout_ms parameter should be added to the automation
call
> instead of having a somewhat global CmdExecutionTimeoutChanger. How does that
> sound to you?

What you're suggesting would have been fine if there wasn't a concept of a
global timeout to begin with. But there is, and subsequent automation calls
depend on it. I just need a way to reliably change & undo it.
If this CL adds code for adding timeout param to all automation calls, I'd cave,
but that's a much larger undertaking, and besides it requires that all
automation calls individually pass the timeout param, which is kludgy.

Consider the following 2 code snippets:

changer = CmdExecutionTimeoutChanger(4 mins)
automation_call_1()
automation_call_2()
automation_call_3()
del changer


automation_call_1(timeout=self.large_test_timeout_ms())
automation_call_2(timeout=self.large_test_timeout_ms())
automation_call_2(timeout=self.large_test_timeout_ms())


The former appears cleaner to me because not each automation call has to pass
the timeout param. You change it and then later undo it when you're done.

> 
> In other words, see how the downloads case was handled: instead of
> test_utils.CallFunctionWithNewTimeout(..., self.WaitForAllDownloadsToComplete)
> we call
> self.WaitForAllDownloadsToComplete(timeout=self.large_test_timeout_ms());

This is nice.

[Huyen, 2011-03-31 02:42:55]

http://codereview.chromium.org/6685099/diff/23001/base/test/test_switches.cc
File base/test/test_switches.cc (left):

http://codereview.chromium.org/6685099/diff/23001/base/test/test_switches.cc#...
base/test/test_switches.cc:16: const char
switches::kUiTestCommandExecutionTimeout[] = "ui-test-timeout";
On 2011/03/29 18:57:54, Paweł Hajdan Jr. wrote:
> Please also remove kUiTestCommandExecutionTimeout from the header file.

Done.

http://codereview.chromium.org/6685099/diff/23001/chrome/test/automation/auto...
File chrome/test/automation/automation_json_requests.h (right):

http://codereview.chromium.org/6685099/diff/23001/chrome/test/automation/auto...
chrome/test/automation/automation_json_requests.h:42: int timeout)
WARN_UNUSED_RESULT;
On 2011/03/29 18:57:54, Paweł Hajdan Jr. wrote:
> nit: Change timeout to timeout_ms (in all places where this CL adds a
"timeout"
> parameter).

Done.

http://codereview.chromium.org/6685099/diff/23001/chrome/test/automation/auto...
File chrome/test/automation/automation_proxy.cc (right):

http://codereview.chromium.org/6685099/diff/23001/chrome/test/automation/auto...
chrome/test/automation/automation_proxy.cc:453:
command_execution_timeout_.InMilliseconds());
On 2011/03/29 20:27:28, Nirnimesh wrote:
> still command_execution_timeout_?
> or action_max_timeout_
Done. Changed it to action_timeout_ to make it consistent with elsewhere.

http://codereview.chromium.org/6685099/diff/23001/chrome/test/automation/auto...
chrome/test/automation/automation_proxy.cc:564: if
(!SendAutomationJSONRequest(this, request, response, &result, timeout))
On 2011/03/29 20:27:28, Nirnimesh wrote:
> all out params (response, result) should go *after* all in params (request,
> timeout). Please change the SendAutomationJSONRequest signature accordingly

Done.

http://codereview.chromium.org/6685099/diff/23001/chrome/test/automation/auto...
File chrome/test/automation/automation_proxy.h (left):

http://codereview.chromium.org/6685099/diff/23001/chrome/test/automation/auto...
chrome/test/automation/automation_proxy.h:247: int
command_execution_timeout_ms() const {
On 2011/03/29 20:27:28, Nirnimesh wrote:
> Please add corresponding getter/setter for action_max_timeout_ms.
> 
> We sometimes need to temporarily change these from pyauto.

Done.

http://codereview.chromium.org/6685099/diff/23001/chrome/test/automation/brow...
File chrome/test/automation/browser_proxy.h (right):

http://codereview.chromium.org/6685099/diff/23001/chrome/test/automation/brow...
chrome/test/automation/browser_proxy.h:227: int timeout) WARN_UNUSED_RESULT;
On 2011/03/29 20:27:28, Nirnimesh wrote:
> timeout should go before response

Done.

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

http://codereview.chromium.org/6685099/diff/23001/chrome/test/functional/down...
chrome/test/functional/downloads.py:218: # Waiting for big file to download
might exceed automation timeout.
On 2011/03/29 18:57:54, Paweł Hajdan Jr. wrote:
> nit: IMHO those two comment lines can now be removed, but it's really up to
you
> and Nirnimesh what to do with them (same below).

Done.

http://codereview.chromium.org/6685099/diff/23001/chrome/test/functional/down...
chrome/test/functional/downloads.py:220:
self.WaitForAllDownloadsToComplete(timeout=self.large_test_timeout_ms());
On 2011/03/29 20:27:28, Nirnimesh wrote:
> What's the value of large_test_timeout_ms?

3 minutes. Running ui_tests and downloads.py this timeout seems to be
sufficient.

http://codereview.chromium.org/6685099/diff/23001/chrome/test/functional/test...
File chrome/test/functional/test_utils.py (left):

http://codereview.chromium.org/6685099/diff/23001/chrome/test/functional/test...
chrome/test/functional/test_utils.py:222: def CallFunctionWithNewTimeout(self,
new_timeout, function):
On 2011/03/29 20:27:28, Nirnimesh wrote:
> Please let this be. And fix CmdExecutionTimeoutChanger to use
action_max_timeout

Done. I also changed CmdExecutionTimeoutChanger to ActionTimeoutChanger to be
consistent.

http://codereview.chromium.org/6685099/diff/23001/chrome/test/pyautolib/pyaut...
File chrome/test/pyautolib/pyauto.py (left):

http://codereview.chromium.org/6685099/diff/23001/chrome/test/pyautolib/pyaut...
chrome/test/pyautolib/pyauto.py:425: class CmdExecutionTimeoutChanger(object):
On 2011/03/29 20:27:28, Nirnimesh wrote:
> Please let this be. Just change it to use action_timeout_ms

Done. Also change class name to ActionTimeoutChanger.

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

http://codereview.chromium.org/6685099/diff/23001/chrome/test/pyautolib/pyaut...
chrome/test/pyautolib/pyauto.py:426: def _GetResultFromJSONRequest(self,
cmd_dict, windex=0):
On 2011/03/29 20:27:28, Nirnimesh wrote:
> Since the _GetResultFromJSONRequest below has a default timeout arg, do we
still
> need this version? Remove it?
Good catch. Nope, removed.

http://codereview.chromium.org/6685099/diff/23001/chrome/test/pyautolib/pyaut...
chrome/test/pyautolib/pyauto.py:482: ret_dict =
json.loads(self._SendJSONRequest(
On 2011/03/29 20:27:28, Nirnimesh wrote:
> move self._SendJSONRequest to the next line.
> It'll look prettier

Done.

http://codereview.chromium.org/6685099/diff/23001/chrome/test/pyautolib/pyaut...
chrome/test/pyautolib/pyauto.py:766: def WaitForAllDownloadsToComplete(self,
windex=0):
On 2011/03/29 20:27:28, Nirnimesh wrote:
> The WaitForAllDownloadsToComplete below has a default timeout param, so this
> version might not be needed.

Done.

http://codereview.chromium.org/6685099/diff/23001/chrome/test/pyautolib/pyaut...
chrome/test/pyautolib/pyauto.py:776: if timeout == -1:  # Default
On 2011/03/29 20:27:28, Nirnimesh wrote:
> Why not just pass timeout ( = -1 ).
> _GetResultFromJSONRequest will replace timeout=-1 with
> timeout=action_max_timeout_ms() anyway

Done.

http://codereview.chromium.org/6685099/diff/23001/chrome/test/pyautolib/pyaut...
File chrome/test/pyautolib/pyautolib.h (right):

http://codereview.chromium.org/6685099/diff/23001/chrome/test/pyautolib/pyaut...
chrome/test/pyautolib/pyautolib.h:170: std::string _SendJSONRequest(int
window_index, const std::string& request);
On 2011/03/29 20:27:28, Nirnimesh wrote:
> Do we need this version anymore?

Removed and used one with timeout.

http://codereview.chromium.org/6685099/diff/23001/chrome/test/pyautolib/pyaut...
File chrome/test/pyautolib/pyautolib.i (left):

http://codereview.chromium.org/6685099/diff/23001/chrome/test/pyautolib/pyaut...
chrome/test/pyautolib/pyautolib.i:197: %feature("docstring", "Get the timeout
(in milli secs) for an automation "
On 2011/03/29 20:27:28, Nirnimesh wrote:
> Please retain these. Just replace command_execution_timout_ms with
> action_max_timeout_ms

Done. This corresponds to AutomationProx::action_timeout_ so it I'm using
action_timeout_ms() and set_action_timeout_ms().

http://codereview.chromium.org/6685099/diff/23001/chrome/test/pyautolib/pyaut...
File chrome/test/pyautolib/pyautolib.i (right):

http://codereview.chromium.org/6685099/diff/23001/chrome/test/pyautolib/pyaut...
chrome/test/pyautolib/pyautolib.i:374: "Returns a JSON dict as a response.  "
On 2011/03/29 20:27:28, Nirnimesh wrote:
> Add some description about the timeout in the docstring

Done.

http://codereview.chromium.org/6685099/diff/23001/chrome/test/ui/ui_test.cc
File chrome/test/ui/ui_test.cc (left):

http://codereview.chromium.org/6685099/diff/23001/chrome/test/ui/ui_test.cc#o...
chrome/test/ui/ui_test.cc:162: // TODO(phajdan.jr): get rid of
set_command_execution_timeout_ms.
On 2011/03/29 20:27:28, Nirnimesh wrote:
> We'll need this

Done.

[Nirnimesh, 2011-03-31 18:37:55]

LGTM.

Please use tryservers and verify pyauto tests on your machine before committing.

http://codereview.chromium.org/6685099/diff/34001/chrome/test/automation/brow...
File chrome/test/automation/browser_proxy.cc (right):

http://codereview.chromium.org/6685099/diff/34001/chrome/test/automation/brow...
chrome/test/automation/browser_proxy.cc:604:
TestTimeouts::action_max_timeout_ms(), &json_response)) {
the indentation is weird. Please either indent by 4 spaces wrt ( on previous
line, or align everything under kJSONCommand

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

http://codereview.chromium.org/6685099/diff/34001/chrome/test/pyautolib/pyaut...
chrome/test/pyautolib/pyauto.py:469: timeout: request timeout
add "(in milli-secs)"

[Huyen, 2011-03-31 18:52:19]

http://codereview.chromium.org/6685099/diff/34001/chrome/test/automation/brow...
File chrome/test/automation/browser_proxy.cc (right):

http://codereview.chromium.org/6685099/diff/34001/chrome/test/automation/brow...
chrome/test/automation/browser_proxy.cc:604:
TestTimeouts::action_max_timeout_ms(), &json_response)) {
On 2011/03/31 18:37:56, Nirnimesh wrote:
> the indentation is weird. Please either indent by 4 spaces wrt ( on previous
> line, or align everything under kJSONCommand

Done.

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

http://codereview.chromium.org/6685099/diff/34001/chrome/test/pyautolib/pyaut...
chrome/test/pyautolib/pyauto.py:469: timeout: request timeout
On 2011/03/31 18:37:56, Nirnimesh wrote:
> add "(in milli-secs)"

Done.

[Paweł Hajdan Jr., 2011-03-31 19:04:19]

I think this will need a few changes before it can be committed. I'll post more
detailed comments in a short while.

[Paweł Hajdan Jr., 2011-03-31 20:11:08]

Just one remaining issue.

http://codereview.chromium.org/6685099/diff/23001/chrome/test/ui/ui_test.cc
File chrome/test/ui/ui_test.cc (left):

http://codereview.chromium.org/6685099/diff/23001/chrome/test/ui/ui_test.cc#o...
chrome/test/ui/ui_test.cc:162: // TODO(phajdan.jr): get rid of
set_command_execution_timeout_ms.
On 2011/03/30 18:43:06, Nirnimesh wrote:
> What you're suggesting would have been fine if there wasn't a concept of a
> global timeout to begin with.

Well, set_command_execution_timeout was a hack since the very beginning (I
remember commenting on it when it was added).

> If this CL adds code for adding timeout param to all automation calls, I'd
cave,
> but that's a much larger undertaking, and besides it requires that all
> automation calls individually pass the timeout param, which is kludgy.

I think I can see your point, but please note the following:

1) Most of the calls can use the default value, which means no changes.
2) I don't think passing the timeout parameter is kludgy. As long as the timeout
values are not hardcoded and come from TestTimeouts, everything should be fine.

> Consider the following 2 code snippets:
> 
> changer = CmdExecutionTimeoutChanger(4 mins)
> automation_call_1()
> automation_call_2()
> automation_call_3()
> del changer
> 
> 
> automation_call_1(timeout=self.large_test_timeout_ms())
> automation_call_2(timeout=self.large_test_timeout_ms())
> automation_call_2(timeout=self.large_test_timeout_ms())
> 
> 
> The former appears cleaner to me because not each automation call has to pass
> the timeout param. You change it and then later undo it when you're done.

My taste is slightly different, and for me changing something temporarily and
switching it back seems ugly.

A possible solution, if you'd like to keep the TimeoutChanger, would be to keep
it only at the pyauto level. In other words, the set_command/action_timeout_ms
methods should be removed as commented here, but nothing should prevent you from
making the TimeoutChanger temporarily change the default timeout pyauto will use
when sending messages (I think there is just one place all pyauto code calls to
send a message to the browser under test).

If that seems rather unclear, please let me know, I can try to explain it better
and more precisely. Generally I'm mostly concerned about the shared automation
code in C++, and leave the Python code up to you.

http://codereview.chromium.org/6685099/diff/34016/chrome/test/automation/auto...
File chrome/test/automation/automation_json_requests.cc (right):

http://codereview.chromium.org/6685099/diff/34016/chrome/test/automation/auto...
chrome/test/automation/automation_json_requests.cc:26:
TestTimeouts::action_max_timeout_ms(), &reply, &success))
nit: I think this should be indented 4 more spaces.

http://codereview.chromium.org/6685099/diff/34016/chrome/test/automation/auto...
File chrome/test/automation/automation_proxy.h (right):

http://codereview.chromium.org/6685099/diff/34016/chrome/test/automation/auto...
chrome/test/automation/automation_proxy.h:255: void set_action_timeout_ms(int
timeout_ms) {
Well, I think this should also be removed.

http://codereview.chromium.org/6685099/diff/34016/chrome/test/automation/brow...
File chrome/test/automation/browser_proxy.cc (right):

http://codereview.chromium.org/6685099/diff/34016/chrome/test/automation/brow...
chrome/test/automation/browser_proxy.cc:604:
TestTimeouts::action_max_timeout_ms(), &json_response)) {
nit: I think this should be indented 4 more spaces.

http://codereview.chromium.org/6685099/diff/34016/chrome/test/ui/ui_test.h
File chrome/test/ui/ui_test.h (right):

http://codereview.chromium.org/6685099/diff/34016/chrome/test/ui/ui_test.h#ne...
chrome/test/ui/ui_test.h:65: void set_action_timeout_ms(int timeout);
This should be removed too.

[Nirnimesh, 2011-03-31 20:20:26]

http://codereview.chromium.org/6685099/diff/23001/chrome/test/ui/ui_test.cc
File chrome/test/ui/ui_test.cc (left):

http://codereview.chromium.org/6685099/diff/23001/chrome/test/ui/ui_test.cc#o...
chrome/test/ui/ui_test.cc:162: // TODO(phajdan.jr): get rid of
set_command_execution_timeout_ms.
 > A possible solution, if you'd like to keep the TimeoutChanger, would be to
keep
> it only at the pyauto level. In other words, the set_command/action_timeout_ms
> methods should be removed as commented here, but nothing should prevent you
from
> making the TimeoutChanger temporarily change the default timeout pyauto will
use
> when sending messages (I think there is just one place all pyauto code calls
to
> send a message to the browser under test).

This solution will work fine for all automation messages send on the json
automation channel. But not for automation calls that are swigged directly from
C++ -- they'd still use the default timeout.
I'd hate to have one timeout for the swigged automation calls and another
pyauto-controllable one for the json calls.

In general I do not quite agree with the premise that all timeout needs will be
met by the timeouts available in timeouts.h
For example, when running stress tests, especially on low-end machines, I expect
automation calls in general to take longer. In this case, changing incrementing
the default timeout globally seems better to me, as opposed to passing timeout
params for every automation call thereafter.

Note despite retaining the ability to change the timeout, this CL still has
merit in that it gets rid of command_execution_timeout_ms, from amongst the list
of similar looking timeouts.

[Nirnimesh, 2011-03-31 20:59:25]

Huyen,

Pawel and I chatted about this. We've resolved that PyAuto still needs to be
able to change the automation timeout, but the way it's implemented could be
through TestTimeouts (rather than through the automation/ui code) -- this will
help further refactorings Pawel has in the pipeline.

So, all timeout changes happen via setters in TestTimeouts. You'd need to add a
setter that changes action_timeout_ms_ (or is it action_max_timeout_ms?) and
make it available to pyauto (pyautolib.h/.cc/.i). AutomationProxy's ctor takes
timeout as an argument which it uses for all automation calls, so you'd need to
have 2 ctors -- one which is the current one (for chrome_frame to use), and
another one which does not take a timeout, but fetches from TestTimeouts for
every time it needs it, not just at initialization. You'd need to ensure that
when the timeout is changed, further automation calls respect it.

Thanks

[Paweł Hajdan Jr., 2011-04-01 06:57:20]

After thinking about it more, I think it'll be better to just land Patch Set 6
with indentation nit fixes (LGTM).

It seems to me that the TestTimeouts changes could result in even more review
round-trips, and that could be potentially frustrating to you.

Patch Set 6 is a step in right direction, and it makes things better. Let's land
it, and if you're interested in doing the rest of the cleanup, that's great (I
do appreciate your effort on this one), but it's up to you.

Now something more about my TestTimeouts ideas. Pushing set_..._timeout down to
TestTimeouts is still not ideal. I mentioned some additional changes to
TestTimeouts because Nirnimesh pointed out to me that it's currently impossible
to control the timeouts from command-line for pyauto.

I'm fine then with a variant of TestTimeouts::Initialize that pyauto could use
to pass timeout values in some other way (in fact I think that'd be a good
change), but the idea is to only do that _once_. Changing one of the timeouts
and then reverting it to the previous value doesn't seem good to me.

[Paweł Hajdan Jr., 2011-04-08 11:04:52]

LGTM with two nits, thank you for doing the cleanup.

http://codereview.chromium.org/6685099/diff/48004/chrome/test/automation/auto...
File chrome/test/automation/automation_json_requests.cc (right):

http://codereview.chromium.org/6685099/diff/48004/chrome/test/automation/auto...
chrome/test/automation/automation_json_requests.cc:26:
TestTimeouts::action_max_timeout_ms(), &reply, &success))
nit: +4 indentation here please as said before.

http://codereview.chromium.org/6685099/diff/48004/chrome/test/automation/brow...
File chrome/test/automation/browser_proxy.cc (right):

http://codereview.chromium.org/6685099/diff/48004/chrome/test/automation/brow...
chrome/test/automation/browser_proxy.cc:604:
TestTimeouts::action_max_timeout_ms(), &json_response)) {
nit: +4 indentation here too.

[Huyen, 2011-04-09 00:08:15]

http://codereview.chromium.org/6685099/diff/48004/chrome/test/automation/auto...
File chrome/test/automation/automation_json_requests.cc (right):

http://codereview.chromium.org/6685099/diff/48004/chrome/test/automation/auto...
chrome/test/automation/automation_json_requests.cc:26:
TestTimeouts::action_max_timeout_ms(), &reply, &success))
On 2011/04/08 11:04:53, Paweł Hajdan Jr. wrote:
> nit: +4 indentation here please as said before.

Done.

http://codereview.chromium.org/6685099/diff/48004/chrome/test/automation/brow...
File chrome/test/automation/browser_proxy.cc (right):

http://codereview.chromium.org/6685099/diff/48004/chrome/test/automation/brow...
chrome/test/automation/browser_proxy.cc:604:
TestTimeouts::action_max_timeout_ms(), &json_response)) {
On 2011/04/08 11:04:53, Paweł Hajdan Jr. wrote:
> nit: +4 indentation here too.

Done.

[Huyen, 2011-04-11 21:34:23]

Nirmimesh, Paweł,
There were some dependencies on chrome frame which required changing
chrome/chrome.gyp and chrome_frame/test/automation_client_mock.h.

Everything looks fine on the try servers. If you can guys have no more comments,
I will go ahead and commit this change.

Thanks!

[Nirnimesh, 2011-04-11 22:49:57]

LGTM still