Android gtest runner: Use first test list result rather than merging

Android gtest runner: Use first test list result rather than merging

Minor simplification and added comment.

[agrieve, 2015-10-08 01:48:36]

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

[agrieve, 2015-10-08 01:50:04]

On 2015/10/08 01:48:36, agrieve wrote:
> mailto:agrieve@chromium.org changed reviewers:
> + mailto:jbudorick@chromium.org

AFAICT, tests are defined using macros, so there shouldn't be a way to get a
different list based on the device. Tests are also sharded between devices with
no regard for which device declared it having a given test, so that's another
reason I think they all have the same test list.

Doesn't really speed things up since this happens in parallel, but does simplify
the logs a bit.

[agrieve, 2015-10-08 02:59:55]

The CQ bit was checked by agrieve@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-08 03:00:13]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1393203002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1393203002/1

[commit-bot: I haz the power, 2015-10-08 04:12:43]

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

[commit-bot: I haz the power, 2015-10-08 04:12:44]

Dry run: This issue passed the CQ dry run.

[jbudorick, 2015-10-08 14:35:41]

https://codereview.chromium.org/1393203002/diff/1/build/android/pylib/local/d...
File build/android/pylib/local/device/local_device_gtest_run.py (right):

https://codereview.chromium.org/1393203002/diff/1/build/android/pylib/local/d...
build/android/pylib/local/device/local_device_gtest_run.py:233: return
list_tests(self._env.devices[0])
The problem with this is handling the case where listing the test fails on one
device.

[agrieve, 2015-10-08 15:49:29]

https://codereview.chromium.org/1393203002/diff/1/build/android/pylib/local/d...
File build/android/pylib/local/device/local_device_gtest_run.py (right):

https://codereview.chromium.org/1393203002/diff/1/build/android/pylib/local/d...
build/android/pylib/local/device/local_device_gtest_run.py:233: return
list_tests(self._env.devices[0])
On 2015/10/08 14:35:41, jbudorick wrote:
> The problem with this is handling the case where listing the test fails on one
> device.

Wondered if that was it.

Okay if I change this to add such a comment and just use the first non-None
result? (and add back in sorting)

[jbudorick, 2015-10-08 15:52:46]

https://codereview.chromium.org/1393203002/diff/1/build/android/pylib/local/d...
File build/android/pylib/local/device/local_device_gtest_run.py (right):

https://codereview.chromium.org/1393203002/diff/1/build/android/pylib/local/d...
build/android/pylib/local/device/local_device_gtest_run.py:233: return
list_tests(self._env.devices[0])
On 2015/10/08 15:49:29, agrieve wrote:
> On 2015/10/08 14:35:41, jbudorick wrote:
> > The problem with this is handling the case where listing the test fails on
one
> > device.
> 
> Wondered if that was it.
> 
> Okay if I change this to add such a comment and just use the first non-None
> result? (and add back in sorting)

Definitely add a comment, and I think that using the first non-None result
should be ok. I debated between that and the current implementation when I wrote
this not that long ago, and I do think the results should be the same on
successful execution.

[agrieve, 2015-10-08 16:11:49]

On 2015/10/08 15:52:46, jbudorick wrote:
>
https://codereview.chromium.org/1393203002/diff/1/build/android/pylib/local/d...
> File build/android/pylib/local/device/local_device_gtest_run.py (right):
> 
>
https://codereview.chromium.org/1393203002/diff/1/build/android/pylib/local/d...
> build/android/pylib/local/device/local_device_gtest_run.py:233: return
> list_tests(self._env.devices[0])
> On 2015/10/08 15:49:29, agrieve wrote:
> > On 2015/10/08 14:35:41, jbudorick wrote:
> > > The problem with this is handling the case where listing the test fails on
> one
> > > device.
> > 
> > Wondered if that was it.
> > 
> > Okay if I change this to add such a comment and just use the first non-None
> > result? (and add back in sorting)
> 
> Definitely add a comment, and I think that using the first non-None result
> should be ok. I debated between that and the current implementation when I
wrote
> this not that long ago, and I do think the results should be the same on
> successful execution.

Actually, wdyt about just querying the devices in series in the case of failure?
I'm assuming failure is infrequent, and want to try and save on log pollution.

[jbudorick, 2015-10-08 16:12:29]

On 2015/10/08 16:11:49, agrieve wrote:
> On 2015/10/08 15:52:46, jbudorick wrote:
> >
>
https://codereview.chromium.org/1393203002/diff/1/build/android/pylib/local/d...
> > File build/android/pylib/local/device/local_device_gtest_run.py (right):
> > 
> >
>
https://codereview.chromium.org/1393203002/diff/1/build/android/pylib/local/d...
> > build/android/pylib/local/device/local_device_gtest_run.py:233: return
> > list_tests(self._env.devices[0])
> > On 2015/10/08 15:49:29, agrieve wrote:
> > > On 2015/10/08 14:35:41, jbudorick wrote:
> > > > The problem with this is handling the case where listing the test fails
on
> > one
> > > > device.
> > > 
> > > Wondered if that was it.
> > > 
> > > Okay if I change this to add such a comment and just use the first
non-None
> > > result? (and add back in sorting)
> > 
> > Definitely add a comment, and I think that using the first non-None result
> > should be ok. I debated between that and the current implementation when I
> wrote
> > this not that long ago, and I do think the results should be the same on
> > successful execution.
> 
> Actually, wdyt about just querying the devices in series in the case of
failure?
> I'm assuming failure is infrequent, and want to try and save on log pollution.

