[Android] Rewrite base test result classes.

[Android] Rewrite base test result classes.

- Use set instead of list
- Encapsulate internals
- Add unit tests
- Separate out the reporting utilities

BUG=167331
NOTRY=True

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

[craigdh, 2013-03-23 00:41:39]

Comments so far. Made it about halfway, nothing major.

https://codereview.chromium.org/12544033/diff/1/build/android/pylib/base/base...
File build/android/pylib/base/base_test_result.py (right):

https://codereview.chromium.org/12544033/diff/1/build/android/pylib/base/base...
build/android/pylib/base/base_test_result.py:7: class TestType(object):
doc string

https://codereview.chromium.org/12544033/diff/1/build/android/pylib/base/base...
build/android/pylib/base/base_test_result.py:23: 
no blank line

https://codereview.chromium.org/12544033/diff/1/build/android/pylib/base/base...
build/android/pylib/base/base_test_result.py:66: 
ditto

https://codereview.chromium.org/12544033/diff/1/build/android/pylib/base/base...
build/android/pylib/base/base_test_result.py:75: tests =
sorted(self._GetType(test_type))
no need for intermediate variable tests

https://codereview.chromium.org/12544033/diff/1/build/android/pylib/base/base...
build/android/pylib/base/base_test_result.py:108: result: An object derived from
BaseTestResult.
I think "An instance of BaseTestResult." is better. Also gives consistency with
AddTestRunResults.

https://codereview.chromium.org/12544033/diff/1/build/android/pylib/base/base...
build/android/pylib/base/base_test_result.py:110: 
no blank line

https://codereview.chromium.org/12544033/diff/1/build/android/pylib/base/base...
build/android/pylib/base/base_test_result.py:115: """Add a set of results to
this set.
doesn't have to be a set, can be any iterable

https://codereview.chromium.org/12544033/diff/1/build/android/pylib/base/base...
build/android/pylib/base/base_test_result.py:118: results: A set of objects
derived from BaseTestResult.
ditto

https://codereview.chromium.org/12544033/diff/1/build/android/pylib/base/base...
build/android/pylib/base/base_test_result.py:139: return set([t for t in
self._results if t.GetType() == test_type])
nit: generator expressions are more scaleable than list comprehensions and
should be used in preference when possible.

https://codereview.chromium.org/12544033/diff/1/build/android/pylib/base/base...
File build/android/pylib/base/base_test_result_unittest.py (right):

https://codereview.chromium.org/12544033/diff/1/build/android/pylib/base/base...
build/android/pylib/base/base_test_result_unittest.py:16: tr = TestRunResults()
no need for intermediate variable tr

https://codereview.chromium.org/12544033/diff/1/build/android/pylib/base/shar...
File build/android/pylib/base/shard_unittest.py (right):

https://codereview.chromium.org/12544033/diff/1/build/android/pylib/base/shar...
build/android/pylib/base/shard_unittest.py:18: from base_test_result import
BaseTestResult
Don't import classes. Should be import base_test_result then
base_test_result.ClassName everywhere else.

https://codereview.chromium.org/12544033/diff/1/build/android/pylib/gtest/tes...
File build/android/pylib/gtest/test_package.py (right):

https://codereview.chromium.org/12544033/diff/1/build/android/pylib/gtest/tes...
build/android/pylib/gtest/test_package.py:13: from pylib.base.base_test_result
import BaseTestResult
import modules, not classes

https://codereview.chromium.org/12544033/diff/1/build/android/pylib/gtest/tes...
File build/android/pylib/gtest/test_runner.py (right):

https://codereview.chromium.org/12544033/diff/1/build/android/pylib/gtest/tes...
build/android/pylib/gtest/test_runner.py:244: def RunTest(self, test):
don't delete the docstring

https://codereview.chromium.org/12544033/diff/1/build/android/pylib/host_driv...
File build/android/pylib/host_driven/python_test_base.py (right):

https://codereview.chromium.org/12544033/diff/1/build/android/pylib/host_driv...
build/android/pylib/host_driven/python_test_base.py:110: # will hold only one
result.
let's assert this as well to make it clear in the code that java_results only
contains one result from _RunJavaTest()

https://codereview.chromium.org/12544033/diff/1/build/android/pylib/host_driv...
build/android/pylib/host_driven/python_test_base.py:120: # return
self._ProcessResults(result, start_ms, duration_ms)
commented out code??? looks like it should be deleted.

https://codereview.chromium.org/12544033/diff/1/build/android/pylib/host_driv...
File build/android/pylib/host_driven/python_test_caller.py (right):

https://codereview.chromium.org/12544033/diff/1/build/android/pylib/host_driv...
build/android/pylib/host_driven/python_test_caller.py:24: test_name: name of the
test which raised an exception.
this patch set isn't consistent about capitalization of the first word in the
argument description.

[frankf, 2013-03-23 01:32:58]

https://codereview.chromium.org/12544033/diff/1/build/android/pylib/base/base...
File build/android/pylib/base/base_test_result.py (right):

https://codereview.chromium.org/12544033/diff/1/build/android/pylib/base/base...
build/android/pylib/base/base_test_result.py:7: class TestType(object):
On 2013/03/23 00:41:39, craigdh wrote:
> doc string

Done.

https://codereview.chromium.org/12544033/diff/1/build/android/pylib/base/base...
build/android/pylib/base/base_test_result.py:23: 
Done, but pylint complains.

On 2013/03/23 00:41:39, craigdh wrote:
> no blank line

https://codereview.chromium.org/12544033/diff/1/build/android/pylib/base/base...
build/android/pylib/base/base_test_result.py:66: 
On 2013/03/23 00:41:39, craigdh wrote:
> ditto

Done.

https://codereview.chromium.org/12544033/diff/1/build/android/pylib/base/base...
build/android/pylib/base/base_test_result.py:75: tests =
sorted(self._GetType(test_type))
On 2013/03/23 00:41:39, craigdh wrote:
> no need for intermediate variable tests

Done.

https://codereview.chromium.org/12544033/diff/1/build/android/pylib/base/base...
build/android/pylib/base/base_test_result.py:108: result: An object derived from
BaseTestResult.
On 2013/03/23 00:41:39, craigdh wrote:
> I think "An instance of BaseTestResult." is better. Also gives consistency
with
> AddTestRunResults.

Done.

https://codereview.chromium.org/12544033/diff/1/build/android/pylib/base/base...
build/android/pylib/base/base_test_result.py:108: result: An object derived from
BaseTestResult.
On 2013/03/23 00:41:39, craigdh wrote:
> I think "An instance of BaseTestResult." is better. Also gives consistency
with
> AddTestRunResults.

Done.

https://codereview.chromium.org/12544033/diff/1/build/android/pylib/base/base...
build/android/pylib/base/base_test_result.py:110: 
On 2013/03/23 00:41:39, craigdh wrote:
> no blank line

Done.

https://codereview.chromium.org/12544033/diff/1/build/android/pylib/base/base...
build/android/pylib/base/base_test_result.py:110: 
On 2013/03/23 00:41:39, craigdh wrote:
> no blank line

Done.

https://codereview.chromium.org/12544033/diff/1/build/android/pylib/base/base...
build/android/pylib/base/base_test_result.py:115: """Add a set of results to
this set.
On 2013/03/23 00:41:39, craigdh wrote:
> doesn't have to be a set, can be any iterable

