[Android] Converts host driven tests to common test_dispatcher

Converts host driven tests to common test_dispatcher

Also renames several files in pylib/host_driven to match the general file
naming scheme.

This change will break existing host-driven tests downstream which are run
though scripts other than test_runner.

NOTRY=True
BUG=176323
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=215944

[bulach, 2013-07-23 17:27:52]

good stuff, thanks for clearing this up! I have some suggestions below: some of
them may not be applicable, I'll be happy to let frank make the final call. :)

since we're going to break it anyways, this is a wonderful opportunity to make
this "host driven" bit clearer and less ambiguous, yay!

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

https://codereview.chromium.org/19537004/diff/1/build/android/pylib/host_driv...
build/android/pylib/host_driven/instrumentation_test_base.py:26: class
InstrumentationPythonTestBase(python_test_base.PythonTestBase):
hehe, everytime I look into "host-driven" or "python-test" my head hurts big
time. :)
in that spirit, a request, since we're renaming it anyways:

1) this class is Instrumentation"Python"TestBase and the file is just
instrumentation_test_base.py...

2) if we're going to split this so that the host-driven tests will aggregate
rather then being one, then this should probably be called
"HostDrivenInstrumentationHelper", or something..


I guess my question is: what is a PythonTestBase and what is a
InstrumentationPythonTestBase?

can we get away with just PythonTestBase, and if the test needs to run
instrumentation, it'll create a "HostDrivenInstrumentationHelper"? having two
base-classes seem a bit confusing, and also "instrumentationpython" doesn't make
much sense..

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

https://codereview.chromium.org/19537004/diff/1/build/android/pylib/host_driv...
build/android/pylib/host_driven/setup.py:162: def
InstrumentationSetup(python_test_root, official_build, annotations,
as above, Setup is an incredibly overloaded term in the test harness world: most
of the time, it means the SetUp / TearDown pair..

neither the function nor the file should be called setup :)

"factory.py" and Factory for this function would probably be better.

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

https://codereview.chromium.org/19537004/diff/1/build/android/pylib/host_driv...
build/android/pylib/host_driven/test_runner.py:21: class
PythonExceptionTestResult(test_result.InstrumentationTestResult):
since we're renaming and willing to break, rather than Python use HostDriven as
a prefix everywhere (i.e., here, HostDriverTestRunner in 46, etc..).

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

https://codereview.chromium.org/19537004/diff/1/build/android/test_runner.py#...
build/android/test_runner.py:27: from pylib.host_driven import setup as
host_driven_setup
oh, I see where the setup is coming from... :-/
well, if we want to keep consistency, so be it... but I'd much prefer this to be
called Factory, it'd be much clearer..

[gkanwar1, 2013-07-23 18:38:55]

Please hold off on further review while I make the changes suggested by bulach.
Thanks!

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

https://codereview.chromium.org/19537004/diff/1/build/android/pylib/host_driv...
build/android/pylib/host_driven/instrumentation_test_base.py:26: class
InstrumentationPythonTestBase(python_test_base.PythonTestBase):
On 2013/07/23 17:27:52, bulach wrote:
> hehe, everytime I look into "host-driven" or "python-test" my head hurts big
> time. :)
> in that spirit, a request, since we're renaming it anyways:
> 
> 1) this class is Instrumentation"Python"TestBase and the file is just
> instrumentation_test_base.py...
> 
> 2) if we're going to split this so that the host-driven tests will aggregate
> rather then being one, then this should probably be called
> "HostDrivenInstrumentationHelper", or something..
> 
> 
> I guess my question is: what is a PythonTestBase and what is a
> InstrumentationPythonTestBase?
> 
> can we get away with just PythonTestBase, and if the test needs to run
> instrumentation, it'll create a "HostDrivenInstrumentationHelper"? having two
> base-classes seem a bit confusing, and also "instrumentationpython" doesn't
make
> much sense..

So the reason we need a new class to derive from is that somehow these tests
need to get the extra instrumentation args, either though __init__ or Setup.
I've chosen to pass them in through __init__ here, since they don't change based
on the device/shard_index. This new class doesn't need to be called a "base
class" though, so...

What I'm going to do in terms of cleaning this up is:
* Rename InstrumentationPythonTestBase --> HostDrivenInstrumentationTest,
instrumentation_test_base.py --> instrumentation_test.py, python_test_base.py
--> test_base.py to match.
* Since searching directories for host-driven tests is specific to host-driven
instrumentation tests only, I will change the functions in setup.py to be a
little more specific to this.

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

https://codereview.chromium.org/19537004/diff/1/build/android/pylib/host_driv...
build/android/pylib/host_driven/setup.py:162: def
InstrumentationSetup(python_test_root, official_build, annotations,
On 2013/07/23 17:27:52, bulach wrote:
> as above, Setup is an incredibly overloaded term in the test harness world:
most
> of the time, it means the SetUp / TearDown pair..
> 
> neither the function nor the file should be called setup :)
> 
> "factory.py" and Factory for this function would probably be better.

This is named setup.py to match the recent sharding changes. You have a good
point that factory.py might be a better name, but I don't think that change
should occur in this CL. Let's leave this as setup.py in this CL, and make a
future change that renames all setup.py files to factory.py files? Frank, what
are your thoughts on that name?

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

https://codereview.chromium.org/19537004/diff/1/build/android/pylib/host_driv...
build/android/pylib/host_driven/test_runner.py:21: class
PythonExceptionTestResult(test_result.InstrumentationTestResult):
On 2013/07/23 17:27:52, bulach wrote:
> since we're renaming and willing to break, rather than Python use HostDriven
as
> a prefix everywhere (i.e., here, HostDriverTestRunner in 46, etc..).

