Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
745793002:
Add AMP support to test runner. (Closed)
|
Created:
6 years, 1 month ago by rnephew (Reviews Here) Modified:
6 years ago CC:
chromium-reviews, klundberg+watch_chromium.org, yfriedman+watch_chromium.org, jbudorick+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionAdd AMP support to test runner.
BUG=
Committed: https://crrev.com/5c49978f095340a59c62faaafe02a9527ec7728b
Cr-Commit-Position: refs/heads/master@{#308139}
Patch Set 1 #
Total comments: 53
Patch Set 2 : #Patch Set 3 : Make config file live up to its name #Patch Set 4 : work with Appurify third_party #Patch Set 5 : get rid of old/done TODOs #
Total comments: 26
Patch Set 6 : address comments on previous patch set #
Total comments: 46
Patch Set 7 : fix gtest apk upload #Patch Set 8 : address comments #Patch Set 9 : seperate trigger and collect - still need to rebase #Patch Set 10 : rebase #
Total comments: 38
Patch Set 11 : address comments #
Total comments: 34
Patch Set 12 : #
Total comments: 28
Patch Set 13 : #
Total comments: 10
Patch Set 14 : #
Total comments: 2
Patch Set 15 : #Patch Set 16 : #Patch Set 17 : fix pylint in pylib/instrumentation/t_r #Messages
Total messages: 35 (6 generated)
rnephew@chromium.org changed reviewers: + jbudorick@chromium.org, klundberg@chromium.org
The big problem I see with this is doing the separate trigger & collection steps. We should discuss that either tomorrow or next week. https://codereview.chromium.org/745793002/diff/1/build/android/pylib/base/rem... File build/android/pylib/base/remote_device_config.py (right): https://codereview.chromium.org/745793002/diff/1/build/android/pylib/base/rem... build/android/pylib/base/remote_device_config.py:1: # Copyright 2013 The Chromium Authors. All rights reserved. These three remote_device_* files should be in build/android/pylib/remote/device/ Also, make sure all of your copyright notices: - say 2014 - have no "(c)" https://codereview.chromium.org/745793002/diff/1/build/android/pylib/base/rem... build/android/pylib/base/remote_device_config.py:35: config_data.append('[robotium]') This should probably be configurable. https://codereview.chromium.org/745793002/diff/1/build/android/pylib/base/rem... build/android/pylib/base/remote_device_config.py:36: config_data.append('runner=org.chromium.native_test.' This will need to be configurable. https://codereview.chromium.org/745793002/diff/1/build/android/pylib/base/rem... build/android/pylib/base/remote_device_config.py:42: if self._app.endswith('.apk'): 1. Don't add support for iOS yet. 2. When we do add support for iOS, I don't think we'll be autodetecting based on file extension. https://codereview.chromium.org/745793002/diff/1/build/android/pylib/base/rem... build/android/pylib/base/remote_device_config.py:49: for key in self._extras: config_data.extend('%s=%s' % (k, v) for k, v in self._extras.iteritems()) https://codereview.chromium.org/745793002/diff/1/build/android/pylib/base/rem... build/android/pylib/base/remote_device_config.py:53: for line in config_data: config_file.write('%s\n' % line for line in config_data) https://codereview.chromium.org/745793002/diff/1/build/android/pylib/base/rem... File build/android/pylib/base/remote_device_environment.py (right): https://codereview.chromium.org/745793002/diff/1/build/android/pylib/base/rem... build/android/pylib/base/remote_device_environment.py:20: self.options = options Don't copy the whole options object. Store what you need and do options validation here. https://codereview.chromium.org/745793002/diff/1/build/android/pylib/base/rem... build/android/pylib/base/remote_device_environment.py:21: self.error_func = error_func error_func should only be used for options errors. (Perhaps I should rename it accordingly.) https://codereview.chromium.org/745793002/diff/1/build/android/pylib/base/rem... build/android/pylib/base/remote_device_environment.py:26: os.environ['APPURIFY_API_PROTO'] = 'http' these should be passable via command line. https://codereview.chromium.org/745793002/diff/1/build/android/pylib/base/rem... build/android/pylib/base/remote_device_environment.py:49: self.error_func('Unable to generate access token.') Should be an exception -- probably a new exception type. https://codereview.chromium.org/745793002/diff/1/build/android/pylib/base/rem... build/android/pylib/base/remote_device_environment.py:55: self.error_func('Unable to revoke access token.') Same. https://codereview.chromium.org/745793002/diff/1/build/android/pylib/base/rem... build/android/pylib/base/remote_device_environment.py:60: self.error_func('Unable to get device list.') Same. https://codereview.chromium.org/745793002/diff/1/build/android/pylib/base/rem... File build/android/pylib/base/remote_device_test_run.py (right): https://codereview.chromium.org/745793002/diff/1/build/android/pylib/base/rem... build/android/pylib/base/remote_device_test_run.py:24: self._wait_time = 5 This should just be a constant (i.e. _WAIT_TIME), probably at class level. https://codereview.chromium.org/745793002/diff/1/build/android/pylib/base/rem... build/android/pylib/base/remote_device_test_run.py:25: self._device = self.SelectDevice() Both SelectDevice and UploadAppToDevice should be done in SetUp, not in __init__. https://codereview.chromium.org/745793002/diff/1/build/android/pylib/base/rem... build/android/pylib/base/remote_device_test_run.py:39: if self._test_instance.TestType() == 'uirobot': elif? https://codereview.chromium.org/745793002/diff/1/build/android/pylib/base/rem... build/android/pylib/base/remote_device_test_run.py:64: def GetTestByName(self, test_name): All of these functions need docstrings. https://codereview.chromium.org/745793002/diff/1/build/android/pylib/base/rem... build/android/pylib/base/remote_device_test_run.py:80: while test_status != 'complete': 'complete' should be const'd. https://codereview.chromium.org/745793002/diff/1/build/android/pylib/base/rem... build/android/pylib/base/remote_device_test_run.py:89: for device in self._env.devices: This would seem like an environment level function...? https://codereview.chromium.org/745793002/diff/1/build/android/pylib/base/rem... build/android/pylib/base/remote_device_test_run.py:100: apk_src, 'raw', name=apk_name) They need an open file instead of a path? That's ... strange. https://codereview.chromium.org/745793002/diff/1/build/android/pylib/base/rem... build/android/pylib/base/remote_device_test_run.py:101: self.TestHttpResponse(upload_results, 'Unable to upload Chrome.') s/Chrome/<apk under test name>/ https://codereview.chromium.org/745793002/diff/1/build/android/pylib/base/rem... build/android/pylib/base/remote_device_test_run.py:109: self.TestHttpResponse(upload_results, 'Unable to upload test.') s/ test./ <test apk name>./ https://codereview.chromium.org/745793002/diff/1/build/android/pylib/base/rem... build/android/pylib/base/remote_device_test_run.py:113: with remote_device_config.RemoteDeviceConfig( I'm wondering if we should just use tempfile and do the config generation in here. Do we anticipate using RemoteDeviceConfig anywhere else? https://codereview.chromium.org/745793002/diff/1/build/android/pylib/base/tes... File build/android/pylib/base/test_instance_factory.py (right): https://codereview.chromium.org/745793002/diff/1/build/android/pylib/base/tes... build/android/pylib/base/test_instance_factory.py:6: from pylib.gtest import gtest_test_instance Ha, be careful with this one. https://codereview.chromium.org/745793002/diff/1/build/android/pylib/uirobot/... File build/android/pylib/uirobot/uirobot_test_instance.py (right): https://codereview.chromium.org/745793002/diff/1/build/android/pylib/uirobot/... build/android/pylib/uirobot/uirobot_test_instance.py:15: constants.GetOutDirectory(), 'apks', 'Chrome.apk') apk name shouldn't be hard-coded. https://codereview.chromium.org/745793002/diff/1/build/android/test_runner.py File build/android/test_runner.py (right): https://codereview.chromium.org/745793002/diff/1/build/android/test_runner.py... build/android/test_runner.py:182: AddDeviceOptions(option_parser) I'm working on results now, but options are next. https://codereview.chromium.org/745793002/diff/1/build/android/test_runner.py... build/android/test_runner.py:937: 'gtest','uirobot', nit: space after the first comma https://codereview.chromium.org/745793002/diff/1/build/android/test_runner.py... build/android/test_runner.py:954: if options.environment == 'remote_device': wip on my end
I am still working on making the config file more configurable, having chrome.apk not hardcoded and using tempfile for config file generation. Any recommendations on chrome.apk? https://codereview.chromium.org/745793002/diff/1/build/android/pylib/base/rem... File build/android/pylib/base/remote_device_config.py (right): https://codereview.chromium.org/745793002/diff/1/build/android/pylib/base/rem... build/android/pylib/base/remote_device_config.py:1: # Copyright 2013 The Chromium Authors. All rights reserved. On 2014/11/21 00:17:24, jbudorick wrote: > These three remote_device_* files should be in > build/android/pylib/remote/device/ > > Also, make sure all of your copyright notices: > - say 2014 > - have no "(c)" Done. https://codereview.chromium.org/745793002/diff/1/build/android/pylib/base/rem... build/android/pylib/base/remote_device_config.py:35: config_data.append('[robotium]') On 2014/11/21 00:17:24, jbudorick wrote: > This should probably be configurable. Added to do for this. Will do before landing CL. (need to wait for appurify to land in third party anyway) https://codereview.chromium.org/745793002/diff/1/build/android/pylib/base/rem... build/android/pylib/base/remote_device_config.py:36: config_data.append('runner=org.chromium.native_test.' On 2014/11/21 00:17:24, jbudorick wrote: > This will need to be configurable. Same. https://codereview.chromium.org/745793002/diff/1/build/android/pylib/base/rem... build/android/pylib/base/remote_device_config.py:42: if self._app.endswith('.apk'): On 2014/11/21 00:17:24, jbudorick wrote: > 1. Don't add support for iOS yet. > 2. When we do add support for iOS, I don't think we'll be autodetecting based on > file extension. Removed. https://codereview.chromium.org/745793002/diff/1/build/android/pylib/base/rem... build/android/pylib/base/remote_device_config.py:49: for key in self._extras: On 2014/11/21 00:17:24, jbudorick wrote: > config_data.extend('%s=%s' % (k, v) for k, v in self._extras.iteritems()) Done. https://codereview.chromium.org/745793002/diff/1/build/android/pylib/base/rem... build/android/pylib/base/remote_device_config.py:53: for line in config_data: On 2014/11/21 00:17:24, jbudorick wrote: > config_file.write('%s\n' % line for line in config_data) That didn't work but I switched to using writelines. https://codereview.chromium.org/745793002/diff/1/build/android/pylib/base/rem... File build/android/pylib/base/remote_device_environment.py (right): https://codereview.chromium.org/745793002/diff/1/build/android/pylib/base/rem... build/android/pylib/base/remote_device_environment.py:20: self.options = options On 2014/11/21 00:17:24, jbudorick wrote: > Don't copy the whole options object. Store what you need and do options > validation here. Done. https://codereview.chromium.org/745793002/diff/1/build/android/pylib/base/rem... build/android/pylib/base/remote_device_environment.py:21: self.error_func = error_func On 2014/11/21 00:17:24, jbudorick wrote: > error_func should only be used for options errors. (Perhaps I should rename it > accordingly.) Done. https://codereview.chromium.org/745793002/diff/1/build/android/pylib/base/rem... build/android/pylib/base/remote_device_environment.py:26: os.environ['APPURIFY_API_PROTO'] = 'http' On 2014/11/21 00:17:25, jbudorick wrote: > these should be passable via command line. Done. https://codereview.chromium.org/745793002/diff/1/build/android/pylib/base/rem... build/android/pylib/base/remote_device_environment.py:49: self.error_func('Unable to generate access token.') On 2014/11/21 00:17:24, jbudorick wrote: > Should be an exception -- probably a new exception type. Done. https://codereview.chromium.org/745793002/diff/1/build/android/pylib/base/rem... build/android/pylib/base/remote_device_environment.py:55: self.error_func('Unable to revoke access token.') On 2014/11/21 00:17:24, jbudorick wrote: > Same. Done. https://codereview.chromium.org/745793002/diff/1/build/android/pylib/base/rem... build/android/pylib/base/remote_device_environment.py:60: self.error_func('Unable to get device list.') On 2014/11/21 00:17:24, jbudorick wrote: > Same. Done. https://codereview.chromium.org/745793002/diff/1/build/android/pylib/base/rem... File build/android/pylib/base/remote_device_test_run.py (right): https://codereview.chromium.org/745793002/diff/1/build/android/pylib/base/rem... build/android/pylib/base/remote_device_test_run.py:24: self._wait_time = 5 On 2014/11/21 00:17:25, jbudorick wrote: > This should just be a constant (i.e. _WAIT_TIME), probably at class level. Done. https://codereview.chromium.org/745793002/diff/1/build/android/pylib/base/rem... build/android/pylib/base/remote_device_test_run.py:25: self._device = self.SelectDevice() On 2014/11/21 00:17:25, jbudorick wrote: > Both SelectDevice and UploadAppToDevice should be done in SetUp, not in > __init__. Done. https://codereview.chromium.org/745793002/diff/1/build/android/pylib/base/rem... build/android/pylib/base/remote_device_test_run.py:39: if self._test_instance.TestType() == 'uirobot': On 2014/11/21 00:17:25, jbudorick wrote: > elif? Done. https://codereview.chromium.org/745793002/diff/1/build/android/pylib/base/rem... build/android/pylib/base/remote_device_test_run.py:64: def GetTestByName(self, test_name): On 2014/11/21 00:17:25, jbudorick wrote: > All of these functions need docstrings. Done. https://codereview.chromium.org/745793002/diff/1/build/android/pylib/base/rem... build/android/pylib/base/remote_device_test_run.py:80: while test_status != 'complete': On 2014/11/21 00:17:25, jbudorick wrote: > 'complete' should be const'd. Done. https://codereview.chromium.org/745793002/diff/1/build/android/pylib/base/rem... build/android/pylib/base/remote_device_test_run.py:89: for device in self._env.devices: On 2014/11/21 00:17:25, jbudorick wrote: > This would seem like an environment level function...? Done. https://codereview.chromium.org/745793002/diff/1/build/android/pylib/base/rem... build/android/pylib/base/remote_device_test_run.py:100: apk_src, 'raw', name=apk_name) On 2014/11/21 00:17:25, jbudorick wrote: > They need an open file instead of a path? That's ... strange. Yep. Thats how they do it on their client too. I tried without it being open and it fails. https://codereview.chromium.org/745793002/diff/1/build/android/pylib/base/rem... build/android/pylib/base/remote_device_test_run.py:101: self.TestHttpResponse(upload_results, 'Unable to upload Chrome.') On 2014/11/21 00:17:25, jbudorick wrote: > s/Chrome/<apk under test name>/ Done. https://codereview.chromium.org/745793002/diff/1/build/android/pylib/base/rem... build/android/pylib/base/remote_device_test_run.py:109: self.TestHttpResponse(upload_results, 'Unable to upload test.') On 2014/11/21 00:17:25, jbudorick wrote: > s/ test./ <test apk name>./ Done. https://codereview.chromium.org/745793002/diff/1/build/android/pylib/base/rem... build/android/pylib/base/remote_device_test_run.py:113: with remote_device_config.RemoteDeviceConfig( On 2014/11/21 00:17:25, jbudorick wrote: > I'm wondering if we should just use tempfile and do the config generation in > here. Do we anticipate using RemoteDeviceConfig anywhere else? Working on this, wont be in the next upload; but will be in the one after that. https://codereview.chromium.org/745793002/diff/1/build/android/pylib/base/tes... File build/android/pylib/base/test_instance_factory.py (right): https://codereview.chromium.org/745793002/diff/1/build/android/pylib/base/tes... build/android/pylib/base/test_instance_factory.py:6: from pylib.gtest import gtest_test_instance On 2014/11/21 00:17:25, jbudorick wrote: > Ha, be careful with this one. I can take it out until you think its ready to go live. https://codereview.chromium.org/745793002/diff/1/build/android/pylib/uirobot/... File build/android/pylib/uirobot/uirobot_test_instance.py (right): https://codereview.chromium.org/745793002/diff/1/build/android/pylib/uirobot/... build/android/pylib/uirobot/uirobot_test_instance.py:15: constants.GetOutDirectory(), 'apks', 'Chrome.apk') On 2014/11/21 00:17:25, jbudorick wrote: > apk name shouldn't be hard-coded. Is there somewhere in constants I can pull this form? I looked but couldn't find anything that looked right. https://codereview.chromium.org/745793002/diff/1/build/android/test_runner.py File build/android/test_runner.py (right): https://codereview.chromium.org/745793002/diff/1/build/android/test_runner.py... build/android/test_runner.py:937: 'gtest','uirobot', On 2014/11/21 00:17:26, jbudorick wrote: > nit: space after the first comma Done.
https://codereview.chromium.org/745793002/diff/1/build/android/pylib/base/rem... File build/android/pylib/base/remote_device_config.py (right): https://codereview.chromium.org/745793002/diff/1/build/android/pylib/base/rem... build/android/pylib/base/remote_device_config.py:35: config_data.append('[robotium]') On 2014/11/21 18:26:46, rnephew1 wrote: > On 2014/11/21 00:17:24, jbudorick wrote: > > This should probably be configurable. > > Added to do for this. Will do before landing CL. (need to wait for appurify to > land in third party anyway) Done in the latest patch set. Still a lot of room for improvement for configuration though.
https://codereview.chromium.org/745793002/diff/80001/build/android/pylib/devi... File build/android/pylib/device/remote_device_environment.py (right): https://codereview.chromium.org/745793002/diff/80001/build/android/pylib/devi... build/android/pylib/device/remote_device_environment.py:1: # Copyright 2014 The Chromium Authors. All rights reserved. These need to be in build/android/pylib/remote/device, not build/android/pylib/device. https://codereview.chromium.org/745793002/diff/80001/build/android/pylib/devi... build/android/pylib/device/remote_device_environment.py:85: def GetDeviceList(self): Why are GetDeviceList and SelectDevice separate? Do we ever need to use self._device_list again? Also, most of these functions -- GetAccessToken, RevokeAccessToken, and this device selection stuff -- look like they should be private. https://codereview.chromium.org/745793002/diff/80001/build/android/pylib/devi... File build/android/pylib/device/remote_device_test_run.py (right): https://codereview.chromium.org/745793002/diff/80001/build/android/pylib/devi... build/android/pylib/device/remote_device_test_run.py:53: if self._test_instance.TestType() == 'gtest': Perhaps these should be in separate classes with a shared implementation of RunTest + other common code? https://codereview.chromium.org/745793002/diff/80001/build/android/pylib/devi... build/android/pylib/device/remote_device_test_run.py:55: runner_type = self.DEFAULT_GTEST_RUNNER_TYPE Log a message here at info level about the default. https://codereview.chromium.org/745793002/diff/80001/build/android/pylib/devi... build/android/pylib/device/remote_device_test_run.py:59: runner_package = self.DEFAULT_GTEST_RUNNER_PACKAGE Same here. https://codereview.chromium.org/745793002/diff/80001/build/android/pylib/devi... build/android/pylib/device/remote_device_test_run.py:86: self.WaitForTest(test_run_id) We're going to have to figure out the trigger/collection separation. https://codereview.chromium.org/745793002/diff/80001/build/android/pylib/devi... build/android/pylib/device/remote_device_test_run.py:117: %(test_name)) nit: space after % You may want to just put the entire parameter on the next line. https://codereview.chromium.org/745793002/diff/80001/build/android/pylib/devi... File build/android/pylib/device/remote_device_utils.py (right): https://codereview.chromium.org/745793002/diff/80001/build/android/pylib/devi... build/android/pylib/device/remote_device_utils.py:1: # Copyright 2014 The Chromium Authors. All rights reserved. I'd prefer a different name for this - the current one implies that this is somehow similar to device_utils.py, which it isn't. https://codereview.chromium.org/745793002/diff/80001/build/android/pylib/uiro... File build/android/pylib/uirobot/uirobot_test_instance.py (right): https://codereview.chromium.org/745793002/diff/80001/build/android/pylib/uiro... build/android/pylib/uirobot/uirobot_test_instance.py:10: class UirobotTestInstance(test_instance.TestInstance): Will we run uirobot tests in any other environments? https://codereview.chromium.org/745793002/diff/80001/build/android/pylib/uiro... build/android/pylib/uirobot/uirobot_test_instance.py:20: constants.GetOutDirectory(), 'apks', 'Chrome.apk') This shouldn't be hard-coded. https://codereview.chromium.org/745793002/diff/80001/build/android/pylib/uiro... build/android/pylib/uirobot/uirobot_test_instance.py:21: self._minutes=options.minutes nit: spaces around = https://codereview.chromium.org/745793002/diff/80001/build/android/test_runne... File build/android/test_runner.py (right): https://codereview.chromium.org/745793002/diff/80001/build/android/test_runne... build/android/test_runner.py:962: # TODO(rnephew): Get rid of this when results handling For now, let's stick with returning instances of BaseTestResult and TestRunResults.
https://codereview.chromium.org/745793002/diff/80001/build/android/test_runne... File build/android/test_runner.py (right): https://codereview.chromium.org/745793002/diff/80001/build/android/test_runne... build/android/test_runner.py:125: group.add_option('--api-key', default='', type='string', We may want --api-key and --api-secret to have corresponding file versions, i.e., maybe we should allow callers to pass them in via files rather than via inline strings.
I still need to make the appurify test results match the expected test results format. https://codereview.chromium.org/745793002/diff/80001/build/android/pylib/devi... File build/android/pylib/device/remote_device_environment.py (right): https://codereview.chromium.org/745793002/diff/80001/build/android/pylib/devi... build/android/pylib/device/remote_device_environment.py:1: # Copyright 2014 The Chromium Authors. All rights reserved. On 2014/11/30 22:57:02, jbudorick wrote: > These need to be in build/android/pylib/remote/device, not > build/android/pylib/device. Done. https://codereview.chromium.org/745793002/diff/80001/build/android/pylib/devi... build/android/pylib/device/remote_device_environment.py:85: def GetDeviceList(self): On 2014/11/30 22:57:02, jbudorick wrote: > Why are GetDeviceList and SelectDevice separate? Do we ever need to use > self._device_list again? > > Also, most of these functions -- GetAccessToken, RevokeAccessToken, and this > device selection stuff -- look like they should be private. Done. https://codereview.chromium.org/745793002/diff/80001/build/android/pylib/devi... File build/android/pylib/device/remote_device_test_run.py (right): https://codereview.chromium.org/745793002/diff/80001/build/android/pylib/devi... build/android/pylib/device/remote_device_test_run.py:53: if self._test_instance.TestType() == 'gtest': On 2014/11/30 22:57:02, jbudorick wrote: > Perhaps these should be in separate classes with a shared implementation of > RunTest + other common code? Done. https://codereview.chromium.org/745793002/diff/80001/build/android/pylib/devi... build/android/pylib/device/remote_device_test_run.py:55: runner_type = self.DEFAULT_GTEST_RUNNER_TYPE On 2014/11/30 22:57:02, jbudorick wrote: > Log a message here at info level about the default. Done. https://codereview.chromium.org/745793002/diff/80001/build/android/pylib/devi... build/android/pylib/device/remote_device_test_run.py:59: runner_package = self.DEFAULT_GTEST_RUNNER_PACKAGE On 2014/11/30 22:57:02, jbudorick wrote: > Same here. Done. https://codereview.chromium.org/745793002/diff/80001/build/android/pylib/devi... build/android/pylib/device/remote_device_test_run.py:86: self.WaitForTest(test_run_id) On 2014/11/30 22:57:02, jbudorick wrote: > We're going to have to figure out the trigger/collection separation. Agreed, for now I'll leave this here for the sake of the rest of it working and add a todo. https://codereview.chromium.org/745793002/diff/80001/build/android/pylib/devi... build/android/pylib/device/remote_device_test_run.py:117: %(test_name)) On 2014/11/30 22:57:02, jbudorick wrote: > nit: space after % > > You may want to just put the entire parameter on the next line. Done. https://codereview.chromium.org/745793002/diff/80001/build/android/pylib/devi... File build/android/pylib/device/remote_device_utils.py (right): https://codereview.chromium.org/745793002/diff/80001/build/android/pylib/devi... build/android/pylib/device/remote_device_utils.py:1: # Copyright 2014 The Chromium Authors. All rights reserved. On 2014/11/30 22:57:02, jbudorick wrote: > I'd prefer a different name for this - the current one implies that this is > somehow similar to device_utils.py, which it isn't. Chaning it to remote_device_helper for now, open to suggestions on names. https://codereview.chromium.org/745793002/diff/80001/build/android/pylib/uiro... File build/android/pylib/uirobot/uirobot_test_instance.py (right): https://codereview.chromium.org/745793002/diff/80001/build/android/pylib/uiro... build/android/pylib/uirobot/uirobot_test_instance.py:10: class UirobotTestInstance(test_instance.TestInstance): On 2014/11/30 22:57:02, jbudorick wrote: > Will we run uirobot tests in any other environments? In theory we could, if appurify gives us the test. https://codereview.chromium.org/745793002/diff/80001/build/android/pylib/uiro... build/android/pylib/uirobot/uirobot_test_instance.py:20: constants.GetOutDirectory(), 'apks', 'Chrome.apk') On 2014/11/30 22:57:02, jbudorick wrote: > This shouldn't be hard-coded. Done. https://codereview.chromium.org/745793002/diff/80001/build/android/pylib/uiro... build/android/pylib/uirobot/uirobot_test_instance.py:21: self._minutes=options.minutes On 2014/11/30 22:57:02, jbudorick wrote: > nit: spaces around = Done. https://codereview.chromium.org/745793002/diff/80001/build/android/test_runne... File build/android/test_runner.py (right): https://codereview.chromium.org/745793002/diff/80001/build/android/test_runne... build/android/test_runner.py:125: group.add_option('--api-key', default='', type='string', On 2014/12/01 17:38:40, jbudorick wrote: > We may want --api-key and --api-secret to have corresponding file versions, > i.e., maybe we should allow callers to pass them in via files rather than via > inline strings. Done. https://codereview.chromium.org/745793002/diff/80001/build/android/test_runne... build/android/test_runner.py:962: # TODO(rnephew): Get rid of this when results handling On 2014/11/30 22:57:02, jbudorick wrote: > For now, let's stick with returning instances of BaseTestResult and > TestRunResults. Got rid of this code, but the appurify stuff doesn't return in the correct format yet.
needs to be rebased on the argparse patch + https://codereview.chromium.org/781573002/ (once it lands) https://codereview.chromium.org/745793002/diff/100001/build/android/pylib/bas... File build/android/pylib/base/test_run_factory.py (right): https://codereview.chromium.org/745793002/diff/100001/build/android/pylib/bas... build/android/pylib/base/test_run_factory.py:10: def CreateTestRun(_options, _env, _test_instance, error_func): Once you start using a parameter, you should remove the underscore. Here, all 3 leading underscores can be removed. https://codereview.chromium.org/745793002/diff/100001/build/android/pylib/bas... build/android/pylib/base/test_run_factory.py:11: if (_options.environment == 'remote_device' and Why check if options.environment == 'remote_device' twice? Why not nested ifs? https://codereview.chromium.org/745793002/diff/100001/build/android/pylib/rem... File build/android/pylib/remote/device/remote_device_environment.py (right): https://codereview.chromium.org/745793002/diff/100001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_environment.py:30: if options.api_key_file: api_key_file and api_key should be mutually exclusive. That's enforceable in argparse. https://codereview.chromium.org/745793002/diff/100001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_environment.py:33: if options.api_secret_file: Same re mutual exclusivity. https://codereview.chromium.org/745793002/diff/100001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_environment.py:58: os.environ['APPURIFY_API_PORT'] = '80' This shouldn't be hard-coded. https://codereview.chromium.org/745793002/diff/100001/build/android/pylib/rem... File build/android/pylib/remote/device/remote_device_gtest_run.py (right): https://codereview.chromium.org/745793002/diff/100001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_gtest_run.py:49: if not self._env.runner_package: This might be a bit more readable with a line break here. https://codereview.chromium.org/745793002/diff/100001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_gtest_run.py:55: self._test_id = self.UploadTestToDevice(runner_type) Same. https://codereview.chromium.org/745793002/diff/100001/build/android/pylib/rem... File build/android/pylib/remote/device/remote_device_test_run.py (right): https://codereview.chromium.org/745793002/diff/100001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py:45: def SetUp(self): This is unnecessary if it's not implemented. https://codereview.chromium.org/745793002/diff/100001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py:56: #TODO(rnephew): Need to seperate invoking from results gather. Here's what I'm thinking with this: - Figure out what we need to keep between trigger + collection. - On trigger, pickle whatever we need to keep in a file passed as an option. - On collection, pass the pickle file in and rebuild from that. https://codereview.chromium.org/745793002/diff/100001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py:63: """Teardown the test run.""" nit: "Teardown" -> "Tear down" https://codereview.chromium.org/745793002/diff/100001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py:67: """Runs when entering with with keyword.""" reword: "Set up the test run when used as a context manager." https://codereview.chromium.org/745793002/diff/100001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py:72: """Runs when exiting with with keyword.""" Similarly, "Tear down the test run when used as a context manager." https://codereview.chromium.org/745793002/diff/100001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py:75: def GetTestByName(self, test_name): Do we anticipate calling this beyond uirobot? If not, should it be here or in the uirobot test run? https://codereview.chromium.org/745793002/diff/100001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py:83: 'Unable to get tests list.') nit: one more space https://codereview.chromium.org/745793002/diff/100001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py:86: return test['test_id'] nit: only one space between "return" and "test" I'm somewhat surprised that pylint didn't hit this. https://codereview.chromium.org/745793002/diff/100001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py:132: with open(self._test_instance.apk, 'rb') as test_src: yeesh. Do we have to have an open file handle? Can we not specify the file? https://codereview.chromium.org/745793002/diff/100001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py:145: config_data = ['[appurify]\n', '[' + runner_type + ']\n'] nit: leave off the '\n' here and below and just do config.write('%s\n' % l for l in config_data) config.flush() below https://codereview.chromium.org/745793002/diff/100001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py:149: config_response = appurify.api.config_upload(self._env.token, Again, do we have to pass an open file handle? https://codereview.chromium.org/745793002/diff/100001/build/android/pylib/rem... File build/android/pylib/remote/device/remote_device_test_run.py.back (right): https://codereview.chromium.org/745793002/diff/100001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py.back:1: # Copyright 2014 The Chromium Authors. All rights reserved. What is this file? https://codereview.chromium.org/745793002/diff/100001/build/android/pylib/uir... File build/android/pylib/uirobot/uirobot_test_instance.py (right): https://codereview.chromium.org/745793002/diff/100001/build/android/pylib/uir... build/android/pylib/uirobot/uirobot_test_instance.py:30: """Setup for test.""" nit: Setup -> Set up https://codereview.chromium.org/745793002/diff/100001/build/android/pylib/uir... build/android/pylib/uirobot/uirobot_test_instance.py:35: """Teardown for test.""" nit: Teardown -> Tear down
https://codereview.chromium.org/745793002/diff/100001/build/android/pylib/bas... File build/android/pylib/base/test_run_factory.py (right): https://codereview.chromium.org/745793002/diff/100001/build/android/pylib/bas... build/android/pylib/base/test_run_factory.py:10: def CreateTestRun(_options, _env, _test_instance, error_func): On 2014/12/03 22:46:09, jbudorick wrote: > Once you start using a parameter, you should remove the underscore. Here, all 3 > leading underscores can be removed. Done. https://codereview.chromium.org/745793002/diff/100001/build/android/pylib/bas... build/android/pylib/base/test_run_factory.py:11: if (_options.environment == 'remote_device' and On 2014/12/03 22:46:09, jbudorick wrote: > Why check if options.environment == 'remote_device' twice? Why not nested ifs? Done. https://codereview.chromium.org/745793002/diff/100001/build/android/pylib/rem... File build/android/pylib/remote/device/remote_device_environment.py (right): https://codereview.chromium.org/745793002/diff/100001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_environment.py:30: if options.api_key_file: On 2014/12/03 22:46:09, jbudorick wrote: > api_key_file and api_key should be mutually exclusive. That's enforceable in > argparse. Done. https://codereview.chromium.org/745793002/diff/100001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_environment.py:33: if options.api_secret_file: On 2014/12/03 22:46:09, jbudorick wrote: > Same re mutual exclusivity. Done. https://codereview.chromium.org/745793002/diff/100001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_environment.py:58: os.environ['APPURIFY_API_PORT'] = '80' On 2014/12/03 22:46:09, jbudorick wrote: > This shouldn't be hard-coded. Done. https://codereview.chromium.org/745793002/diff/100001/build/android/pylib/rem... File build/android/pylib/remote/device/remote_device_gtest_run.py (right): https://codereview.chromium.org/745793002/diff/100001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_gtest_run.py:49: if not self._env.runner_package: On 2014/12/03 22:46:09, jbudorick wrote: > This might be a bit more readable with a line break here. Done. https://codereview.chromium.org/745793002/diff/100001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_gtest_run.py:55: self._test_id = self.UploadTestToDevice(runner_type) On 2014/12/03 22:46:09, jbudorick wrote: > Same. Done. https://codereview.chromium.org/745793002/diff/100001/build/android/pylib/rem... File build/android/pylib/remote/device/remote_device_test_run.py (right): https://codereview.chromium.org/745793002/diff/100001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py:45: def SetUp(self): On 2014/12/03 22:46:09, jbudorick wrote: > This is unnecessary if it's not implemented. This is the base class that uirobot_runner and gtest_runner build on. This is just so any future tests implement their own setup; I just used the same error you used in base/environment.py https://codereview.chromium.org/745793002/diff/100001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py:56: #TODO(rnephew): Need to seperate invoking from results gather. On 2014/12/03 22:46:09, jbudorick wrote: > Here's what I'm thinking with this: > - Figure out what we need to keep between trigger + collection. > - On trigger, pickle whatever we need to keep in a file passed as an option. > - On collection, pass the pickle file in and rebuild from that. The only thing that should be required is the test_run_id. The collection step can get a new token. I'll add remotedeviceoptions --trigger and --collect, and have that choose which codepath it goes down. Does that work for you? https://codereview.chromium.org/745793002/diff/100001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py:63: """Teardown the test run.""" On 2014/12/03 22:46:09, jbudorick wrote: > nit: "Teardown" -> "Tear down" Done. https://codereview.chromium.org/745793002/diff/100001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py:67: """Runs when entering with with keyword.""" On 2014/12/03 22:46:09, jbudorick wrote: > reword: "Set up the test run when used as a context manager." Done. https://codereview.chromium.org/745793002/diff/100001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py:72: """Runs when exiting with with keyword.""" On 2014/12/03 22:46:09, jbudorick wrote: > Similarly, "Tear down the test run when used as a context manager." Done. https://codereview.chromium.org/745793002/diff/100001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py:75: def GetTestByName(self, test_name): On 2014/12/03 22:46:09, jbudorick wrote: > Do we anticipate calling this beyond uirobot? If not, should it be here or in > the uirobot test run? Currently I do not plan on it. But if we can somehow find out if there were any changes between the current build of the test and the last upload of the test, then we could use this in those instances. Thats why I picked it to go here instead of in UIRobot. https://codereview.chromium.org/745793002/diff/100001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py:83: 'Unable to get tests list.') On 2014/12/03 22:46:09, jbudorick wrote: > nit: one more space Done. https://codereview.chromium.org/745793002/diff/100001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py:86: return test['test_id'] On 2014/12/03 22:46:09, jbudorick wrote: > nit: only one space between "return" and "test" > > I'm somewhat surprised that pylint didn't hit this. Done. https://codereview.chromium.org/745793002/diff/100001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py:132: with open(self._test_instance.apk, 'rb') as test_src: On 2014/12/03 22:46:09, jbudorick wrote: > yeesh. Do we have to have an open file handle? Can we not specify the file? Unfortunately that's how they do it. It fails if I pass the file path. https://codereview.chromium.org/745793002/diff/100001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py:145: config_data = ['[appurify]\n', '[' + runner_type + ']\n'] On 2014/12/03 22:46:09, jbudorick wrote: > nit: leave off the '\n' here and below and just do > > config.write('%s\n' % l for l in config_data) > config.flush() > > below Done. https://codereview.chromium.org/745793002/diff/100001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py:149: config_response = appurify.api.config_upload(self._env.token, On 2014/12/03 22:46:09, jbudorick wrote: > Again, do we have to pass an open file handle? See above. https://codereview.chromium.org/745793002/diff/100001/build/android/pylib/rem... File build/android/pylib/remote/device/remote_device_test_run.py.back (right): https://codereview.chromium.org/745793002/diff/100001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py.back:1: # Copyright 2014 The Chromium Authors. All rights reserved. On 2014/12/03 22:46:10, jbudorick wrote: > What is this file? This shouldn't be here. https://codereview.chromium.org/745793002/diff/100001/build/android/pylib/uir... File build/android/pylib/uirobot/uirobot_test_instance.py (right): https://codereview.chromium.org/745793002/diff/100001/build/android/pylib/uir... build/android/pylib/uirobot/uirobot_test_instance.py:30: """Setup for test.""" On 2014/12/03 22:46:10, jbudorick wrote: > nit: Setup -> Set up Done. https://codereview.chromium.org/745793002/diff/100001/build/android/pylib/uir... build/android/pylib/uirobot/uirobot_test_instance.py:35: """Teardown for test.""" On 2014/12/03 22:46:10, jbudorick wrote: > nit: Teardown -> Tear down Done.
gtest test instance has landed, so it's rebase time https://codereview.chromium.org/745793002/diff/100001/build/android/pylib/rem... File build/android/pylib/remote/device/remote_device_test_run.py (right): https://codereview.chromium.org/745793002/diff/100001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py:45: def SetUp(self): On 2014/12/03 23:49:25, rnephew1 wrote: > On 2014/12/03 22:46:09, jbudorick wrote: > > This is unnecessary if it's not implemented. > > This is the base class that uirobot_runner and gtest_runner build on. This is > just so any future tests implement their own setup; I just used the same error > you used in base/environment.py This inherits from TestRun, which has already defined SetUp to raise a NotImplementedError. https://codereview.chromium.org/745793002/diff/100001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py:56: #TODO(rnephew): Need to seperate invoking from results gather. On 2014/12/03 23:49:25, rnephew1 wrote: > On 2014/12/03 22:46:09, jbudorick wrote: > > Here's what I'm thinking with this: > > - Figure out what we need to keep between trigger + collection. > > - On trigger, pickle whatever we need to keep in a file passed as an option. > > - On collection, pass the pickle file in and rebuild from that. > > The only thing that should be required is the test_run_id. The collection step > can get a new token. > I'll add remotedeviceoptions --trigger and --collect, and have that choose which > codepath it goes down. Does that work for you? Both will need to take a file parameter, but yeah, that sounds ok. https://codereview.chromium.org/745793002/diff/100001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py:75: def GetTestByName(self, test_name): On 2014/12/03 23:49:26, rnephew1 wrote: > On 2014/12/03 22:46:09, jbudorick wrote: > > Do we anticipate calling this beyond uirobot? If not, should it be here or in > > the uirobot test run? > > Currently I do not plan on it. But if we can somehow find out if there were any > changes between the current build of the test and the last upload of the test, > then we could use this in those instances. Thats why I picked it to go here > instead of in UIRobot. Going to have to talk to you more about this one. https://codereview.chromium.org/745793002/diff/100001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py:132: with open(self._test_instance.apk, 'rb') as test_src: On 2014/12/03 23:49:25, rnephew1 wrote: > On 2014/12/03 22:46:09, jbudorick wrote: > > yeesh. Do we have to have an open file handle? Can we not specify the file? > > Unfortunately that's how they do it. It fails if I pass the file path. :(
I've got a lot of comments, but this is generally looking pretty good. https://codereview.chromium.org/745793002/diff/180001/build/android/pylib/bas... File build/android/pylib/base/environment_factory.py (right): https://codereview.chromium.org/745793002/diff/180001/build/android/pylib/bas... build/android/pylib/base/environment_factory.py:7: def CreateEnvironment(_args, error_func): _args -> args, you're using it now https://codereview.chromium.org/745793002/diff/180001/build/android/pylib/bas... File build/android/pylib/base/test_run_factory.py (right): https://codereview.chromium.org/745793002/diff/180001/build/android/pylib/bas... build/android/pylib/base/test_run_factory.py:8: def CreateTestRun(options, env, test_instance, error_func): rename options -> args https://codereview.chromium.org/745793002/diff/180001/build/android/pylib/bas... build/android/pylib/base/test_run_factory.py:9: if (options.environment == 'remote_device'): no parentheses around the if condition https://codereview.chromium.org/745793002/diff/180001/build/android/pylib/bas... build/android/pylib/base/test_run_factory.py:10: if (test_instance.TestType() == 'gtest'): same https://codereview.chromium.org/745793002/diff/180001/build/android/pylib/bas... build/android/pylib/base/test_run_factory.py:12: if (test_instance.TestType() == 'uirobot'): same https://codereview.chromium.org/745793002/diff/180001/build/android/pylib/bas... build/android/pylib/base/test_run_factory.py:17: error_func('No test runs are currently supported.') rephrase https://codereview.chromium.org/745793002/diff/180001/build/android/pylib/rem... File build/android/pylib/remote/device/remote_device_environment.py (right): https://codereview.chromium.org/745793002/diff/180001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_environment.py:57: self.trigger = args.trigger Why are these public? If other classes need access, that should be handled via a @property. https://codereview.chromium.org/745793002/diff/180001/build/android/pylib/rem... File build/android/pylib/remote/device/remote_device_gtest_run.py (right): https://codereview.chromium.org/745793002/diff/180001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_gtest_run.py:65: def PackageTestResults(self): This should return something. You can return an empty TestRunResults object for now, but if you do, TODO yourself and address that in your next CL. https://codereview.chromium.org/745793002/diff/180001/build/android/pylib/rem... File build/android/pylib/remote/device/remote_device_test_run.py (right): https://codereview.chromium.org/745793002/diff/180001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py:41: self._trigger_and_collect = (True if not self._env.trigger Why is this necessary? Why can't the "if not self._trigger_and_collect" lines just check the opposite? (with, perhaps, some reworking of how exactly the options are handled) https://codereview.chromium.org/745793002/diff/180001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py:58: if not self._trigger_and_collect: (following from above: why can't this just be if not self._collect) https://codereview.chromium.org/745793002/diff/180001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py:59: with open(self._env.trigger, 'w+') as test_run_id_file: Why w+ and not just w? We don't need to read the file... https://codereview.chromium.org/745793002/diff/180001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py:85: def PackageTestResults(self): I might name this "ParseTestResults" or similar. https://codereview.chromium.org/745793002/diff/180001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py:103: def DownloadTestResults(self, results_path): Do any of these functions (PackageTestResults, GetTestByName, DownloadTestResults, WaitForTest, UploadAppToDevice, UploadTestToDevice, and SetTestConfig) need to be public? If not, they should be preceded by an underscore. (They can still be called by derived classes.) https://codereview.chromium.org/745793002/diff/180001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py:109: if results_path: When would results_path be empty? Should that be an error? It almost certainly shouldn't be silent... https://codereview.chromium.org/745793002/diff/180001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py:123: test_check_res = appurify.api.tests_check_result(self._env.token, We should check before the first sleep. https://codereview.chromium.org/745793002/diff/180001/build/android/pylib/rem... File build/android/pylib/remote/device/remote_device_uirobot_run.py (right): https://codereview.chromium.org/745793002/diff/180001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_uirobot_run.py:55: pass This should return something, even if it's an empty TestRunResults object. https://codereview.chromium.org/745793002/diff/180001/build/android/test_runn... File build/android/test_runner.py (right): https://codereview.chromium.org/745793002/diff/180001/build/android/test_runn... build/android/test_runner.py:137: api_key_group.add_argument('--api-key', default='', nit: handle api_key_group and api_secret_group all at once, and separately from the main group. e.g. group.add_argument(...) ... api_key_group = group.add_mutually_exclusive_group() api_key_group.add_argument('--api-key', ...) api_key_group.add_argument('--api-key-file', ...) api_secret_group = group.add_mutually_exclusive_group() api_secret_group.add_argument('--api-secret', ...) api_secret_group.add_argument('--api-secret-file', ...) https://codereview.chromium.org/745793002/diff/180001/build/android/test_runn... build/android/test_runner.py:915: return # Not returning results, only triggering. return 0 https://codereview.chromium.org/745793002/diff/180001/build/android/test_runn... build/android/test_runner.py:928: return results I feel like I've written this a bunch locally, but this needs to be return 0 if results.DidRunPass() else 1
https://codereview.chromium.org/745793002/diff/180001/build/android/pylib/bas... File build/android/pylib/base/environment_factory.py (right): https://codereview.chromium.org/745793002/diff/180001/build/android/pylib/bas... build/android/pylib/base/environment_factory.py:7: def CreateEnvironment(_args, error_func): On 2014/12/04 21:51:33, jbudorick wrote: > _args -> args, you're using it now Done. https://codereview.chromium.org/745793002/diff/180001/build/android/pylib/bas... File build/android/pylib/base/test_run_factory.py (right): https://codereview.chromium.org/745793002/diff/180001/build/android/pylib/bas... build/android/pylib/base/test_run_factory.py:8: def CreateTestRun(options, env, test_instance, error_func): On 2014/12/04 21:51:33, jbudorick wrote: > rename options -> args Done. https://codereview.chromium.org/745793002/diff/180001/build/android/pylib/bas... build/android/pylib/base/test_run_factory.py:9: if (options.environment == 'remote_device'): On 2014/12/04 21:51:33, jbudorick wrote: > no parentheses around the if condition Done. https://codereview.chromium.org/745793002/diff/180001/build/android/pylib/bas... build/android/pylib/base/test_run_factory.py:10: if (test_instance.TestType() == 'gtest'): On 2014/12/04 21:51:33, jbudorick wrote: > same Done. https://codereview.chromium.org/745793002/diff/180001/build/android/pylib/bas... build/android/pylib/base/test_run_factory.py:12: if (test_instance.TestType() == 'uirobot'): On 2014/12/04 21:51:33, jbudorick wrote: > same Done. https://codereview.chromium.org/745793002/diff/180001/build/android/pylib/bas... build/android/pylib/base/test_run_factory.py:17: error_func('No test runs are currently supported.') On 2014/12/04 21:51:33, jbudorick wrote: > rephrase Done. https://codereview.chromium.org/745793002/diff/180001/build/android/pylib/rem... File build/android/pylib/remote/device/remote_device_environment.py (right): https://codereview.chromium.org/745793002/diff/180001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_environment.py:57: self.trigger = args.trigger On 2014/12/04 21:51:33, jbudorick wrote: > Why are these public? If other classes need access, that should be handled via a > @property. Done. https://codereview.chromium.org/745793002/diff/180001/build/android/pylib/rem... File build/android/pylib/remote/device/remote_device_gtest_run.py (right): https://codereview.chromium.org/745793002/diff/180001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_gtest_run.py:65: def PackageTestResults(self): On 2014/12/04 21:51:34, jbudorick wrote: > This should return something. You can return an empty TestRunResults object for > now, but if you do, TODO yourself and address that in your next CL. Done. https://codereview.chromium.org/745793002/diff/180001/build/android/pylib/rem... File build/android/pylib/remote/device/remote_device_test_run.py (right): https://codereview.chromium.org/745793002/diff/180001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py:41: self._trigger_and_collect = (True if not self._env.trigger On 2014/12/04 21:51:34, jbudorick wrote: > Why is this necessary? Why can't the "if not self._trigger_and_collect" lines > just check the opposite? (with, perhaps, some reworking of how exactly the > options are handled) Done. https://codereview.chromium.org/745793002/diff/180001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py:58: if not self._trigger_and_collect: On 2014/12/04 21:51:34, jbudorick wrote: > (following from above: why can't this just be if not self._collect) Done. https://codereview.chromium.org/745793002/diff/180001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py:59: with open(self._env.trigger, 'w+') as test_run_id_file: On 2014/12/04 21:51:34, jbudorick wrote: > Why w+ and not just w? We don't need to read the file... Done. https://codereview.chromium.org/745793002/diff/180001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py:85: def PackageTestResults(self): On 2014/12/04 21:51:34, jbudorick wrote: > I might name this "ParseTestResults" or similar. Done. https://codereview.chromium.org/745793002/diff/180001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py:103: def DownloadTestResults(self, results_path): On 2014/12/04 21:51:34, jbudorick wrote: > Do any of these functions (PackageTestResults, GetTestByName, > DownloadTestResults, WaitForTest, UploadAppToDevice, UploadTestToDevice, and > SetTestConfig) need to be public? If not, they should be preceded by an > underscore. (They can still be called by derived classes.) Done. https://codereview.chromium.org/745793002/diff/180001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py:109: if results_path: On 2014/12/04 21:51:34, jbudorick wrote: > When would results_path be empty? Should that be an error? It almost certainly > shouldn't be silent... This is only set when you want to download the full results from appurify (video/logs/etc.) Not just get the results from the json it returns. https://codereview.chromium.org/745793002/diff/180001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py:123: test_check_res = appurify.api.tests_check_result(self._env.token, On 2014/12/04 21:51:34, jbudorick wrote: > We should check before the first sleep. Done. https://codereview.chromium.org/745793002/diff/180001/build/android/pylib/rem... File build/android/pylib/remote/device/remote_device_uirobot_run.py (right): https://codereview.chromium.org/745793002/diff/180001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_uirobot_run.py:55: pass On 2014/12/04 21:51:34, jbudorick wrote: > This should return something, even if it's an empty TestRunResults object. Done. https://codereview.chromium.org/745793002/diff/180001/build/android/test_runn... File build/android/test_runner.py (right): https://codereview.chromium.org/745793002/diff/180001/build/android/test_runn... build/android/test_runner.py:137: api_key_group.add_argument('--api-key', default='', On 2014/12/04 21:51:34, jbudorick wrote: > nit: handle api_key_group and api_secret_group all at once, and separately from > the main group. e.g. > > group.add_argument(...) > ... > > api_key_group = group.add_mutually_exclusive_group() > api_key_group.add_argument('--api-key', ...) > api_key_group.add_argument('--api-key-file', ...) > > api_secret_group = group.add_mutually_exclusive_group() > api_secret_group.add_argument('--api-secret', ...) > api_secret_group.add_argument('--api-secret-file', ...) Done. https://codereview.chromium.org/745793002/diff/180001/build/android/test_runn... build/android/test_runner.py:915: return # Not returning results, only triggering. On 2014/12/04 21:51:34, jbudorick wrote: > return 0 Done. https://codereview.chromium.org/745793002/diff/180001/build/android/test_runn... build/android/test_runner.py:928: return results On 2014/12/04 21:51:34, jbudorick wrote: > I feel like I've written this a bunch locally, but this needs to be > > return 0 if results.DidRunPass() else 1 Done.
Very close. Mostly nits. https://codereview.chromium.org/745793002/diff/200001/build/android/pylib/bas... File build/android/pylib/base/test_run_factory.py (right): https://codereview.chromium.org/745793002/diff/200001/build/android/pylib/bas... build/android/pylib/base/test_run_factory.py:14: env,test_instance) nit: space after the comma I'm surprised pylint isn't whining about this. https://codereview.chromium.org/745793002/diff/200001/build/android/pylib/bas... build/android/pylib/base/test_run_factory.py:18: test_instance.TestType(), args.environment )) nit: no space after environment https://codereview.chromium.org/745793002/diff/200001/build/android/pylib/gte... File build/android/pylib/gtest/gtest_test_instance.py (right): https://codereview.chromium.org/745793002/diff/200001/build/android/pylib/gte... build/android/pylib/gtest/gtest_test_instance.py:45: #TODO(jbudorick): Support multiple test suites. nit: space before TODO https://codereview.chromium.org/745793002/diff/200001/build/android/pylib/gte... build/android/pylib/gtest/gtest_test_instance.py:47: raise ValueError("Currently supports only 1 suite") nit: clarify that gtest in platform mode only support a single suite https://codereview.chromium.org/745793002/diff/200001/build/android/pylib/rem... File build/android/pylib/remote/device/remote_device_environment.py (right): https://codereview.chromium.org/745793002/diff/200001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_environment.py:58: self._trigger = True Dynamic typing! In all seriousness, this could be a little dangerous, since you're opening a file with these names. https://codereview.chromium.org/745793002/diff/200001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_environment.py:110: raise remote_device_helper.RemoteDeviceError('No device found: %s %s' %( nits: space after %, put entire contents on subsequent line https://codereview.chromium.org/745793002/diff/200001/build/android/pylib/rem... File build/android/pylib/remote/device/remote_device_gtest_run.py (right): https://codereview.chromium.org/745793002/diff/200001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_gtest_run.py:12: from pylib.base.base_test_result import TestRunResults import base_test_result, not TestRunResults https://codereview.chromium.org/745793002/diff/200001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_gtest_run.py:29: def __init__(self, env, test_instance): If this is just calling the base class constructor, I don't think you need to define it. https://codereview.chromium.org/745793002/diff/200001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_gtest_run.py:66: #TODO(rnephew): Populate test results object. nit: space after TODO https://codereview.chromium.org/745793002/diff/200001/build/android/pylib/rem... File build/android/pylib/remote/device/remote_device_test_run.py (right): https://codereview.chromium.org/745793002/diff/200001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py:44: I wonder if we should define SetUp in here to do the trigger check? e.g. #override def SetUp(self): if self._env.trigger: self._SetUpImpl() def _SetUpImpl(self): raise NotImplementedError Thoughts? Do you think there'd be a case where we'd have to do test-type specific set up if we weren't triggering? https://codereview.chromium.org/745793002/diff/200001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py:49: #trigger nit: this doesn't add anything. Remove it. https://codereview.chromium.org/745793002/diff/200001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py:60: #collect Same. https://codereview.chromium.org/745793002/diff/200001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py:133: remote_device_helper.TestHttpResponse(upload_results, Either indent subsequent lines to the depth of the first parameter or put all parameters on subsequent lines and indent four spaces. https://codereview.chromium.org/745793002/diff/200001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py:155: config_data = ['[appurify]', '[' + runner_type + ']'] nit: '[%s]' % runner_type (for consistency if nothing else) https://codereview.chromium.org/745793002/diff/200001/build/android/pylib/rem... File build/android/pylib/remote/device/remote_device_uirobot_run.py (right): https://codereview.chromium.org/745793002/diff/200001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_uirobot_run.py:54: #TODO(rnephew): Populate test results object. nit: space after TODO
https://codereview.chromium.org/745793002/diff/200001/build/android/pylib/rem... File build/android/pylib/remote/device/remote_device_test_run.py (right): https://codereview.chromium.org/745793002/diff/200001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py:57: with open(self._env.trigger, 'w') as test_run_id_file: similar to below w.r.t. sanity-checking the type of self._env.trigger https://codereview.chromium.org/745793002/diff/200001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py:63: with open(self._env.collect, 'r') as test_run_id_file: To follow up on my dynamic typing comment: we should probably check the type of self._env.collect for sanity here. e.g., above this line, something like assert isinstance(self._env.collect, basestring)
https://codereview.chromium.org/745793002/diff/200001/build/android/pylib/bas... File build/android/pylib/base/test_run_factory.py (right): https://codereview.chromium.org/745793002/diff/200001/build/android/pylib/bas... build/android/pylib/base/test_run_factory.py:14: env,test_instance) On 2014/12/05 01:01:46, jbudorick wrote: > nit: space after the comma > > I'm surprised pylint isn't whining about this. Done. https://codereview.chromium.org/745793002/diff/200001/build/android/pylib/bas... build/android/pylib/base/test_run_factory.py:18: test_instance.TestType(), args.environment )) On 2014/12/05 01:01:46, jbudorick wrote: > nit: no space after environment Done. https://codereview.chromium.org/745793002/diff/200001/build/android/pylib/gte... File build/android/pylib/gtest/gtest_test_instance.py (right): https://codereview.chromium.org/745793002/diff/200001/build/android/pylib/gte... build/android/pylib/gtest/gtest_test_instance.py:45: #TODO(jbudorick): Support multiple test suites. On 2014/12/05 01:01:46, jbudorick wrote: > nit: space before TODO Done. https://codereview.chromium.org/745793002/diff/200001/build/android/pylib/gte... build/android/pylib/gtest/gtest_test_instance.py:47: raise ValueError("Currently supports only 1 suite") On 2014/12/05 01:01:46, jbudorick wrote: > nit: clarify that gtest in platform mode only support a single suite Done. https://codereview.chromium.org/745793002/diff/200001/build/android/pylib/rem... File build/android/pylib/remote/device/remote_device_environment.py (right): https://codereview.chromium.org/745793002/diff/200001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_environment.py:58: self._trigger = True On 2014/12/05 01:01:46, jbudorick wrote: > Dynamic typing! > > In all seriousness, this could be a little dangerous, since you're opening a > file with these names. Added assert to make sure when a file is opened its a string. https://codereview.chromium.org/745793002/diff/200001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_environment.py:110: raise remote_device_helper.RemoteDeviceError('No device found: %s %s' %( On 2014/12/05 01:01:46, jbudorick wrote: > nits: space after %, put entire contents on subsequent line Done. https://codereview.chromium.org/745793002/diff/200001/build/android/pylib/rem... File build/android/pylib/remote/device/remote_device_gtest_run.py (right): https://codereview.chromium.org/745793002/diff/200001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_gtest_run.py:12: from pylib.base.base_test_result import TestRunResults On 2014/12/05 01:01:46, jbudorick wrote: > import base_test_result, not TestRunResults Done. https://codereview.chromium.org/745793002/diff/200001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_gtest_run.py:29: def __init__(self, env, test_instance): On 2014/12/05 01:01:46, jbudorick wrote: > If this is just calling the base class constructor, I don't think you need to > define it. Done. https://codereview.chromium.org/745793002/diff/200001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_gtest_run.py:66: #TODO(rnephew): Populate test results object. On 2014/12/05 01:01:46, jbudorick wrote: > nit: space after TODO Done. https://codereview.chromium.org/745793002/diff/200001/build/android/pylib/rem... File build/android/pylib/remote/device/remote_device_test_run.py (right): https://codereview.chromium.org/745793002/diff/200001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py:44: On 2014/12/05 01:01:46, jbudorick wrote: > I wonder if we should define SetUp in here to do the trigger check? e.g. > > #override > def SetUp(self): > if self._env.trigger: > self._SetUpImpl() > > def _SetUpImpl(self): > raise NotImplementedError > > Thoughts? Do you think there'd be a case where we'd have to do test-type > specific set up if we weren't triggering? Done. https://codereview.chromium.org/745793002/diff/200001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py:49: #trigger On 2014/12/05 01:01:46, jbudorick wrote: > nit: this doesn't add anything. Remove it. Done. https://codereview.chromium.org/745793002/diff/200001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py:57: with open(self._env.trigger, 'w') as test_run_id_file: On 2014/12/05 01:03:37, jbudorick wrote: > similar to below w.r.t. sanity-checking the type of self._env.trigger Done. https://codereview.chromium.org/745793002/diff/200001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py:60: #collect On 2014/12/05 01:01:46, jbudorick wrote: > Same. Done. https://codereview.chromium.org/745793002/diff/200001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py:63: with open(self._env.collect, 'r') as test_run_id_file: On 2014/12/05 01:03:37, jbudorick wrote: > To follow up on my dynamic typing comment: we should probably check the type of > self._env.collect for sanity here. > > e.g., above this line, something like > > assert isinstance(self._env.collect, basestring) Done. https://codereview.chromium.org/745793002/diff/200001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py:133: remote_device_helper.TestHttpResponse(upload_results, On 2014/12/05 01:01:46, jbudorick wrote: > Either indent subsequent lines to the depth of the first parameter or put all > parameters on subsequent lines and indent four spaces. Done. https://codereview.chromium.org/745793002/diff/200001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py:155: config_data = ['[appurify]', '[' + runner_type + ']'] On 2014/12/05 01:01:46, jbudorick wrote: > nit: '[%s]' % runner_type (for consistency if nothing else) Done. https://codereview.chromium.org/745793002/diff/200001/build/android/pylib/rem... File build/android/pylib/remote/device/remote_device_uirobot_run.py (right): https://codereview.chromium.org/745793002/diff/200001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_uirobot_run.py:54: #TODO(rnephew): Populate test results object. On 2014/12/05 01:01:46, jbudorick wrote: > nit: space after TODO Done.
https://codereview.chromium.org/745793002/diff/220001/build/android/pylib/rem... File build/android/pylib/remote/device/remote_device_environment.py (right): https://codereview.chromium.org/745793002/diff/220001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_environment.py:91: 'Unable to generate access token.') nit: one more space https://codereview.chromium.org/745793002/diff/220001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_environment.py:98: 'Unable to revoke access token.') same https://codereview.chromium.org/745793002/diff/220001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_environment.py:104: 'Unable to generate access token.') same https://codereview.chromium.org/745793002/diff/220001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_environment.py:107: if (device['name'] == self._remote_device and nit: put 'and' on the next line https://codereview.chromium.org/745793002/diff/220001/build/android/pylib/rem... File build/android/pylib/remote/device/remote_device_gtest_run.py (right): https://codereview.chromium.org/745793002/diff/220001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_gtest_run.py:33: """Setup the triggering of a test run.""" nit: Setup -> Set up https://codereview.chromium.org/745793002/diff/220001/build/android/pylib/rem... File build/android/pylib/remote/device/remote_device_test_run.py (right): https://codereview.chromium.org/745793002/diff/220001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py:56: "File for storing test_run_id must be a string.") nit: single quotes https://codereview.chromium.org/745793002/diff/220001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py:63: "File for storing test_run_id must be a string.") nit: single quotes https://codereview.chromium.org/745793002/diff/220001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py:87: """Setup a test run.""" nit: Setup -> Set up https://codereview.chromium.org/745793002/diff/220001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py:92: """Setup the triggering of a test run.""" nit: Setup -> Set up https://codereview.chromium.org/745793002/diff/220001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py:137: return test_check_res.json()['response']['status'] return self._results['status'] ? https://codereview.chromium.org/745793002/diff/220001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py:166: config = tempfile.TemporaryFile() Not sure how I missed this before. Just use the temp file as a context manager: with tempfile.TemporaryFile() as config: ... You can omit config.close() https://codereview.chromium.org/745793002/diff/220001/build/android/pylib/rem... File build/android/pylib/remote/device/remote_device_uirobot_run.py (right): https://codereview.chromium.org/745793002/diff/220001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_uirobot_run.py:22: """Run gtests and uirobot tests on a remote device.""" I don't think this is running gtests. https://codereview.chromium.org/745793002/diff/220001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_uirobot_run.py:40: """Setup the triggering of a test run.""" nit: Setup -> Set up https://codereview.chromium.org/745793002/diff/220001/build/android/pylib/uir... File build/android/pylib/uirobot/uirobot_test_instance.py (right): https://codereview.chromium.org/745793002/diff/220001/build/android/pylib/uir... build/android/pylib/uirobot/uirobot_test_instance.py:39: def apk(self): Can you remove this? It doesn't look like it gets called (and it would be useless if it did).
https://codereview.chromium.org/745793002/diff/220001/build/android/pylib/rem... File build/android/pylib/remote/device/remote_device_environment.py (right): https://codereview.chromium.org/745793002/diff/220001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_environment.py:91: 'Unable to generate access token.') On 2014/12/05 17:27:00, jbudorick wrote: > nit: one more space Done. https://codereview.chromium.org/745793002/diff/220001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_environment.py:98: 'Unable to revoke access token.') On 2014/12/05 17:27:00, jbudorick wrote: > same Done. https://codereview.chromium.org/745793002/diff/220001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_environment.py:104: 'Unable to generate access token.') On 2014/12/05 17:27:00, jbudorick wrote: > same Done. https://codereview.chromium.org/745793002/diff/220001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_environment.py:107: if (device['name'] == self._remote_device and On 2014/12/05 17:27:00, jbudorick wrote: > nit: put 'and' on the next line Done. https://codereview.chromium.org/745793002/diff/220001/build/android/pylib/rem... File build/android/pylib/remote/device/remote_device_gtest_run.py (right): https://codereview.chromium.org/745793002/diff/220001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_gtest_run.py:33: """Setup the triggering of a test run.""" On 2014/12/05 17:27:00, jbudorick wrote: > nit: Setup -> Set up Done. https://codereview.chromium.org/745793002/diff/220001/build/android/pylib/rem... File build/android/pylib/remote/device/remote_device_test_run.py (right): https://codereview.chromium.org/745793002/diff/220001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py:56: "File for storing test_run_id must be a string.") On 2014/12/05 17:27:00, jbudorick wrote: > nit: single quotes Done. https://codereview.chromium.org/745793002/diff/220001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py:63: "File for storing test_run_id must be a string.") On 2014/12/05 17:27:00, jbudorick wrote: > nit: single quotes Done. https://codereview.chromium.org/745793002/diff/220001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py:87: """Setup a test run.""" On 2014/12/05 17:27:00, jbudorick wrote: > nit: Setup -> Set up Done. https://codereview.chromium.org/745793002/diff/220001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py:92: """Setup the triggering of a test run.""" On 2014/12/05 17:27:00, jbudorick wrote: > nit: Setup -> Set up Done. https://codereview.chromium.org/745793002/diff/220001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py:137: return test_check_res.json()['response']['status'] On 2014/12/05 17:27:00, jbudorick wrote: > return self._results['status'] > > ? Done. https://codereview.chromium.org/745793002/diff/220001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py:166: config = tempfile.TemporaryFile() On 2014/12/05 17:27:00, jbudorick wrote: > Not sure how I missed this before. Just use the temp file as a context manager: > > with tempfile.TemporaryFile() as config: > ... > > You can omit config.close() Done. https://codereview.chromium.org/745793002/diff/220001/build/android/pylib/rem... File build/android/pylib/remote/device/remote_device_uirobot_run.py (right): https://codereview.chromium.org/745793002/diff/220001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_uirobot_run.py:22: """Run gtests and uirobot tests on a remote device.""" On 2014/12/05 17:27:00, jbudorick wrote: > I don't think this is running gtests. Done. https://codereview.chromium.org/745793002/diff/220001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_uirobot_run.py:40: """Setup the triggering of a test run.""" On 2014/12/05 17:27:00, jbudorick wrote: > nit: Setup -> Set up Done. https://codereview.chromium.org/745793002/diff/220001/build/android/pylib/uir... File build/android/pylib/uirobot/uirobot_test_instance.py (right): https://codereview.chromium.org/745793002/diff/220001/build/android/pylib/uir... build/android/pylib/uirobot/uirobot_test_instance.py:39: def apk(self): On 2014/12/05 17:27:00, jbudorick wrote: > Can you remove this? It doesn't look like it gets called (and it would be > useless if it did). Done.
This should be the last round. https://codereview.chromium.org/745793002/diff/240001/build/android/pylib/gte... File build/android/pylib/gtest/gtest_test_instance.py (right): https://codereview.chromium.org/745793002/diff/240001/build/android/pylib/gte... build/android/pylib/gtest/gtest_test_instance.py:47: raise ValueError("Platform mode currently supports only 1 gtest suite") nit: single quotes https://codereview.chromium.org/745793002/diff/240001/build/android/pylib/rem... File build/android/pylib/remote/device/remote_device_gtest_run.py (right): https://codereview.chromium.org/745793002/diff/240001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_gtest_run.py:38: logging.debug('Using default runner type: %s', self.DEFAULT_RUNNER_TYPE) nit: log this at info level https://codereview.chromium.org/745793002/diff/240001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_gtest_run.py:44: logging.debug('Using default runner package: %s', nit: log this at info level (debug only gets triggered by verbose count >= 2) https://codereview.chromium.org/745793002/diff/240001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_gtest_run.py:45: self.DEFAULT_RUNNER_TYPE) nit: two more spaces https://codereview.chromium.org/745793002/diff/240001/build/android/pylib/rem... File build/android/pylib/remote/device/remote_device_uirobot_run.py (right): https://codereview.chromium.org/745793002/diff/240001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_uirobot_run.py:44: logging.debug('Using default runner type: %s', self.DEFAULT_RUNNER_TYPE) again, log at info level
https://codereview.chromium.org/745793002/diff/240001/build/android/pylib/gte... File build/android/pylib/gtest/gtest_test_instance.py (right): https://codereview.chromium.org/745793002/diff/240001/build/android/pylib/gte... build/android/pylib/gtest/gtest_test_instance.py:47: raise ValueError("Platform mode currently supports only 1 gtest suite") On 2014/12/05 21:35:12, jbudorick wrote: > nit: single quotes Done. https://codereview.chromium.org/745793002/diff/240001/build/android/pylib/rem... File build/android/pylib/remote/device/remote_device_gtest_run.py (right): https://codereview.chromium.org/745793002/diff/240001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_gtest_run.py:38: logging.debug('Using default runner type: %s', self.DEFAULT_RUNNER_TYPE) On 2014/12/05 21:35:12, jbudorick wrote: > nit: log this at info level Done. https://codereview.chromium.org/745793002/diff/240001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_gtest_run.py:44: logging.debug('Using default runner package: %s', On 2014/12/05 21:35:12, jbudorick wrote: > nit: log this at info level (debug only gets triggered by verbose count >= 2) Done. https://codereview.chromium.org/745793002/diff/240001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_gtest_run.py:45: self.DEFAULT_RUNNER_TYPE) On 2014/12/05 21:35:12, jbudorick wrote: > nit: two more spaces Done. https://codereview.chromium.org/745793002/diff/240001/build/android/pylib/rem... File build/android/pylib/remote/device/remote_device_uirobot_run.py (right): https://codereview.chromium.org/745793002/diff/240001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_uirobot_run.py:44: logging.debug('Using default runner type: %s', self.DEFAULT_RUNNER_TYPE) On 2014/12/05 21:35:12, jbudorick wrote: > again, log at info level Done.
lgtm w/ nit https://codereview.chromium.org/745793002/diff/260001/build/android/pylib/rem... File build/android/pylib/remote/device/remote_device_test_run.py (right): https://codereview.chromium.org/745793002/diff/260001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py:174: 'Unable to upload test config.') nit: one more space
https://codereview.chromium.org/745793002/diff/260001/build/android/pylib/rem... File build/android/pylib/remote/device/remote_device_test_run.py (right): https://codereview.chromium.org/745793002/diff/260001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_test_run.py:174: 'Unable to upload test config.') On 2014/12/05 21:53:15, jbudorick wrote: > nit: one more space Done.
The CQ bit was checked by rnephew@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/745793002/280001
The CQ bit was unchecked by rnephew@chromium.org
Added requests to path in r_d_environment.py, r_d_test_run.py, r_d_test_gtest_run, and r_d_uirobot_run. Commiting as soon as John gives me the green light.
lgtm
The CQ bit was checked by rnephew@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/745793002/300001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by rnephew@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/745793002/320001
Message was sent while issue was closed.
Committed patchset #17 (id:320001)
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/5c49978f095340a59c62faaafe02a9527ec7728b Cr-Commit-Position: refs/heads/master@{#308139} |
