Removing command_execution_timeout_ms in favor of action_max_timeout_ms.

chrome/test/ui/ui_test.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/test/ui/ui_test.h"
 
 #if defined(OS_POSIX)
 #include <signal.h>
 #include <sys/types.h>
 #endif
@@ skipping 141 matching lines @@
 
   std::wstring error_msg =
       L"Encountered an unexpected crash in the program during this test.";
   if (expected_crashes_ > 0 && actual_crashes == 0) {
     error_msg += L"  ";
     error_msg += kFailedNoCrashService;
   }
   EXPECT_EQ(expected_crashes_, actual_crashes) << error_msg;
 }
 
-// TODO(phajdan.jr): get rid of set_command_execution_timeout_ms.

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

We'll need this

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

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:30]

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:39]

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]

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.
This is nice.

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

Done.

[Paweł Hajdan Jr., 2011/03/31 20:11:09]

Well, set_command_execution_timeout was a hack since the very beginning (I
remember commenting on it when it was added).
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.
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.

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

 > A possible solution, if you'd like to keep the TimeoutChanger, would be to
keep
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.

-void UITestBase::set_command_execution_timeout_ms(int timeout) {
-  automation()->set_command_execution_timeout_ms(timeout);
-  VLOG(1) << "Automation command execution timeout set to " << timeout << " ms";
-}
-
 ProxyLauncher* UITestBase::CreateProxyLauncher() {
   return new AnonymousProxyLauncher(false);
 }
 
 bool UITestBase::ShouldFilterInet() {
   return true;
 }
 
 void UITestBase::SetLaunchSwitches() {
   // We need cookies on file:// for things like the page cycler.
@@ skipping 681 matching lines @@
     incorrect_state_count++;
   }
 
   LOG(INFO) << "Elapsed time: " << (base::Time::Now() - start).InSecondsF()
             << " seconds"
             << " call failed " << fail_count << " times"
             << " state was incorrect " << incorrect_state_count << " times";
   ADD_FAILURE() << "Timeout reached in " << __FUNCTION__;
   return false;
 }