Good point. I'll make this change throughout.

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

https://codereview.chromium.org/19537004/diff/1/build/android/test_runner.py#...
build/android/test_runner.py:27: from pylib.host_driven import setup as
host_driven_setup
On 2013/07/23 17:27:52, bulach wrote:
> oh, I see where the setup is coming from... :-/
> well, if we want to keep consistency, so be it... but I'd much prefer this to
be
> called Factory, it'd be much clearer..

See my other comment.

[gkanwar1, 2013-07-23 20:45:05]

Patch set 2 has several fixes, ptal. Thanks.

[frankf, 2013-07-24 01:04:54]

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

https://codereview.chromium.org/19537004/diff/1/build/android/pylib/host_driv...
build/android/pylib/host_driven/instrumentation_test_base.py:26: class
InstrumentationPythonTestBase(python_test_base.PythonTestBase):
I think we should follow the accepted python unittest terminology. So the
inhertance would be:

unittest.TestCase < hostDrivenTestCase < xxxHostDrivenTestCase

On 2013/07/23 18:38:55, gkanwar1 wrote:
> On 2013/07/23 17:27:52, bulach wrote:
> > hehe, everytime I look into "host-driven" or "python-test" my head hurts big
> > time. :)
> > in that spirit, a request, since we're renaming it anyways:
> > 
> > 1) this class is Instrumentation"Python"TestBase and the file is just
> > instrumentation_test_base.py...
> > 
> > 2) if we're going to split this so that the host-driven tests will aggregate
> > rather then being one, then this should probably be called
> > "HostDrivenInstrumentationHelper", or something..
> > 
> > 
> > I guess my question is: what is a PythonTestBase and what is a
> > InstrumentationPythonTestBase?
> > 
> > can we get away with just PythonTestBase, and if the test needs to run
> > instrumentation, it'll create a "HostDrivenInstrumentationHelper"? having
two
> > base-classes seem a bit confusing, and also "instrumentationpython" doesn't
> make
> > much sense..
> 
> So the reason we need a new class to derive from is that somehow these tests
> need to get the extra instrumentation args, either though __init__ or Setup.
> I've chosen to pass them in through __init__ here, since they don't change
based
> on the device/shard_index. This new class doesn't need to be called a "base
> class" though, so...
> 
> What I'm going to do in terms of cleaning this up is:
> * Rename InstrumentationPythonTestBase --> HostDrivenInstrumentationTest,
> instrumentation_test_base.py --> instrumentation_test.py, python_test_base.py
> --> test_base.py to match.
> * Since searching directories for host-driven tests is specific to host-driven
> instrumentation tests only, I will change the functions in setup.py to be a
> little more specific to this.

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

https://codereview.chromium.org/19537004/diff/1/build/android/pylib/host_driv...
build/android/pylib/host_driven/test_runner.py:21: class
PythonExceptionTestResult(test_result.InstrumentationTestResult):
On 2013/07/23 18:38:55, gkanwar1 wrote:
> On 2013/07/23 17:27:52, bulach wrote:
> > since we're renaming and willing to break, rather than Python use HostDriven
> as
> > a prefix everywhere (i.e., here, HostDriverTestRunner in 46, etc..).
> 
> Good point. I'll make this change throughout.

SGTM

https://codereview.chromium.org/19537004/diff/10001/build/android/pylib/host_...
File build/android/pylib/host_driven/setup.py (right):

https://codereview.chromium.org/19537004/diff/10001/build/android/pylib/host_...
build/android/pylib/host_driven/setup.py:15: from pylib.base import shard
you mean test_dispatcher?

https://codereview.chromium.org/19537004/diff/10001/build/android/pylib/host_...
File build/android/pylib/host_driven/test_base.py (right):

