[Android] Implement gtest and local in platform mode.

[Android] Implement gtest and local in platform mode.

BUG=428729
Committed: https://crrev.com/8f4952879b1ed5fb544e1ad86730672722c93e2e
Cr-Commit-Position: refs/heads/master@{#308441}

[jbudorick, 2014-12-09 01:48:26]

jbudorick@chromium.org changed reviewers:
+ klundberg@chromium.org, mikecase@chromium.org, rnephew@chromium.org

[jbudorick, 2014-12-09 01:48:27]

This has a number of merge conflicts with Randy's AMP CL
(https://codereview.chromium.org/745793002/), but they shouldn't be too
difficult to resolve.

I'll add owners for chrome/ and testing/ once we work through everything in
build/android/.

[klundberg, 2014-12-09 02:30:47]

https://codereview.chromium.org/788753002/diff/40001/build/android/pylib/base...
File build/android/pylib/base/test_run_factory.py (right):

https://codereview.chromium.org/788753002/diff/40001/build/android/pylib/base...
build/android/pylib/base/test_run_factory.py:11: 
This empty line seems out of place/unneeded.

https://codereview.chromium.org/788753002/diff/40001/build/android/pylib/base...
build/android/pylib/base/test_run_factory.py:16: # TODO(jbudorick) Add local
instrumentation test runs.
Kinda nitpicky: I know that Randy is adding some support for remote tests. But I
didn't see a TODO for adding instrumentation test support for remote devices. It
just seems like if we have TODO for adding it for local, we should also have
TODO for adding it for remote (or just have a single TODO for both).

https://codereview.chromium.org/788753002/diff/40001/build/android/pylib/gtes...
File build/android/pylib/gtest/gtest_test_instance.py (right):

https://codereview.chromium.org/788753002/diff/40001/build/android/pylib/gtes...
build/android/pylib/gtest/gtest_test_instance.py:41: # TODO(jbudorick): Make
this a classmethod of GtestTestInstance once
s/classmethod/class method

https://codereview.chromium.org/788753002/diff/40001/build/android/pylib/gtes...
build/android/pylib/gtest/gtest_test_instance.py:80: # TODO(jbudorick): accept
theirs during merge conflicts
I'm assuming this is just a note for yourself until you have merged with Randy's
CL?

https://codereview.chromium.org/788753002/diff/40001/build/android/pylib/gtes...
File build/android/pylib/gtest/local_device_gtest_run.py (right):

https://codereview.chromium.org/788753002/diff/40001/build/android/pylib/gtes...
build/android/pylib/gtest/local_device_gtest_run.py:26: _RE_TEST_STATUS =
re.compile(
Might be worth having a note that these are needed due to the way we parse the
output (and maybe even have a TODO to remove once we transition away from
parsing stdout)?

https://codereview.chromium.org/788753002/diff/40001/build/android/pylib/gtes...
build/android/pylib/gtest/local_device_gtest_run.py:31: # Maybe this should move
to the test instance?
If this a question for the reviewers?
If this is only used here, I think it is fine to have it here.

https://codereview.chromium.org/788753002/diff/40001/build/android/pylib/gtes...
build/android/pylib/gtest/local_device_gtest_run.py:54:
self._server_factories.append(test_server_spawner_factory)
I think I remember that different tests have different factories. But where will
additional factories be added?
Just trying to understand how _server_factories will be populated with more than
this factory.

https://codereview.chromium.org/788753002/diff/40001/build/android/pylib/gtes...
build/android/pylib/gtest/local_device_gtest_run.py:63: def
individual_device_set_up(dev, hdt):
I'm not sure what hdt is supposed to stand for?
Would it be possible to use a more meaningful name? (even if this was what was
used before).

https://codereview.chromium.org/788753002/diff/40001/build/android/pylib/gtes...
build/android/pylib/gtest/local_device_gtest_run.py:64: # install test apk
Install test APK.

https://codereview.chromium.org/788753002/diff/40001/build/android/pylib/gtes...
build/android/pylib/gtest/local_device_gtest_run.py:67: # push data deps
Push data dependencies.

https://codereview.chromium.org/788753002/diff/40001/build/android/pylib/loca...
File build/android/pylib/local/device/local_device_environment.py (right):

https://codereview.chromium.org/788753002/diff/40001/build/android/pylib/loca...
build/android/pylib/local/device/local_device_environment.py:22: #
TODO(jbudorick) This can be refined to support filters etc.
: after (jbudorick)

https://codereview.chromium.org/788753002/diff/40001/build/android/pylib/loca...
File build/android/pylib/local/device/local_device_test_run.py (right):

https://codereview.chromium.org/788753002/diff/40001/build/android/pylib/loca...
build/android/pylib/local/device/local_device_test_run.py:27: if
isinstance(tests, test_collection.TestCollection):
Should this really be called for each test in tests? Or after the loop?

https://codereview.chromium.org/788753002/diff/40001/build/android/pylib/loca...
build/android/pylib/local/device/local_device_test_run.py:39: for tr in
try_results:
I know why you are using tr but I personally prefer longer more meaningful
names.

[jbudorick, 2014-12-09 15:46:52]

https://codereview.chromium.org/788753002/diff/40001/build/android/pylib/base...
File build/android/pylib/base/test_run_factory.py (right):

https://codereview.chromium.org/788753002/diff/40001/build/android/pylib/base...
build/android/pylib/base/test_run_factory.py:11: 
On 2014/12/09 02:30:47, klundberg wrote:
> This empty line seems out of place/unneeded.

Done.

https://codereview.chromium.org/788753002/diff/40001/build/android/pylib/base...
build/android/pylib/base/test_run_factory.py:16: # TODO(jbudorick) Add local
instrumentation test runs.
On 2014/12/09 02:30:46, klundberg wrote:
> Kinda nitpicky: I know that Randy is adding some support for remote tests. But
I
> didn't see a TODO for adding instrumentation test support for remote devices.
It
> just seems like if we have TODO for adding it for local, we should also have
> TODO for adding it for remote (or just have a single TODO for both).

Added a TODO(rnephew) for remote_device instrumentation test runs.

https://codereview.chromium.org/788753002/diff/40001/build/android/pylib/gtes...
File build/android/pylib/gtest/gtest_test_instance.py (right):

https://codereview.chromium.org/788753002/diff/40001/build/android/pylib/gtes...
build/android/pylib/gtest/gtest_test_instance.py:41: # TODO(jbudorick): Make
this a classmethod of GtestTestInstance once
On 2014/12/09 02:30:47, klundberg wrote:
> s/classmethod/class method 

Done.

https://codereview.chromium.org/788753002/diff/40001/build/android/pylib/gtes...
build/android/pylib/gtest/gtest_test_instance.py:80: # TODO(jbudorick): accept
theirs during merge conflicts
On 2014/12/09 02:30:47, klundberg wrote:
> I'm assuming this is just a note for yourself until you have merged with
Randy's
> CL?

Yes, it is. I will probably hold this CL until Randy's lands, as I think his is
more urgent.

https://codereview.chromium.org/788753002/diff/40001/build/android/pylib/gtes...
File build/android/pylib/gtest/local_device_gtest_run.py (right):

https://codereview.chromium.org/788753002/diff/40001/build/android/pylib/gtes...
build/android/pylib/gtest/local_device_gtest_run.py:26: _RE_TEST_STATUS =
re.compile(
On 2014/12/09 02:30:47, klundberg wrote:
> Might be worth having a note that these are needed due to the way we parse the
> output (and maybe even have a TODO to remove once we transition away from
> parsing stdout)?

Done.

https://codereview.chromium.org/788753002/diff/40001/build/android/pylib/gtes...
build/android/pylib/gtest/local_device_gtest_run.py:31: # Maybe this should move
to the test instance?
On 2014/12/09 02:30:47, klundberg wrote:
> If this a question for the reviewers?
> If this is only used here, I think it is fine to have it here.

No, this was a question for me. Whether or not this moves up to the test
instance depends on how we solve the net test server for remote_device.
Rephrased as a TODO.

https://codereview.chromium.org/788753002/diff/40001/build/android/pylib/gtes...
build/android/pylib/gtest/local_device_gtest_run.py:54:
self._server_factories.append(test_server_spawner_factory)
On 2014/12/09 02:30:47, klundberg wrote:
> I think I remember that different tests have different factories. But where
will
> additional factories be added?
> Just trying to understand how _server_factories will be populated with more
than
> this factory.

At the time, I wasn't sure if the gtests would need any other test servers to be
spun up (e.g. the http server). I've removed _server_factories and switched to
direct creation of LocalTestServerSpawner as necessary.

https://codereview.chromium.org/788753002/diff/40001/build/android/pylib/gtes...
build/android/pylib/gtest/local_device_gtest_run.py:63: def
individual_device_set_up(dev, hdt):
On 2014/12/09 02:30:47, klundberg wrote:
> I'm not sure what hdt is supposed to stand for?
> Would it be possible to use a more meaningful name? (even if this was what was
> used before).

shorthand for host_device_tuples

https://codereview.chromium.org/788753002/diff/40001/build/android/pylib/gtes...
build/android/pylib/gtest/local_device_gtest_run.py:64: # install test apk
On 2014/12/09 02:30:47, klundberg wrote:
> Install test APK.

Done.

https://codereview.chromium.org/788753002/diff/40001/build/android/pylib/gtes...
build/android/pylib/gtest/local_device_gtest_run.py:67: # push data deps
On 2014/12/09 02:30:47, klundberg wrote:
> Push data dependencies.

Done.

https://codereview.chromium.org/788753002/diff/40001/build/android/pylib/loca...
File build/android/pylib/local/device/local_device_test_run.py (right):

https://codereview.chromium.org/788753002/diff/40001/build/android/pylib/loca...
build/android/pylib/local/device/local_device_test_run.py:27: if
isinstance(tests, test_collection.TestCollection):
On 2014/12/09 02:30:47, klundberg wrote:
> Should this really be called for each test in tests? Or after the loop?

Yep, we have to tell the TestCollection each time we finish a test.

https://codereview.chromium.org/788753002/diff/40001/build/android/pylib/loca...
build/android/pylib/local/device/local_device_test_run.py:39: for tr in
try_results:
On 2014/12/09 02:30:47, klundberg wrote:
> I know why you are using tr but I personally prefer longer more meaningful
> names. 

expanded:
 tr -> try_result
 r -> result

[klundberg, 2014-12-09 15:55:33]

LGTM with one nit and once you have merged with Randy's change (and removed the
TODO for the merge:-)).

https://codereview.chromium.org/788753002/diff/60001/build/android/pylib/gtes...
File build/android/pylib/gtest/local_device_gtest_run.py (right):

https://codereview.chromium.org/788753002/diff/60001/build/android/pylib/gtes...
build/android/pylib/gtest/local_device_gtest_run.py:50: self._server_factories =
[]
Should be removed now that you no longer use it anywhere.

[rnephew (Reviews Here), 2014-12-09 15:58:37]

On 2014/12/09 15:55:33, klundberg wrote:
> LGTM with one nit and once you have merged with Randy's change (and removed
the
> TODO for the merge:-)).
> 
>
https://codereview.chromium.org/788753002/diff/60001/build/android/pylib/gtes...
> File build/android/pylib/gtest/local_device_gtest_run.py (right):
> 
>
https://codereview.chromium.org/788753002/diff/60001/build/android/pylib/gtes...
> build/android/pylib/gtest/local_device_gtest_run.py:50: self._server_factories
=
> []
> Should be removed now that you no longer use it anywhere.