I'd rather do it in parallel. It's less infrequent than you might think :(

[agrieve, 2015-10-08 17:11:02]

On 2015/10/08 16:12:29, jbudorick wrote:
> On 2015/10/08 16:11:49, agrieve wrote:
> > On 2015/10/08 15:52:46, jbudorick wrote:
> > >
> >
>
https://codereview.chromium.org/1393203002/diff/1/build/android/pylib/local/d...
> > > File build/android/pylib/local/device/local_device_gtest_run.py (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1393203002/diff/1/build/android/pylib/local/d...
> > > build/android/pylib/local/device/local_device_gtest_run.py:233: return
> > > list_tests(self._env.devices[0])
> > > On 2015/10/08 15:49:29, agrieve wrote:
> > > > On 2015/10/08 14:35:41, jbudorick wrote:
> > > > > The problem with this is handling the case where listing the test
fails
> on
> > > one
> > > > > device.
> > > > 
> > > > Wondered if that was it.
> > > > 
> > > > Okay if I change this to add such a comment and just use the first
> non-None
> > > > result? (and add back in sorting)
> > > 
> > > Definitely add a comment, and I think that using the first non-None result
> > > should be ok. I debated between that and the current implementation when I
> > wrote
> > > this not that long ago, and I do think the results should be the same on
> > > successful execution.
> > 
> > Actually, wdyt about just querying the devices in series in the case of
> failure?
> > I'm assuming failure is infrequent, and want to try and save on log
pollution.
> 
> I'd rather do it in parallel. It's less infrequent than you might think :(

:( done.

[jbudorick, 2015-10-08 20:39:37]

https://codereview.chromium.org/1393203002/diff/20001/build/android/pylib/loc...
File build/android/pylib/local/device/local_device_gtest_run.py (right):

https://codereview.chromium.org/1393203002/diff/20001/build/android/pylib/loc...
build/android/pylib/local/device/local_device_gtest_run.py:234: return
sorted(next(l for l in test_lists if l))
How does this handle the filter resolving to no tests?

[agrieve, 2015-10-09 00:13:51]

https://codereview.chromium.org/1393203002/diff/20001/build/android/pylib/loc...
File build/android/pylib/local/device/local_device_gtest_run.py (right):

https://codereview.chromium.org/1393203002/diff/20001/build/android/pylib/loc...
build/android/pylib/local/device/local_device_gtest_run.py:234: return
sorted(next(l for l in test_lists if l))
On 2015/10/08 20:39:37, jbudorick wrote:
> How does this handle the filter resolving to no tests?

Yikes! nice catch! Fixed.

[jbudorick, 2015-10-09 00:32:30]

almost missed the switch from "not l" to "l is not None"

lgtm

[agrieve, 2015-10-09 01:00:54]

The CQ bit was checked by agrieve@chromium.org

[commit-bot: I haz the power, 2015-10-09 01:02:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1393203002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1393203002/40001

[commit-bot: I haz the power, 2015-10-09 03:35:11]

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

[commit-bot: I haz the power, 2015-10-09 03:35:12]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[agrieve, 2015-10-09 14:36:50]

The CQ bit was checked by agrieve@chromium.org

[commit-bot: I haz the power, 2015-10-09 14:37:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1393203002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1393203002/40001

[jbudorick, 2015-10-09 16:15:55]

On 2015/10/09 03:35:12, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

those three failures look strangely consistent...

[agrieve, 2015-10-09 17:14:40]

On 2015/10/09 16:15:55, jbudorick wrote:
> On 2015/10/09 03:35:12, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
> 
> those three failures look strangely consistent...

Is there more there than a timeout? Looks like telemetry_perf_unittests failed
on the next run...

There's actually no change in how the command is run though, only a change in
how the results are merged afterwards, so I don't get it...

[commit-bot: I haz the power, 2015-10-09 17:52:25]

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

[commit-bot: I haz the power, 2015-10-09 17:52:26]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[agrieve, 2015-10-09 20:05:57]

On 2015/10/09 17:52:26, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

Abandoning. Only theory I have is that if all devices fail, the new way would
throw and the old way would return an empty test list. That said, looking at
other runs it seems to be working fine. Not going to waste any more time on it.

[jbudorick, 2015-10-09 20:57:31]

On 2015/10/09 20:05:57, agrieve wrote:
> On 2015/10/09 17:52:26, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
> 
> Abandoning. Only theory I have is that if all devices fail, the new way would
> throw and the old way would return an empty test list. That said, looking at
> other runs it seems to be working fine. Not going to waste any more time on
it.

k. I'm seeing this issue elsewhere as well, so I'll be separately investigating.