https://codereview.chromium.org/19537004/diff/10001/build/android/pylib/host_...
build/android/pylib/host_driven/test_base.py:5: """Base class for Android
host-driven tests.
remove 'Android' from docs, it's implied.

https://codereview.chromium.org/19537004/diff/10001/build/android/pylib/host_...
build/android/pylib/host_driven/test_base.py:8: tests. It is similar to the
Python unitttest module in that the user's tests
Didn't we discuss using unittest? You'll get a lot of stuff for free including
asserts.

https://codereview.chromium.org/19537004/diff/10001/build/android/pylib/host_...
build/android/pylib/host_driven/test_base.py:30: # to be pickled, a requirement
for the multiprocessing module.
This is no longer a requirement, there's no multiprocessing anymore

https://codereview.chromium.org/19537004/diff/10001/build/android/pylib/host_...
build/android/pylib/host_driven/test_base.py:52: logging.warning('Running
host-driven test: %s', self.test_name)
warning -> info

https://codereview.chromium.org/19537004/diff/10001/build/android/pylib/host_...
File build/android/pylib/host_driven/test_runner.py (right):

https://codereview.chromium.org/19537004/diff/10001/build/android/pylib/host_...
build/android/pylib/host_driven/test_runner.py:44: class
HostDrivenTestRunner(base_test_runner.BaseTestRunner):
By deriving from BaseTestRunner, you're inheriting SetUp/TearDown which do a
bunch of stuff including killing forwarder etc. Make sure, these don't interfere
with the assumption in existing tests. You can make these a NOOP if required.

https://codereview.chromium.org/19537004/diff/10001/build/android/pylib/host_...
build/android/pylib/host_driven/test_runner.py:89: test.TearDown()
There's no exception handling here. Think about the failure cases.

[gkanwar1, 2013-07-24 17:38:04]

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

https://codereview.chromium.org/19537004/diff/1/build/android/pylib/host_driv...
build/android/pylib/host_driven/instrumentation_test_base.py:26: class
InstrumentationPythonTestBase(python_test_base.PythonTestBase):
On 2013/07/24 01:04:55, frankf wrote:
> I think we should follow the accepted python unittest terminology. So the
> inhertance would be:
> 
> unittest.TestCase < hostDrivenTestCase < xxxHostDrivenTestCase
> 
> On 2013/07/23 18:38:55, gkanwar1 wrote:
> > On 2013/07/23 17:27:52, bulach wrote:
> > > hehe, everytime I look into "host-driven" or "python-test" my head hurts
big
> > > time. :)
> > > in that spirit, a request, since we're renaming it anyways:
> > > 
> > > 1) this class is Instrumentation"Python"TestBase and the file is just
> > > instrumentation_test_base.py...
> > > 
> > > 2) if we're going to split this so that the host-driven tests will
aggregate
> > > rather then being one, then this should probably be called
> > > "HostDrivenInstrumentationHelper", or something..
> > > 
> > > 
> > > I guess my question is: what is a PythonTestBase and what is a
> > > InstrumentationPythonTestBase?
> > > 
> > > can we get away with just PythonTestBase, and if the test needs to run
> > > instrumentation, it'll create a "HostDrivenInstrumentationHelper"? having
> two
> > > base-classes seem a bit confusing, and also "instrumentationpython"
doesn't
> > make
> > > much sense..
> > 
> > So the reason we need a new class to derive from is that somehow these tests
> > need to get the extra instrumentation args, either though __init__ or Setup.
> > I've chosen to pass them in through __init__ here, since they don't change
> based
> > on the device/shard_index. This new class doesn't need to be called a "base
> > class" though, so...
> > 
> > What I'm going to do in terms of cleaning this up is:
> > * Rename InstrumentationPythonTestBase --> HostDrivenInstrumentationTest,
> > instrumentation_test_base.py --> instrumentation_test.py,
python_test_base.py
> > --> test_base.py to match.
> > * Since searching directories for host-driven tests is specific to
host-driven
> > instrumentation tests only, I will change the functions in setup.py to be a
> > little more specific to this.
> 

Done.

https://codereview.chromium.org/19537004/diff/10001/build/android/pylib/host_...
File build/android/pylib/host_driven/setup.py (right):

https://codereview.chromium.org/19537004/diff/10001/build/android/pylib/host_...
build/android/pylib/host_driven/setup.py:15: from pylib.base import shard
On 2013/07/24 01:04:55, frankf wrote:
> you mean test_dispatcher?

Actually, this should just get removed, since it's not used. Done.

https://codereview.chromium.org/19537004/diff/10001/build/android/pylib/host_...
File build/android/pylib/host_driven/test_base.py (right):

https://codereview.chromium.org/19537004/diff/10001/build/android/pylib/host_...
build/android/pylib/host_driven/test_base.py:5: """Base class for Android
host-driven tests.
On 2013/07/24 01:04:55, frankf wrote:
> remove 'Android' from docs, it's implied.

Done.

https://codereview.chromium.org/19537004/diff/10001/build/android/pylib/host_...
build/android/pylib/host_driven/test_base.py:8: tests. It is similar to the
Python unitttest module in that the user's tests
On 2013/07/24 01:04:55, frankf wrote:
> Didn't we discuss using unittest? You'll get a lot of stuff for free including
> asserts.

Yes, I was intending to look into that after this CL. I didn't want to touch too
many things at once.

https://codereview.chromium.org/19537004/diff/10001/build/android/pylib/host_...
build/android/pylib/host_driven/test_base.py:30: # to be pickled, a requirement
for the multiprocessing module.
On 2013/07/24 01:04:55, frankf wrote:
> This is no longer a requirement, there's no multiprocessing anymore

Removing just the comment for now. Let's revisit this when looking into
incorporating unittest (see my other comment).

