create temp avd when running tests on multiple emulators

create temp avd when running tests on multiple emulators

BUG=137487
TEST=./build/android/run_tests.py -s ui_unittests -e 2



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

[Wei James(wistoch), 2012-07-16 10:03:23]

cannot share avd between multiple emulators as we need to use sd card etc.

[bulach, 2012-07-16 11:39:13]

oh, this is really nice, thanks James!

I have a few small suggestions below and nits below:

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

http://chromiumcodereview.appspot.com/10784011/diff/1/build/android/emulator....
build/android/emulator.py:116: def __init__(self, avd=None,
fast_and_loose=False):
I think there's only one caller for this. please, remove both default values for
this param!
(in general, adding a default param before an existing one would break the code:
existing callers say "Emulator(foo)", would be passing foo as avd, and not
fast_and_loose... so we'd have to add as the last param, but even better, it's
best to remove default values altogheter :)

also, probably best to name it as new_avd_name, the document saying:

avd_name: if set, will create a new temporary AVD.

http://chromiumcodereview.appspot.com/10784011/diff/1/build/android/emulator....
build/android/emulator.py:142: if avd != None:
nit: no need to compare, just "if avd:" should be enough.

http://chromiumcodereview.appspot.com/10784011/diff/1/build/android/emulator....
build/android/emulator.py:151: def CreateAVD(self, avd):
nit: add a _ prefix to indicate this is not public.

http://chromiumcodereview.appspot.com/10784011/diff/1/build/android/emulator....
build/android/emulator.py:162: popen = subprocess.Popen(args=avd_command,
nit: rather than popen, maybe name this as avd_process?

http://chromiumcodereview.appspot.com/10784011/diff/1/build/android/emulator....
build/android/emulator.py:165: stderr=subprocess.STDOUT)
nit: indent 163-165 with the ( from 162

http://chromiumcodereview.appspot.com/10784011/diff/1/build/android/emulator....
build/android/emulator.py:172: def DeleteAVD(self):
nit: _ prefix

http://chromiumcodereview.appspot.com/10784011/diff/1/build/android/emulator....
build/android/emulator.py:180: popen = subprocess.Popen(args=avd_command,
nit: avd_process?

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

http://chromiumcodereview.appspot.com/10784011/diff/1/build/android/run_tests...
build/android/run_tests.py:328: avd_name =  None if n == 0 else
'run_tests_avd_%d' % n
maybe clearer as:
avd_name = None
if n > 0:
  # Creates a temporary AVD for the extra emulators.
  avd_name = 'run_tests_avd_%d' % n

[Wei James(wistoch), 2012-07-16 13:04:37]

thanks for the comments. 

all issues fixed. thanks

http://codereview.chromium.org/10784011/diff/1/build/android/emulator.py
File build/android/emulator.py (right):

http://codereview.chromium.org/10784011/diff/1/build/android/emulator.py#newc...
build/android/emulator.py:116: def __init__(self, avd=None,
fast_and_loose=False):
On 2012/07/16 11:39:13, bulach wrote:
> I think there's only one caller for this. please, remove both default values
for
> this param!
> (in general, adding a default param before an existing one would break the
code:
> existing callers say "Emulator(foo)", would be passing foo as avd, and not
> fast_and_loose... so we'd have to add as the last param, but even better, it's
> best to remove default values altogheter :)
> 
> also, probably best to name it as new_avd_name, the document saying:
> 
> avd_name: if set, will create a new temporary AVD.

fixed. thanks

http://codereview.chromium.org/10784011/diff/1/build/android/emulator.py#newc...
build/android/emulator.py:142: if avd != None:
On 2012/07/16 11:39:13, bulach wrote:
> nit: no need to compare, just "if avd:" should be enough.

fixed. thanks

http://codereview.chromium.org/10784011/diff/1/build/android/emulator.py#newc...
build/android/emulator.py:151: def CreateAVD(self, avd):
On 2012/07/16 11:39:13, bulach wrote:
> nit: add a _ prefix to indicate this is not public.
fixed. thanks

http://codereview.chromium.org/10784011/diff/1/build/android/emulator.py#newc...
build/android/emulator.py:162: popen = subprocess.Popen(args=avd_command,
On 2012/07/16 11:39:13, bulach wrote:
> nit: rather than popen, maybe name this as avd_process?

fixed. thanks

http://codereview.chromium.org/10784011/diff/1/build/android/emulator.py#newc...
build/android/emulator.py:165: stderr=subprocess.STDOUT)
On 2012/07/16 11:39:13, bulach wrote:
> nit: indent 163-165 with the ( from 162

fixed. thanks

http://codereview.chromium.org/10784011/diff/1/build/android/emulator.py#newc...
build/android/emulator.py:172: def DeleteAVD(self):
On 2012/07/16 11:39:13, bulach wrote:
> nit: _ prefix

fixed. thanks

http://codereview.chromium.org/10784011/diff/1/build/android/emulator.py#newc...
build/android/emulator.py:180: popen = subprocess.Popen(args=avd_command,
On 2012/07/16 11:39:13, bulach wrote:
> nit: avd_process?

fixed. thanks

http://codereview.chromium.org/10784011/diff/1/build/android/run_tests.py
File build/android/run_tests.py (right):

http://codereview.chromium.org/10784011/diff/1/build/android/run_tests.py#new...
build/android/run_tests.py:328: avd_name =  None if n == 0 else
'run_tests_avd_%d' % n
On 2012/07/16 11:39:13, bulach wrote:
> maybe clearer as:
> avd_name = None
> if n > 0:
>   # Creates a temporary AVD for the extra emulators.
>   avd_name = 'run_tests_avd_%d' % n

fixed. thanks

[bulach, 2012-07-16 13:14:35]

lgtm, thanks! just some nits below.

http://codereview.chromium.org/10784011/diff/5/build/android/emulator.py
File build/android/emulator.py (right):

http://codereview.chromium.org/10784011/diff/5/build/android/emulator.py#newc...
build/android/emulator.py:169: self.avd = avd_name
nit: 169 is unnecessary.

http://codereview.chromium.org/10784011/diff/5/build/android/emulator.py#newc...
build/android/emulator.py:182: stderr=subprocess.STDOUT)
nit: probably best to add a
avd_process.wait() here?

[Wei James(wistoch), 2012-07-16 13:18:41]

fixed. thanks

http://codereview.chromium.org/10784011/diff/5/build/android/emulator.py
File build/android/emulator.py (right):

http://codereview.chromium.org/10784011/diff/5/build/android/emulator.py#newc...
build/android/emulator.py:169: self.avd = avd_name
On 2012/07/16 13:14:35, bulach wrote:
> nit: 169 is unnecessary.
fixed. thanks

http://codereview.chromium.org/10784011/diff/5/build/android/emulator.py#newc...
build/android/emulator.py:182: stderr=subprocess.STDOUT)
On 2012/07/16 13:14:35, bulach wrote:
> nit: probably best to add a
> avd_process.wait() here?

fixed. thanks

[commit-bot: I haz the power, 2012-07-16 14:00:29]

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

[commit-bot: I haz the power, 2012-07-16 15:22:59]

Change committed as 146804