Android: further simplication for test runners.

Android: further simplication for test runners.

Overtime, there has been a few changes in the way we run:
- individual test suite versus all suites.
- single device versus sharded.
- debug info.

This patch simplifies the many different code paths and ensures
we'll have less breakages (and a more consistent way to add features).

BUG=
TEST=existing android tests (single device, multiple devices, emulator).


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

[bulach, 2012-07-13 18:08:53]

Hi everyone,

The main goal of this patch is to simplify the test runners: there were
different and inconsistent code paths for different options way too high up in
the call stack.

This patch simplifies it considerably, and then the options are only tested
deeper down where actually needed (so no more _RunATestSuite versus RunTest
versus etc..)

[John Grabowski, 2012-07-13 18:17:43]

LGTM

http://codereview.chromium.org/10777017/diff/4001/build/android/pylib/debug_i...
File build/android/pylib/debug_info.py (right):

http://codereview.chromium.org/10777017/diff/4001/build/android/pylib/debug_i...
build/android/pylib/debug_info.py:45: os.makedirs(self.log_dir)
cleanup when done?  No guarantee ZipAndCleanup() is called, so maybe
shutil.rmtree(log_src_dir) in dtor

http://codereview.chromium.org/10777017/diff/4001/build/android/pylib/debug_i...
build/android/pylib/debug_info.py:78: self._GetSignatureFromGTestFilter()[:64] +
'_' +
Seems a little fragile.
How about GetSignatureFromGTestFilter() (or similar mechanism) returns a tuple
and you pull out the nth item that you want?

[gone, 2012-07-14 00:33:36]

https://chromiumcodereview.appspot.com/10777017/diff/4001/build/android/pylib...
File build/android/pylib/debug_info.py (right):

https://chromiumcodereview.appspot.com/10777017/diff/4001/build/android/pylib...
build/android/pylib/debug_info.py:48: self.log_file_name = None
Doesn't 48 cancel out 46?

[bulach, 2012-07-16 08:51:16]

thanks all! addressed all comments, however a few of them were related to
improving documentation rather than changing code.. another quick look please?

http://codereview.chromium.org/10777017/diff/4001/build/android/pylib/debug_i...
File build/android/pylib/debug_info.py (right):

http://codereview.chromium.org/10777017/diff/4001/build/android/pylib/debug_i...
build/android/pylib/debug_info.py:45: os.makedirs(self.log_dir)
On 2012/07/13 18:17:44, John Grabowski wrote:
> cleanup when done?  No guarantee ZipAndCleanup() is called, so maybe
> shutil.rmtree(log_src_dir) in dtor

good point, but I think the only way to achieve this is by documentation: the
collection is done as a complete separate / asynchronous step as the (static)
ZipAndCleanup.. if we added a dtor of some sort in here, it'd happen prior to
ZipAndCleanup..

I improved the docstring, let me know if it's clearer.

http://codereview.chromium.org/10777017/diff/4001/build/android/pylib/debug_i...
build/android/pylib/debug_info.py:48: self.log_file_name = None
On 2012/07/14 00:33:36, dfalcantara wrote:
> Doesn't 48 cancel out 46?

Done.

http://codereview.chromium.org/10777017/diff/4001/build/android/pylib/debug_i...
build/android/pylib/debug_info.py:78: self._GetSignatureFromGTestFilter()[:64] +
'_' +
On 2012/07/13 18:17:44, John Grabowski wrote:
> Seems a little fragile.
> How about GetSignatureFromGTestFilter() (or similar mechanism) returns a tuple
> and you pull out the nth item that you want?

agreed. but again, not sure if picking the nth item would help much.. afaict,
this function is only used to generate a reasonable file name, right?
I moved this to _GetSignatureFromGTestFilter, and added a comment there
explaining why it needs trimming..

[gone, 2012-07-16 16:46:53]

Just a nit, but LGTM.

https://chromiumcodereview.appspot.com/10777017/diff/9001/build/android/pylib...
File build/android/pylib/debug_info.py (right):