https://codereview.chromium.org/19537004/diff/10001/build/android/pylib/host_...
build/android/pylib/host_driven/test_base.py:52: logging.warning('Running
host-driven test: %s', self.test_name)
On 2013/07/24 01:04:55, frankf wrote:
> warning -> info

Done.

https://codereview.chromium.org/19537004/diff/10001/build/android/pylib/host_...
File build/android/pylib/host_driven/test_runner.py (right):

https://codereview.chromium.org/19537004/diff/10001/build/android/pylib/host_...
build/android/pylib/host_driven/test_runner.py:44: class
HostDrivenTestRunner(base_test_runner.BaseTestRunner):
On 2013/07/24 01:04:55, frankf wrote:
> By deriving from BaseTestRunner, you're inheriting SetUp/TearDown which do a
> bunch of stuff including killing forwarder etc. Make sure, these don't
interfere
> with the assumption in existing tests. You can make these a NOOP if required.

Yes, I took a look at the BaseTestRunner before making this change. Currently
Setup just calls InstallTestPackage() and PushDataDeps() both of which are NOOPs
in BaseTestRunner. I think leaving this in is potentially useful if some
host-driven test type wants to override those without duplicating the setup
framework.

https://codereview.chromium.org/19537004/diff/10001/build/android/pylib/host_...
build/android/pylib/host_driven/test_runner.py:89: test.TearDown()
On 2013/07/24 01:04:55, frankf wrote:
> There's no exception handling here. Think about the failure cases.

Ah, I removed the try/excepts because we're using the Reraiser thread, but I
still need to make sure exceptions in Run() let TearDown happen before raising.
Fixed.

[frankf, 2013-07-24 23:33:42]

https://codereview.chromium.org/19537004/diff/18001/build/android/pylib/host_...
File build/android/pylib/host_driven/instrumentation_test_case.py (right):

https://codereview.chromium.org/19537004/diff/18001/build/android/pylib/host_...
build/android/pylib/host_driven/instrumentation_test_case.py:94: A
TestRunResults object containing a result for this host-driven test.
This is not accurate. A host-driven test can make multiple calls to this.

https://codereview.chromium.org/19537004/diff/18001/build/android/pylib/host_...
build/android/pylib/host_driven/instrumentation_test_case.py:104: java_results =
self._RunJavaTest(fname, suite, test_name)
this should be singular

https://codereview.chromium.org/19537004/diff/18001/build/android/pylib/host_...
build/android/pylib/host_driven/instrumentation_test_case.py:123: def
_GetPackageName(self, fname):
If the above two methods are not reused, combine them and rename parameters:
suite -> class
test -> method

https://codereview.chromium.org/19537004/diff/18001/build/android/pylib/host_...
File build/android/pylib/host_driven/test_case.py (right):

https://codereview.chromium.org/19537004/diff/18001/build/android/pylib/host_...
build/android/pylib/host_driven/test_case.py:9: inherit from this case and add
their tests in that case.
Use more precise language. A "test" in this context could be anything.

https://codereview.chromium.org/19537004/diff/18001/build/android/pylib/host_...
File build/android/pylib/host_driven/test_runner.py (right):

https://codereview.chromium.org/19537004/diff/18001/build/android/pylib/host_...
build/android/pylib/host_driven/test_runner.py:19: class
HostDrivenExceptionTestResult(test_result.InstrumentationTestResult):
Where is this used?

https://codereview.chromium.org/19537004/diff/18001/build/android/pylib/host_...
build/android/pylib/host_driven/test_runner.py:20: """A test result from an
exception in a host-driven test."""
Rephrase

https://codereview.chromium.org/19537004/diff/18001/build/android/pylib/host_...
build/android/pylib/host_driven/test_runner.py:80: A TestRunResults object which
contains any results produced by the test
Although you use TestRunResults(), name variables singular if it holds a single
test result

https://codereview.chromium.org/19537004/diff/18001/build/android/pylib/host_...
build/android/pylib/host_driven/test_runner.py:89: failed = False
failed -> exception_thrown

https://codereview.chromium.org/19537004/diff/18001/build/android/pylib/host_...
build/android/pylib/host_driven/test_runner.py:90: results = test.Run()
Document what kind of exception can be thrown by Run() in its module

[gkanwar1, 2013-07-25 00:27:55]

Uploaded a new patch set to address issues, ptal.

https://codereview.chromium.org/19537004/diff/18001/build/android/pylib/host_...
File build/android/pylib/host_driven/instrumentation_test_case.py (right):

https://codereview.chromium.org/19537004/diff/18001/build/android/pylib/host_...
build/android/pylib/host_driven/instrumentation_test_case.py:94: A
TestRunResults object containing a result for this host-driven test.
On 2013/07/24 23:33:43, frankf wrote:
> This is not accurate. A host-driven test can make multiple calls to this.

Done.

https://codereview.chromium.org/19537004/diff/18001/build/android/pylib/host_...
build/android/pylib/host_driven/instrumentation_test_case.py:104: java_results =
self._RunJavaTest(fname, suite, test_name)
On 2013/07/24 23:33:43, frankf wrote:
> this should be singular

Done.

https://codereview.chromium.org/19537004/diff/18001/build/android/pylib/host_...
build/android/pylib/host_driven/instrumentation_test_case.py:123: def
_GetPackageName(self, fname):
On 2013/07/24 23:33:43, frankf wrote:
> If the above two methods are not reused, combine them and rename parameters:
> suite -> class
> test -> method

Done, but with suite -> test_case instead, since class is a reserved keyword,
and test_case is actually more specific/accurate.

https://codereview.chromium.org/19537004/diff/18001/build/android/pylib/host_...
File build/android/pylib/host_driven/test_case.py (right):

https://codereview.chromium.org/19537004/diff/18001/build/android/pylib/host_...
build/android/pylib/host_driven/test_case.py:9: inherit from this case and add
their tests in that case.
On 2013/07/24 23:33:43, frankf wrote:
> Use more precise language. A "test" in this context could be anything.

Done.

https://codereview.chromium.org/19537004/diff/18001/build/android/pylib/host_...
File build/android/pylib/host_driven/test_runner.py (right):

https://codereview.chromium.org/19537004/diff/18001/build/android/pylib/host_...
build/android/pylib/host_driven/test_runner.py:19: class
HostDrivenExceptionTestResult(test_result.InstrumentationTestResult):
On 2013/07/24 23:33:43, frankf wrote:
> Where is this used?

Ah, this is not used any more (all references were removed below). Removed.

https://codereview.chromium.org/19537004/diff/18001/build/android/pylib/host_...
build/android/pylib/host_driven/test_runner.py:80: A TestRunResults object which
contains any results produced by the test
On 2013/07/24 23:33:43, frankf wrote:
> Although you use TestRunResults(), name variables singular if it holds a
single
> test result

Done.

https://codereview.chromium.org/19537004/diff/18001/build/android/pylib/host_...
build/android/pylib/host_driven/test_runner.py:89: failed = False
On 2013/07/24 23:33:43, frankf wrote:
> failed -> exception_thrown

Done.

https://codereview.chromium.org/19537004/diff/18001/build/android/pylib/host_...
build/android/pylib/host_driven/test_runner.py:90: results = test.Run()
On 2013/07/24 23:33:43, frankf wrote:
> Document what kind of exception can be thrown by Run() in its module

Done for InstrumentationHostDrivenTestCase (regular TestCase has no defined
exceptions, since the method could be any arbitrary Python).

[frankf, 2013-07-25 01:24:21]

https://codereview.chromium.org/19537004/diff/25001/build/android/pylib/host_...
File build/android/pylib/host_driven/instrumentation_test_case.py (right):

https://codereview.chromium.org/19537004/diff/25001/build/android/pylib/host_...
build/android/pylib/host_driven/instrumentation_test_case.py:9: tests, each one
on a unique instrumentation TestRunner object.
Still not there:

Each test method in the derived class can call _RunJavaTests to run
instrumentation tests on the corresponding device via an instrumentation test
runner.

https://codereview.chromium.org/19537004/diff/25001/build/android/pylib/host_...
File build/android/pylib/host_driven/setup.py (right):

https://codereview.chromium.org/19537004/diff/25001/build/android/pylib/host_...
build/android/pylib/host_driven/setup.py:92: def _GetTestsFromClass(test_class,
extra_args):
Please go through all docs and clean up the terminology. Just for this method,
you're referring to:

- test object
- test method
- class object
- test case object

https://codereview.chromium.org/19537004/diff/25001/build/android/pylib/host_...
File build/android/pylib/host_driven/test_case.py (right):

https://codereview.chromium.org/19537004/diff/25001/build/android/pylib/host_...
build/android/pylib/host_driven/test_case.py:12: of its tests. The test runner
gives it the name of the test the instance will
"one of its tests" -> one test method in the derived class

https://codereview.chromium.org/19537004/diff/25001/build/android/pylib/host_...
build/android/pylib/host_driven/test_case.py:13: run. The test runner calls
SetUp with the device ID which the test will run
test -> test method everywhere

https://codereview.chromium.org/19537004/diff/25001/build/android/pylib/host_...
build/android/pylib/host_driven/test_case.py:14: against. The runner runs the
test method itself, collecting the result, and
runner -> test runner

https://codereview.chromium.org/19537004/diff/25001/build/android/pylib/host_...
build/android/pylib/host_driven/test_case.py:51: return getattr(self,
self.test_name)()
Explain what this runnable is

[gkanwar1, 2013-07-25 21:15:40]

Dealt with the outstanding comments, also ran gpylint on all the files and made
a few small fixes. ptal, thanks!

https://codereview.chromium.org/19537004/diff/25001/build/android/pylib/host_...
File build/android/pylib/host_driven/instrumentation_test_case.py (right):

https://codereview.chromium.org/19537004/diff/25001/build/android/pylib/host_...
build/android/pylib/host_driven/instrumentation_test_case.py:9: tests, each one
on a unique instrumentation TestRunner object.
On 2013/07/25 01:24:21, frankf wrote:
> Still not there:
> 
> Each test method in the derived class can call _RunJavaTests to run
> instrumentation tests on the corresponding device via an instrumentation test
> runner.

Done.

https://codereview.chromium.org/19537004/diff/25001/build/android/pylib/host_...
File build/android/pylib/host_driven/setup.py (right):

https://codereview.chromium.org/19537004/diff/25001/build/android/pylib/host_...
build/android/pylib/host_driven/setup.py:92: def _GetTestsFromClass(test_class,
extra_args):
On 2013/07/25 01:24:21, frankf wrote:
> Please go through all docs and clean up the terminology. Just for this method,
> you're referring to:
> 
> - test object
> - test method
> - class object
> - test case object

Done.

https://codereview.chromium.org/19537004/diff/25001/build/android/pylib/host_...
File build/android/pylib/host_driven/test_case.py (right):

https://codereview.chromium.org/19537004/diff/25001/build/android/pylib/host_...
build/android/pylib/host_driven/test_case.py:12: of its tests. The test runner
gives it the name of the test the instance will
On 2013/07/25 01:24:21, frankf wrote:
> "one of its tests" -> one test method in the derived class

Done.

https://codereview.chromium.org/19537004/diff/25001/build/android/pylib/host_...
build/android/pylib/host_driven/test_case.py:13: run. The test runner calls
SetUp with the device ID which the test will run
On 2013/07/25 01:24:21, frankf wrote:
> test -> test method everywhere

Done.

https://codereview.chromium.org/19537004/diff/25001/build/android/pylib/host_...
build/android/pylib/host_driven/test_case.py:14: against. The runner runs the
test method itself, collecting the result, and
On 2013/07/25 01:24:21, frankf wrote:
> runner -> test runner

Done.

https://codereview.chromium.org/19537004/diff/25001/build/android/pylib/host_...
build/android/pylib/host_driven/test_case.py:51: return getattr(self,
self.test_name)()
On 2013/07/25 01:24:21, frankf wrote:
> Explain what this runnable is

Done.

[gkanwar1, 2013-07-25 21:17:03]

On 2013/07/25 21:15:40, gkanwar1 wrote:
> Dealt with the outstanding comments, also ran gpylint on all the files and
made
> a few small fixes. ptal, thanks!
> 
>
https://codereview.chromium.org/19537004/diff/25001/build/android/pylib/host_...
> File build/android/pylib/host_driven/instrumentation_test_case.py (right):
> 
>
https://codereview.chromium.org/19537004/diff/25001/build/android/pylib/host_...
> build/android/pylib/host_driven/instrumentation_test_case.py:9: tests, each
one
> on a unique instrumentation TestRunner object.
> On 2013/07/25 01:24:21, frankf wrote:
> > Still not there:
> > 
> > Each test method in the derived class can call _RunJavaTests to run
> > instrumentation tests on the corresponding device via an instrumentation
test
> > runner.
> 
> Done.
> 
>
https://codereview.chromium.org/19537004/diff/25001/build/android/pylib/host_...
> File build/android/pylib/host_driven/setup.py (right):
> 
>
https://codereview.chromium.org/19537004/diff/25001/build/android/pylib/host_...
> build/android/pylib/host_driven/setup.py:92: def
_GetTestsFromClass(test_class,
> extra_args):
> On 2013/07/25 01:24:21, frankf wrote:
> > Please go through all docs and clean up the terminology. Just for this
method,
> > you're referring to:
> > 
> > - test object
> > - test method
> > - class object
> > - test case object
> 
> Done.
> 
>
https://codereview.chromium.org/19537004/diff/25001/build/android/pylib/host_...
> File build/android/pylib/host_driven/test_case.py (right):
> 
>
https://codereview.chromium.org/19537004/diff/25001/build/android/pylib/host_...
> build/android/pylib/host_driven/test_case.py:12: of its tests. The test runner
> gives it the name of the test the instance will
> On 2013/07/25 01:24:21, frankf wrote:
> > "one of its tests" -> one test method in the derived class
> 
> Done.
> 
>
https://codereview.chromium.org/19537004/diff/25001/build/android/pylib/host_...
> build/android/pylib/host_driven/test_case.py:13: run. The test runner calls
> SetUp with the device ID which the test will run
> On 2013/07/25 01:24:21, frankf wrote:
> > test -> test method everywhere
> 
> Done.
> 
>
https://codereview.chromium.org/19537004/diff/25001/build/android/pylib/host_...
> build/android/pylib/host_driven/test_case.py:14: against. The runner runs the
> test method itself, collecting the result, and
> On 2013/07/25 01:24:21, frankf wrote:
> > runner -> test runner
> 
> Done.
> 
>
https://codereview.chromium.org/19537004/diff/25001/build/android/pylib/host_...
> build/android/pylib/host_driven/test_case.py:51: return getattr(self,
> self.test_name)()
> On 2013/07/25 01:24:21, frankf wrote:
> > Explain what this runnable is
> 
> Done.

Note: I'm currently working on re-adding the tag on test names for host-driven
instrumentation tests. I will make another patch set for that shortly.

[gkanwar1, 2013-07-25 21:58:55]

https://codereview.chromium.org/19537004/diff/18001/build/android/pylib/host_...
File build/android/pylib/host_driven/test_runner.py (right):

https://codereview.chromium.org/19537004/diff/18001/build/android/pylib/host_...
build/android/pylib/host_driven/test_runner.py:19: class
HostDrivenExceptionTestResult(test_result.InstrumentationTestResult):
On 2013/07/25 00:27:55, gkanwar1 wrote:
> On 2013/07/24 23:33:43, frankf wrote:
> > Where is this used?
> 
> Ah, this is not used any more (all references were removed below). Removed.

I thought this over some more -- we actually only want to trigger exceptions in
the testing framework itself on the main thread. Exceptions here should be
converted into an exception test result... adding this back, and tweaking to fit
with the new host-driven tagging.

[gkanwar1, 2013-07-25 22:30:01]

Fixed exception handling, added tagging of host-driven tests, and rebased onto
master. ptal, thanks!

[frankf, 2013-07-25 22:50:06]

https://codereview.chromium.org/19537004/diff/49001/build/android/pylib/host_...
File build/android/pylib/host_driven/instrumentation_test_case.py (right):

https://codereview.chromium.org/19537004/diff/49001/build/android/pylib/host_...
build/android/pylib/host_driven/instrumentation_test_case.py:67: suite: Name of
the Java test suite (e.g. FooTest)
suite -> class

https://codereview.chromium.org/19537004/diff/49001/build/android/pylib/host_...
build/android/pylib/host_driven/instrumentation_test_case.py:132: """Composes a
fully-qualified name for the test method."""
Specify args/return, provide examples

https://codereview.chromium.org/19537004/diff/49001/build/android/pylib/host_...
File build/android/pylib/host_driven/test_runner.py (right):

https://codereview.chromium.org/19537004/diff/49001/build/android/pylib/host_...
build/android/pylib/host_driven/test_runner.py:45: """Thin wrapper around a list
of HostDrivenTestCase instances.
This is not true

https://codereview.chromium.org/19537004/diff/49001/build/android/pylib/host_...
build/android/pylib/host_driven/test_runner.py:48: tests within its lifetime.
Tests will receive the device_id and shard_index.
Remove this

https://codereview.chromium.org/19537004/diff/49001/build/android/pylib/host_...
build/android/pylib/host_driven/test_runner.py:51: DEFAULT_PORT + shard_index)
if the test so wishes.
Move this to init

https://codereview.chromium.org/19537004/diff/49001/build/android/pylib/host_...
build/android/pylib/host_driven/test_runner.py:54: # override
#override everywhere

https://codereview.chromium.org/19537004/diff/49001/build/android/pylib/instr...
File build/android/pylib/instrumentation/test_jar.py (right):

https://codereview.chromium.org/19537004/diff/49001/build/android/pylib/instr...
build/android/pylib/instrumentation/test_jar.py:219: def IsHostDrivenTest(test):
Why does this need to be in test_jar.py? Let's address it in a future CL

[gkanwar1, 2013-07-26 01:39:06]

https://codereview.chromium.org/19537004/diff/49001/build/android/pylib/host_...
File build/android/pylib/host_driven/instrumentation_test_case.py (right):

https://codereview.chromium.org/19537004/diff/49001/build/android/pylib/host_...
build/android/pylib/host_driven/instrumentation_test_case.py:67: suite: Name of
the Java test suite (e.g. FooTest)
On 2013/07/25 22:50:06, frankf wrote:
> suite -> class

Done.

https://codereview.chromium.org/19537004/diff/49001/build/android/pylib/host_...
build/android/pylib/host_driven/instrumentation_test_case.py:132: """Composes a
fully-qualified name for the test method."""
On 2013/07/25 22:50:06, frankf wrote:
> Specify args/return, provide examples

Done.

https://codereview.chromium.org/19537004/diff/49001/build/android/pylib/host_...
File build/android/pylib/host_driven/test_runner.py (right):

https://codereview.chromium.org/19537004/diff/49001/build/android/pylib/host_...
build/android/pylib/host_driven/test_runner.py:45: """Thin wrapper around a list
of HostDrivenTestCase instances.
On 2013/07/25 22:50:06, frankf wrote:
> This is not true

Updated.

https://codereview.chromium.org/19537004/diff/49001/build/android/pylib/host_...
build/android/pylib/host_driven/test_runner.py:48: tests within its lifetime.
Tests will receive the device_id and shard_index.
On 2013/07/25 22:50:06, frankf wrote:
> Remove this

Done.

https://codereview.chromium.org/19537004/diff/49001/build/android/pylib/host_...
build/android/pylib/host_driven/test_runner.py:51: DEFAULT_PORT + shard_index)
if the test so wishes.
On 2013/07/25 22:50:06, frankf wrote:
> Move this to init