My biggest worry is I don't know how long it will take to get mine landed, since
it is dependent on getting Requests into third party.

[jbudorick, 2014-12-09 16:00:23]

On 2014/12/09 15:58:37, rnephew1 wrote:
> On 2014/12/09 15:55:33, klundberg wrote:
> > LGTM with one nit and once you have merged with Randy's change (and removed
> the
> > TODO for the merge:-)).
> > 
> >
>
https://codereview.chromium.org/788753002/diff/60001/build/android/pylib/gtes...
> > File build/android/pylib/gtest/local_device_gtest_run.py (right):
> > 
> >
>
https://codereview.chromium.org/788753002/diff/60001/build/android/pylib/gtes...
> > build/android/pylib/gtest/local_device_gtest_run.py:50:
self._server_factories
> =
> > []
> > Should be removed now that you no longer use it anywhere.
> 
> My biggest worry is I don't know how long it will take to get mine landed,
since
> it is dependent on getting Requests into third party.

Fortunately, this one isn't too urgent, and it not landing for a little while
won't block me elsewhere.

[jbudorick, 2014-12-10 22:17:40]

Patchset #5 (id:80001) has been deleted

[jbudorick, 2014-12-11 01:01:02]

Revised to support the executable tests (breakpad_unittests and
sandbox_linux_unittests) and retries.

