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..e405cb68169a667ef713e079f30f2173105b7571
--- /dev/null
+++ b/build/android/test_wrapper/test_runner_wrapper.py
@@ -0,0 +1,57 @@
+#!/usr/bin/env python
+# 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
+

[ghost stip (do not use), 2016/07/27 01:14:18]

nit: two newlines before top level declarations

[nicholaslin, 2016/07/27 19:08:06]

Done.

+def CommandParser():
+  # Parses the command line arguments being passed in
+  parser = argparse.ArgumentParser()
+  parser.add_argument('--test-runner-command',

[ghost stip (do not use), 2016/07/27 01:14:17]

so I think we have to redo this a bit. we should synthesize the logdog command
in this file, and give flags fpr all the confirurable stuff. then we can specify
the test-runner command as nargs

example:

parser.add_argument('--logdog-binary')
parser.add_argument('--project')
parser.add_argument('--prefix')
parser.add_argument('--source')
[...]
parser.add_argument('test_command', nargs='+')  # note the lack of dash

logdog_cmd = [args.logdog_binary,
    '-project', args.project,
    '-output', args.output, ...]

test_command = args.test_command

then you can do this:
python test_runner_wrapper.py --logdog-binary /bin/logdog_butler --project
chromium --output logdog,host=luci-logdog-dev.appspot.com --
bin/run_chromium_public_test --cool_test_arugument
--haha-this-wont-be-interpreted --double-dash-by-itself-means-stop-parsing

you want to avoid parsing and combining by spaces, a world of hurt lies beyond.
using this means we can stay with native shell parsing while still having a
reasonable footprint

[nicholaslin, 2016/07/27 19:08:06]

Do we need a test_command argument? Placing all logdog arguments in front of the
test_runner args in mb.py and using parse_known_args should give the same
result. 

Using main as an example:

logdog_args, test_cmd = parser.parse_known_args(sys.argv[1:])

and the command is:
./test_runner_wrapper.py --logdog-bin-cmd ./../../bin/logdog_butler --project
chromium --output logdog,host=luci-logdog-dev.appspot.com --prefix
android/swarming/logcats/testing3145 --source /tmp/logcats/test1 --name
unified_logcats ../../../out-gn/Debug/bin/run_ui_android_unittests
--logcat-output-file /tmp/logcats/test1 -v [extra generated arguments]

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

I'd prefer using the named positional argument and then -- to separate the
parsing. it's what people tend to expect

+                      help=('Command line arguments to run android tests'))
+  parser.add_argument('--logdog-command',
+                      help=('Logdog command line arguments'))
+  return parser
+

[ghost stip (do not use), 2016/07/27 01:14:17]

newline

[nicholaslin, 2016/07/27 19:08:05]

Done.

+def LogdogParser():
+  # Parses the useful logdog flags for generating the URL
+  # This includes the project, prefix, and name flags
+  parser = argparse.ArgumentParser()
+  parser.add_argument('-project')

[ghost stip (do not use), 2016/07/27 01:14:17]

add help here, maybe just saying "passed to logdog"

[nicholaslin, 2016/07/27 19:08:05]

Specifying the logdog args in the CommandParser makes this parser redundant.

+  parser.add_argument('-prefix')
+  parser.add_argument('-name')
+  return parser
+

[ghost stip (do not use), 2016/07/27 01:14:17]

newline

[nicholaslin, 2016/07/27 19:08:06]

Done.

+def CreateUrl(project, prefix, name):
+  url_prefix = prefix.replace('/', '%2F')
+  url = ('https://luci-logdog-dev.appspot.com/v/?s={0}%2F'

[ghost stip (do not use), 2016/07/27 01:14:18]

we use % instead of .format():

url = ('https://luci-logdog-dev.appspot.com/v/?s=%s%%2F'
          '%s%%2F%%2B%%2F%s' % (project, url_prefix, name))

note the double %% for %2F and the like

[nicholaslin, 2016/07/27 19:08:06]

Done.

+         '{1}%2F%2B%2F{2}').format(project, url_prefix, name)
+  return url

[ghost stip (do not use), 2016/07/27 01:14:17]

can just make this return ('https://' ....) instead of saving url and
immediately returning it

[nicholaslin, 2016/07/27 19:08:06]

Done.

+

[ghost stip (do not use), 2016/07/27 01:14:18]

newline

[nicholaslin, 2016/07/27 19:08:06]

Done.

+def main():
+  parser = CommandParser()
+  args, extra_args = parser.parse_known_args(sys.argv[1:])
+  test_cmd = args.test_runner_command.split() + extra_args
+  if args.logdog_command:
+    subprocess.check_call(test_cmd)
+    cmd = args.logdog_command
+    if '${SWARMING_TASK_ID}' in cmd:

[ghost stip (do not use), 2016/07/27 01:14:17]

weird, but does give you flexibility in mb.py. I'll allow it.

+      cmd = cmd.replace('${SWARMING_TASK_ID}',
+                        os.environ.get('SWARMING_TASK_ID'))
+    cmd = cmd.split()

[ghost stip (do not use), 2016/07/27 01:14:17]

this is going to sound nuts but I think we should use json here

[nicholaslin, 2016/07/27 19:08:06]

The new command parser specifies all logdog arguments. Do we still need to use
json?

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

nope, this was leftover from an older review pass. sorry about that

+    logdog_parser = LogdogParser()
+    url_args = logdog_parser.parse_known_args(cmd)[0]
+    logging.basicConfig(level=logging.DEBUG)

[ghost stip (do not use), 2016/07/27 01:14:18]

remove lines 49 and 50

[nicholaslin, 2016/07/27 19:08:06]

Done.

+    logging.info("NOTE: below arguments are for streaming logcats to logdog")
+    url = CreateUrl(url_args.project, url_args.prefix, url_args.name)
+    logging.info('Logcats are located at: %s', url)
+    return subprocess.call(cmd)
+  return subprocess.call(test_cmd)
+
+if __name__ == '__main__':
+  sys.exit(main())