restarting adb server before each testing start will cause conflict for multiple emulators

Restarting adb server before each testing start will cause conflict for multiple emulators

Sometimes there is conflict when several tests restart adb server at the same time.
Originally, restarting adb server is to fix the issue of connection lost with adb server when emulator booting, as this case already handled in AndroidCommand::WaitForSystemBootCompleted(), so it can be removed now. 

Also fix a typo.

BUG=
TEST=./build/android/run_tests.py -e -n 5

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

[Wei James(wistoch), 2012-07-17 08:05:03]

I have tested it for several times with all default unit tests and it works very
stable with 5 emulators. No any issue appears until now. 

For 10 emulators, as it will shutdown all emulators and then launch new
emulators for each test suite, sometimes it will hang, the system load is heavy.


is there any consideration to restart all emulators for each test suite? can I
launch all the emulators and run all test suites on the same emulators and then
shutdown them when all test suites finished? 

thanks

[bulach, 2012-07-17 10:26:08]

lgtm, thanks! just some nits and suggestions.

as for starting the emulator only once for all test suites: we don't fully wipe
real devices, so theoretically it should be fine to keep the emulators for all
test suites... 
but as mentioned, we're not really using emulators, so we'd appreciate your help
in testing such configuration.

http://chromiumcodereview.appspot.com/10781032/diff/1/build/android/pylib/and...
File build/android/pylib/android_commands.py (right):

http://chromiumcodereview.appspot.com/10781032/diff/1/build/android/pylib/and...
build/android/pylib/android_commands.py:206: 
nit: the styleguide requires two \n between top-levels, so please add one here,
then another one before KillAdbServer, and another one before StartAdbServer

http://chromiumcodereview.appspot.com/10781032/diff/1/build/android/run_tests.py
File build/android/run_tests.py (right):

http://chromiumcodereview.appspot.com/10781032/diff/1/build/android/run_tests...
build/android/run_tests.py:341: # which will break the connection between adb
server and emulator.
the comment seems to indicate this is needed per emulator.. perhaps something
like this in 337:
def WaitForEmulatorAndRestartAdb(emulator):
  emulator.ConfirmLaunch(True)
  android_commands.RestartAdbServer()
map(WaitForEmulatorAndRestartAdb, buildbot_emulators)

[John Grabowski, 2012-07-17 18:09:22]

http://chromiumcodereview.appspot.com/10781032/diff/1/build/android/emulator.py
File build/android/emulator.py (right):