https://codereview.chromium.org/788753002/diff/60001/build/android/pylib/gtes...
File build/android/pylib/gtest/local_device_gtest_run.py (right):

https://codereview.chromium.org/788753002/diff/60001/build/android/pylib/gtes...
build/android/pylib/gtest/local_device_gtest_run.py:50: self._server_factories =
[]
On 2014/12/09 15:55:33, klundberg wrote:
> Should be removed now that you no longer use it anywhere.

Done.

[klundberg, 2014-12-11 03:45:39]

Still lgtm

Just one minor question.

https://codereview.chromium.org/788753002/diff/100001/build/android/pylib/gte...
File build/android/pylib/gtest/gtest_test_instance.py (right):

https://codereview.chromium.org/788753002/diff/100001/build/android/pylib/gte...
build/android/pylib/gtest/gtest_test_instance.py:124: if self._suite ==
'breakpad_unittests':
Anything we can do to avoid this special case in the future?
Just seems annoying that we have to special case it.

[jbudorick, 2014-12-11 17:43:24]

https://codereview.chromium.org/788753002/diff/100001/build/android/pylib/gte...
File build/android/pylib/gtest/gtest_test_instance.py (right):

https://codereview.chromium.org/788753002/diff/100001/build/android/pylib/gte...
build/android/pylib/gtest/gtest_test_instance.py:124: if self._suite ==
'breakpad_unittests':
On 2014/12/11 03:45:38, klundberg wrote:
> Anything we can do to avoid this special case in the future?
> Just seems annoying that we have to special case it.