Done.

https://codereview.chromium.org/19537004/diff/49001/build/android/pylib/host_...
build/android/pylib/host_driven/test_runner.py:54: # override
On 2013/07/25 22:50:06, frankf wrote:
> #override everywhere

gpylint was complaining about it which was why I changed this originally...
reverting to '#override'.

https://codereview.chromium.org/19537004/diff/49001/build/android/pylib/instr...
File build/android/pylib/instrumentation/test_jar.py (right):

https://codereview.chromium.org/19537004/diff/49001/build/android/pylib/instr...
build/android/pylib/instrumentation/test_jar.py:219: def IsHostDrivenTest(test):
On 2013/07/25 22:50:06, frankf wrote:
> Why does this need to be in test_jar.py? Let's address it in a future CL

sgtm

[frankf, 2013-07-26 23:21:37]

lgtm.

Please wait for Craig as well.

[craigdh, 2013-07-26 23:38:01]

lgtm with a very minor nit.

https://codereview.chromium.org/19537004/diff/61001/build/android/pylib/host_...
File build/android/pylib/host_driven/instrumentation_test_case.py (right):

https://codereview.chromium.org/19537004/diff/61001/build/android/pylib/host_...
build/android/pylib/host_driven/instrumentation_test_case.py:46: other args: All
passed through to the instrumentation test runner. See the
I don't see them being passed through, seems like they are set right in this
__init__ method.