http://chromiumcodereview.appspot.com/10781032/diff/1/build/android/emulator....
build/android/emulator.py:152: avd_command = [
add docstring. doc return values.

http://chromiumcodereview.appspot.com/10781032/diff/1/build/android/run_tests.py
File build/android/run_tests.py (right):

http://chromiumcodereview.appspot.com/10781032/diff/1/build/android/run_tests...
build/android/run_tests.py:341: # which will break the connection between adb
server and emulator.
On 2012/07/17 10:26:08, bulach wrote:
> the comment seems to indicate this is needed per emulator.. perhaps something
> like this in 337:
> def WaitForEmulatorAndRestartAdb(emulator):
>   emulator.ConfirmLaunch(True)
>   android_commands.RestartAdbServer()
> map(WaitForEmulatorAndRestartAdb, buildbot_emulators)

agree

[Wei James(wistoch), 2012-07-20 04:07:01]

jrg, bulach,

sorry for later response. 

As several bulach's patches landed, I have tested it with it for several times.
The test is more stable and I updated the patch to address this change and also
the issues in your comments. thanks

http://chromiumcodereview.appspot.com/10781032/diff/1/build/android/emulator.py
File build/android/emulator.py (right):

http://chromiumcodereview.appspot.com/10781032/diff/1/build/android/emulator....
build/android/emulator.py:152: avd_command = [
On 2012/07/17 18:09:22, John Grabowski wrote:
> add docstring. doc return values.

fixed. thanks

http://chromiumcodereview.appspot.com/10781032/diff/1/build/android/run_tests.py
File build/android/run_tests.py (right):

http://chromiumcodereview.appspot.com/10781032/diff/1/build/android/run_tests...
build/android/run_tests.py:341: # which will break the connection between adb
server and emulator.
On 2012/07/17 18:09:22, John Grabowski wrote:
> On 2012/07/17 10:26:08, bulach wrote:
> > the comment seems to indicate this is needed per emulator.. perhaps
something
> > like this in 337:
> > def WaitForEmulatorAndRestartAdb(emulator):
> >   emulator.ConfirmLaunch(True)
> >   android_commands.RestartAdbServer()
> > map(WaitForEmulatorAndRestartAdb, buildbot_emulators)
> 
> agree

after applying several patches of bulach, I run the tests for several times, it
is stable even without this one. 
As we already handle the connection lost issue in
AndroidCommand::WaitForSystemBootCompleted, I think removing it here is ok.

[bulach, 2012-07-20 11:28:51]

lgtm, thanks for testing it!

just one nit with the doc, we either need to update it or to actually test for
the result.

http://chromiumcodereview.appspot.com/10781032/diff/7001/build/android/emulat...
File build/android/emulator.py (right):

http://chromiumcodereview.appspot.com/10781032/diff/7001/build/android/emulat...
build/android/emulator.py:154: Return avd_name if succeed.
nit: hmm, looks like it's never checking for any result and always returning the
avd_name regardless?

[Wei James(wistoch), 2012-07-20 12:48:24]

all issues addressed. thanks

http://chromiumcodereview.appspot.com/10781032/diff/7001/build/android/emulat...
File build/android/emulator.py (right):

http://chromiumcodereview.appspot.com/10781032/diff/7001/build/android/emulat...
build/android/emulator.py:154: Return avd_name if succeed.
On 2012/07/20 11:28:51, bulach wrote:
> nit: hmm, looks like it's never checking for any result and always returning
the
> avd_name regardless?

fixed. thanks

[Wei James(wistoch), 2012-07-23 14:57:13]

On 2012/07/20 12:48:24, James Wei wrote:
> all issues addressed. thanks
> 
>
http://chromiumcodereview.appspot.com/10781032/diff/7001/build/android/emulat...
> File build/android/emulator.py (right):
> 
>
http://chromiumcodereview.appspot.com/10781032/diff/7001/build/android/emulat...
> build/android/emulator.py:154: Return avd_name if succeed.
> On 2012/07/20 11:28:51, bulach wrote:
> > nit: hmm, looks like it's never checking for any result and always returning
> the
> > avd_name regardless?
> 
> fixed. thanks

hi, bulach, 
all issues in your comments fixed, could you help to review the new patch? can I
land this patch? thanks

[John Grabowski, 2012-07-23 19:21:15]

LGTM

http://chromiumcodereview.appspot.com/10781032/diff/12001/build/android/emula...
File build/android/emulator.py (right):

http://chromiumcodereview.appspot.com/10781032/diff/12001/build/android/emula...
build/android/emulator.py:177: avd_command = [
If the test crashes (e.g. python error), do we "leak" AVD binaries in /tmp?

[Wei James(wistoch), 2012-07-24 00:58:03]

On 2012/07/23 19:21:15, John Grabowski wrote:
> LGTM
> 
>
http://chromiumcodereview.appspot.com/10781032/diff/12001/build/android/emula...
> File build/android/emulator.py (right):
> 
>
http://chromiumcodereview.appspot.com/10781032/diff/12001/build/android/emula...
> build/android/emulator.py:177: avd_command = [
> If the test crashes (e.g. python error), do we "leak" AVD binaries in /tmp?

yes. the AVD will not be deleted if test crashes due to python errors.   But it
will be overrided by future tests and deleted when test exits normally. 

I have not found a good way to resolve it.

[John Grabowski, 2012-07-24 01:00:53]

On 2012/07/24 00:58:03, James Wei wrote:
> On 2012/07/23 19:21:15, John Grabowski wrote:
> > LGTM
> > 
> >
>
http://chromiumcodereview.appspot.com/10781032/diff/12001/build/android/emula...
> > File build/android/emulator.py (right):
> > 
> >
>
http://chromiumcodereview.appspot.com/10781032/diff/12001/build/android/emula...
> > build/android/emulator.py:177: avd_command = [
> > If the test crashes (e.g. python error), do we "leak" AVD binaries in /tmp?
> 
> yes. the AVD will not be deleted if test crashes due to python errors.   But
it
> will be overrided by future tests and deleted when test exits normally. 
> 
> I have not found a good way to resolve it.

On startup look in temp dir.  If you see files with the characteristic name,
give warnings.  On clobber build, delete them all.

Or something.

[Wei James(wistoch), 2012-07-24 06:03:00]

On 2012/07/24 01:00:53, John Grabowski wrote:
> On 2012/07/24 00:58:03, James Wei wrote:
> > On 2012/07/23 19:21:15, John Grabowski wrote:
> > > LGTM
> > > 
> > >
> >
>
http://chromiumcodereview.appspot.com/10781032/diff/12001/build/android/emula...
> > > File build/android/emulator.py (right):
> > > 
> > >
> >
>
http://chromiumcodereview.appspot.com/10781032/diff/12001/build/android/emula...
> > > build/android/emulator.py:177: avd_command = [
> > > If the test crashes (e.g. python error), do we "leak" AVD binaries in
/tmp?
> > 
> > yes. the AVD will not be deleted if test crashes due to python errors.   But
> it
> > will be overrided by future tests and deleted when test exits normally. 
> > 
> > I have not found a good way to resolve it.
> 
> On startup look in temp dir.  If you see files with the characteristic name,
> give warnings.  On clobber build, delete them all.
> 
> Or something.

fixed with deleting all temp AVDs when test starts. thanks

[John Grabowski, 2012-07-24 18:06:56]

Still LGTM

[commit-bot: I haz the power, 2012-07-24 21:36:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/james.wei@intel.com/10781032/17001

[commit-bot: I haz the power, 2012-07-24 23:14:08]

Change committed as 148242