Done.

https://codereview.chromium.org/12544033/diff/1/build/android/pylib/base/base...
build/android/pylib/base/base_test_result.py:115: """Add a set of results to
this set.
On 2013/03/23 00:41:39, craigdh wrote:
> doesn't have to be a set, can be any iterable

Done.

https://codereview.chromium.org/12544033/diff/1/build/android/pylib/base/base...
build/android/pylib/base/base_test_result.py:118: results: A set of objects
derived from BaseTestResult.
On 2013/03/23 00:41:39, craigdh wrote:
> ditto

Done.

https://codereview.chromium.org/12544033/diff/1/build/android/pylib/base/base...
build/android/pylib/base/base_test_result.py:118: results: A set of objects
derived from BaseTestResult.
On 2013/03/23 00:41:39, craigdh wrote:
> ditto

Done.

https://codereview.chromium.org/12544033/diff/1/build/android/pylib/base/base...
build/android/pylib/base/base_test_result.py:139: return set([t for t in
self._results if t.GetType() == test_type])
I want the return type to be a set though.

On 2013/03/23 00:41:39, craigdh wrote:
> nit: generator expressions are more scaleable than list comprehensions and
> should be used in preference when possible.

https://codereview.chromium.org/12544033/diff/1/build/android/pylib/base/base...
File build/android/pylib/base/base_test_result_unittest.py (right):

https://codereview.chromium.org/12544033/diff/1/build/android/pylib/base/base...
build/android/pylib/base/base_test_result_unittest.py:16: tr = TestRunResults()
On 2013/03/23 00:41:39, craigdh wrote:
> no need for intermediate variable tr

Done.

https://codereview.chromium.org/12544033/diff/1/build/android/pylib/base/shar...
File build/android/pylib/base/shard_unittest.py (right):

https://codereview.chromium.org/12544033/diff/1/build/android/pylib/base/shar...
build/android/pylib/base/shard_unittest.py:18: from base_test_result import
BaseTestResult
On 2013/03/23 00:41:39, craigdh wrote:
> Don't import classes. Should be import base_test_result then
> base_test_result.ClassName everywhere else.

Done.

https://codereview.chromium.org/12544033/diff/1/build/android/pylib/gtest/tes...
File build/android/pylib/gtest/test_package.py (right):

https://codereview.chromium.org/12544033/diff/1/build/android/pylib/gtest/tes...
build/android/pylib/gtest/test_package.py:13: from pylib.base.base_test_result
import BaseTestResult
On 2013/03/23 00:41:39, craigdh wrote:
> import modules, not classes

Done.

https://codereview.chromium.org/12544033/diff/1/build/android/pylib/gtest/tes...
File build/android/pylib/gtest/test_runner.py (right):

https://codereview.chromium.org/12544033/diff/1/build/android/pylib/gtest/tes...
build/android/pylib/gtest/test_runner.py:244: def RunTest(self, test):
It's a derived method. Let's not duplicate comments which might go out of sync.

On 2013/03/23 00:41:39, craigdh wrote:
> don't delete the docstring

https://codereview.chromium.org/12544033/diff/1/build/android/pylib/host_driv...
File build/android/pylib/host_driven/python_test_base.py (right):

https://codereview.chromium.org/12544033/diff/1/build/android/pylib/host_driv...
build/android/pylib/host_driven/python_test_base.py:110: # will hold only one
result.
On 2013/03/23 00:41:39, craigdh wrote:
> let's assert this as well to make it clear in the code that java_results only
> contains one result from _RunJavaTest()

Done.

https://codereview.chromium.org/12544033/diff/1/build/android/pylib/host_driv...
build/android/pylib/host_driven/python_test_base.py:120: # return
self._ProcessResults(result, start_ms, duration_ms)
On 2013/03/23 00:41:39, craigdh wrote:
> commented out code??? looks like it should be deleted.

Done.

[craigdh, 2013-03-25 18:06:42]

https://codereview.chromium.org/12544033/diff/1/build/android/pylib/base/base...
File build/android/pylib/base/base_test_result.py (right):

https://codereview.chromium.org/12544033/diff/1/build/android/pylib/base/base...
build/android/pylib/base/base_test_result.py:139: return set([t for t in
self._results if t.GetType() == test_type])
It still will be, there just won't be a (possibly huge) temporary list in memory
while creating it. Just remove the [ and ].

On 2013/03/23 01:32:59, frankf wrote:
> I want the return type to be a set though.
> 
> On 2013/03/23 00:41:39, craigdh wrote:
> > nit: generator expressions are more scaleable than list comprehensions and
> > should be used in preference when possible.
>

[craigdh, 2013-03-25 19:08:45]

lgtm with nits (assuming everything works perfectly :)

https://codereview.chromium.org/12544033/diff/12001/build/android/pylib/utils...
File build/android/pylib/utils/report_results.py (right):

https://codereview.chromium.org/12544033/diff/12001/build/android/pylib/utils...
build/android/pylib/utils/report_results.py:16: def _LogToFile(results,
test_type, test_suite, build_type):
full of race conditions, remove it or at least stick a try/except around it with
a TODO so it doesn't bring everything else down when it goes...

https://codereview.chromium.org/12544033/diff/12001/build/android/run_monkey_...
File build/android/run_monkey_test.py (right):

https://codereview.chromium.org/12544033/diff/12001/build/android/run_monkey_...
build/android/run_monkey_test.py:50: 
nit: no blank line

[frankf, 2013-03-25 22:37:39]

https://codereview.chromium.org/12544033/diff/12001/build/android/pylib/utils...
File build/android/pylib/utils/report_results.py (right):

https://codereview.chromium.org/12544033/diff/12001/build/android/pylib/utils...
build/android/pylib/utils/report_results.py:16: def _LogToFile(results,
test_type, test_suite, build_type):
This is not called from multiple threads.

On 2013/03/25 19:08:45, craigdh wrote:
> full of race conditions, remove it or at least stick a try/except around it
with
> a TODO so it doesn't bring everything else down when it goes...

https://codereview.chromium.org/12544033/diff/12001/build/android/run_monkey_...
File build/android/run_monkey_test.py (right):

https://codereview.chromium.org/12544033/diff/12001/build/android/run_monkey_...
build/android/run_monkey_test.py:50: 
On 2013/03/25 19:08:45, craigdh wrote:
> nit: no blank line

Done.

[craigdh, 2013-03-25 23:29:20]

https://codereview.chromium.org/12544033/diff/35002/build/android/pylib/base/...
File build/android/pylib/base/base_test_result.py (right):

https://codereview.chromium.org/12544033/diff/35002/build/android/pylib/base/...
build/android/pylib/base/base_test_result.py:9: """Class enumerating test
types."""
update docstring

https://codereview.chromium.org/12544033/diff/35002/build/android/pylib/base/...
build/android/pylib/base/base_test_result.py:98: return ''.join([x.ljust(15) for
x in s])
nit: no need for the [ and ]

[commit-bot: I haz the power, 2013-03-25 23:54:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/frankf@chromium.org/12544033/35002

[frankf, 2013-03-26 00:29:18]

Committed patchset #4 manually as r190525 (presubmit successful).