(Reland) Use logdog butler subcommand to run tests.

(Reland) Use logdog butler subcommand to run tests.

Previously we ran tests, without setting butler environment variables.
This will run into NotBootstrappedError when we try to upload test
results through logdog.

In this cl, we use logdog butler subcommand to run tests, which will set
butler environment variables before hand.

BUG=692287
Review-Url: https://codereview.chromium.org/2695963003
Cr-Commit-Position: refs/heads/master@{#456976}
Committed: https://chromium.googlesource.com/chromium/src/+/de2df28c86b13227040aba5f9f5a1d1f869b81eb

patch from issue 2695963003 at patchset 820001 (http://crrev.com/2695963003#ps820001)

Review-Url: https://codereview.chromium.org/2753993002
Cr-Commit-Position: refs/heads/master@{#458264}
Committed: https://chromium.googlesource.com/chromium/src/+/1ad3437632699fb98fdd46a3ba6ad8d63cf10ee5

[BigBossZhiling, 2017-03-16 01:38:46]

The CQ bit was checked by hzl@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-16 01:39:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-16 02:47:45]

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

[commit-bot: I haz the power, 2017-03-16 02:47:45]

Dry run: This issue passed the CQ dry run.

[BigBossZhiling, 2017-03-17 19:01:41]

Description was changed from

==========
(Reland) Use logdog butler subcommand to run tests.

Previously we ran tests, without setting butler environment variables.
This will run into NotBootstrappedError when we try to upload test
results through logdog.

In this cl, we use logdog butler subcommand to run tests, which will set
butler environment variables before hand.

BUG=692287

Review-Url: https://codereview.chromium.org/2695963003
Cr-Commit-Position: refs/heads/master@{#456976}
Committed:
https://chromium.googlesource.com/chromium/src/+/de2df28c86b13227040aba5f9f5a...

patch from issue 2695963003 at patchset 820001
(http://crrev.com/2695963003#ps820001)
==========

to

==========
(Reland) Use logdog butler subcommand to run tests.

Previously we ran tests, without setting butler environment variables.
This will run into NotBootstrappedError when we try to upload test
results through logdog.

In this cl, we use logdog butler subcommand to run tests, which will set
butler environment variables before hand.

BUG=692287

Review-Url: https://codereview.chromium.org/2695963003
Cr-Commit-Position: refs/heads/master@{#456976}
Committed:
https://chromium.googlesource.com/chromium/src/+/de2df28c86b13227040aba5f9f5a...

patch from issue 2695963003 at patchset 820001
(http://crrev.com/2695963003#ps820001)
==========

[BigBossZhiling, 2017-03-17 19:01:42]

hzl@google.com changed reviewers:
+ jbudorick@chromium.org

[jbudorick, 2017-03-17 19:20:21]

https://codereview.chromium.org/2753993002/diff/140001/build/android/pylib/an...
File build/android/pylib/android/logdog_logcat_monitor.py (right):

https://codereview.chromium.org/2753993002/diff/140001/build/android/pylib/an...
build/android/pylib/android/logdog_logcat_monitor.py:63: iter_timeout=0.05):
This *might* be spinning too much. How's the performance with a slightly higher
timeout value?

https://codereview.chromium.org/2753993002/diff/140001/build/android/pylib/lo...
File build/android/pylib/local/device/local_device_instrumentation_test_run.py
(right):

https://codereview.chromium.org/2753993002/diff/140001/build/android/pylib/lo...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:299:
logdog_upload_start = time.time()
We will not want to include this timing/tracing logic in the final version of
this CL, though it's fine to have it now while we tune the iter_timeout.

https://codereview.chromium.org/2753993002/diff/140001/build/android/test_wra...
File build/android/test_wrapper/logdog_wrapper.py (right):

https://codereview.chromium.org/2753993002/diff/140001/build/android/test_wra...
build/android/test_wrapper/logdog_wrapper.py:37:
parser.add_argument('--swarming-bot-file', required=False,
If this is passing down to the test runner's --target-devices-file, it's
probably easiest if it too is --target-devices-file.

[BigBossZhiling, 2017-03-20 03:38:52]

On 2017/03/17 19:20:21, jbudorick wrote:
>
https://codereview.chromium.org/2753993002/diff/140001/build/android/pylib/an...
> File build/android/pylib/android/logdog_logcat_monitor.py (right):
> 
>
https://codereview.chromium.org/2753993002/diff/140001/build/android/pylib/an...
> build/android/pylib/android/logdog_logcat_monitor.py:63: iter_timeout=0.05):
> This *might* be spinning too much. How's the performance with a slightly
higher
> timeout value?
> 
>
https://codereview.chromium.org/2753993002/diff/140001/build/android/pylib/lo...
> File build/android/pylib/local/device/local_device_instrumentation_test_run.py
> (right):
> 
>
https://codereview.chromium.org/2753993002/diff/140001/build/android/pylib/lo...
> build/android/pylib/local/device/local_device_instrumentation_test_run.py:299:
> logdog_upload_start = time.time()
> We will not want to include this timing/tracing logic in the final version of
> this CL, though it's fine to have it now while we tune the iter_timeout.
> 
>
https://codereview.chromium.org/2753993002/diff/140001/build/android/test_wra...
> File build/android/test_wrapper/logdog_wrapper.py (right):
> 
>
https://codereview.chromium.org/2753993002/diff/140001/build/android/test_wra...
> build/android/test_wrapper/logdog_wrapper.py:37:
> parser.add_argument('--swarming-bot-file', required=False,
> If this is passing down to the test runner's --target-devices-file, it's
> probably easiest if it too is --target-devices-file.

Patch 8 is 13:35 and patch 9 is 13:29

[BigBossZhiling, 2017-03-20 04:19:02]

On 2017/03/20 03:38:52, BigBossZhiling wrote:
> On 2017/03/17 19:20:21, jbudorick wrote:
> >
>
https://codereview.chromium.org/2753993002/diff/140001/build/android/pylib/an...
> > File build/android/pylib/android/logdog_logcat_monitor.py (right):
> > 
> >
>
https://codereview.chromium.org/2753993002/diff/140001/build/android/pylib/an...
> > build/android/pylib/android/logdog_logcat_monitor.py:63: iter_timeout=0.05):
> > This *might* be spinning too much. How's the performance with a slightly
> higher
> > timeout value?
> > 
> >
>
https://codereview.chromium.org/2753993002/diff/140001/build/android/pylib/lo...
> > File
build/android/pylib/local/device/local_device_instrumentation_test_run.py
> > (right):
> > 
> >
>
https://codereview.chromium.org/2753993002/diff/140001/build/android/pylib/lo...
> >
build/android/pylib/local/device/local_device_instrumentation_test_run.py:299:
> > logdog_upload_start = time.time()
> > We will not want to include this timing/tracing logic in the final version
of
> > this CL, though it's fine to have it now while we tune the iter_timeout.
> > 
> >
>
https://codereview.chromium.org/2753993002/diff/140001/build/android/test_wra...
> > File build/android/test_wrapper/logdog_wrapper.py (right):
> > 
> >
>
https://codereview.chromium.org/2753993002/diff/140001/build/android/test_wra...
> > build/android/test_wrapper/logdog_wrapper.py:37:
> > parser.add_argument('--swarming-bot-file', required=False,
> > If this is passing down to the test runner's --target-devices-file, it's
> > probably easiest if it too is --target-devices-file.
> 
> Patch 8 is 13:35 and patch 9 is 13:29

Patch 10 is 12:54

[BigBossZhiling, 2017-03-20 06:39:11]

On 2017/03/20 04:19:02, BigBossZhiling wrote:
> On 2017/03/20 03:38:52, BigBossZhiling wrote:
> > On 2017/03/17 19:20:21, jbudorick wrote:
> > >
> >
>
https://codereview.chromium.org/2753993002/diff/140001/build/android/pylib/an...
> > > File build/android/pylib/android/logdog_logcat_monitor.py (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2753993002/diff/140001/build/android/pylib/an...
> > > build/android/pylib/android/logdog_logcat_monitor.py:63:
iter_timeout=0.05):
> > > This *might* be spinning too much. How's the performance with a slightly
> > higher
> > > timeout value?
> > > 
> > >
> >
>
https://codereview.chromium.org/2753993002/diff/140001/build/android/pylib/lo...
> > > File
> build/android/pylib/local/device/local_device_instrumentation_test_run.py
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2753993002/diff/140001/build/android/pylib/lo...
> > >
> build/android/pylib/local/device/local_device_instrumentation_test_run.py:299:
> > > logdog_upload_start = time.time()
> > > We will not want to include this timing/tracing logic in the final version
> of
> > > this CL, though it's fine to have it now while we tune the iter_timeout.
> > > 
> > >
> >
>
https://codereview.chromium.org/2753993002/diff/140001/build/android/test_wra...
> > > File build/android/test_wrapper/logdog_wrapper.py (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2753993002/diff/140001/build/android/test_wra...
> > > build/android/test_wrapper/logdog_wrapper.py:37:
> > > parser.add_argument('--swarming-bot-file', required=False,
> > > If this is passing down to the test runner's --target-devices-file, it's
> > > probably easiest if it too is --target-devices-file.
> > 
> > Patch 8 is 13:35 and patch 9 is 13:29
> 
> Patch 10 is 12:54

patch 11 is 14:43

[BigBossZhiling, 2017-03-20 07:23:14]

On 2017/03/20 06:39:11, BigBossZhiling wrote:
> On 2017/03/20 04:19:02, BigBossZhiling wrote:
> > On 2017/03/20 03:38:52, BigBossZhiling wrote:
> > > On 2017/03/17 19:20:21, jbudorick wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/2753993002/diff/140001/build/android/pylib/an...
> > > > File build/android/pylib/android/logdog_logcat_monitor.py (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2753993002/diff/140001/build/android/pylib/an...
> > > > build/android/pylib/android/logdog_logcat_monitor.py:63:
> iter_timeout=0.05):
> > > > This *might* be spinning too much. How's the performance with a slightly
> > > higher
> > > > timeout value?
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2753993002/diff/140001/build/android/pylib/lo...
> > > > File
> > build/android/pylib/local/device/local_device_instrumentation_test_run.py
> > > > (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2753993002/diff/140001/build/android/pylib/lo...
> > > >
> >
build/android/pylib/local/device/local_device_instrumentation_test_run.py:299:
> > > > logdog_upload_start = time.time()
> > > > We will not want to include this timing/tracing logic in the final
version
> > of
> > > > this CL, though it's fine to have it now while we tune the iter_timeout.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2753993002/diff/140001/build/android/test_wra...
> > > > File build/android/test_wrapper/logdog_wrapper.py (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2753993002/diff/140001/build/android/test_wra...
> > > > build/android/test_wrapper/logdog_wrapper.py:37:
> > > > parser.add_argument('--swarming-bot-file', required=False,
> > > > If this is passing down to the test runner's --target-devices-file, it's
> > > > probably easiest if it too is --target-devices-file.
> > > 
> > > Patch 8 is 13:35 and patch 9 is 13:29
> > 
> > Patch 10 is 12:54
> 
> patch 11 is 14:43

patch 12 is 13:03

[jbudorick, 2017-03-20 13:25:58]

It'd be worthwhile to try a number lower than 0.05, too, especially given the
results of your experiments thus far.

[BigBossZhiling, 2017-03-20 17:24:42]

patch 13 is 13:26

[BigBossZhiling, 2017-03-20 19:25:22]

On 2017/03/20 17:24:42, BigBossZhiling wrote:
> patch 13 is 13:26

patch 14 is 15:27

[BigBossZhiling, 2017-03-20 22:05:38]

On 2017/03/20 19:25:22, BigBossZhiling wrote:
> On 2017/03/20 17:24:42, BigBossZhiling wrote:
> > patch 13 is 13:26
> 
> patch 14 is 15:27

patch 15 first run 14:26 and second run 15:51. patch 16 is 16:46

[BigBossZhiling, 2017-03-20 22:05:39]

On 2017/03/20 19:25:22, BigBossZhiling wrote:
> On 2017/03/20 17:24:42, BigBossZhiling wrote:
> > patch 13 is 13:26
> 
> patch 14 is 15:27

patch 15 first run 14:26 and second run 15:51. patch 16 is 16:46

[BigBossZhiling, 2017-03-20 22:09:50]

After the fix, we should not see a ridiculous bump in time any more. However the
results of multiple runs of the same patch shows that time varies for even the
same patch and in the case of two runs of patch 15, they differ by 1 minute and
a half. Thus I suggest that we land this quickly, so that the law of large
numbers will tell us the true effect of this cl by averaging the time hundreds
of try jobs.

https://codereview.chromium.org/2753993002/diff/140001/build/android/pylib/an...
File build/android/pylib/android/logdog_logcat_monitor.py (right):

https://codereview.chromium.org/2753993002/diff/140001/build/android/pylib/an...
build/android/pylib/android/logdog_logcat_monitor.py:63: iter_timeout=0.05):
On 2017/03/17 19:20:21, jbudorick wrote:
> This *might* be spinning too much. How's the performance with a slightly
higher
> timeout value?

Acknowledged.

https://codereview.chromium.org/2753993002/diff/140001/build/android/pylib/lo...
File build/android/pylib/local/device/local_device_instrumentation_test_run.py
(right):

https://codereview.chromium.org/2753993002/diff/140001/build/android/pylib/lo...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:299:
logdog_upload_start = time.time()
On 2017/03/17 19:20:21, jbudorick wrote:
> We will not want to include this timing/tracing logic in the final version of
> this CL, though it's fine to have it now while we tune the iter_timeout.

Done.

https://codereview.chromium.org/2753993002/diff/140001/build/android/test_wra...
File build/android/test_wrapper/logdog_wrapper.py (right):

https://codereview.chromium.org/2753993002/diff/140001/build/android/test_wra...
build/android/test_wrapper/logdog_wrapper.py:37:
parser.add_argument('--swarming-bot-file', required=False,
On 2017/03/17 19:20:21, jbudorick wrote:
> If this is passing down to the test runner's --target-devices-file, it's
> probably easiest if it too is --target-devices-file.

Done.

[jbudorick, 2017-03-20 22:31:18]

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

[jbudorick, 2017-03-20 22:31:19]

lgtm w/ nit

+dpranke for //tools/mb

https://codereview.chromium.org/2753993002/diff/320001/build/android/pylib/lo...
File build/android/pylib/local/device/local_device_instrumentation_test_run.py
(right):

https://codereview.chromium.org/2753993002/diff/320001/build/android/pylib/lo...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:38:
LOGCAT_FILTERS = ['*:e', 'chromium:v', 'cr_chromium:v',
I think we can just take 'cr_*:v' rather than the list of cr_ filter specs you
have here, some of which are very specific to the linker tests.

[BigBossZhiling, 2017-03-20 22:37:13]

https://codereview.chromium.org/2753993002/diff/320001/build/android/pylib/lo...
File build/android/pylib/local/device/local_device_instrumentation_test_run.py
(right):

https://codereview.chromium.org/2753993002/diff/320001/build/android/pylib/lo...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:38:
LOGCAT_FILTERS = ['*:e', 'chromium:v', 'cr_chromium:v',
On 2017/03/20 22:31:19, jbudorick wrote:
> I think we can just take 'cr_*:v' rather than the list of cr_ filter specs you
> have here, some of which are very specific to the linker tests.

Done.

[Dirk Pranke, 2017-03-20 23:12:48]

https://codereview.chromium.org/2753993002/diff/340001/tools/mb/mb.py
File tools/mb/mb.py (right):

https://codereview.chromium.org/2753993002/diff/340001/tools/mb/mb.py#newcode...
tools/mb/mb.py:1093: '--logdog-bin-cmd', '../../bin/logdog_butler']
is this path right? what directory is that relative to?

[BigBossZhiling, 2017-03-20 23:30:51]

On 2017/03/20 23:12:48, Dirk Pranke wrote:
> https://codereview.chromium.org/2753993002/diff/340001/tools/mb/mb.py
> File tools/mb/mb.py (right):
> 
>
https://codereview.chromium.org/2753993002/diff/340001/tools/mb/mb.py#newcode...
> tools/mb/mb.py:1093: '--logdog-bin-cmd', '../../bin/logdog_butler']
> is this path right? what directory is that relative to?

Thanks for the quick review. ../../bin/logdog_butler is right, because otherwise
we will not be able to see "record_to_stream", which is a function of
logdog_butler, in the logcat. As with the directory that it is relative to, I
think it is relative to out-gn/Debug.

[BigBossZhiling, 2017-03-20 23:30:52]

On 2017/03/20 23:12:48, Dirk Pranke wrote:
> https://codereview.chromium.org/2753993002/diff/340001/tools/mb/mb.py
> File tools/mb/mb.py (right):
> 
>
https://codereview.chromium.org/2753993002/diff/340001/tools/mb/mb.py#newcode...
> tools/mb/mb.py:1093: '--logdog-bin-cmd', '../../bin/logdog_butler']
> is this path right? what directory is that relative to?

Thanks for the quick review. ../../bin/logdog_butler is right, because otherwise
we will not be able to see "record_to_stream", which is a function of
logdog_butler, in the logcat. As with the directory that it is relative to, I
think it is relative to out-gn/Debug.

[Dirk Pranke, 2017-03-20 23:38:54]

On 2017/03/20 23:30:52, BigBossZhiling wrote:
> On 2017/03/20 23:12:48, Dirk Pranke wrote:
> > https://codereview.chromium.org/2753993002/diff/340001/tools/mb/mb.py
> > File tools/mb/mb.py (right):
> > 
> >
>
https://codereview.chromium.org/2753993002/diff/340001/tools/mb/mb.py#newcode...
> > tools/mb/mb.py:1093: '--logdog-bin-cmd', '../../bin/logdog_butler']
> > is this path right? what directory is that relative to?
> 
> Thanks for the quick review. ../../bin/logdog_butler is right, because
otherwise
> we will not be able to see "record_to_stream", which is a function of
> logdog_butler, in the logcat. As with the directory that it is relative to, I
> think it is relative to out-gn/Debug.

If the path is relative to //out-gn/Debug (which makes sense, since that is the
default for commands in the isolate),
then that would point to //bin/logdog_butler.

AFAIK, there is no //bin directory?

-- Dirk

[BigBossZhiling, 2017-03-20 23:41:33]

On 2017/03/20 23:38:54, Dirk Pranke wrote:
> On 2017/03/20 23:30:52, BigBossZhiling wrote:
> > On 2017/03/20 23:12:48, Dirk Pranke wrote:
> > > https://codereview.chromium.org/2753993002/diff/340001/tools/mb/mb.py
> > > File tools/mb/mb.py (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2753993002/diff/340001/tools/mb/mb.py#newcode...
> > > tools/mb/mb.py:1093: '--logdog-bin-cmd', '../../bin/logdog_butler']
> > > is this path right? what directory is that relative to?
> > 
> > Thanks for the quick review. ../../bin/logdog_butler is right, because
> otherwise
> > we will not be able to see "record_to_stream", which is a function of
> > logdog_butler, in the logcat. As with the directory that it is relative to,
I
> > think it is relative to out-gn/Debug.
> 
> If the path is relative to //out-gn/Debug (which makes sense, since that is
the
> default for commands in the isolate),
> then that would point to //bin/logdog_butler.
> 
> AFAIK, there is no //bin directory?
> 
> -- Dirk

Thanks for another quick reply.

https://codesearch.chromium.org/chromium/src/testing/buildbot/chromium.linux....
We create the new bin directory for logdog_butler.

[Dirk Pranke, 2017-03-20 23:50:37]

On 2017/03/20 23:41:33, BigBossZhiling wrote:
>
https://codesearch.chromium.org/chromium/src/testing/buildbot/chromium.linux....
> We create the new bin directory for logdog_butler.

Ah. I was guessing it might be something like that. I'm a bit unhappy to use up
a top-level directory name for this without a more widely-thought-through plan
for where to put cipd binaries, but I guess if we're already doing that now we
don't need to stop just for this.

lgtm.

[BigBossZhiling, 2017-03-20 23:51:07]

The CQ bit was checked by hzl@google.com

[BigBossZhiling, 2017-03-20 23:51:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
jbudorick@chromium.org
Link to the patchset: https://codereview.chromium.org/2753993002/#ps340001
(title: "address John's comments on logdog filter")

[commit-bot: I haz the power, 2017-03-20 23:52:02]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-21 00:58:27]

CQ is committing da patch.

Bot data: {"patchset_id": 340001, "attempt_start_ts": 1490053867468220,
"parent_rev": "0ff8b19608421be5fa5b53e90c097e4992723b90", "commit_rev":
"1ad3437632699fb98fdd46a3ba6ad8d63cf10ee5"}

[commit-bot: I haz the power, 2017-03-21 00:59:18]

Description was changed from

==========
(Reland) Use logdog butler subcommand to run tests.

Previously we ran tests, without setting butler environment variables.
This will run into NotBootstrappedError when we try to upload test
results through logdog.

In this cl, we use logdog butler subcommand to run tests, which will set
butler environment variables before hand.

BUG=692287

Review-Url: https://codereview.chromium.org/2695963003
Cr-Commit-Position: refs/heads/master@{#456976}
Committed:
https://chromium.googlesource.com/chromium/src/+/de2df28c86b13227040aba5f9f5a...

patch from issue 2695963003 at patchset 820001
(http://crrev.com/2695963003#ps820001)
==========

to

==========
(Reland) Use logdog butler subcommand to run tests.

Previously we ran tests, without setting butler environment variables.
This will run into NotBootstrappedError when we try to upload test
results through logdog.

In this cl, we use logdog butler subcommand to run tests, which will set
butler environment variables before hand.

BUG=692287

Review-Url: https://codereview.chromium.org/2695963003
Cr-Commit-Position: refs/heads/master@{#456976}
Committed:
https://chromium.googlesource.com/chromium/src/+/de2df28c86b13227040aba5f9f5a...

patch from issue 2695963003 at patchset 820001
(http://crrev.com/2695963003#ps820001)

Review-Url: https://codereview.chromium.org/2753993002
Cr-Commit-Position: refs/heads/master@{#458264}
Committed:
https://chromium.googlesource.com/chromium/src/+/1ad3437632699fb98fdd46a3ba6a...
==========

[commit-bot: I haz the power, 2017-03-21 00:59:19]

Committed patchset #18 (id:340001) as
https://chromium.googlesource.com/chromium/src/+/1ad3437632699fb98fdd46a3ba6a...

[jbudorick, 2017-03-22 00:12:07]

A revert of this CL (patchset #18 id:340001) has been created in
https://codereview.chromium.org/2767633003/ by jbudorick@chromium.org.

The reason for reverting is:
https://build.chromium.org/p/chromium.android/builders/Marshmallow%2064%20bit....