Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2163833003:
Logdog for logcats (Closed)
|
Created:
4 years, 5 months ago by nicholaslin Modified:
4 years, 4 months ago CC:
chromium-reviews, jbudorick+watch_chromium.org, mikecase+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd logdog_butler cipd package to every swarming gtest_test on android tryservers (linux_android_rel_ng, android_n5x_swarming_rel).
Add wrapper around android test runner for swarming tasks. This wrapper calls logdog to stream logcats after the completion of the python executable calling test_runner.
Adding device serials to every line of logcat logs.
Sample of unified logcats: https://luci-logdog.appspot.com/v/?s=chromium%2Fswarming%2F301f0f97eea1a511%2Flogcats%2F%2B%2Ffile:_b_swarm_slave_w_ioVXQmGw_logcats
BUG=448050
Committed: https://crrev.com/3dbe50ffaca641ea962d385e2ac9ac0bec75fbbb
Cr-Commit-Position: refs/heads/master@{#411530}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Move logs to luci-logdog-dev #Patch Set 3 : Reduce logs namespace #Patch Set 4 : Comment out service-account-json until production #Patch Set 5 : Change logdog parameters #Patch Set 6 : Try new test runner format #
Total comments: 4
Patch Set 7 : Add wrapper to test runner, delete old comments #Patch Set 8 : Fix gn file #Patch Set 9 : Update packages, update git revision for cipd package #Patch Set 10 : Update logdog revision + test_runner_wrapper for stream name support #
Total comments: 29
Patch Set 11 : Address comments #Patch Set 12 : Fix mb.py #Patch Set 13 : Address comments cont. #
Total comments: 27
Patch Set 14 : Address comments part 3 #
Total comments: 26
Patch Set 15 : Address comments part 4 #Patch Set 16 : Fix viewable link in logdog wrapper #Patch Set 17 : Merge master into branch #Patch Set 18 : Fix logdog wrapper call command #Patch Set 19 : Fix logdog live stream #Patch Set 20 : Move from dev to production #
Total comments: 2
Patch Set 21 : Update chromium.android.json #
Total comments: 5
Patch Set 22 : Address comments 5 #
Total comments: 1
Patch Set 23 : Move logging arguments #Patch Set 24 : Add exception of failed phones #Patch Set 25 : Update json files to include links #
Messages
Total messages: 66 (33 generated)
nicholaslin@google.com changed reviewers: + jbudorick@chromium.org, stip@chromium.org
Here is an updated CL at adding unified logcats for android swarming tasks using logdog. PTAL. https://codereview.chromium.org/2163833003/diff/1/build/android/pylib/local/d... File build/android/pylib/local/device/local_device_environment.py (right): https://codereview.chromium.org/2163833003/diff/1/build/android/pylib/local/d... build/android/pylib/local/device/local_device_environment.py:137: format(m._adb.GetDeviceSerial()), I'll change this to m.get_device_serial or something similar once https://codereview.chromium.org/2112643004/diff/40001/devil/devil/android/log... lands. https://codereview.chromium.org/2163833003/diff/1/build/android/pylib/local/d... build/android/pylib/local/device/local_device_environment.py:145: if self._logdog_command and self._logdog_stream: As of right now I don't have a good way to make logdog less intrusive. I realize having two logdog arguments is confusing but I need a way to get the swarming_task_id into the logdog command. As for the URL creation if it's too messy, I can definitely move it into another file or create a new function. https://codereview.chromium.org/2163833003/diff/1/tools/mb/mb.py File tools/mb/mb.py (right): https://codereview.chromium.org/2163833003/diff/1/tools/mb/mb.py#newcode1010 tools/mb/mb.py:1010: '--logcat-output-file', '${ISOLATED_OUTDIR}/logcats', another option here would be to keep the logcat-output-dir and add a logdog related argument argument
https://codereview.chromium.org/2163833003/diff/100001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_environment.py (right): https://codereview.chromium.org/2163833003/diff/100001/build/android/pylib/lo... build/android/pylib/local/device/local_device_environment.py:155: url = 'https://luci-logdog-dev.appspot.com/v/?s=chromium%2F{0}%2F%2B%2F{1}'.format(url_prefix, url_suffix) 80 chars, and are we switching back to prod for this CL? https://codereview.chromium.org/2163833003/diff/100001/tools/mb/mb.py File tools/mb/mb.py (right): https://codereview.chromium.org/2163833003/diff/100001/tools/mb/mb.py#newcode... tools/mb/mb.py:1003: #'-service-account-json', why the commented out code?
Updated this CL to include a wrapper which contains all logdog logic. The only change to the current testing framework is adding device serials to the logcats. ptal. https://codereview.chromium.org/2163833003/diff/100001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_environment.py (right): https://codereview.chromium.org/2163833003/diff/100001/build/android/pylib/lo... build/android/pylib/local/device/local_device_environment.py:155: url = 'https://luci-logdog-dev.appspot.com/v/?s=chromium%2F{0}%2F%2B%2F{1}'.format(url_prefix, url_suffix) On 2016/07/20 22:24:23, stip wrote: > 80 chars, and are we switching back to prod for this CL? I would assume so. Will talk to Dan later. https://codereview.chromium.org/2163833003/diff/100001/tools/mb/mb.py File tools/mb/mb.py (right): https://codereview.chromium.org/2163833003/diff/100001/tools/mb/mb.py#newcode... tools/mb/mb.py:1003: #'-service-account-json', On 2016/07/20 22:24:23, stip wrote: > why the commented out code? These credentials are for the production luci-logdog not the developer luci-logdog-dev.
Description was changed from ========== Adding logdog support for unified logcats on android swarming tasks. BUG=448050 ========== to ========== Adding a wrapper around the test_runner on android swarming tasks. This wrapper calls logdog to stream logcats after the completion of test_runner. Adding device serials to every line of logcat logs. BUG=448050 ==========
Description was changed from ========== Adding a wrapper around the test_runner on android swarming tasks. This wrapper calls logdog to stream logcats after the completion of test_runner. Adding device serials to every line of logcat logs. BUG=448050 ========== to ========== Adding a wrapper around the test_runner on android swarming tasks. This wrapper calls logdog to stream logcats after the completion of the binary command passed into test_runner. Adding device serials to every line of logcat logs. BUG=448050 ==========
Description was changed from ========== Adding a wrapper around the test_runner on android swarming tasks. This wrapper calls logdog to stream logcats after the completion of the binary command passed into test_runner. Adding device serials to every line of logcat logs. BUG=448050 ========== to ========== Add logdog_butler cipd package to every swarming gtest_test on android tryservers (linux_android_rel_ng, android_n5x_swarming_rel). Add wrapper around android test runner for swarming tasks. This wrapper calls logdog to stream logcats after the completion of the binary command passed into test_runner. Adding device serials to every line of logcat logs. BUG=448050 ==========
Description was changed from ========== Add logdog_butler cipd package to every swarming gtest_test on android tryservers (linux_android_rel_ng, android_n5x_swarming_rel). Add wrapper around android test runner for swarming tasks. This wrapper calls logdog to stream logcats after the completion of the binary command passed into test_runner. Adding device serials to every line of logcat logs. BUG=448050 ========== to ========== Add logdog_butler cipd package to every swarming gtest_test on android tryservers (linux_android_rel_ng, android_n5x_swarming_rel). Add wrapper around android test runner for swarming tasks. This wrapper calls logdog to stream logcats after the completion of the python executable calling test_runner. Adding device serials to every line of logcat logs. BUG=448050 ==========
Description was changed from ========== Add logdog_butler cipd package to every swarming gtest_test on android tryservers (linux_android_rel_ng, android_n5x_swarming_rel). Add wrapper around android test runner for swarming tasks. This wrapper calls logdog to stream logcats after the completion of the python executable calling test_runner. Adding device serials to every line of logcat logs. BUG=448050 ========== to ========== Add logdog_butler cipd package to every swarming gtest_test on android tryservers (linux_android_rel_ng, android_n5x_swarming_rel). Add wrapper around android test runner for swarming tasks. This wrapper calls logdog to stream logcats after the completion of the python executable calling test_runner. Adding device serials to every line of logcat logs. Sample of unified logcats: https://luci-logdog.appspot.com/v/?s=chromium%2Fswarming%2F301f0f97eea1a511%2... BUG=448050 ==========
nicholaslin@google.com changed reviewers: + dnj@chromium.org, dpranke@chromium.org
PTAL.
https://codereview.chromium.org/2163833003/diff/180001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_environment.py (right): https://codereview.chromium.org/2163833003/diff/180001/build/android/pylib/lo... build/android/pylib/local/device/local_device_environment.py:192: add_device_args = ['sed', '-i', '-e', rename to add_device_cmd https://codereview.chromium.org/2163833003/diff/180001/build/android/test_wra... File build/android/test_wrapper/test_runner_wrapper.py (right): https://codereview.chromium.org/2163833003/diff/180001/build/android/test_wra... build/android/test_wrapper/test_runner_wrapper.py:11: nit: two newlines before top level declarations https://codereview.chromium.org/2163833003/diff/180001/build/android/test_wra... build/android/test_wrapper/test_runner_wrapper.py:15: parser.add_argument('--test-runner-command', 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 https://codereview.chromium.org/2163833003/diff/180001/build/android/test_wra... build/android/test_wrapper/test_runner_wrapper.py:20: newline https://codereview.chromium.org/2163833003/diff/180001/build/android/test_wra... build/android/test_wrapper/test_runner_wrapper.py:25: parser.add_argument('-project') add help here, maybe just saying "passed to logdog" https://codereview.chromium.org/2163833003/diff/180001/build/android/test_wra... build/android/test_wrapper/test_runner_wrapper.py:29: newline https://codereview.chromium.org/2163833003/diff/180001/build/android/test_wra... build/android/test_wrapper/test_runner_wrapper.py:32: url = ('https://luci-logdog-dev.appspot.com/v/?s={0}%2F' 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 https://codereview.chromium.org/2163833003/diff/180001/build/android/test_wra... build/android/test_wrapper/test_runner_wrapper.py:34: return url can just make this return ('https://' ....) instead of saving url and immediately returning it https://codereview.chromium.org/2163833003/diff/180001/build/android/test_wra... build/android/test_wrapper/test_runner_wrapper.py:35: newline https://codereview.chromium.org/2163833003/diff/180001/build/android/test_wra... build/android/test_wrapper/test_runner_wrapper.py:43: if '${SWARMING_TASK_ID}' in cmd: weird, but does give you flexibility in mb.py. I'll allow it. https://codereview.chromium.org/2163833003/diff/180001/build/android/test_wra... build/android/test_wrapper/test_runner_wrapper.py:46: cmd = cmd.split() this is going to sound nuts but I think we should use json here https://codereview.chromium.org/2163833003/diff/180001/build/android/test_wra... build/android/test_wrapper/test_runner_wrapper.py:49: logging.basicConfig(level=logging.DEBUG) remove lines 49 and 50 https://codereview.chromium.org/2163833003/diff/180001/tools/mb/mb.py File tools/mb/mb.py (right): https://codereview.chromium.org/2163833003/diff/180001/tools/mb/mb.py#newcode... tools/mb/mb.py:1017: #'-service-account-json', don't commit commented-out code https://codereview.chromium.org/2163833003/diff/180001/tools/mb/mb.py#newcode... tools/mb/mb.py:1031: '--test-runner-command', ' '.join(test_cmdline), this should just be the arguments of the program. joining and splitting by space is dangerous
https://codereview.chromium.org/2163833003/diff/180001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_environment.py (right): https://codereview.chromium.org/2163833003/diff/180001/build/android/pylib/lo... build/android/pylib/local/device/local_device_environment.py:192: add_device_args = ['sed', '-i', '-e', rename to add_device_cmd https://codereview.chromium.org/2163833003/diff/180001/build/android/test_wra... File build/android/test_wrapper/test_runner_wrapper.py (right): https://codereview.chromium.org/2163833003/diff/180001/build/android/test_wra... build/android/test_wrapper/test_runner_wrapper.py:11: nit: two newlines before top level declarations https://codereview.chromium.org/2163833003/diff/180001/build/android/test_wra... build/android/test_wrapper/test_runner_wrapper.py:15: parser.add_argument('--test-runner-command', 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 https://codereview.chromium.org/2163833003/diff/180001/build/android/test_wra... build/android/test_wrapper/test_runner_wrapper.py:20: newline https://codereview.chromium.org/2163833003/diff/180001/build/android/test_wra... build/android/test_wrapper/test_runner_wrapper.py:25: parser.add_argument('-project') add help here, maybe just saying "passed to logdog" https://codereview.chromium.org/2163833003/diff/180001/build/android/test_wra... build/android/test_wrapper/test_runner_wrapper.py:29: newline https://codereview.chromium.org/2163833003/diff/180001/build/android/test_wra... build/android/test_wrapper/test_runner_wrapper.py:32: url = ('https://luci-logdog-dev.appspot.com/v/?s={0}%2F' 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 https://codereview.chromium.org/2163833003/diff/180001/build/android/test_wra... build/android/test_wrapper/test_runner_wrapper.py:34: return url can just make this return ('https://' ....) instead of saving url and immediately returning it https://codereview.chromium.org/2163833003/diff/180001/build/android/test_wra... build/android/test_wrapper/test_runner_wrapper.py:35: newline https://codereview.chromium.org/2163833003/diff/180001/build/android/test_wra... build/android/test_wrapper/test_runner_wrapper.py:43: if '${SWARMING_TASK_ID}' in cmd: weird, but does give you flexibility in mb.py. I'll allow it. https://codereview.chromium.org/2163833003/diff/180001/build/android/test_wra... build/android/test_wrapper/test_runner_wrapper.py:46: cmd = cmd.split() this is going to sound nuts but I think we should use json here https://codereview.chromium.org/2163833003/diff/180001/build/android/test_wra... build/android/test_wrapper/test_runner_wrapper.py:49: logging.basicConfig(level=logging.DEBUG) remove lines 49 and 50 https://codereview.chromium.org/2163833003/diff/180001/tools/mb/mb.py File tools/mb/mb.py (right): https://codereview.chromium.org/2163833003/diff/180001/tools/mb/mb.py#newcode... tools/mb/mb.py:1017: #'-service-account-json', don't commit commented-out code https://codereview.chromium.org/2163833003/diff/180001/tools/mb/mb.py#newcode... tools/mb/mb.py:1031: '--test-runner-command', ' '.join(test_cmdline), this should just be the arguments of the program. joining and splitting by space is dangerous
ptal. https://codereview.chromium.org/2163833003/diff/180001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_environment.py (right): https://codereview.chromium.org/2163833003/diff/180001/build/android/pylib/lo... build/android/pylib/local/device/local_device_environment.py:192: add_device_args = ['sed', '-i', '-e', On 2016/07/27 01:14:17, stip wrote: > rename to add_device_cmd Done. https://codereview.chromium.org/2163833003/diff/180001/build/android/test_wra... File build/android/test_wrapper/test_runner_wrapper.py (right): https://codereview.chromium.org/2163833003/diff/180001/build/android/test_wra... build/android/test_wrapper/test_runner_wrapper.py:11: On 2016/07/27 01:14:18, stip wrote: > nit: two newlines before top level declarations Done. https://codereview.chromium.org/2163833003/diff/180001/build/android/test_wra... build/android/test_wrapper/test_runner_wrapper.py:15: parser.add_argument('--test-runner-command', On 2016/07/27 01:14:17, stip wrote: > 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 http://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 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] https://codereview.chromium.org/2163833003/diff/180001/build/android/test_wra... build/android/test_wrapper/test_runner_wrapper.py:20: On 2016/07/27 01:14:17, stip wrote: > newline Done. https://codereview.chromium.org/2163833003/diff/180001/build/android/test_wra... build/android/test_wrapper/test_runner_wrapper.py:25: parser.add_argument('-project') On 2016/07/27 01:14:17, stip wrote: > add help here, maybe just saying "passed to logdog" Specifying the logdog args in the CommandParser makes this parser redundant. https://codereview.chromium.org/2163833003/diff/180001/build/android/test_wra... build/android/test_wrapper/test_runner_wrapper.py:29: On 2016/07/27 01:14:17, stip wrote: > newline Done. https://codereview.chromium.org/2163833003/diff/180001/build/android/test_wra... build/android/test_wrapper/test_runner_wrapper.py:32: url = ('https://luci-logdog-dev.appspot.com/v/?s={0}%2F' On 2016/07/27 01:14:18, stip wrote: > 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 Done. https://codereview.chromium.org/2163833003/diff/180001/build/android/test_wra... build/android/test_wrapper/test_runner_wrapper.py:34: return url On 2016/07/27 01:14:17, stip wrote: > can just make this return ('https://' ....) instead of saving url and > immediately returning it Done. https://codereview.chromium.org/2163833003/diff/180001/build/android/test_wra... build/android/test_wrapper/test_runner_wrapper.py:35: On 2016/07/27 01:14:18, stip wrote: > newline Done. https://codereview.chromium.org/2163833003/diff/180001/build/android/test_wra... build/android/test_wrapper/test_runner_wrapper.py:46: cmd = cmd.split() On 2016/07/27 01:14:17, stip wrote: > this is going to sound nuts but I think we should use json here The new command parser specifies all logdog arguments. Do we still need to use json? https://codereview.chromium.org/2163833003/diff/180001/build/android/test_wra... build/android/test_wrapper/test_runner_wrapper.py:49: logging.basicConfig(level=logging.DEBUG) On 2016/07/27 01:14:18, stip wrote: > remove lines 49 and 50 Done. https://codereview.chromium.org/2163833003/diff/180001/tools/mb/mb.py File tools/mb/mb.py (right): https://codereview.chromium.org/2163833003/diff/180001/tools/mb/mb.py#newcode... tools/mb/mb.py:1017: #'-service-account-json', On 2016/07/27 01:14:18, stip wrote: > don't commit commented-out code This will be uncommented before deployment. Hopefully with logdog switched to luci-logdog instead of luci-logdog-dev. Otherwise will need authentication for the dev account on swarming bots. https://codereview.chromium.org/2163833003/diff/180001/tools/mb/mb.py#newcode... tools/mb/mb.py:1031: '--test-runner-command', ' '.join(test_cmdline), On 2016/07/27 01:14:18, stip wrote: > this should just be the arguments of the program. joining and splitting by space > is dangerous Done.
https://codereview.chromium.org/2163833003/diff/240001/build/android/BUILD.gn File build/android/BUILD.gn (right): https://codereview.chromium.org/2163833003/diff/240001/build/android/BUILD.gn... build/android/BUILD.gn:116: "//build/android/test_wrapper/test_runner_wrapper.py", nit: since this is in //build/android, this should just be "test_wrapper/test_runner_wrapper.py" (% name change) https://codereview.chromium.org/2163833003/diff/240001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_environment.py (right): https://codereview.chromium.org/2163833003/diff/240001/build/android/pylib/lo... build/android/pylib/local/device/local_device_environment.py:192: add_device_cmd = ['sed', '-i', '-e', Can you do this in-process rather than calling out to sed? https://codereview.chromium.org/2163833003/diff/240001/build/android/test_wra... File build/android/test_wrapper/test_runner_wrapper.py (right): https://codereview.chromium.org/2163833003/diff/240001/build/android/test_wra... build/android/test_wrapper/test_runner_wrapper.py:1: #!/usr/bin/env python This name is too generic. If this wrapper is specific to logdog, it should have logdog in the name. https://codereview.chromium.org/2163833003/diff/240001/build/android/test_wra... build/android/test_wrapper/test_runner_wrapper.py:42: if args.logdog_bin_cmd: When would we use this wrapper without a logdog_bin_cmd? https://codereview.chromium.org/2163833003/diff/240001/build/android/test_wra... build/android/test_wrapper/test_runner_wrapper.py:46: os.environ.get('SWARMING_TASK_ID')) 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 https://codereview.chromium.org/2163833003/diff/240001/build/android/test_wra... build/android/test_wrapper/test_runner_wrapper.py:52: ('-name=%s'% args.name)] nits: no parens, space before % https://codereview.chromium.org/2163833003/diff/240001/testing/buildbot/chrom... File testing/buildbot/chromium.android.json (right): https://codereview.chromium.org/2163833003/diff/240001/testing/buildbot/chrom... testing/buildbot/chromium.android.json:21: "cipd_packages": [ That generator idea is becoming more attractive.
almost there. john's comments plus these: https://codereview.chromium.org/2163833003/diff/180001/build/android/test_wra... File build/android/test_wrapper/test_runner_wrapper.py (right): https://codereview.chromium.org/2163833003/diff/180001/build/android/test_wra... build/android/test_wrapper/test_runner_wrapper.py:15: parser.add_argument('--test-runner-command', On 2016/07/27 19:08:06, nicholaslin wrote: > On 2016/07/27 01:14:17, stip wrote: > > 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 http://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 > 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 http://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] I'd prefer using the named positional argument and then -- to separate the parsing. it's what people tend to expect https://codereview.chromium.org/2163833003/diff/180001/build/android/test_wra... build/android/test_wrapper/test_runner_wrapper.py:46: cmd = cmd.split() On 2016/07/27 19:08:06, nicholaslin wrote: > On 2016/07/27 01:14:17, stip wrote: > > this is going to sound nuts but I think we should use json here > > The new command parser specifies all logdog arguments. Do we still need to use > json? nope, this was leftover from an older review pass. sorry about that https://codereview.chromium.org/2163833003/diff/240001/build/android/test_wra... File build/android/test_wrapper/test_runner_wrapper.py (right): https://codereview.chromium.org/2163833003/diff/240001/build/android/test_wra... build/android/test_wrapper/test_runner_wrapper.py:17: help=('Command for running logdog butler binary')) you don't need the parens here. it can be just help='whatever' https://codereview.chromium.org/2163833003/diff/240001/build/android/test_wra... build/android/test_wrapper/test_runner_wrapper.py:17: help=('Command for running logdog butler binary')) required=True IIUC, all of these should be required right? https://codereview.chromium.org/2163833003/diff/240001/build/android/test_wra... build/android/test_wrapper/test_runner_wrapper.py:20: parser.add_argument('--output', nit: rework the --output flag to just be server name: parser.add_argument('--logdog-server', default='luci-logdog.appspot.com') https://codereview.chromium.org/2163833003/diff/240001/build/android/test_wra... build/android/test_wrapper/test_runner_wrapper.py:35: return ('https://luci-logdog-dev.appspot.com/v/?s=%s%%2F' use args.logdog_server as specified above https://codereview.chromium.org/2163833003/diff/240001/build/android/test_wra... build/android/test_wrapper/test_runner_wrapper.py:41: args, test_cmd = parser.parse_known_args(sys.argv[1:]) if not test_cmd: parser.error('must specify a command to run after the logdog flags) parser.error calls sys.exit(2) after printing https://codereview.chromium.org/2163833003/diff/240001/build/android/test_wra... build/android/test_wrapper/test_runner_wrapper.py:42: if args.logdog_bin_cmd: On 2016/07/29 00:52:40, jbudorick wrote: > When would we use this wrapper without a logdog_bin_cmd? should make required in the parser https://codereview.chromium.org/2163833003/diff/240001/build/android/test_wra... build/android/test_wrapper/test_runner_wrapper.py:50: '-output', args.output, '-prefix', args.prefix] '-output', 'logdog,host=%s' % args.logdog_server,
Patchset #14 (id:260001) has been deleted
ptal https://codereview.chromium.org/2163833003/diff/240001/build/android/BUILD.gn File build/android/BUILD.gn (right): https://codereview.chromium.org/2163833003/diff/240001/build/android/BUILD.gn... build/android/BUILD.gn:116: "//build/android/test_wrapper/test_runner_wrapper.py", On 2016/07/29 00:52:39, jbudorick wrote: > nit: since this is in //build/android, this should just be > > "test_wrapper/test_runner_wrapper.py" > > (% name change) Done. https://codereview.chromium.org/2163833003/diff/240001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_environment.py (right): https://codereview.chromium.org/2163833003/diff/240001/build/android/pylib/lo... build/android/pylib/local/device/local_device_environment.py:192: add_device_cmd = ['sed', '-i', '-e', On 2016/07/29 00:52:39, jbudorick wrote: > Can you do this in-process rather than calling out to sed? Done. https://codereview.chromium.org/2163833003/diff/240001/build/android/test_wra... File build/android/test_wrapper/test_runner_wrapper.py (right): https://codereview.chromium.org/2163833003/diff/240001/build/android/test_wra... build/android/test_wrapper/test_runner_wrapper.py:1: #!/usr/bin/env python On 2016/07/29 00:52:39, jbudorick wrote: > This name is too generic. If this wrapper is specific to logdog, it should have > logdog in the name. Done. Should the folder be renamed as well? https://codereview.chromium.org/2163833003/diff/240001/build/android/test_wra... build/android/test_wrapper/test_runner_wrapper.py:17: help=('Command for running logdog butler binary')) On 2016/07/29 01:09:08, stip wrote: > you don't need the parens here. it can be just help='whatever' Done. https://codereview.chromium.org/2163833003/diff/240001/build/android/test_wra... build/android/test_wrapper/test_runner_wrapper.py:17: help=('Command for running logdog butler binary')) On 2016/07/29 01:09:08, stip wrote: > required=True > > IIUC, all of these should be required right? 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. https://codereview.chromium.org/2163833003/diff/240001/build/android/test_wra... build/android/test_wrapper/test_runner_wrapper.py:20: parser.add_argument('--output', On 2016/07/29 01:09:08, stip wrote: > nit: rework the --output flag to just be server name: > > parser.add_argument('--logdog-server', default='luci-logdog.appspot.com') Done. https://codereview.chromium.org/2163833003/diff/240001/build/android/test_wra... build/android/test_wrapper/test_runner_wrapper.py:35: return ('https://luci-logdog-dev.appspot.com/v/?s=%s%%2F' On 2016/07/29 01:09:08, stip wrote: > use args.logdog_server as specified above Done. https://codereview.chromium.org/2163833003/diff/240001/build/android/test_wra... build/android/test_wrapper/test_runner_wrapper.py:41: args, test_cmd = parser.parse_known_args(sys.argv[1:]) On 2016/07/29 01:09:08, stip wrote: > if not test_cmd: > parser.error('must specify a command to run after the logdog flags) > > parser.error calls sys.exit(2) after printing Done. https://codereview.chromium.org/2163833003/diff/240001/build/android/test_wra... build/android/test_wrapper/test_runner_wrapper.py:42: if args.logdog_bin_cmd: On 2016/07/29 00:52:40, jbudorick wrote: > When would we use this wrapper without a logdog_bin_cmd? 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. https://codereview.chromium.org/2163833003/diff/240001/build/android/test_wra... build/android/test_wrapper/test_runner_wrapper.py:46: os.environ.get('SWARMING_TASK_ID')) On 2016/07/29 00:52:40, jbudorick wrote: > 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 Done. https://codereview.chromium.org/2163833003/diff/240001/build/android/test_wra... build/android/test_wrapper/test_runner_wrapper.py:50: '-output', args.output, '-prefix', args.prefix] On 2016/07/29 01:09:08, stip wrote: > '-output', 'logdog,host=%s' % args.logdog_server, Done. https://codereview.chromium.org/2163833003/diff/240001/build/android/test_wra... build/android/test_wrapper/test_runner_wrapper.py:52: ('-name=%s'% args.name)] On 2016/07/29 00:52:40, jbudorick wrote: > nits: no parens, space before % Done. https://codereview.chromium.org/2163833003/diff/240001/build/android/test_wra... build/android/test_wrapper/test_runner_wrapper.py:53: if args.service_account_json: Requiring all logdog arguments, so taking this part out.
ever so close... https://codereview.chromium.org/2163833003/diff/280001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_environment.py (right): https://codereview.chromium.org/2163833003/diff/280001/build/android/pylib/lo... build/android/pylib/local/device/local_device_environment.py:12: import fileinput nit: sort all system imports https://codereview.chromium.org/2163833003/diff/280001/build/android/pylib/lo... build/android/pylib/local/device/local_device_environment.py:193: for line in fileinput.input([m.output_file], inplace=True): cool! didn't know about fileinput https://codereview.chromium.org/2163833003/diff/280001/build/android/pylib/lo... build/android/pylib/local/device/local_device_environment.py:194: sys.stdout.write('Device(%s) %s' % (m.adb.GetDeviceSerial(), line)) does stdout work here? that is surprising https://codereview.chromium.org/2163833003/diff/280001/build/android/test_wra... File build/android/test_wrapper/logdog_wrapper.py (right): https://codereview.chromium.org/2163833003/diff/280001/build/android/test_wra... build/android/test_wrapper/logdog_wrapper.py:38: return ('https://%s/v/?s=%s%%2F%s%%2F%%2B%%2F%s' lmao https://codereview.chromium.org/2163833003/diff/280001/build/android/test_wra... build/android/test_wrapper/logdog_wrapper.py:47: subprocess.check_call(test_cmd) we should do result = subprocess.call(test_cmd) and then later return result that way the test wrapper always returns the return value of the test, not of the wrapper or logdog. https://codereview.chromium.org/2163833003/diff/280001/tools/mb/mb.py File tools/mb/mb.py (right): https://codereview.chromium.org/2163833003/diff/280001/tools/mb/mb.py#newcode... tools/mb/mb.py:1014: '--logdog-bin-cmd', './../../bin/logdog_butler', CHROMIUM_SRC_DIR/bin/ ? see line 31 https://codereview.chromium.org/2163833003/diff/280001/tools/mb/mb.py#newcode... tools/mb/mb.py:1025: '--logcat-output-file', '${ISOLATED_OUTDIR}/logcats', unified_logcats? https://codereview.chromium.org/2163833003/diff/280001/tools/mb/mb.py#newcode... tools/mb/mb.py:1029: cmdline = (['./../../build/android/logdog_wrapper/logdog_test_wrapper.py'] os.path.join(CHROMIUM_SRC_DIR, 'build', 'android') etc
https://codereview.chromium.org/2163833003/diff/280001/build/android/test_wra... File build/android/test_wrapper/logdog_wrapper.py (right): https://codereview.chromium.org/2163833003/diff/280001/build/android/test_wra... build/android/test_wrapper/logdog_wrapper.py:23: default='luci-logdog.appspot.com', Please use "services-dot-luci-logdog.appspot.com" as the default. The "services" module is a more efficient backend for stream registration with the same behavior. https://codereview.chromium.org/2163833003/diff/280001/build/android/test_wra... build/android/test_wrapper/logdog_wrapper.py:38: return ('https://%s/v/?s=%s%%2F%s%%2F%%2B%%2F%s' On 2016/07/29 21:56:40, stip wrote: > lmao Er can we use something like urllib.quote (https://docs.python.org/2/library/urllib.html#urllib.quote) for the query string part of this? https://codereview.chromium.org/2163833003/diff/280001/build/android/test_wra... build/android/test_wrapper/logdog_wrapper.py:57: 'stream', '-source', args.source, I see we gave up on the "serve" approach w/ the library. That's a shame :( We lose all sorts of cool stuff, like: * Accurate timestamps * Live streaming of the logs If you're not using either of those, maybe consider not using LogDog at all and just uploading the files to Google Storage *shrug* https://codereview.chromium.org/2163833003/diff/280001/tools/mb/mb.py File tools/mb/mb.py (right): https://codereview.chromium.org/2163833003/diff/280001/tools/mb/mb.py#newcode... tools/mb/mb.py:1016: '--logdog-server', 'luci-logdog.appspot.com', Specifying this twice? Either use the default or remove the default value above. https://codereview.chromium.org/2163833003/diff/280001/tools/mb/mb.py#newcode... tools/mb/mb.py:1018: '/creds/service_accounts/service-account-luci-logdog-publisher.json', Are we comfortable hardcoding this in "chromium/src"? This will also not work on GCE, but that probably doesn't matter.
ptal. https://codereview.chromium.org/2163833003/diff/280001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_environment.py (right): https://codereview.chromium.org/2163833003/diff/280001/build/android/pylib/lo... build/android/pylib/local/device/local_device_environment.py:12: import fileinput On 2016/07/29 21:56:40, stip wrote: > nit: sort all system imports Done. https://codereview.chromium.org/2163833003/diff/280001/build/android/pylib/lo... build/android/pylib/local/device/local_device_environment.py:194: sys.stdout.write('Device(%s) %s' % (m.adb.GetDeviceSerial(), line)) On 2016/07/29 21:56:40, stip wrote: > does stdout work here? that is surprising From the local testing I did, yes. inplace=true moves input to a temporary file and writes the stdout to the original file. https://codereview.chromium.org/2163833003/diff/280001/build/android/test_wra... File build/android/test_wrapper/logdog_wrapper.py (right): https://codereview.chromium.org/2163833003/diff/280001/build/android/test_wra... build/android/test_wrapper/logdog_wrapper.py:23: default='luci-logdog.appspot.com', On 2016/07/29 22:35:34, dnj wrote: > Please use "services-dot-luci-logdog.appspot.com" as the default. The "services" > module is a more efficient backend for stream registration with the same > behavior. Done, but clicking on this seems to show an empty link? https://codereview.chromium.org/2163833003/diff/280001/build/android/test_wra... build/android/test_wrapper/logdog_wrapper.py:38: return ('https://%s/v/?s=%s%%2F%s%%2F%%2B%%2F%s' On 2016/07/29 22:35:34, dnj wrote: > On 2016/07/29 21:56:40, stip wrote: > > lmao > > Er can we use something like urllib.quote > (https://docs.python.org/2/library/urllib.html#urllib.quote) for the query > string part of this? Done. https://codereview.chromium.org/2163833003/diff/280001/build/android/test_wra... build/android/test_wrapper/logdog_wrapper.py:47: subprocess.check_call(test_cmd) On 2016/07/29 21:56:40, stip wrote: > we should do result = subprocess.call(test_cmd) > > and then later return result > > that way the test wrapper always returns the return value of the test, not of > the wrapper or logdog. Done. https://codereview.chromium.org/2163833003/diff/280001/tools/mb/mb.py File tools/mb/mb.py (right): https://codereview.chromium.org/2163833003/diff/280001/tools/mb/mb.py#newcode... tools/mb/mb.py:1014: '--logdog-bin-cmd', './../../bin/logdog_butler', On 2016/07/29 21:56:40, stip wrote: > CHROMIUM_SRC_DIR/bin/ ? see line 31 Tried that before. That's for the buildbot slave not the swarming slave. https://codereview.chromium.org/2163833003/diff/280001/tools/mb/mb.py#newcode... tools/mb/mb.py:1016: '--logdog-server', 'luci-logdog.appspot.com', On 2016/07/29 22:35:34, dnj wrote: > Specifying this twice? Either use the default or remove the default value above. Removed it here. https://codereview.chromium.org/2163833003/diff/280001/tools/mb/mb.py#newcode... tools/mb/mb.py:1025: '--logcat-output-file', '${ISOLATED_OUTDIR}/logcats', On 2016/07/29 21:56:40, stip wrote: > unified_logcats? Done. Do we want it to be named unified logcats? The location doesn't really matter for logdog since name overwrites the file location. https://codereview.chromium.org/2163833003/diff/280001/tools/mb/mb.py#newcode... tools/mb/mb.py:1029: cmdline = (['./../../build/android/logdog_wrapper/logdog_test_wrapper.py'] On 2016/07/29 21:56:40, stip wrote: > os.path.join(CHROMIUM_SRC_DIR, 'build', 'android') etc see above.
https://codereview.chromium.org/2163833003/diff/280001/build/android/test_wra... File build/android/test_wrapper/logdog_wrapper.py (right): https://codereview.chromium.org/2163833003/diff/280001/build/android/test_wra... build/android/test_wrapper/logdog_wrapper.py:23: default='luci-logdog.appspot.com', On 2016/07/29 23:54:06, nicholaslin wrote: > On 2016/07/29 22:35:34, dnj wrote: > > Please use "services-dot-luci-logdog.appspot.com" as the default. The > "services" > > module is a more efficient backend for stream registration with the same > > behavior. > > Done, but clicking on this seems to show an empty link? Yeah it's a services endpoint, not a user-facing endpoint. https://codereview.chromium.org/2163833003/diff/280001/tools/mb/mb.py File tools/mb/mb.py (right): https://codereview.chromium.org/2163833003/diff/280001/tools/mb/mb.py#newcode... tools/mb/mb.py:1018: '/creds/service_accounts/service-account-luci-logdog-publisher.json', On 2016/07/29 22:35:34, dnj wrote: > Are we comfortable hardcoding this in "chromium/src"? This will also not work on > GCE, but that probably doesn't matter. Ping on this. We're baking Infra credential paths in to "chromium/src" here. I don't know if this is something that we want so far away from actual infra-homed code. Let's discuss alternatives, specifically, there is an option for me to actually bake this into LogDog Butler itself so that neither the JSON path nor the "host" is needed. I'll put together a CL for this, since it's something I've been meaning to do for a while.
https://codereview.chromium.org/2163833003/diff/280001/build/android/test_wra... File build/android/test_wrapper/logdog_wrapper.py (right): https://codereview.chromium.org/2163833003/diff/280001/build/android/test_wra... build/android/test_wrapper/logdog_wrapper.py:23: default='luci-logdog.appspot.com', On 2016/07/30 15:47:48, dnj wrote: > On 2016/07/29 23:54:06, nicholaslin wrote: > > On 2016/07/29 22:35:34, dnj wrote: > > > Please use "services-dot-luci-logdog.appspot.com" as the default. The > > "services" > > > module is a more efficient backend for stream registration with the same > > > behavior. > > > > Done, but clicking on this seems to show an empty link? > > Yeah it's a services endpoint, not a user-facing endpoint. Got it. The streams go to services-dot-luci-logdog.appspot.com and the viewable link is still luci-logdog.appspot.com/... https://codereview.chromium.org/2163833003/diff/280001/tools/mb/mb.py File tools/mb/mb.py (right): https://codereview.chromium.org/2163833003/diff/280001/tools/mb/mb.py#newcode... tools/mb/mb.py:1018: '/creds/service_accounts/service-account-luci-logdog-publisher.json', On 2016/07/30 15:47:48, dnj wrote: > On 2016/07/29 22:35:34, dnj wrote: > > Are we comfortable hardcoding this in "chromium/src"? This will also not work > on > > GCE, but that probably doesn't matter. > > Ping on this. We're baking Infra credential paths in to "chromium/src" here. I > don't know if this is something that we want so far away from actual infra-homed > code. > > Let's discuss alternatives, specifically, there is an option for me to actually > bake this into LogDog Butler itself so that neither the JSON path nor the "host" > is needed. I'll put together a CL for this, since it's something I've been > meaning to do for a while. Noted. Will think about alternatives.
stip@chromium.org changed reviewers: + vadimsh@chromium.org
lgtm from my side. I don't have a strong preference about moving the cred specification. while dnj is out, maybe vadimsh@ has some opinions https://codereview.chromium.org/2163833003/diff/400001/build/android/test_wra... File build/android/test_wrapper/logdog_wrapper.py (right): https://codereview.chromium.org/2163833003/diff/400001/build/android/test_wra... build/android/test_wrapper/logdog_wrapper.py:51: url = CreateUrl('luci-logdog.appspot.com', args.project, args.prefix, nit: would be nice to link this to --logdog-server somehow, but I'm willing to follow this up in another CL
I'm not happy about the duplication in *.json files, but stip@ assured me there'll be some improvement there soon. lgtm to unblock, but make sure Dan knows how to bump logdog version used here. The creds situation is temporary, the butler will use per-task service accounts eventually (when this is implemented, we'll just remove --service-account-json option). (Also, mb.py is high unintuitive place for this stuff IMHO. I'd expect it to be in recipes somewhere. But I don' know how android tests work...).
stip@chromium.org changed reviewers: + maruel@chromium.org
M-A, Dirk had some concerns that this breaks idempotency. While sending data to another web service is not ideal here, it does include task and shard IDs which does make it idempotent on the outputs. Do you have any opinion on this?
On 2016/08/08 21:08:39, stip wrote: > M-A, Dirk had some concerns that this breaks idempotency. While sending data to > another web service is not ideal here, it does include task and shard IDs which > does make it idempotent on the outputs. Do you have any opinion on this? There was questions about idempotency; - If the inputs are constant, it's idempotent. The outputs do not have to be strictly speaking constant; as we do not force deterministic output. While this goal is laudable, the mere fact of including timestamps and test case duration makes this not possible. - There's the issue of contacting external services but we can consider logdog as an internal service. It can make infrastructure failure appear as task failure but I assume this shouldn't be happening often. Filed https://github.com/luci/luci-py/issues/275 to track this. So this change is fine from this PoV.
On 2016/08/08 21:12:36, M-A Ruel wrote: > On 2016/08/08 21:08:39, stip wrote: > > M-A, Dirk had some concerns that this breaks idempotency. While sending data > to > > another web service is not ideal here, it does include task and shard IDs > which > > does make it idempotent on the outputs. Do you have any opinion on this? Actually, my concerns weren't about idempotency; I didn't even think of that aspect :). I was more concerned about two things: - whether a job should ever try to talk to something off-machine. M-A seems to say that that's okay in this case. - whether this would be the right way to get arbitrary test artifacts (in this case logcat output) from jobs into arbitrary locations (in this case, logdog). I feel like doing this piecemeal for specific integrations is perhaps not the best way to do this, and we should have some standardized ways to do this for specific types of output (test results to the flakiness dashboard, logs to logdog, other artifacts to google storage buckets, etc.). I can possibly be okay with the piecemeal approach if it is a *step on the path* towards a generic solution. I also *really* don't want to have to configure all of these credentials per builder in the JSON files, particularly not in a manual way. M-A, do you have thoughts on the latter two things?
On 2016/08/08 22:01:57, Dirk Pranke wrote: > M-A, do you have thoughts on the latter two things? Yes, I do. They'll be addressed later.
On 2016/08/09 00:27:42, M-A Ruel wrote: > On 2016/08/08 22:01:57, Dirk Pranke wrote: > > M-A, do you have thoughts on the latter two things? > > Yes, I do. They'll be addressed later. Meaning you'll add comments to this CL before approving, or meaning you think this CL is fine for now?
On 2016/08/09 01:01:48, Dirk Pranke wrote: > On 2016/08/09 00:27:42, M-A Ruel wrote: > > On 2016/08/08 22:01:57, Dirk Pranke wrote: > > > M-A, do you have thoughts on the latter two things? > > > > Yes, I do. They'll be addressed later. > > Meaning you'll add comments to this CL before approving, or meaning you think > this CL is fine for now? No, I'll comment later, no need to block this CL. lgtm
dpranke@, jbudorick@ ptal https://codereview.chromium.org/2163833003/diff/400001/build/android/test_wra... File build/android/test_wrapper/logdog_wrapper.py (right): https://codereview.chromium.org/2163833003/diff/400001/build/android/test_wra... build/android/test_wrapper/logdog_wrapper.py:51: url = CreateUrl('luci-logdog.appspot.com', args.project, args.prefix, On 2016/08/04 23:46:48, stip wrote: > nit: would be nice to link this to --logdog-server somehow, but I'm willing to > follow this up in another CL My guess is once Dan puts together the logdog CL --logdog-server will only be used here.
I really don't like having to specify these things via the swarming spec, because it's way too fragile to maintain these things in the //testing/buildbot/*.json files. Ideally, we would fix that and clean up the way we pass things back and forth through swarming (as per maruel's "I'll comment later" except doing this now rather than later). But, all that said, I'm just going to be grumpy instead of super annoying, so lgtm for now and I'll really hope that this does get cleaned up shortly.
https://codereview.chromium.org/2163833003/diff/420001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_environment.py (right): https://codereview.chromium.org/2163833003/diff/420001/build/android/pylib/lo... build/android/pylib/local/device/local_device_environment.py:194: sys.stdout.write('Device(%s) %s' % (m.adb.GetDeviceSerial(), line)) Why is this writing all logcat to stdout? https://codereview.chromium.org/2163833003/diff/420001/build/android/test_wra... File build/android/test_wrapper/logdog_wrapper.py (right): https://codereview.chromium.org/2163833003/diff/420001/build/android/test_wra... build/android/test_wrapper/logdog_wrapper.py:8: import argparse nit: alphabetize plz https://codereview.chromium.org/2163833003/diff/420001/testing/buildbot/chrom... File testing/buildbot/chromium.android.json (right): https://codereview.chromium.org/2163833003/diff/420001/testing/buildbot/chrom... testing/buildbot/chromium.android.json:21: "cipd_packages": [ 😬😬😬
ptal https://codereview.chromium.org/2163833003/diff/420001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_environment.py (right): https://codereview.chromium.org/2163833003/diff/420001/build/android/pylib/lo... build/android/pylib/local/device/local_device_environment.py:194: sys.stdout.write('Device(%s) %s' % (m.adb.GetDeviceSerial(), line)) On 2016/08/10 00:54:39, jbudorick wrote: > Why is this writing all logcat to stdout? Changed. https://codereview.chromium.org/2163833003/diff/420001/build/android/test_wra... File build/android/test_wrapper/logdog_wrapper.py (right): https://codereview.chromium.org/2163833003/diff/420001/build/android/test_wra... build/android/test_wrapper/logdog_wrapper.py:8: import argparse On 2016/08/10 00:54:39, jbudorick wrote: > nit: alphabetize plz Done.
build/android/ lgtm w/ nit https://codereview.chromium.org/2163833003/diff/440001/build/android/test_wra... File build/android/test_wrapper/logdog_wrapper.py (right): https://codereview.chromium.org/2163833003/diff/440001/build/android/test_wra... build/android/test_wrapper/logdog_wrapper.py:53: logging.basicConfig(level=logging.INFO) nit: Do this immediately after argument parsing.
The CQ bit was checked by nicholaslin@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by nicholaslin@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by nicholaslin@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by nicholaslin@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by nicholaslin@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by nicholaslin@google.com
The CQ bit was checked by nicholaslin@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from vadimsh@chromium.org, stip@chromium.org, maruel@chromium.org, dpranke@chromium.org, jbudorick@chromium.org Link to the patchset: https://codereview.chromium.org/2163833003/#ps500001 (title: "Update json files to include links")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add logdog_butler cipd package to every swarming gtest_test on android tryservers (linux_android_rel_ng, android_n5x_swarming_rel). Add wrapper around android test runner for swarming tasks. This wrapper calls logdog to stream logcats after the completion of the python executable calling test_runner. Adding device serials to every line of logcat logs. Sample of unified logcats: https://luci-logdog.appspot.com/v/?s=chromium%2Fswarming%2F301f0f97eea1a511%2... BUG=448050 ========== to ========== Add logdog_butler cipd package to every swarming gtest_test on android tryservers (linux_android_rel_ng, android_n5x_swarming_rel). Add wrapper around android test runner for swarming tasks. This wrapper calls logdog to stream logcats after the completion of the python executable calling test_runner. Adding device serials to every line of logcat logs. Sample of unified logcats: https://luci-logdog.appspot.com/v/?s=chromium%2Fswarming%2F301f0f97eea1a511%2... BUG=448050 ==========
Message was sent while issue was closed.
Committed patchset #25 (id:500001)
Message was sent while issue was closed.
Description was changed from ========== Add logdog_butler cipd package to every swarming gtest_test on android tryservers (linux_android_rel_ng, android_n5x_swarming_rel). Add wrapper around android test runner for swarming tasks. This wrapper calls logdog to stream logcats after the completion of the python executable calling test_runner. Adding device serials to every line of logcat logs. Sample of unified logcats: https://luci-logdog.appspot.com/v/?s=chromium%2Fswarming%2F301f0f97eea1a511%2... BUG=448050 ========== to ========== Add logdog_butler cipd package to every swarming gtest_test on android tryservers (linux_android_rel_ng, android_n5x_swarming_rel). Add wrapper around android test runner for swarming tasks. This wrapper calls logdog to stream logcats after the completion of the python executable calling test_runner. Adding device serials to every line of logcat logs. Sample of unified logcats: https://luci-logdog.appspot.com/v/?s=chromium%2Fswarming%2F301f0f97eea1a511%2... BUG=448050 Committed: https://crrev.com/3dbe50ffaca641ea962d385e2ac9ac0bec75fbbb Cr-Commit-Position: refs/heads/master@{#411530} ==========
Message was sent while issue was closed.
Patchset 25 (id:??) landed as https://crrev.com/3dbe50ffaca641ea962d385e2ac9ac0bec75fbbb Cr-Commit-Position: refs/heads/master@{#411530}
Message was sent while issue was closed.
A revert of this CL (patchset #25 id:500001) has been created in https://codereview.chromium.org/2240853002/ by kolos@chromium.org. The reason for reverting is: This CL crashed mojo_test_apk on Android. See details at crbug.com/637213.
Message was sent while issue was closed.
A revert of this CL (patchset #25 id:500001) has been created in https://codereview.chromium.org/2243903002/ by xidachen@chromium.org. The reason for reverting is: As mentioned here: https://bugs.chromium.org/p/chromium/issues/detail?id=637213 This CL seems to cause failure to this bot: https://build.chromium.org/p/chromium.linux/builders/Android%20Tests/builds/3.... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
