Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1946773002:
[BattOr] Add support to battor_wrapper to autodetect if battor is attached. (Closed)
|
Created:
4 years, 7 months ago by rnephew (Reviews Here) Modified:
4 years, 7 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 support to battor_wrapper to autodetect if battor is attached.
BUG=catapult:#2202
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/0130399cd7cee4d5555bd00a0899ccde113a15d4
Patch Set 1 #
Total comments: 22
Patch Set 2 : #
Total comments: 5
Patch Set 3 : #
Messages
Total messages: 15 (6 generated)
rnephew@chromium.org changed reviewers: + charliea@chromium.org, nednguyen@google.com
This requires https://codereview.chromium.org/1945733002/ to land first.
Patchset #1 (id:1) has been deleted
https://codereview.chromium.org/1946773002/diff/20001/common/battor/battor/ba... File common/battor/battor/battor_wrapper.py (right): https://codereview.chromium.org/1946773002/diff/20001/common/battor/battor/ba... common/battor/battor/battor_wrapper.py:21: def AutoDetectBattOr(test_platform, android_device=None, So is the purpose of this to determine whether BattOr tracing is available on a given host? https://codereview.chromium.org/1946773002/diff/20001/common/battor/battor/ba... common/battor/battor/battor_wrapper.py:23: """Returns True if BattOr is detected in expected place.""" Seems like "in expected place" is kind of redundant. (AutoDetect kind of implies that there are some expected places to look) https://codereview.chromium.org/1946773002/diff/20001/common/battor/battor/ba... common/battor/battor/battor_wrapper.py:26: raise battor_error.BattorError('Must past android device serial when ' 'Must pass Android device serial number when running autodetect on Android.'? https://codereview.chromium.org/1946773002/diff/20001/common/battor/battor/ba... common/battor/battor/battor_wrapper.py:26: raise battor_error.BattorError('Must past android device serial when ' I'm confused - isn't the point of AutoDetect that you *don't* have to pass any information about the device in order to find it? If there's a single Android device plugged in, shouldn't we be able to just find the BattOr? https://codereview.chromium.org/1946773002/diff/20001/common/battor/battor/ba... common/battor/battor/battor_wrapper.py:28: if not android_device_map and not android_device_file: android_device_map = (if android_device_file battor_device_mapping.ReadSerialMapFile(android_device_file) else battor_device_mapping.GenerateSerialMap()) https://codereview.chromium.org/1946773002/diff/20001/common/battor/battor/ba... File common/battor/battor/battor_wrapper_unittest.py (right): https://codereview.chromium.org/1946773002/diff/20001/common/battor/battor/ba... common/battor/battor/battor_wrapper_unittest.py:35: self._serial_tools_return = [('COM4', 'JUNK', '')] I don't think this should have any default return values. (If these values are necessary for a test below to work properly, they should be specified there.) https://codereview.chromium.org/1946773002/diff/20001/common/battor/battor/ba... common/battor/battor/battor_wrapper_unittest.py:58: # TODO(rnephew): Add Mac monkey patches when supported. nit: should this be under the windows/linux/android monkey patches? https://codereview.chromium.org/1946773002/diff/20001/common/battor/battor/ba... common/battor/battor/battor_wrapper_unittest.py:94: android_device_map={'abc', '123'})) nit (here and elsewhere): any reason for changing from 'abc123' as the device name to 'abc' as the device name and '123' as the battor name? I don't really care what the names are, as long as they're obvious and consistent. https://codereview.chromium.org/1946773002/diff/20001/common/battor/battor/ba... common/battor/battor/battor_wrapper_unittest.py:103: self._get_battor_list_return.append('battor') My preference: self._get_battor_list_return = ['battor'] Whenever possible, unit tests should be obvious and self-contained, and using 'append' makes it seem like there's something outside of the unit test that needs to be considered https://codereview.chromium.org/1946773002/diff/20001/common/battor/battor/ba... common/battor/battor/battor_wrapper_unittest.py:120: def testWinWithoutBattor(self): nit (here and for Mac, Linux): set self._get_battor_list_return = [] here, even though it's not functionally necessary It makes the test self-contained by making it clear what behavior you're testing.
https://codereview.chromium.org/1946773002/diff/20001/common/battor/battor/ba... File common/battor/battor/battor_wrapper.py (right): https://codereview.chromium.org/1946773002/diff/20001/common/battor/battor/ba... common/battor/battor/battor_wrapper.py:21: def AutoDetectBattOr(test_platform, android_device=None, On 2016/05/04 15:45:42, charliea wrote: > So is the purpose of this to determine whether BattOr tracing is available on a > given host? Well kinda, its practical purpose is to be used by battor_tracing_agents IsSupported method. https://codereview.chromium.org/1946773002/diff/20001/common/battor/battor/ba... common/battor/battor/battor_wrapper.py:26: raise battor_error.BattorError('Must past android device serial when ' On 2016/05/04 15:45:42, charliea wrote: > I'm confused - isn't the point of AutoDetect that you *don't* have to pass any > information about the device in order to find it? If there's a single Android > device plugged in, shouldn't we be able to just find the BattOr? Its the base amount of information needed to determine if a battor can be used on a given android device.
https://codereview.chromium.org/1946773002/diff/20001/common/battor/battor/ba... File common/battor/battor/battor_wrapper.py (right): https://codereview.chromium.org/1946773002/diff/20001/common/battor/battor/ba... common/battor/battor/battor_wrapper.py:23: """Returns True if BattOr is detected in expected place.""" On 2016/05/04 15:45:42, charliea wrote: > Seems like "in expected place" is kind of redundant. (AutoDetect kind of implies > that there are some expected places to look) Done. https://codereview.chromium.org/1946773002/diff/20001/common/battor/battor/ba... common/battor/battor/battor_wrapper.py:28: if not android_device_map and not android_device_file: On 2016/05/04 15:45:42, charliea wrote: > android_device_map = (if android_device_file > battor_device_mapping.ReadSerialMapFile(android_device_file) > else battor_device_mapping.GenerateSerialMap()) That is not equivalent to what I have, and to make it so would make it less readable. I'll add a comment so its more clear what happens if neither if statement is triggered. https://codereview.chromium.org/1946773002/diff/20001/common/battor/battor/ba... File common/battor/battor/battor_wrapper_unittest.py (right): https://codereview.chromium.org/1946773002/diff/20001/common/battor/battor/ba... common/battor/battor/battor_wrapper_unittest.py:35: self._serial_tools_return = [('COM4', 'JUNK', '')] On 2016/05/04 15:45:42, charliea wrote: > I don't think this should have any default return values. (If these values are > necessary for a test below to work properly, they should be specified there.) Done. https://codereview.chromium.org/1946773002/diff/20001/common/battor/battor/ba... common/battor/battor/battor_wrapper_unittest.py:58: # TODO(rnephew): Add Mac monkey patches when supported. On 2016/05/04 15:45:42, charliea wrote: > nit: should this be under the windows/linux/android monkey patches? It is, I have the spaces under the different patches for linux for readability because there are so many. https://codereview.chromium.org/1946773002/diff/20001/common/battor/battor/ba... common/battor/battor/battor_wrapper_unittest.py:94: android_device_map={'abc', '123'})) On 2016/05/04 15:45:42, charliea wrote: > nit (here and elsewhere): any reason for changing from 'abc123' as the device > name to 'abc' as the device name and '123' as the battor name? I don't really > care what the names are, as long as they're obvious and consistent. Done. https://codereview.chromium.org/1946773002/diff/20001/common/battor/battor/ba... common/battor/battor/battor_wrapper_unittest.py:103: self._get_battor_list_return.append('battor') On 2016/05/04 15:45:42, charliea wrote: > My preference: > > self._get_battor_list_return = ['battor'] > > Whenever possible, unit tests should be obvious and self-contained, and using > 'append' makes it seem like there's something outside of the unit test that > needs to be considered Done. https://codereview.chromium.org/1946773002/diff/20001/common/battor/battor/ba... common/battor/battor/battor_wrapper_unittest.py:120: def testWinWithoutBattor(self): On 2016/05/04 15:45:42, charliea wrote: > nit (here and for Mac, Linux): set self._get_battor_list_return = [] here, even > though it's not functionally necessary > > It makes the test self-contained by making it clear what behavior you're > testing. Done.
lgtm if Charlie is happy https://codereview.chromium.org/1946773002/diff/40001/common/battor/battor/ba... File common/battor/battor/battor_wrapper.py (right): https://codereview.chromium.org/1946773002/diff/40001/common/battor/battor/ba... common/battor/battor/battor_wrapper.py:21: def AutoDetectBattOr(test_platform, android_device=None, s/AutoDetectBattOr/IsBattorConnected "if IsBattorConnected(...) " would be clearer to code readers than "if AutoDetectBattOr(...)"
lgtm w/ comments https://codereview.chromium.org/1946773002/diff/20001/common/battor/battor/ba... File common/battor/battor/battor_wrapper.py (right): https://codereview.chromium.org/1946773002/diff/20001/common/battor/battor/ba... common/battor/battor/battor_wrapper.py:26: raise battor_error.BattorError('Must past android device serial when ' On 2016/05/04 15:51:41, rnephew1 wrote: > On 2016/05/04 15:45:42, charliea wrote: > > I'm confused - isn't the point of AutoDetect that you *don't* have to pass any > > information about the device in order to find it? If there's a single Android > > device plugged in, shouldn't we be able to just find the BattOr? > > Its the base amount of information needed to determine if a battor can be used > on a given android device. Reading this more carefully, makes sense now https://codereview.chromium.org/1946773002/diff/20001/common/battor/battor/ba... common/battor/battor/battor_wrapper.py:28: if not android_device_map and not android_device_file: On 2016/05/04 16:46:46, rnephew1 wrote: > On 2016/05/04 15:45:42, charliea wrote: > > android_device_map = (if android_device_file > > battor_device_mapping.ReadSerialMapFile(android_device_file) > > else battor_device_mapping.GenerateSerialMap()) > > That is not equivalent to what I have, and to make it so would make it less > readable. I'll add a comment so its more clear what happens if neither if > statement is triggered. I see what you mean about these not being equivalent. I think it might be clearer though if you do: if not android_device_map: if android_device_file: android_device_map = battor_device_mapping.ReadSerialMapFile(android_device_file) else android_device_map = battor_device_mapping.GenerateSerialMap() (this seems sufficiently clear where you could probably get rid of the comment) or something like that. Having the `not android_device_map` condition in both the if and the else if makes things a little hard to parse. https://codereview.chromium.org/1946773002/diff/40001/common/battor/battor/ba... File common/battor/battor/battor_wrapper.py (right): https://codereview.chromium.org/1946773002/diff/40001/common/battor/battor/ba... common/battor/battor/battor_wrapper.py:21: def AutoDetectBattOr(test_platform, android_device=None, On 2016/05/04 17:37:05, nednguyen wrote: > s/AutoDetectBattOr/IsBattorConnected > > "if IsBattorConnected(...) " would be clearer to code readers than "if > AutoDetectBattOr(...)" Agreed. https://codereview.chromium.org/1946773002/diff/40001/common/battor/battor/ba... File common/battor/battor/battor_wrapper_unittest.py (right): https://codereview.chromium.org/1946773002/diff/40001/common/battor/battor/ba... common/battor/battor/battor_wrapper_unittest.py:32: class AutoDetectBattorTest(unittest.TestCase): Think this should be "IsBattorDetectedTest" after Ned's suggested change
Patchset #3 (id:60001) has been deleted
https://codereview.chromium.org/1946773002/diff/20001/common/battor/battor/ba... File common/battor/battor/battor_wrapper.py (right): https://codereview.chromium.org/1946773002/diff/20001/common/battor/battor/ba... common/battor/battor/battor_wrapper.py:28: if not android_device_map and not android_device_file: On 2016/05/05 15:14:24, charliea wrote: > On 2016/05/04 16:46:46, rnephew1 wrote: > > On 2016/05/04 15:45:42, charliea wrote: > > > android_device_map = (if android_device_file > > > battor_device_mapping.ReadSerialMapFile(android_device_file) > > > else battor_device_mapping.GenerateSerialMap()) > > > > That is not equivalent to what I have, and to make it so would make it less > > readable. I'll add a comment so its more clear what happens if neither if > > statement is triggered. > > I see what you mean about these not being equivalent. I think it might be > clearer though if you do: > > if not android_device_map: > if android_device_file: > android_device_map = > battor_device_mapping.ReadSerialMapFile(android_device_file) > else > android_device_map = battor_device_mapping.GenerateSerialMap() > > (this seems sufficiently clear where you could probably get rid of the comment) > > or something like that. Having the `not android_device_map` condition in both > the if and the else if makes things a little hard to parse. > Done. https://codereview.chromium.org/1946773002/diff/40001/common/battor/battor/ba... File common/battor/battor/battor_wrapper.py (right): https://codereview.chromium.org/1946773002/diff/40001/common/battor/battor/ba... common/battor/battor/battor_wrapper.py:21: def AutoDetectBattOr(test_platform, android_device=None, On 2016/05/05 15:14:24, charliea wrote: > On 2016/05/04 17:37:05, nednguyen wrote: > > s/AutoDetectBattOr/IsBattorConnected > > > > "if IsBattorConnected(...) " would be clearer to code readers than "if > > AutoDetectBattOr(...)" > > Agreed. Done. https://codereview.chromium.org/1946773002/diff/40001/common/battor/battor/ba... File common/battor/battor/battor_wrapper_unittest.py (right): https://codereview.chromium.org/1946773002/diff/40001/common/battor/battor/ba... common/battor/battor/battor_wrapper_unittest.py:32: class AutoDetectBattorTest(unittest.TestCase): On 2016/05/05 15:14:24, charliea wrote: > Think this should be "IsBattorDetectedTest" after Ned's suggested change 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, charliea@chromium.org Link to the patchset: https://codereview.chromium.org/1946773002/#ps80001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1946773002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1946773002/80001
Message was sent while issue was closed.
Description was changed from ========== [BattOr] Add support to battor_wrapper to autodetect if battor is attached. BUG=catapult:#2202 ========== to ========== [BattOr] Add support to battor_wrapper to autodetect if battor is attached. BUG=catapult:#2202 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:80001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
