Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1828143004:
[Battor] Add python wrapper for communicating with battor_agent_binary (Closed)
|
Created:
4 years, 9 months ago by rnephew (Reviews Here) Modified:
4 years, 8 months ago CC:
catapult-reviews_chromium.org Base URL:
git@github.com:catapult-project/catapult@master Target Ref:
refs/heads/master Project:
catapult Visibility:
Public. |
Description[Battor] Add python wrapper for communicating with battor_agent_binary
BUG=chromium:574888
R=aiolos@chromium.org, charliea@chromium.org, nednguyen@google.com
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/4411881631aa959b820380e88a39b3eda86bf75c
Patch Set 1 #
Total comments: 6
Patch Set 2 : #
Total comments: 36
Patch Set 3 : #
Total comments: 24
Patch Set 4 : #
Total comments: 4
Patch Set 5 : Split StopTracing and collection of results #
Total comments: 1
Patch Set 6 : Make StopTracing not wait for results. #Patch Set 7 : Fix small bug in GetBattor... #Patch Set 8 : dependencies #
Total comments: 2
Patch Set 9 : rename tests #Patch Set 10 : move to common #
Total comments: 29
Patch Set 11 : #Patch Set 12 : #Patch Set 13 : default timeout is how long trace ran #
Total comments: 6
Patch Set 14 : #
Total comments: 8
Patch Set 15 : alexs script landed #
Total comments: 10
Patch Set 16 : #Patch Set 17 : #
Messages
Total messages: 49 (7 generated)
rnephew@chromium.org changed reviewers: + aiolos@chromium.org, alexandermont@chromium.org, nednguyen@google.com, zhenw@chromium.org
Note: This CL requires https://codereview.chromium.org/1823193002/ to finish.
rnephew@chromium.org changed reviewers: + charliea@chromium.org
lgtm with some comments https://codereview.chromium.org/1828143004/diff/1/tools/battor/battor/battor_... File tools/battor/battor/battor_wrapper.py (right): https://codereview.chromium.org/1828143004/diff/1/tools/battor/battor/battor_... tools/battor/battor/battor_wrapper.py:23: if config: No need to parameterize this, I think? https://codereview.chromium.org/1828143004/diff/1/tools/battor/battor/battor_... tools/battor/battor/battor_wrapper.py:97: raise TypeError('Sync_Id must be a string.') s/Sync_Id/sync_id https://codereview.chromium.org/1828143004/diff/1/tools/battor/battor/binary_... File tools/battor/battor/binary_dependencies.json (right): https://codereview.chromium.org/1828143004/diff/1/tools/battor/battor/binary_... tools/battor/battor/binary_dependencies.json:1: { Can you rename this to battor_binary_dependencies.json?
I also made some changes to how you pass it the platform and device id. I did this to decouple it from telemetry. Its old way depended on it being passed a platform_backend object and use backend.Device if it were android and backend.GetOsName(). I didn't want it to rely on telemetry at all so I just have these passed directly now. PTAL. Still need Alex's CL to land for this to work, and unittests are still incoming. https://codereview.chromium.org/1828143004/diff/1/tools/battor/battor/battor_... File tools/battor/battor/battor_wrapper.py (right): https://codereview.chromium.org/1828143004/diff/1/tools/battor/battor/battor_... tools/battor/battor/battor_wrapper.py:23: if config: On 2016/03/24 20:34:59, nednguyen wrote: > No need to parameterize this, I think? Done. https://codereview.chromium.org/1828143004/diff/1/tools/battor/battor/battor_... tools/battor/battor/battor_wrapper.py:97: raise TypeError('Sync_Id must be a string.') On 2016/03/24 20:34:59, nednguyen wrote: > s/Sync_Id/sync_id Done. https://codereview.chromium.org/1828143004/diff/1/tools/battor/battor/binary_... File tools/battor/battor/binary_dependencies.json (right): https://codereview.chromium.org/1828143004/diff/1/tools/battor/battor/binary_... tools/battor/battor/binary_dependencies.json:1: { On 2016/03/24 20:34:59, nednguyen wrote: > Can you rename this to battor_binary_dependencies.json? Done.
https://codereview.chromium.org/1828143004/diff/20001/tools/battor/battor/bat... File tools/battor/battor/battor_wrapper.py (right): https://codereview.chromium.org/1828143004/diff/20001/tools/battor/battor/bat... tools/battor/battor/battor_wrapper.py:36: self._battor_path = find_usb_devices.GetBattorPathFromPhoneSerial(device) If I recall correctly, this would also requires you pass in the device_mapping.json file? https://codereview.chromium.org/1828143004/diff/20001/tools/battor/battor/bat... tools/battor/battor/battor_wrapper.py:55: self.StartShell() I would remove the start_shell variable & let the call site calling StartShell directly. Generally, constructor should have no side effect if possible
https://codereview.chromium.org/1828143004/diff/20001/tools/battor/battor/bat... File tools/battor/battor/battor_wrapper.py (right): https://codereview.chromium.org/1828143004/diff/20001/tools/battor/battor/bat... tools/battor/battor/battor_wrapper.py:36: self._battor_path = find_usb_devices.GetBattorPathFromPhoneSerial(device) On 2016/03/24 22:30:13, nednguyen wrote: > If I recall correctly, this would also requires you pass in the > device_mapping.json file? If you do not pass it it, it will find it for you using other functions in find_usb_devices. https://codereview.chromium.org/1828143004/diff/20001/tools/battor/battor/bat... tools/battor/battor/battor_wrapper.py:55: self.StartShell() On 2016/03/24 22:30:13, nednguyen wrote: > I would remove the start_shell variable & let the call site calling StartShell > directly. Generally, constructor should have no side effect if possible Done locally. Working on unittests, it will be in the patchset with those.
https://codereview.chromium.org/1828143004/diff/20001/tools/battor/battor/bat... File tools/battor/battor/battor_wrapper.py (right): https://codereview.chromium.org/1828143004/diff/20001/tools/battor/battor/bat... tools/battor/battor/battor_wrapper.py:36: self._battor_path = find_usb_devices.GetBattorPathFromPhoneSerial(device) On 2016/03/24 22:46:01, rnephew1 wrote: > On 2016/03/24 22:30:13, nednguyen wrote: > > If I recall correctly, this would also requires you pass in the > > device_mapping.json file? > > If you do not pass it it, it will find it for you using other functions in > find_usb_devices. If we don't specify the other option here, this means this is default to use the topology that Alex set up? How would the clients of BattorWrapper specify a different topology?
https://codereview.chromium.org/1828143004/diff/20001/tools/battor/battor/bat... File tools/battor/battor/battor_wrapper.py (right): https://codereview.chromium.org/1828143004/diff/20001/tools/battor/battor/bat... tools/battor/battor/battor_wrapper.py:36: self._battor_path = find_usb_devices.GetBattorPathFromPhoneSerial(device) On 2016/03/25 02:57:06, nednguyen wrote: > On 2016/03/24 22:46:01, rnephew1 wrote: > > On 2016/03/24 22:30:13, nednguyen wrote: > > > If I recall correctly, this would also requires you pass in the > > > device_mapping.json file? > > > > If you do not pass it it, it will find it for you using other functions in > > find_usb_devices. > > If we don't specify the other option here, this means this is default to use the > topology that Alex set up? How would the clients of BattorWrapper specify a > different topology? You can pass it one directly, I also plan to modify this once changes are made to pass a battor path/battor mapping directly into telemetry. I can make the changes now though, I'll have there be an optional paramater for battor_map_config.
https://codereview.chromium.org/1828143004/diff/20001/tools/battor/battor/bat... File tools/battor/battor/battor_wrapper.py (right): https://codereview.chromium.org/1828143004/diff/20001/tools/battor/battor/bat... tools/battor/battor/battor_wrapper.py:26: self, test_platform, device=None, start_shell=True, battor_path=None): What is `device` in this case? https://codereview.chromium.org/1828143004/diff/20001/tools/battor/battor/bat... tools/battor/battor/battor_wrapper.py:26: self, test_platform, device=None, start_shell=True, battor_path=None): In what cases would you not want `start_shell=True`? https://codereview.chromium.org/1828143004/diff/20001/tools/battor/battor/bat... tools/battor/battor/battor_wrapper.py:26: self, test_platform, device=None, start_shell=True, battor_path=None): (Actually, based on these questions, this function could probably use a header describing what its parameters do) https://codereview.chromium.org/1828143004/diff/20001/tools/battor/battor/bat... tools/battor/battor/battor_wrapper.py:30: if (isinstance(battor_path, basestring) Is there any way that you could abstract some of this out into a GetBattOrPath() function? This __init__ has quite a bit going on in it. That also allows us to short-circuit a little more cleanly when we find the right path, like: if (isinstance(battor_path, basestring) and find_usb_devices.IsBattor(battor_path, device_tree)): return ... if (other thing) return ... https://codereview.chromium.org/1828143004/diff/20001/tools/battor/battor/bat... tools/battor/battor/battor_wrapper.py:33: if test_platform == 'android': I'm sort of confused by this. I understand that sometimes we'll be running Telemetry tests where the host (which this program is a part of IIUC) will be connected to both an Android device and a BattOr. But it seems like, if this program is a BattOr wrapper, it should just be concerned with issuing commands to a specified BattOr from Telemetry. Why does it care if the test platform is Android or not? (I could very well be missing something here) https://codereview.chromium.org/1828143004/diff/20001/tools/battor/battor/bat... tools/battor/battor/battor_wrapper.py:41: 'For non android platforms, only one battor can be attached unless ' s/non android/non-Android https://codereview.chromium.org/1828143004/diff/20001/tools/battor/battor/bat... tools/battor/battor/battor_wrapper.py:41: 'For non android platforms, only one battor can be attached unless ' s/battor/BattOr https://codereview.chromium.org/1828143004/diff/20001/tools/battor/battor/bat... tools/battor/battor/battor_wrapper.py:61: raise BattorError('Battor did not complete command \'%s\' correctly.\n' s/Battor/BattOr https://codereview.chromium.org/1828143004/diff/20001/tools/battor/battor/bat... tools/battor/battor/battor_wrapper.py:67: logging.warning('Battor shell already started.') s/Battor/BattOr https://codereview.chromium.org/1828143004/diff/20001/tools/battor/battor/bat... tools/battor/battor/battor_wrapper.py:71: battor_cmd.append('--bator-path=%s' % self._battor_path) This should be --battor-path https://codereview.chromium.org/1828143004/diff/20001/tools/battor/battor/bat... tools/battor/battor/battor_wrapper.py:84: # TODO(rnephew): Change this when support for spliting StopTracing Not sure what this comment means. Could you clarify it? Also, I think "spliting" should probably be "splitting". https://codereview.chromium.org/1828143004/diff/20001/tools/battor/battor/bat... tools/battor/battor/battor_wrapper.py:98: # TODO(rnephew): When battor binary supports spliting StopTracing and s/battor/BattOr https://codereview.chromium.org/1828143004/diff/20001/tools/battor/battor/bat... tools/battor/battor/battor_wrapper.py:98: # TODO(rnephew): When battor binary supports spliting StopTracing and s/spliting/splitting https://codereview.chromium.org/1828143004/diff/20001/tools/battor/battor/bat... tools/battor/battor/battor_wrapper.py:99: # results colletion, this will be implemented. colletion/collection
I made some changes in order to make unittesting easier as well. Mostly just splitting the actual calls to the battor agent binary into their own _function-name_Impl methods. https://codereview.chromium.org/1828143004/diff/20001/tools/battor/battor/bat... File tools/battor/battor/battor_wrapper.py (right): https://codereview.chromium.org/1828143004/diff/20001/tools/battor/battor/bat... tools/battor/battor/battor_wrapper.py:26: self, test_platform, device=None, start_shell=True, battor_path=None): On 2016/03/25 16:47:55, charliea wrote: > In what cases would you not want `start_shell=True`? I got rid of start_shell=True. The user of this should call it directly to start the shell. https://codereview.chromium.org/1828143004/diff/20001/tools/battor/battor/bat... tools/battor/battor/battor_wrapper.py:26: self, test_platform, device=None, start_shell=True, battor_path=None): On 2016/03/25 16:47:54, charliea wrote: > What is `device` in this case? android device. I have changed it to better reflect that. https://codereview.chromium.org/1828143004/diff/20001/tools/battor/battor/bat... tools/battor/battor/battor_wrapper.py:26: self, test_platform, device=None, start_shell=True, battor_path=None): On 2016/03/25 16:47:54, charliea wrote: > (Actually, based on these questions, this function could probably use a header > describing what its parameters do) Done. https://codereview.chromium.org/1828143004/diff/20001/tools/battor/battor/bat... tools/battor/battor/battor_wrapper.py:30: if (isinstance(battor_path, basestring) On 2016/03/25 16:47:54, charliea wrote: > Is there any way that you could abstract some of this out into a GetBattOrPath() > function? This __init__ has quite a bit going on in it. > > That also allows us to short-circuit a little more cleanly when we find the > right path, like: > > if (isinstance(battor_path, basestring) > and find_usb_devices.IsBattor(battor_path, device_tree)): > return ... > > if (other thing) > return ... > Done. https://codereview.chromium.org/1828143004/diff/20001/tools/battor/battor/bat... tools/battor/battor/battor_wrapper.py:33: if test_platform == 'android': On 2016/03/25 16:47:54, charliea wrote: > I'm sort of confused by this. I understand that sometimes we'll be running > Telemetry tests where the host (which this program is a part of IIUC) will be > connected to both an Android device and a BattOr. But it seems like, if this > program is a BattOr wrapper, it should just be concerned with issuing commands > to a specified BattOr from Telemetry. Why does it care if the test platform is > Android or not? > > (I could very well be missing something here) It also tries to find the correct battor based off what is passed in. That way if there are 7 battors, like in the lab setup, you can pass it an optional device serial and it will find which battor goes with that device. Its not strictly a wrapper I guess. Maybe it should be renamed to battor_util from battor_wrapper https://codereview.chromium.org/1828143004/diff/20001/tools/battor/battor/bat... tools/battor/battor/battor_wrapper.py:36: self._battor_path = find_usb_devices.GetBattorPathFromPhoneSerial(device) On 2016/03/25 04:56:30, rnephew1 wrote: > On 2016/03/25 02:57:06, nednguyen wrote: > > On 2016/03/24 22:46:01, rnephew1 wrote: > > > On 2016/03/24 22:30:13, nednguyen wrote: > > > > If I recall correctly, this would also requires you pass in the > > > > device_mapping.json file? > > > > > > If you do not pass it it, it will find it for you using other functions in > > > find_usb_devices. > > > > If we don't specify the other option here, this means this is default to use > the > > topology that Alex set up? How would the clients of BattorWrapper specify a > > different topology? > > You can pass it one directly, I also plan to modify this once changes are made > to pass a battor path/battor mapping directly into telemetry. I can make the > changes now though, I'll have there be an optional paramater for > battor_map_config. Done. https://codereview.chromium.org/1828143004/diff/20001/tools/battor/battor/bat... tools/battor/battor/battor_wrapper.py:41: 'For non android platforms, only one battor can be attached unless ' On 2016/03/25 16:47:54, charliea wrote: > s/non android/non-Android Done. https://codereview.chromium.org/1828143004/diff/20001/tools/battor/battor/bat... tools/battor/battor/battor_wrapper.py:41: 'For non android platforms, only one battor can be attached unless ' On 2016/03/25 16:47:54, charliea wrote: > s/battor/BattOr Done. https://codereview.chromium.org/1828143004/diff/20001/tools/battor/battor/bat... tools/battor/battor/battor_wrapper.py:55: self.StartShell() On 2016/03/24 22:46:01, rnephew1 wrote: > On 2016/03/24 22:30:13, nednguyen wrote: > > I would remove the start_shell variable & let the call site calling StartShell > > directly. Generally, constructor should have no side effect if possible > > Done locally. Working on unittests, it will be in the patchset with those. Done. https://codereview.chromium.org/1828143004/diff/20001/tools/battor/battor/bat... tools/battor/battor/battor_wrapper.py:61: raise BattorError('Battor did not complete command \'%s\' correctly.\n' On 2016/03/25 16:47:55, charliea wrote: > s/Battor/BattOr Done. https://codereview.chromium.org/1828143004/diff/20001/tools/battor/battor/bat... tools/battor/battor/battor_wrapper.py:67: logging.warning('Battor shell already started.') On 2016/03/25 16:47:55, charliea wrote: > s/Battor/BattOr Done. https://codereview.chromium.org/1828143004/diff/20001/tools/battor/battor/bat... tools/battor/battor/battor_wrapper.py:71: battor_cmd.append('--bator-path=%s' % self._battor_path) On 2016/03/25 16:47:54, charliea wrote: > This should be --battor-path Done. https://codereview.chromium.org/1828143004/diff/20001/tools/battor/battor/bat... tools/battor/battor/battor_wrapper.py:84: # TODO(rnephew): Change this when support for spliting StopTracing On 2016/03/25 16:47:54, charliea wrote: > Not sure what this comment means. Could you clarify it? Also, I think "spliting" > should probably be "splitting". Added crbug. https://codereview.chromium.org/1828143004/diff/20001/tools/battor/battor/bat... tools/battor/battor/battor_wrapper.py:98: # TODO(rnephew): When battor binary supports spliting StopTracing and On 2016/03/25 16:47:54, charliea wrote: > s/battor/BattOr Done. https://codereview.chromium.org/1828143004/diff/20001/tools/battor/battor/bat... tools/battor/battor/battor_wrapper.py:98: # TODO(rnephew): When battor binary supports spliting StopTracing and On 2016/03/25 16:47:54, charliea wrote: > s/battor/BattOr Done. https://codereview.chromium.org/1828143004/diff/20001/tools/battor/battor/bat... tools/battor/battor/battor_wrapper.py:99: # results colletion, this will be implemented. On 2016/03/25 16:47:54, charliea wrote: > colletion/collection Done.
Getting a lot closer! I'll leave the unit test code review to someone that's a lot more familiar w/ Python/Telemetry unit tests https://codereview.chromium.org/1828143004/diff/40001/tools/battor/battor/bat... File tools/battor/battor/battor_wrapper.py (right): https://codereview.chromium.org/1828143004/diff/40001/tools/battor/battor/bat... tools/battor/battor/battor_wrapper.py:27: """Initialize BattOr object. Should this be "Constructor."? https://code.google.com/p/chromium/codesearch#chromium/src/third_party/catapu... https://codereview.chromium.org/1828143004/diff/40001/tools/battor/battor/bat... tools/battor/battor/battor_wrapper.py:33: battor_map: BattOr device map mapping device serial to BattOr serial. When you say "serial", do you mean "serial number" or "serial port"? (Presumably port, but it might be worth clarifying given that Android serial numbers are a thing. https://codereview.chromium.org/1828143004/diff/40001/tools/battor/battor/bat... tools/battor/battor/battor_wrapper.py:56: else: nit: generally Chrome prefers the "happy path" to be the path of least indentation. Also, after some control statement that terminates execution (raise, return), it prefers for there not to be an else (https://www.chromium.org/developers/coding-style). In other words, I think this should be: if (not (isinstance(battor_path, basestring) and find_usb_devices.IsBattor(battor_path, device_tree)): raise BattorError('Passed a path to a BattOr that is not a BattOr.') return battor_path https://codereview.chromium.org/1828143004/diff/40001/tools/battor/battor/bat... tools/battor/battor/battor_wrapper.py:57: raise BattorError('Passed a path to a BattOr that is not a BattOr.') The wording here is a little awkward. Maybe "An invalid BattOr path was specified."? https://codereview.chromium.org/1828143004/diff/40001/tools/battor/battor/bat... tools/battor/battor/battor_wrapper.py:58: elif test_platform == 'android': Same as above - given that all branches before this return control to elsewhere, I think Chromium code style prefers that you ditch the else part of this, just making it if test_platform == 'android': https://codereview.chromium.org/1828143004/diff/40001/tools/battor/battor/bat... tools/battor/battor/battor_wrapper.py:63: else: Same (no need for an else here) https://codereview.chromium.org/1828143004/diff/40001/tools/battor/battor/bat... tools/battor/battor/battor_wrapper.py:109: # TODO(rnephew): Change this when support for splitting StopTracing nit: Comments should be full sentences. "Change this when we've split StopTracing() and GetResults()" (Assuming that we still need this) https://codereview.chromium.org/1828143004/diff/40001/tools/battor/battor/bat... tools/battor/battor/battor_wrapper.py:110: # and results collection. crbug/597650 nit: The way I've seen this is TODO(rnephew): Change this when support for splitting StopTracing and results collection. (See crbug.com/597650.) (Note the "See" and the extra .com so that the bug becomes linkable in code search) https://code.google.com/p/chromium/codesearch#chromium/src/third_party/catapu... https://codereview.chromium.org/1828143004/diff/40001/tools/battor/battor/bat... File tools/battor/battor/battor_wrapper_unittest.py (right): https://codereview.chromium.org/1828143004/diff/40001/tools/battor/battor/bat... tools/battor/battor/battor_wrapper_unittest.py:36: # self._o_get_battor_path_from_phone_serial = ( Generally, when you have code like this, it's a good idea to add a comment along the lines of: # DO NOT SUBMIT until this code is uncommented. It relies on crrev.com/blah. Chromium actually has a presubmit that blocks submission when it sees DO NOT SUBMIT in files, so it's a good safeguard to make sure you don't accidentally leave it in. It's also a good signal to reviews that the commented out code is left intentionally. https://codereview.chromium.org/1828143004/diff/40001/tools/battor/battor/bat... tools/battor/battor/battor_wrapper_unittest.py:36: # self._o_get_battor_path_from_phone_serial = ( Just out of curiosity, what's the self._o_ syntax? (I don't know much about Python unit testing) https://codereview.chromium.org/1828143004/diff/40001/tools/battor/battor/bat... tools/battor/battor/battor_wrapper_unittest.py:49: # find_usb_devices.GetBattorPathFromPhoneSerial = ( Same https://codereview.chromium.org/1828143004/diff/40001/tools/battor/battor/bat... tools/battor/battor/battor_wrapper_unittest.py:78: # TODO(rnephew): This test cannot be finished until alex's CL lands. Same
https://codereview.chromium.org/1828143004/diff/40001/tools/battor/battor/bat... File tools/battor/battor/battor_wrapper.py (right): https://codereview.chromium.org/1828143004/diff/40001/tools/battor/battor/bat... tools/battor/battor/battor_wrapper.py:27: """Initialize BattOr object. On 2016/03/25 18:52:24, charliea wrote: > Should this be "Constructor."? > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/catapu... Done. https://codereview.chromium.org/1828143004/diff/40001/tools/battor/battor/bat... tools/battor/battor/battor_wrapper.py:33: battor_map: BattOr device map mapping device serial to BattOr serial. On 2016/03/25 18:52:24, charliea wrote: > When you say "serial", do you mean "serial number" or "serial port"? (Presumably > port, but it might be worth clarifying given that Android serial numbers are a > thing. I meant path. https://codereview.chromium.org/1828143004/diff/40001/tools/battor/battor/bat... tools/battor/battor/battor_wrapper.py:56: else: On 2016/03/25 18:52:24, charliea wrote: > nit: generally Chrome prefers the "happy path" to be the path of least > indentation. Also, after some control statement that terminates execution > (raise, return), it prefers for there not to be an else > (https://www.chromium.org/developers/coding-style). In other words, I think this > should be: > > if (not (isinstance(battor_path, basestring) and > find_usb_devices.IsBattor(battor_path, device_tree)): > raise BattorError('Passed a path to a BattOr that is not a BattOr.') > > return battor_path Done. https://codereview.chromium.org/1828143004/diff/40001/tools/battor/battor/bat... tools/battor/battor/battor_wrapper.py:57: raise BattorError('Passed a path to a BattOr that is not a BattOr.') On 2016/03/25 18:52:24, charliea wrote: > The wording here is a little awkward. Maybe "An invalid BattOr path was > specified."? Done. https://codereview.chromium.org/1828143004/diff/40001/tools/battor/battor/bat... tools/battor/battor/battor_wrapper.py:58: elif test_platform == 'android': On 2016/03/25 18:52:24, charliea wrote: > Same as above - given that all branches before this return control to elsewhere, > I think Chromium code style prefers that you ditch the else part of this, just > making it > > if test_platform == 'android': Done. https://codereview.chromium.org/1828143004/diff/40001/tools/battor/battor/bat... tools/battor/battor/battor_wrapper.py:63: else: On 2016/03/25 18:52:24, charliea wrote: > Same (no need for an else here) Done. https://codereview.chromium.org/1828143004/diff/40001/tools/battor/battor/bat... tools/battor/battor/battor_wrapper.py:109: # TODO(rnephew): Change this when support for splitting StopTracing On 2016/03/25 18:52:24, charliea wrote: > nit: Comments should be full sentences. > > "Change this when we've split StopTracing() and GetResults()" > > (Assuming that we still need this) Done. https://codereview.chromium.org/1828143004/diff/40001/tools/battor/battor/bat... tools/battor/battor/battor_wrapper.py:110: # and results collection. crbug/597650 On 2016/03/25 18:52:24, charliea wrote: > nit: The way I've seen this is TODO(rnephew): Change this when support for > splitting StopTracing and results collection. (See crbug.com/597650.) > > (Note the "See" and the extra .com so that the bug becomes linkable in code > search) > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/catapu... Done. https://codereview.chromium.org/1828143004/diff/40001/tools/battor/battor/bat... File tools/battor/battor/battor_wrapper_unittest.py (right): https://codereview.chromium.org/1828143004/diff/40001/tools/battor/battor/bat... tools/battor/battor/battor_wrapper_unittest.py:36: # self._o_get_battor_path_from_phone_serial = ( On 2016/03/25 18:52:24, charliea wrote: > Just out of curiosity, what's the self._o_ syntax? (I don't know much about > Python unit testing) It stands for original. I'm not sure there is any recommended syntax for this. It probably doesn't need the _o_. Getting rid of it. https://codereview.chromium.org/1828143004/diff/40001/tools/battor/battor/bat... tools/battor/battor/battor_wrapper_unittest.py:36: # self._o_get_battor_path_from_phone_serial = ( On 2016/03/25 18:52:25, charliea wrote: > Generally, when you have code like this, it's a good idea to add a comment along > the lines of: > > # DO NOT SUBMIT until this code is uncommented. It relies on crrev.com/blah. > > Chromium actually has a presubmit that blocks submission when it sees DO NOT > SUBMIT in files, so it's a good safeguard to make sure you don't accidentally > leave it in. It's also a good signal to reviews that the commented out code is > left intentionally. Done. https://codereview.chromium.org/1828143004/diff/40001/tools/battor/battor/bat... tools/battor/battor/battor_wrapper_unittest.py:49: # find_usb_devices.GetBattorPathFromPhoneSerial = ( On 2016/03/25 18:52:25, charliea wrote: > Same Done. https://codereview.chromium.org/1828143004/diff/40001/tools/battor/battor/bat... tools/battor/battor/battor_wrapper_unittest.py:78: # TODO(rnephew): This test cannot be finished until alex's CL lands. On 2016/03/25 18:52:25, charliea wrote: > Same Done.
https://codereview.chromium.org/1828143004/diff/60001/tools/battor/battor/bat... File tools/battor/battor/battor_binary_dependencies.json (right): https://codereview.chromium.org/1828143004/diff/60001/tools/battor/battor/bat... tools/battor/battor/battor_binary_dependencies.json:5: "cloud_storage_base_folder": "chrome-partner-telemetry/battor", Ugg. I apparently had a copy-paste fail earlier. This should be binary_dependencies, not chrome-partner-telemetry. I'm really sorry about that. https://codereview.chromium.org/1828143004/diff/60001/tools/battor/battor/bat... tools/battor/battor/battor_binary_dependencies.json:13: "cloud_storage_hash": "...", You still need to add the mac and win deps.
Split stopping and collection, with one question about catapult best practices. https://codereview.chromium.org/1828143004/diff/80001/tools/battor/battor/bat... File tools/battor/battor/battor_wrapper.py (right): https://codereview.chromium.org/1828143004/diff/80001/tools/battor/battor/bat... tools/battor/battor/battor_wrapper.py:128: time.sleep(.1) What is the preferred method in catapult to do something like this?
On 2016/03/25 19:48:59, aiolos wrote: > https://codereview.chromium.org/1828143004/diff/60001/tools/battor/battor/bat... > File tools/battor/battor/battor_binary_dependencies.json (right): > > https://codereview.chromium.org/1828143004/diff/60001/tools/battor/battor/bat... > tools/battor/battor/battor_binary_dependencies.json:5: > "cloud_storage_base_folder": "chrome-partner-telemetry/battor", > Ugg. I apparently had a copy-paste fail earlier. This should be > binary_dependencies, not chrome-partner-telemetry. I'm really sorry about that. > > https://codereview.chromium.org/1828143004/diff/60001/tools/battor/battor/bat... > tools/battor/battor/battor_binary_dependencies.json:13: "cloud_storage_hash": > "...", > You still need to add the mac and win deps. Charlie is getting me different binaries for different systems. I will fix both of these when I get those.
> Charlie is getting me different binaries for different systems. I will fix both > of these when I get those. Done. Let me know if you need anything else :)
https://codereview.chromium.org/1828143004/diff/60001/tools/battor/battor/bat... File tools/battor/battor/battor_binary_dependencies.json (right): https://codereview.chromium.org/1828143004/diff/60001/tools/battor/battor/bat... tools/battor/battor/battor_binary_dependencies.json:5: "cloud_storage_base_folder": "chrome-partner-telemetry/battor", On 2016/03/25 19:48:59, aiolos wrote: > Ugg. I apparently had a copy-paste fail earlier. This should be > binary_dependencies, not chrome-partner-telemetry. I'm really sorry about that. Done. https://codereview.chromium.org/1828143004/diff/60001/tools/battor/battor/bat... tools/battor/battor/battor_binary_dependencies.json:13: "cloud_storage_hash": "...", On 2016/03/25 19:48:59, aiolos wrote: > You still need to add the mac and win deps. Done.
lgtm with some comment on unittest style. Also we will need to move this to catapult/common https://codereview.chromium.org/1828143004/diff/140001/tools/battor/battor/ba... File tools/battor/battor/battor_wrapper_unittest.py (right): https://codereview.chromium.org/1828143004/diff/140001/tools/battor/battor/ba... tools/battor/battor/battor_wrapper_unittest.py:75: def testInit_androidWithBattor(self): style nit: testInitAndroidWithBattor. And same change for other test methods
Moved to common/battor https://codereview.chromium.org/1828143004/diff/140001/tools/battor/battor/ba... File tools/battor/battor/battor_wrapper_unittest.py (right): https://codereview.chromium.org/1828143004/diff/140001/tools/battor/battor/ba... tools/battor/battor/battor_wrapper_unittest.py:75: def testInit_androidWithBattor(self): On 2016/03/28 16:36:18, nednguyen wrote: > style nit: testInitAndroidWithBattor. And same change for other test methods Done.
https://codereview.chromium.org/1828143004/diff/180001/common/battor/battor/b... File common/battor/battor/battor_wrapper.py (right): https://codereview.chromium.org/1828143004/diff/180001/common/battor/battor/b... common/battor/battor/battor_wrapper.py:20: class BattorWrapper(object): Could you add a pydoc for this class? a la https://github.com/catapult-project/catapult/blob/40afc53353daf28b955397c8e27... https://codereview.chromium.org/1828143004/diff/180001/common/battor/battor/b... common/battor/battor/battor_wrapper.py:29: """Construtor. nit: s/Construtor/constructor https://codereview.chromium.org/1828143004/diff/180001/common/battor/battor/b... common/battor/battor/battor_wrapper.py:42: _battor_shell: Asubprocess running the bator_agent_binary nit: s/Asubprocess/a subprocess https://codereview.chromium.org/1828143004/diff/180001/common/battor/battor/b... common/battor/battor/battor_wrapper.py:43: _trace_results_path: Path to BattOr trace results. nit: Maybe add "file" at the end of this sentence? https://codereview.chromium.org/1828143004/diff/180001/common/battor/battor/b... common/battor/battor/battor_wrapper.py:48: 'battor_binary_dependencies.json') nit: Is this proper python indentation? Looks like maybe it should be to the left https://codereview.chromium.org/1828143004/diff/180001/common/battor/battor/b... common/battor/battor/battor_wrapper.py:52: self._battor_agent_binary = dm.FetchPath( Should you check for if this returns 'None' and throw if it does? https://github.com/catapult-project/catapult/blob/78a6f08e843d2a4752bcb7ecac4... https://codereview.chromium.org/1828143004/diff/180001/common/battor/battor/b... common/battor/battor/battor_wrapper.py:57: tf = tempfile.NamedTemporaryFile(delete=False) I'm kind of confused by this. So you open up the temp file, record its name, and immediately close it? Why? (If this is what you meant to do, it's probably worth documenting why in a comment, as others will have the same question.) https://codereview.chromium.org/1828143004/diff/180001/common/battor/battor/b... common/battor/battor/battor_wrapper.py:57: tf = tempfile.NamedTemporaryFile(delete=False) nit: `tf` isn't a great variable name - it could mean just about anything. How about `tmp` or `tmp_file`? Those are a lot more idiomatic. https://codereview.chromium.org/1828143004/diff/180001/common/battor/battor/b... common/battor/battor/battor_wrapper.py:72: raise BattorError('Must specify device for android platform.') nit: In plain text (comments/strings), you switch between 'android' and 'Android' a bunch in this file. I don't care too much which one you pick, but it's probably worth picking one for consistency. https://codereview.chromium.org/1828143004/diff/180001/common/battor/battor/b... common/battor/battor/battor_wrapper.py:76: # Not android and no explicilty passed BattOr. nit: s/explicilty/explicitly https://codereview.chromium.org/1828143004/diff/180001/common/battor/battor/b... common/battor/battor/battor_wrapper.py:83: def _SendBattorCommandImpl(self, cmd, return_results=True): nit: It looks like these functions are in somewhat of a random order. The most common ordering I've seen is: - public functions that are overriding a base class's version - public functions added in this class - private functions The specific ordering isn't as important as it is to just have some sort of an ordering, but the one above usually works pretty well. https://codereview.chromium.org/1828143004/diff/180001/common/battor/battor/b... common/battor/battor/battor_wrapper.py:128: while self.IsShellRunning(): It looks like you can replace this loop with: self.battor_shell.wait(timeout) https://docs.python.org/3/library/subprocess.html#subprocess.Popen.wait That'll handle another problem: it probably makes sense to add *some* sort of timeout to this: otherwise -it'll just never shut down if the battor agent hangs. I guess this is only if Telemetry doesn't already have some sort of overall timeout where it sends a signal when things take too long. (I don't know enough about Telemetry to say one way or another.)
https://codereview.chromium.org/1828143004/diff/180001/common/battor/battor/b... File common/battor/battor/battor_wrapper.py (right): https://codereview.chromium.org/1828143004/diff/180001/common/battor/battor/b... common/battor/battor/battor_wrapper.py:48: 'battor_binary_dependencies.json') On 2016/03/29 19:56:03, charliea wrote: > nit: Is this proper python indentation? Looks like maybe it should be to the > left Functionally, yes. But style wise, no. See https://google.github.io/styleguide/pyguide.html#Indentation It would be nice if you put it on the following line with a hanging indent to match the style in the rest of the file, but I think it's too long for both parameters to dirname to fit on one line. https://codereview.chromium.org/1828143004/diff/180001/common/battor/battor/b... common/battor/battor/battor_wrapper.py:52: self._battor_agent_binary = dm.FetchPath( On 2016/03/29 19:56:03, charliea wrote: > Should you check for if this returns 'None' and throw if it does? > > https://github.com/catapult-project/catapult/blob/78a6f08e843d2a4752bcb7ecac4... DependencyManager.FetchPath (which is returned by binary_manager.FetchPath) should throw an exception if the path isn't found, or if it doesn't exist. https://github.com/catapult-project/catapult/blob/78a6f08e843d2a4752bcb7ecac4... https://codereview.chromium.org/1828143004/diff/180001/common/battor/battor/b... File common/battor/battor/battor_wrapper_unittest.py (right): https://codereview.chromium.org/1828143004/diff/180001/common/battor/battor/b... common/battor/battor/battor_wrapper_unittest.py:163: self._battor.StopTracing() Assert that Tracing has stopped?
https://codereview.chromium.org/1828143004/diff/180001/common/battor/battor/b... File common/battor/battor/battor_wrapper.py (right): https://codereview.chromium.org/1828143004/diff/180001/common/battor/battor/b... common/battor/battor/battor_wrapper.py:52: self._battor_agent_binary = dm.FetchPath( On 2016/03/29 22:30:32, aiolos wrote: > On 2016/03/29 19:56:03, charliea wrote: > > Should you check for if this returns 'None' and throw if it does? > > > > > https://github.com/catapult-project/catapult/blob/78a6f08e843d2a4752bcb7ecac4... > > DependencyManager.FetchPath (which is returned by binary_manager.FetchPath) > should throw an exception if the path isn't found, or if it doesn't exist. > https://github.com/catapult-project/catapult/blob/78a6f08e843d2a4752bcb7ecac4... Ack, sorry about that. Looks like I was looking at BinaryManager.FetchPath, not DependencyManager.FetchPath
https://codereview.chromium.org/1828143004/diff/180001/common/battor/battor/b... File common/battor/battor/battor_wrapper.py (right): https://codereview.chromium.org/1828143004/diff/180001/common/battor/battor/b... common/battor/battor/battor_wrapper.py:20: class BattorWrapper(object): On 2016/03/29 19:56:03, charliea wrote: > Could you add a pydoc for this class? a la > https://github.com/catapult-project/catapult/blob/40afc53353daf28b955397c8e27... Done. https://codereview.chromium.org/1828143004/diff/180001/common/battor/battor/b... common/battor/battor/battor_wrapper.py:29: """Construtor. On 2016/03/29 19:56:03, charliea wrote: > nit: s/Construtor/constructor Done. https://codereview.chromium.org/1828143004/diff/180001/common/battor/battor/b... common/battor/battor/battor_wrapper.py:42: _battor_shell: Asubprocess running the bator_agent_binary On 2016/03/29 19:56:03, charliea wrote: > nit: s/Asubprocess/a subprocess Done. https://codereview.chromium.org/1828143004/diff/180001/common/battor/battor/b... common/battor/battor/battor_wrapper.py:43: _trace_results_path: Path to BattOr trace results. On 2016/03/29 19:56:03, charliea wrote: > nit: Maybe add "file" at the end of this sentence? Done. https://codereview.chromium.org/1828143004/diff/180001/common/battor/battor/b... common/battor/battor/battor_wrapper.py:48: 'battor_binary_dependencies.json') On 2016/03/29 19:56:03, charliea wrote: > nit: Is this proper python indentation? Looks like maybe it should be to the > left Done. https://codereview.chromium.org/1828143004/diff/180001/common/battor/battor/b... common/battor/battor/battor_wrapper.py:52: self._battor_agent_binary = dm.FetchPath( On 2016/03/29 22:30:32, aiolos wrote: > On 2016/03/29 19:56:03, charliea wrote: > > Should you check for if this returns 'None' and throw if it does? > > > > > https://github.com/catapult-project/catapult/blob/78a6f08e843d2a4752bcb7ecac4... > > DependencyManager.FetchPath (which is returned by binary_manager.FetchPath) > should throw an exception if the path isn't found, or if it doesn't exist. > https://github.com/catapult-project/catapult/blob/78a6f08e843d2a4752bcb7ecac4... Acknowledged. https://codereview.chromium.org/1828143004/diff/180001/common/battor/battor/b... common/battor/battor/battor_wrapper.py:57: tf = tempfile.NamedTemporaryFile(delete=False) On 2016/03/29 19:56:03, charliea wrote: > I'm kind of confused by this. So you open up the temp file, record its name, and > immediately close it? Why? (If this is what you meant to do, it's probably worth > documenting why in a comment, as others will have the same question.) Its creating a blank file it can later dump the results into. Its to make sure there is no collision later; even though it would be highly unlikely. I'll move it directly to where it is used so its less confusing. https://codereview.chromium.org/1828143004/diff/180001/common/battor/battor/b... common/battor/battor/battor_wrapper.py:72: raise BattorError('Must specify device for android platform.') On 2016/03/29 19:56:03, charliea wrote: > nit: In plain text (comments/strings), you switch between 'android' and > 'Android' a bunch in this file. I don't care too much which one you pick, but > it's probably worth picking one for consistency. Done. https://codereview.chromium.org/1828143004/diff/180001/common/battor/battor/b... common/battor/battor/battor_wrapper.py:76: # Not android and no explicilty passed BattOr. On 2016/03/29 19:56:03, charliea wrote: > nit: s/explicilty/explicitly Done. https://codereview.chromium.org/1828143004/diff/180001/common/battor/battor/b... common/battor/battor/battor_wrapper.py:83: def _SendBattorCommandImpl(self, cmd, return_results=True): On 2016/03/29 19:56:03, charliea wrote: > nit: It looks like these functions are in somewhat of a random order. The most > common ordering I've seen is: > > - public functions that are overriding a base class's version > - public functions added in this class > - private functions > > The specific ordering isn't as important as it is to just have some sort of an > ordering, but the one above usually works pretty well. Done. https://codereview.chromium.org/1828143004/diff/180001/common/battor/battor/b... common/battor/battor/battor_wrapper.py:128: while self.IsShellRunning(): On 2016/03/29 19:56:03, charliea wrote: > It looks like you can replace this loop with: > > self.battor_shell.wait(timeout) > > https://docs.python.org/3/library/subprocess.html#subprocess.Popen.wait > > That'll handle another problem: it probably makes sense to add *some* sort of > timeout to this: otherwise -it'll just never shut down if the battor agent > hangs. I guess this is only if Telemetry doesn't already have some sort of > overall timeout where it sends a signal when things take too long. (I don't know > enough about Telemetry to say one way or another.) Done. https://codereview.chromium.org/1828143004/diff/180001/common/battor/battor/b... File common/battor/battor/battor_wrapper_unittest.py (right): https://codereview.chromium.org/1828143004/diff/180001/common/battor/battor/b... common/battor/battor/battor_wrapper_unittest.py:163: self._battor.StopTracing() On 2016/03/29 22:30:33, aiolos wrote: > Assert that Tracing has stopped? Done.
On 2016/03/30 15:19:37, rnephew1 wrote: > https://codereview.chromium.org/1828143004/diff/180001/common/battor/battor/b... > File common/battor/battor/battor_wrapper.py (right): > > https://codereview.chromium.org/1828143004/diff/180001/common/battor/battor/b... > common/battor/battor/battor_wrapper.py:20: class BattorWrapper(object): > On 2016/03/29 19:56:03, charliea wrote: > > Could you add a pydoc for this class? a la > > > https://github.com/catapult-project/catapult/blob/40afc53353daf28b955397c8e27... > > Done. > > https://codereview.chromium.org/1828143004/diff/180001/common/battor/battor/b... > common/battor/battor/battor_wrapper.py:29: """Construtor. > On 2016/03/29 19:56:03, charliea wrote: > > nit: s/Construtor/constructor > > Done. > > https://codereview.chromium.org/1828143004/diff/180001/common/battor/battor/b... > common/battor/battor/battor_wrapper.py:42: _battor_shell: Asubprocess running > the bator_agent_binary > On 2016/03/29 19:56:03, charliea wrote: > > nit: s/Asubprocess/a subprocess > > Done. > > https://codereview.chromium.org/1828143004/diff/180001/common/battor/battor/b... > common/battor/battor/battor_wrapper.py:43: _trace_results_path: Path to BattOr > trace results. > On 2016/03/29 19:56:03, charliea wrote: > > nit: Maybe add "file" at the end of this sentence? > > Done. > > https://codereview.chromium.org/1828143004/diff/180001/common/battor/battor/b... > common/battor/battor/battor_wrapper.py:48: 'battor_binary_dependencies.json') > On 2016/03/29 19:56:03, charliea wrote: > > nit: Is this proper python indentation? Looks like maybe it should be to the > > left > > Done. > > https://codereview.chromium.org/1828143004/diff/180001/common/battor/battor/b... > common/battor/battor/battor_wrapper.py:52: self._battor_agent_binary = > dm.FetchPath( > On 2016/03/29 22:30:32, aiolos wrote: > > On 2016/03/29 19:56:03, charliea wrote: > > > Should you check for if this returns 'None' and throw if it does? > > > > > > > > > https://github.com/catapult-project/catapult/blob/78a6f08e843d2a4752bcb7ecac4... > > > > DependencyManager.FetchPath (which is returned by binary_manager.FetchPath) > > should throw an exception if the path isn't found, or if it doesn't exist. > > > https://github.com/catapult-project/catapult/blob/78a6f08e843d2a4752bcb7ecac4... > > Acknowledged. > > https://codereview.chromium.org/1828143004/diff/180001/common/battor/battor/b... > common/battor/battor/battor_wrapper.py:57: tf = > tempfile.NamedTemporaryFile(delete=False) > On 2016/03/29 19:56:03, charliea wrote: > > I'm kind of confused by this. So you open up the temp file, record its name, > and > > immediately close it? Why? (If this is what you meant to do, it's probably > worth > > documenting why in a comment, as others will have the same question.) > > Its creating a blank file it can later dump the results into. Its to make sure > there is no collision later; even though it would be highly unlikely. I'll move > it directly to where it is used so its less confusing. > > https://codereview.chromium.org/1828143004/diff/180001/common/battor/battor/b... > common/battor/battor/battor_wrapper.py:72: raise BattorError('Must specify > device for android platform.') > On 2016/03/29 19:56:03, charliea wrote: > > nit: In plain text (comments/strings), you switch between 'android' and > > 'Android' a bunch in this file. I don't care too much which one you pick, but > > it's probably worth picking one for consistency. > > Done. > > https://codereview.chromium.org/1828143004/diff/180001/common/battor/battor/b... > common/battor/battor/battor_wrapper.py:76: # Not android and no explicilty > passed BattOr. > On 2016/03/29 19:56:03, charliea wrote: > > nit: s/explicilty/explicitly > > Done. > > https://codereview.chromium.org/1828143004/diff/180001/common/battor/battor/b... > common/battor/battor/battor_wrapper.py:83: def _SendBattorCommandImpl(self, cmd, > return_results=True): > On 2016/03/29 19:56:03, charliea wrote: > > nit: It looks like these functions are in somewhat of a random order. The most > > common ordering I've seen is: > > > > - public functions that are overriding a base class's version > > - public functions added in this class > > - private functions > > > > The specific ordering isn't as important as it is to just have some sort of an > > ordering, but the one above usually works pretty well. > > Done. > > https://codereview.chromium.org/1828143004/diff/180001/common/battor/battor/b... > common/battor/battor/battor_wrapper.py:128: while self.IsShellRunning(): > On 2016/03/29 19:56:03, charliea wrote: > > It looks like you can replace this loop with: > > > > self.battor_shell.wait(timeout) > > > > https://docs.python.org/3/library/subprocess.html#subprocess.Popen.wait > > > > That'll handle another problem: it probably makes sense to add *some* sort of > > timeout to this: otherwise -it'll just never shut down if the battor agent > > hangs. I guess this is only if Telemetry doesn't already have some sort of > > overall timeout where it sends a signal when things take too long. (I don't > know > > enough about Telemetry to say one way or another.) > > Done. > > https://codereview.chromium.org/1828143004/diff/180001/common/battor/battor/b... > File common/battor/battor/battor_wrapper_unittest.py (right): > > https://codereview.chromium.org/1828143004/diff/180001/common/battor/battor/b... > common/battor/battor/battor_wrapper_unittest.py:163: self._battor.StopTracing() > On 2016/03/29 22:30:33, aiolos wrote: > > Assert that Tracing has stopped? > > Done. I added that if you do not pass CollectTraceData() a timeout, it will wait as long as the trace ran. This should help stop problems of false timeouts when tracing data is still being dumped.
non-owner lgtm w/ nits Thanks for doing this Randy! Very excited about how this is coming together. https://codereview.chromium.org/1828143004/diff/180001/common/battor/battor/b... File common/battor/battor/battor_wrapper.py (right): https://codereview.chromium.org/1828143004/diff/180001/common/battor/battor/b... common/battor/battor/battor_wrapper.py:57: tf = tempfile.NamedTemporaryFile(delete=False) On 2016/03/30 15:19:37, rnephew1 wrote: > On 2016/03/29 19:56:03, charliea wrote: > > I'm kind of confused by this. So you open up the temp file, record its name, > and > > immediately close it? Why? (If this is what you meant to do, it's probably > worth > > documenting why in a comment, as others will have the same question.) > > Its creating a blank file it can later dump the results into. Its to make sure > there is no collision later; even though it would be highly unlikely. I'll move > it directly to where it is used so its less confusing. Ahhh, makes sense. Thanks for the explanation. Would you mind adding a comment above "tf = tempfile.Named..." about why we're doing this? My guess is that most people reading and not super familiar with the code will be confused about what's going on. https://codereview.chromium.org/1828143004/diff/240001/common/battor/battor/b... File common/battor/battor/battor_wrapper.py (right): https://codereview.chromium.org/1828143004/diff/240001/common/battor/battor/b... common/battor/battor/battor_wrapper.py:23: To specify a battor, initialize with battor_path=<PATH>. nit: I think "A class for communicating with a BattOr in python." should be sufficient. What the parameters do is already described enough in a better place (by the constructor that accepts them). https://codereview.chromium.org/1828143004/diff/240001/common/battor/battor/b... common/battor/battor/battor_wrapper.py:34: """construtor. s/construtor./Constructor. (This must have gotten messed up?)
https://codereview.chromium.org/1828143004/diff/240001/common/battor/battor/b... File common/battor/battor/battor_wrapper_unittest.py (right): https://codereview.chromium.org/1828143004/diff/240001/common/battor/battor/b... common/battor/battor/battor_wrapper_unittest.py:39: #find_usb_devices.GetBattorPathFromPhoneSerial) lgtm once this code is uncommented. nit: the second line should have a 4-space indent.
https://codereview.chromium.org/1828143004/diff/240001/common/battor/battor/b... File common/battor/battor/battor_wrapper.py (right): https://codereview.chromium.org/1828143004/diff/240001/common/battor/battor/b... common/battor/battor/battor_wrapper.py:23: To specify a battor, initialize with battor_path=<PATH>. On 2016/03/30 18:58:33, charliea wrote: > nit: I think "A class for communicating with a BattOr in python." should be > sufficient. What the parameters do is already described enough in a better place > (by the constructor that accepts them). Done. https://codereview.chromium.org/1828143004/diff/240001/common/battor/battor/b... common/battor/battor/battor_wrapper.py:34: """construtor. On 2016/03/30 18:58:33, charliea wrote: > s/construtor./Constructor. > > (This must have gotten messed up?) Done. https://codereview.chromium.org/1828143004/diff/240001/common/battor/battor/b... File common/battor/battor/battor_wrapper_unittest.py (right): https://codereview.chromium.org/1828143004/diff/240001/common/battor/battor/b... common/battor/battor/battor_wrapper_unittest.py:39: #find_usb_devices.GetBattorPathFromPhoneSerial) On 2016/03/30 20:14:14, aiolos wrote: > lgtm once this code is uncommented. > > nit: the second line should have a 4-space indent. Acknowledged.
Alex's CL landed. Made changes to work with his new mapping file. PTAL.
https://codereview.chromium.org/1828143004/diff/280001/common/battor/battor/b... File common/battor/battor/battor_wrapper.py (right): https://codereview.chromium.org/1828143004/diff/280001/common/battor/battor/b... common/battor/battor/battor_wrapper.py:106: self._battor_shell.wait() Did you mean to get rid of the timeout here? https://codereview.chromium.org/1828143004/diff/280001/common/battor/battor/b... File common/battor/battor/battor_wrapper_unittest.py (right): https://codereview.chromium.org/1828143004/diff/280001/common/battor/battor/b... common/battor/battor/battor_wrapper_unittest.py:10: from devil.utils import find_usb_devices nit: are these in the right order?
lgtm again with nits https://codereview.chromium.org/1828143004/diff/280001/common/battor/battor/b... File common/battor/battor/battor_wrapper.py (right): https://codereview.chromium.org/1828143004/diff/280001/common/battor/battor/b... common/battor/battor/battor_wrapper.py:32: test_platform: Platform BattOr is attached to. nit: s/test_platform/target_platform
https://codereview.chromium.org/1828143004/diff/280001/common/battor/battor/b... File common/battor/battor/battor_wrapper.py (right): https://codereview.chromium.org/1828143004/diff/280001/common/battor/battor/b... common/battor/battor/battor_wrapper.py:32: test_platform: Platform BattOr is attached to. On 2016/04/04 17:29:14, nednguyen wrote: > nit: s/test_platform/target_platform Done. https://codereview.chromium.org/1828143004/diff/280001/common/battor/battor/b... common/battor/battor/battor_wrapper.py:106: self._battor_shell.wait() On 2016/04/04 17:25:46, charliea wrote: > Did you mean to get rid of the timeout here? Yes, I meant to add a comment here before emailing it out explaining why. The wait only exists in python 3.X. https://codereview.chromium.org/1828143004/diff/280001/common/battor/battor/b... File common/battor/battor/battor_wrapper_unittest.py (right): https://codereview.chromium.org/1828143004/diff/280001/common/battor/battor/b... common/battor/battor/battor_wrapper_unittest.py:10: from devil.utils import find_usb_devices On 2016/04/04 17:25:46, charliea wrote: > nit: are these in the right order? Hmm, wonder why the linter didn't catch that.
Still lgtm. https://codereview.chromium.org/1828143004/diff/280001/common/battor/battor/b... File common/battor/battor/battor_wrapper_unittest.py (right): https://codereview.chromium.org/1828143004/diff/280001/common/battor/battor/b... common/battor/battor/battor_wrapper_unittest.py:10: from devil.utils import find_usb_devices On 2016/04/04 17:25:46, charliea wrote: > nit: are these in the right order? Nope. The battor import should be in a third section below the other catapult-imports. And the two devil imports should switch order. Import sections should be: python library, third party imports, catapult imports, project imports. Within each section, the imports should be organized alphabetically, using the full import.
https://codereview.chromium.org/1828143004/diff/280001/common/battor/battor/b... File common/battor/battor/battor_wrapper_unittest.py (right): https://codereview.chromium.org/1828143004/diff/280001/common/battor/battor/b... common/battor/battor/battor_wrapper_unittest.py:10: from devil.utils import find_usb_devices On 2016/04/04 17:37:36, aiolos wrote: > On 2016/04/04 17:25:46, charliea wrote: > > nit: are these in the right order? > > Nope. The battor import should be in a third section below the other > catapult-imports. And the two devil imports should switch order. > > Import sections should be: python library, third party imports, catapult > imports, project imports. > > Within each section, the imports should be organized alphabetically, using the > full import. Could you file a bug on the linter not catching the import order?
https://codereview.chromium.org/1828143004/diff/280001/common/battor/battor/b... File common/battor/battor/battor_wrapper_unittest.py (right): https://codereview.chromium.org/1828143004/diff/280001/common/battor/battor/b... common/battor/battor/battor_wrapper_unittest.py:10: from devil.utils import find_usb_devices On 2016/04/04 17:38:50, aiolos wrote: > On 2016/04/04 17:37:36, aiolos wrote: > > On 2016/04/04 17:25:46, charliea wrote: > > > nit: are these in the right order? > > > > Nope. The battor import should be in a third section below the other > > catapult-imports. And the two devil imports should switch order. > > > > Import sections should be: python library, third party imports, catapult > > imports, project imports. > > > > Within each section, the imports should be organized alphabetically, using the > > full import. > > Could you file a bug on the linter not catching the import order? Bug filed here: https://github.com/catapult-project/catapult/issues/2207
Patchset #16 (id:300001) has been deleted
https://codereview.chromium.org/1828143004/diff/280001/common/battor/battor/b... File common/battor/battor/battor_wrapper_unittest.py (right): https://codereview.chromium.org/1828143004/diff/280001/common/battor/battor/b... common/battor/battor/battor_wrapper_unittest.py:10: from devil.utils import find_usb_devices On 2016/04/04 17:46:23, rnephew1 wrote: > On 2016/04/04 17:38:50, aiolos wrote: > > On 2016/04/04 17:37:36, aiolos wrote: > > > On 2016/04/04 17:25:46, charliea wrote: > > > > nit: are these in the right order? > > > > > > Nope. The battor import should be in a third section below the other > > > catapult-imports. And the two devil imports should switch order. > > > > > > Import sections should be: python library, third party imports, catapult > > > imports, project imports. > > > > > > Within each section, the imports should be organized alphabetically, using > the > > > full import. > > > > Could you file a bug on the linter not catching the import order? > > Bug filed here: > https://github.com/catapult-project/catapult/issues/2207 Thank you!
lgtm w/ comments https://codereview.chromium.org/1828143004/diff/260001/common/battor/battor/b... File common/battor/battor/battor_wrapper.py (right): https://codereview.chromium.org/1828143004/diff/260001/common/battor/battor/b... common/battor/battor/battor_wrapper.py:88: # Create tmp file to reserve location for saving results. nit: s/tmp/temp https://codereview.chromium.org/1828143004/diff/260001/common/battor/battor/b... File common/battor/battor/battor_wrapper_unittest.py (right): https://codereview.chromium.org/1828143004/diff/260001/common/battor/battor/b... common/battor/battor/battor_wrapper_unittest.py:36: # DO NOT SUBMIT until this code is uncommented. Shouldn't this be uncommented? https://codereview.chromium.org/1828143004/diff/260001/common/battor/battor/b... common/battor/battor/battor_wrapper_unittest.py:51: # DO NOT SUBMIT until this code is uncommented. Same https://codereview.chromium.org/1828143004/diff/260001/common/battor/battor/b... common/battor/battor/battor_wrapper_unittest.py:82: # DO NOT SUBMIT until this code is finished. Same
https://codereview.chromium.org/1828143004/diff/260001/common/battor/battor/b... File common/battor/battor/battor_wrapper.py (right): https://codereview.chromium.org/1828143004/diff/260001/common/battor/battor/b... common/battor/battor/battor_wrapper.py:88: # Create tmp file to reserve location for saving results. On 2016/04/07 16:40:39, charliea wrote: > nit: s/tmp/temp Done. https://codereview.chromium.org/1828143004/diff/260001/common/battor/battor/b... File common/battor/battor/battor_wrapper_unittest.py (right): https://codereview.chromium.org/1828143004/diff/260001/common/battor/battor/b... common/battor/battor/battor_wrapper_unittest.py:36: # DO NOT SUBMIT until this code is uncommented. On 2016/04/07 16:40:39, charliea wrote: > Shouldn't this be uncommented? Done. https://codereview.chromium.org/1828143004/diff/260001/common/battor/battor/b... common/battor/battor/battor_wrapper_unittest.py:51: # DO NOT SUBMIT until this code is uncommented. On 2016/04/07 16:40:39, charliea wrote: > Same Done. https://codereview.chromium.org/1828143004/diff/260001/common/battor/battor/b... common/battor/battor/battor_wrapper_unittest.py:82: # DO NOT SUBMIT until this code is finished. On 2016/04/07 16:40:39, charliea wrote: > Same Done.
The CQ bit was checked by rnephew@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com, aiolos@chromium.org, charliea@chromium.org Link to the patchset: https://codereview.chromium.org/1828143004/#ps340001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1828143004/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1828143004/340001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Catapult Mac Tryserver on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Ma...)
On 2016/04/07 16:58:20, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > Catapult Mac Tryserver on master.tryserver.client.catapult (JOB_FAILED, > https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Ma...) Dev server is down. If you run the test locally & it passed, feel free to git cl land.
Description was changed from ========== [Battor] Add python wrapper for communicating with battor_agent_binary BUG=chromium:574888 ========== to ========== [Battor] Add python wrapper for communicating with battor_agent_binary BUG=chromium:574888 R=aiolos@chromium.org, charliea@chromium.org, nednguyen@google.com Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #17 (id:340001) manually as 4411881631aa959b820380e88a39b3eda86bf75c (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