[gkanwar1, 2013-07-26 23:43:43]

https://codereview.chromium.org/19537004/diff/61001/build/android/pylib/host_...
File build/android/pylib/host_driven/instrumentation_test_case.py (right):

https://codereview.chromium.org/19537004/diff/61001/build/android/pylib/host_...
build/android/pylib/host_driven/instrumentation_test_case.py:46: other args: All
passed through to the instrumentation test runner. See the
On 2013/07/26 23:38:01, craigdh wrote:
> I don't see them being passed through, seems like they are set right in this
> __init__ method.

They saved to object variables and then passed into the constructor for the java
TestRunner in _RunJavaTest. I could rephrase this comment if you think it's
worth it.

[craigdh, 2013-07-26 23:46:10]

https://codereview.chromium.org/19537004/diff/61001/build/android/pylib/host_...
File build/android/pylib/host_driven/instrumentation_test_case.py (right):

https://codereview.chromium.org/19537004/diff/61001/build/android/pylib/host_...
build/android/pylib/host_driven/instrumentation_test_case.py:46: other args: All
passed through to the instrumentation test runner. See the
On 2013/07/26 23:43:44, gkanwar1 wrote:
> On 2013/07/26 23:38:01, craigdh wrote:
> > I don't see them being passed through, seems like they are set right in this
> > __init__ method.
> 
> They saved to object variables and then passed into the constructor for the
java
> TestRunner in _RunJavaTest. I could rephrase this comment if you think it's
> worth it.

