Logdog for logcats

build/android/test_wrapper/test_runner_wrapper.py

Index: build/android/test_wrapper/test_runner_wrapper.py
diff --git a/build/android/test_wrapper/test_runner_wrapper.py b/build/android/test_wrapper/test_runner_wrapper.py
new file mode 100755
index 0000000000000000000000000000000000000000..e32210b30b1e840b3429194c5a36f5d2703cdc25
--- /dev/null
+++ b/build/android/test_wrapper/test_runner_wrapper.py
@@ -0,0 +1,63 @@
+#!/usr/bin/env python

[jbudorick, 2016/07/29 00:52:39]

This name is too generic. If this wrapper is specific to logdog, it should have
logdog in the name.

[nicholaslin, 2016/07/29 20:50:20]

Done. Should the folder be renamed as well?

+# Copyright 2016 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+import argparse
+import sys
+import os
+import subprocess
+import logging
+
+
+def CommandParser():
+  # Parses the command line arguments being passed in
+  parser = argparse.ArgumentParser()
+  parser.add_argument('--logdog-bin-cmd',
+                      help=('Command for running logdog butler binary'))

[ghost stip (do not use), 2016/07/29 01:09:08]

you don't need the parens here. it can be just help='whatever'

[ghost stip (do not use), 2016/07/29 01:09:08]

required=True

IIUC, all of these should be required right?

[nicholaslin, 2016/07/29 20:50:19]

Done.

[nicholaslin, 2016/07/29 20:50:20]

I'll make logdog arguments required. 

I was thinking about the case of local testing. For example, copying the
isolated inputs command  ./test_runner_wrapper run_ui_android_unittests ...
while ignoring all the logdog arguments this will still run fine right now.
Requiring logdog arguments may cause errors that are confusion to people copying
commands from the isolated inputs. If this additional complexity is not an issue
requiring logdog commands is a good idea.

Do we need to consider the case where logdog may be used differently in the
future? (i.e stream, run, etc.) These will all have different arguments.

+  parser.add_argument('--project',
+                      help=('Name of logdog project'))
+  parser.add_argument('--output',

[ghost stip (do not use), 2016/07/29 01:09:08]

nit: rework the --output flag to just be server name:

parser.add_argument('--logdog-server', default='luci-logdog.appspot.com')

[nicholaslin, 2016/07/29 20:50:20]

Done.

+                      help=('Format of logdog output'))
+  parser.add_argument('--service-account-json',
+                      help=('Location of authentication json'))
+  parser.add_argument('--prefix',
+                      help=('Prefix to be used for logdog stream'))
+  parser.add_argument('--source',
+                      help=('Location of file for logdog to stream'))
+  parser.add_argument('--name',
+                      help=('Name to be used for logdog stream'))
+  return parser
+
+
+def CreateUrl(project, prefix, name):
+  url_prefix = prefix.replace('/', '%2F')
+  return ('https://luci-logdog-dev.appspot.com/v/?s=%s%%2F'

[ghost stip (do not use), 2016/07/29 01:09:08]

use args.logdog_server as specified above

[nicholaslin, 2016/07/29 20:50:20]

Done.

+          '%s%%2F%%2B%%2F%s' % (project, url_prefix, name))
+
+
+def main():
+  parser = CommandParser()
+  args, test_cmd = parser.parse_known_args(sys.argv[1:])

[ghost stip (do not use), 2016/07/29 01:09:08]

if not test_cmd:
  parser.error('must specify a command to run after the logdog flags)

parser.error calls sys.exit(2) after printing

[nicholaslin, 2016/07/29 20:50:20]

Done.

+  if args.logdog_bin_cmd:

[jbudorick, 2016/07/29 00:52:40]

When would we use this wrapper without a logdog_bin_cmd?

[ghost stip (do not use), 2016/07/29 01:09:07]

should make required in the parser

[nicholaslin, 2016/07/29 20:50:20]

I'll rework this file to only consider the case where logdog is being used.
Point about using this wrapper without logdog_bin_cmd is above.

+    subprocess.check_call(test_cmd)
+    if '${SWARMING_TASK_ID}' in args.prefix:
+      args.prefix = args.prefix.replace('${SWARMING_TASK_ID}',
+                        os.environ.get('SWARMING_TASK_ID'))

[jbudorick, 2016/07/29 00:52:40]

nit: indentation is off here. either indent to the depth of the first parameter
or drop the first parameter onto a subsequent line and indent 4 spaces

[nicholaslin, 2016/07/29 20:50:20]

Done.

+    url = CreateUrl(args.project, args.prefix, args.name)
+    logging.info('Logcats are located at: %s', url)
+    logdog_cmd = [args.logdog_bin_cmd, '-project', args.project,
+                  '-output', args.output, '-prefix', args.prefix]

[ghost stip (do not use), 2016/07/29 01:09:08]

'-output', 'logdog,host=%s' % args.logdog_server, 

[nicholaslin, 2016/07/29 20:50:20]

Done.

+    stream_cmd = ['stream', '-source', args.source, '-stream',
+                  ('-name=%s'% args.name)]

[jbudorick, 2016/07/29 00:52:40]

nits: no parens, space before %

[nicholaslin, 2016/07/29 20:50:20]

Done.

+    if args.service_account_json:

[nicholaslin, 2016/07/29 20:50:20]

Requiring all logdog arguments, so taking this part out.

+      cmd = (logdog_cmd + ['-service-account-json', args.service_account_json]
+             + stream_cmd)
+    else:
+      cmd = logdog_cmd + stream_cmd
+    return subprocess.call(cmd)
+  return subprocess.call(test_cmd)
+
+
+if __name__ == '__main__':
+  sys.exit(main())