Agreed, it is annoying. At a glance, though, it appears that it's not easily
resolvable. The external storage directory thing is handled in a file in //base,
which //breakpad does not appear to use.

[jbudorick, 2014-12-13 02:13:03]

jbudorick@chromium.org changed reviewers:
+ tedchoc@chromium.org

[jbudorick, 2014-12-13 02:13:04]

+tedchoc for chrome/test/android/ and testing/android/ OWNERS

[Ted C, 2014-12-15 18:41:54]

On 2014/12/13 02:13:04, jbudorick wrote:
> +tedchoc for chrome/test/android/ and testing/android/ OWNERS

OWNERS - lgtm

[jbudorick, 2014-12-15 20:01:53]

The CQ bit was checked by jbudorick@chromium.org

[commit-bot: I haz the power, 2014-12-15 20:03:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/788753002/160001

[commit-bot: I haz the power, 2014-12-15 21:02:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-15 21:02:20]

Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)

[jbudorick, 2014-12-15 21:23:31]

The CQ bit was checked by jbudorick@chromium.org

[commit-bot: I haz the power, 2014-12-15 21:23:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/788753002/180001

[commit-bot: I haz the power, 2014-12-15 22:23:48]

Committed patchset #9 (id:180001)

[commit-bot: I haz the power, 2014-12-15 22:24:35]

Patchset 9 (id:??) landed as
https://crrev.com/8f4952879b1ed5fb544e1ad86730672722c93e2e
Cr-Commit-Position: refs/heads/master@{#308441}