It's not a big deal. Up to you.

[gkanwar1, 2013-07-26 23:48:19]

On 2013/07/26 23:46:10, craigdh wrote:
>
https://codereview.chromium.org/19537004/diff/61001/build/android/pylib/host_...
> File build/android/pylib/host_driven/instrumentation_test_case.py (right):
> 
>
https://codereview.chromium.org/19537004/diff/61001/build/android/pylib/host_...
> build/android/pylib/host_driven/instrumentation_test_case.py:46: other args:
All
> passed through to the instrumentation test runner. See the
> On 2013/07/26 23:43:44, gkanwar1 wrote:
> > On 2013/07/26 23:38:01, craigdh wrote:
> > > I don't see them being passed through, seems like they are set right in
this
> > > __init__ method.
> > 
> > They saved to object variables and then passed into the constructor for the
> java
> > TestRunner in _RunJavaTest. I could rephrase this comment if you think it's
> > worth it.
> 
> It's not a big deal. Up to you.

I removed support for uiautomator host-driven tests since that interface was
broken and there currently don't exist any uiautomator host-driven tests.

Since I was making a change anyway, I went ahead and updated the comment also.

[bulach, 2013-07-29 09:30:57]

I'm obviously fine if you guys want to go ahead with this :)

however, I wanted to clarify my points below. I think we're replacing a
relatively small implementation detail that only annoys the "monkey" runner
(i.e., the ability to run java tests from this "host-driven"), with a somewhat
heavy-weight concept (parallel class hierarchy)... more below:

