Refactor android test results logging.

Refactor android test results logging.

BUG=165529
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=173795

[Isaac (away), 2012-12-18 04:17:43]

(drive by)

https://codereview.chromium.org/11616010/diff/1/build/android/run_monkey_test.py
File build/android/run_monkey_test.py (right):

https://codereview.chromium.org/11616010/diff/1/build/android/run_monkey_test...
build/android/run_monkey_test.py:111: result.LogFull('Monkey',
named arguments here too?

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

https://codereview.chromium.org/11616010/diff/1/build/android/run_tests.py#ne...
build/android/run_tests.py:276: test_results.LogFull('Unit test',
can we use named arguments here?

[craigdh, 2012-12-18 17:41:59]

lgtm with suggestion

https://codereview.chromium.org/11616010/diff/1/build/android/pylib/test_resu...
File build/android/pylib/test_result.py (right):

https://codereview.chromium.org/11616010/diff/1/build/android/pylib/test_resu...
build/android/pylib/test_result.py:135: os.mkdir(log_file_path)
Consider os.makedirs if you want this to always succeed? Or is
constants.CHROME_DIR/out/build_type guaranteed to exist?

https://codereview.chromium.org/11616010/diff/1/build/android/pylib/test_resu...
build/android/pylib/test_result.py:214: summary += ['TESTS_TO_RUN=%d\n' %
(len(all_tests))]
nit: no need for the () around len()

[nilesh, 2012-12-18 18:08:31]

Nit

https://codereview.chromium.org/11616010/diff/1/build/android/pylib/test_resu...
File build/android/pylib/test_result.py (right):

https://codereview.chromium.org/11616010/diff/1/build/android/pylib/test_resu...
build/android/pylib/test_result.py:191: all_test: A list of all tests in this
test suite. This is used to
s/all_test/all_tests

The comment is not true. all_tests is the list of all tests which we are
supposed to run for this test suite.  For e.g disabled tests are not present in
this list. I do not remember what happens when a test filter is specified, but
you can check.
It may also be None if the test list is not available.

[frankf, 2012-12-18 21:33:50]

PTAL.

https://codereview.chromium.org/11616010/diff/1/build/android/pylib/test_resu...
File build/android/pylib/test_result.py (right):

https://codereview.chromium.org/11616010/diff/1/build/android/pylib/test_resu...
build/android/pylib/test_result.py:135: os.mkdir(log_file_path)
Yes, it needs to exists. Even on the bots where we're not building chrome, we
need to have the out/[Release|Debug] which includes minimum dependencies for
running tests.

On 2012/12/18 17:41:59, craigdh wrote:
> Consider os.makedirs if you want this to always succeed? Or is
> constants.CHROME_DIR/out/build_type guaranteed to exist?

https://codereview.chromium.org/11616010/diff/1/build/android/pylib/test_resu...
build/android/pylib/test_result.py:191: all_test: A list of all tests in this
test suite. This is used to
Reworded the description. all_tests is the final filtered list and is only given
for gtests since a crash causes the rest of the tests in the package to not run.

On 2012/12/18 18:08:31, nileshagrawal1 wrote:
> s/all_test/all_tests
> 
> The comment is not true. all_tests is the list of all tests which we are
> supposed to run for this test suite.  For e.g disabled tests are not present
in
> this list. I do not remember what happens when a test filter is specified, but
> you can check.
> It may also be None if the test list is not available.

https://codereview.chromium.org/11616010/diff/1/build/android/pylib/test_resu...
build/android/pylib/test_result.py:214: summary += ['TESTS_TO_RUN=%d\n' %
(len(all_tests))]
On 2012/12/18 17:41:59, craigdh wrote:
> nit: no need for the () around len()

Done.

https://codereview.chromium.org/11616010/diff/1/build/android/run_monkey_test.py
File build/android/run_monkey_test.py (right):

https://codereview.chromium.org/11616010/diff/1/build/android/run_monkey_test...
build/android/run_monkey_test.py:111: result.LogFull('Monkey',
On 2012/12/18 04:17:43, Isaac wrote:
> named arguments here too?

Done.

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

https://codereview.chromium.org/11616010/diff/1/build/android/run_tests.py#ne...
build/android/run_tests.py:276: test_results.LogFull('Unit test',
On 2012/12/18 04:17:43, Isaac wrote:
> can we use named arguments here?

Done.

[nilesh, 2012-12-18 21:46:17]

LGTM

[Yaron, 2012-12-18 21:53:31]

Removing myself since you have Nilesh's approval

[Yaron, 2012-12-18 21:53:31]

Removing myself since you have Nilesh's approval