https://chromiumcodereview.appspot.com/10777017/diff/9001/build/android/pylib...
build/android/pylib/debug_info.py:23: """A helper class to collect relate debug
information for a gtest.
nit: relate -> related

[bulach, 2012-07-16 17:16:45]

thanks!

http://codereview.chromium.org/10777017/diff/9001/build/android/pylib/debug_i...
File build/android/pylib/debug_info.py (right):

http://codereview.chromium.org/10777017/diff/9001/build/android/pylib/debug_i...
build/android/pylib/debug_info.py:23: """A helper class to collect relate debug
information for a gtest.
On 2012/07/16 16:46:53, dfalcantara wrote:
> nit: relate -> related

Done.

[commit-bot: I haz the power, 2012-07-17 10:17:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bulach@chromium.org/10777017/9002

[commit-bot: I haz the power, 2012-07-17 11:39:51]

Change committed as 146985

[nilesh, 2012-07-17 17:40:44]

On 2012/07/17 11:39:51, I haz the power (commit-bot) wrote:
> Change committed as 146985

Did this break the paths for test output
http://build.chromium.org/p/chromium.fyi/builders/Chromium%20Linux%20Android/...
?

[nilesh, 2012-07-17 17:41:42]

On 2012/07/17 17:40:44, nileshagrawal1 wrote:
> On 2012/07/17 11:39:51, I haz the power (commit-bot) wrote:
> > Change committed as 146985
> 
> Did this break the paths for test output
>
http://build.chromium.org/p/chromium.fyi/builders/Chromium%2520Linux%2520Andr...
> ?

Another try on the link:
http://build.chromium.org/p/chromium.fyi/builders/Chromium%20Linux%20Android/...

[nilesh, 2012-07-17 17:43:33]

On 2012/07/17 17:41:42, nileshagrawal1 wrote:
> On 2012/07/17 17:40:44, nileshagrawal1 wrote:
> > On 2012/07/17 11:39:51, I haz the power (commit-bot) wrote:
> > > Change committed as 146985
> > 
> > Did this break the paths for test output
> >
>
http://build.chromium.org/p/chromium.fyi/builders/Chromium%252520Linux%252520...
> > ?
> 
> Another try on the link:

http://build.chromium.org/p/chromium.fyi/builders/Chromium Linux
Android/builds/11114

Build number 11114 on the android FYI bot.

[nilesh, 2012-07-17 17:44:49]

http://build.chromium.org/p/chromium.fyi/builders/Chromium%20Linux%20Android/...


On Tue, Jul 17, 2012 at 10:43 AM, <nileshagrawal@chromium.org> wrote:

> On 2012/07/17 17:41:42, nileshagrawal1 wrote:
>
>> On 2012/07/17 17:40:44, nileshagrawal1 wrote:
>> > On 2012/07/17 11:39:51, I haz the power (commit-bot) wrote:
>> > > Change committed as 146985
>> >
>> > Did this break the paths for test output
>> >
>>
>
> http://build.chromium.org/p/**chromium.fyi/builders/**
>
Chromium%252520Linux%**252520Android/builds/11114<http://build.chromium.org/p/chromium.fyi/builders/Chromium%252520Linux%252520Android/builds/11114>
>
>  > ?
>>
>
>  Another try on the link:
>>
>
>
http://build.chromium.org/p/**chromium.fyi/builders/Chromium<http://build.chr...
> Android/builds/11114
>
> Build number 11114 on the android FYI bot.
>
>
>
https://chromiumcodereview.**appspot.com/10777017/<https://chromiumcodereview...
>

[nilesh, 2012-07-17 18:14:33]

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

http://codereview.chromium.org/10777017/diff/9002/build/android/run_tests.py#...
build/android/run_tests.py:312: sharder = TestSharder(attached_devices,
options.test_suite,
We are now using TestSharder in all cases. However, test_package.GetAllTests()
won't work on unrooted devices, as it relies on the shell process being able to
read stdout.txt from the application directory.

[bulach, 2012-07-18 08:57:58]

nilesh: ouch, sorry about that... let me try to fix it.