https://codereview.chromium.org/19537004/diff/69001/build/android/pylib/host_...
File build/android/pylib/host_driven/instrumentation_test_case.py (right):

https://codereview.chromium.org/19537004/diff/69001/build/android/pylib/host_...
build/android/pylib/host_driven/instrumentation_test_case.py:27: class
InstrumentationHostDrivenTestCase(test_case.HostDrivenTestCase):
I'm still a bit concerned about making mixing up "instrumentation" and "host
driven" as a base-class, I think it'll make it more confusing than already is..
:(

from my pov, at this level, we have three types of tests:
1) gtests: c++ tests. can call into java, but that's an implementation detail.
2) instrumentation: java tests. can call into c++, but that's an implementation
detail.
3) host-driven: python tests. can call into java, but that's an implementation
detail.

I agree that the existing base-class makes it too "fat" for the monkey test and
we'd rather split the part that runs the instrumentation from the host-driven.
however, currently it's just a couple of protected methods that the monkey test
can safely ignore...

I think two parallel hierarchies makes it more confusing, and it'd probably be
simpler to use aggregation (i.e., a helper class) rather than inheritance.

some suggestions:
1. make this a "helper" class, rather than a top-level base class.
2. let the tests use this helper class as needed.

wdyt?

[gkanwar1, 2013-08-02 17:47:30]

Based on the discussion in the doc, I merged instrumentation_test_case back into
test_case. This also rebases onto the options object change.

[frankf, 2013-08-02 18:18:38]

lgtm

https://codereview.chromium.org/19537004/diff/74001/chrome/android/host_drive...
File chrome/android/host_driven_tests/DummyTest.py (right):

https://codereview.chromium.org/19537004/diff/74001/chrome/android/host_drive...
chrome/android/host_driven_tests/DummyTest.py:18: """Dummy host-driven test for
testing the framework itself."""
So you still need a downstream patch because of these renames.

[frankf, 2013-08-02 18:50:40]

On 2013/08/02 18:18:38, frankf wrote:
> lgtm
> 
>
https://codereview.chromium.org/19537004/diff/74001/chrome/android/host_drive...
> File chrome/android/host_driven_tests/DummyTest.py (right):
> 
>
https://codereview.chromium.org/19537004/diff/74001/chrome/android/host_drive...
> chrome/android/host_driven_tests/DummyTest.py:18: """Dummy host-driven test
for
> testing the framework itself."""
> So you still need a downstream patch because of these renames.

Also, base/sharded_tests_queue.py should also be removed.

[gkanwar1, 2013-08-02 19:56:21]

https://codereview.chromium.org/19537004/diff/74001/chrome/android/host_drive...
File chrome/android/host_driven_tests/DummyTest.py (right):

https://codereview.chromium.org/19537004/diff/74001/chrome/android/host_drive...
chrome/android/host_driven_tests/DummyTest.py:18: """Dummy host-driven test for
testing the framework itself."""
On 2013/08/02 18:18:39, frankf wrote:
> So you still need a downstream patch because of these renames.

Good point. I also need to update Popular Urls tests to match the new
host-driven system. Working on a downstream patch now.

[bulach, 2013-08-05 10:18:20]

lgtm, good stuff! some small suggestions re: docs, feel free to go ahead once
frank and craig are happy.

https://codereview.chromium.org/19537004/diff/109001/build/android/pylib/host...
File build/android/pylib/host_driven/test_case.py (left):

https://codereview.chromium.org/19537004/diff/109001/build/android/pylib/host...
build/android/pylib/host_driven/test_case.py:17: Tests can basically do whatever
they want in the test methods, such as call
I think this bit should still remain, it's part of the core functionality here..

https://codereview.chromium.org/19537004/diff/109001/build/android/pylib/host...
File build/android/pylib/host_driven/test_case.py (right):

https://codereview.chromium.org/19537004/diff/109001/build/android/pylib/host...
build/android/pylib/host_driven/test_case.py:79: # instrumentation_options being
defined.
nit: I think 75 is not very descriptive, and 77-79 could be part of the
docstring after 82, wdyt?

[gkanwar1, 2013-08-05 17:35:49]

https://codereview.chromium.org/19537004/diff/109001/build/android/pylib/host...
File build/android/pylib/host_driven/test_case.py (left):

https://codereview.chromium.org/19537004/diff/109001/build/android/pylib/host...
build/android/pylib/host_driven/test_case.py:17: Tests can basically do whatever
they want in the test methods, such as call
On 2013/08/05 10:18:20, bulach wrote:
> I think this bit should still remain, it's part of the core functionality
here..

Good point. I think I removed this when moving Java test functionality out of
this file, and didn't notice that it needed to be re-added. Done with some
modifications.

https://codereview.chromium.org/19537004/diff/109001/build/android/pylib/host...
File build/android/pylib/host_driven/test_case.py (right):

https://codereview.chromium.org/19537004/diff/109001/build/android/pylib/host...
build/android/pylib/host_driven/test_case.py:79: # instrumentation_options being
defined.
On 2013/08/05 10:18:20, bulach wrote:
> nit: I think 75 is not very descriptive, and 77-79 could be part of the
> docstring after 82, wdyt?

Fair enough. I think it makes sense to move this the docstings of both
_RunJavaTests and _RunJavaTest.

[commit-bot: I haz the power, 2013-08-06 19:08:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gkanwar@chromium.org/19537004/129001

[commit-bot: I haz the power, 2013-08-06 19:10:18]

Change committed as 215944