Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
64553012:
[android] Adds AdbWrapper which is the base of the eventual AndroidCommands replacement. (Closed)
|
Created:
7 years, 1 month ago by craigdh Modified:
7 years ago Reviewers:
frankf CC:
chromium-reviews, craigdh+watch_chromium.org, bulach+watch_chromium.org, yfriedman+watch_chromium.org, ilevy-cc_chromium.org, klundberg+watch_chromium.org, frankf+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[android] Adds AdbWrapper which is the base of the eventual AndroidCommands replacement.
BUG=318387
TEST=included
NOTRY=True
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=238123
Patch Set 1 #Patch Set 2 : rebase after crrev.com/60043003 #
Total comments: 20
Patch Set 3 : addressed comments #
Total comments: 8
Patch Set 4 : adds tests #Patch Set 5 : added chromium header to test file #
Total comments: 20
Patch Set 6 : addressed comments #
Messages
Total messages: 10 (0 generated)
ping?
https://codereview.chromium.org/64553012/diff/20001/build/android/pylib/devic... File build/android/pylib/device/adb_wrapper.py (right): https://codereview.chromium.org/64553012/diff/20001/build/android/pylib/devic... build/android/pylib/device/adb_wrapper.py:5: """Module wrapping Android's Adb tool.""" "adb" https://codereview.chromium.org/64553012/diff/20001/build/android/pylib/devic... build/android/pylib/device/adb_wrapper.py:12: _DEFAULT_TIMEOUT = 30 add comments for each global constant https://codereview.chromium.org/64553012/diff/20001/build/android/pylib/devic... build/android/pylib/device/adb_wrapper.py:25: 'adb command \'%s\' failed with message: %s' % (cmd, msg)) add quotation marks around msg https://codereview.chromium.org/64553012/diff/20001/build/android/pylib/devic... build/android/pylib/device/adb_wrapper.py:28: class CommandTimeoutError(BaseError): docstring, run pylint https://codereview.chromium.org/64553012/diff/20001/build/android/pylib/devic... build/android/pylib/device/adb_wrapper.py:41: self._device_serial = str(device_serial) hmm, why wouldn't this be a str? https://codereview.chromium.org/64553012/diff/20001/build/android/pylib/devic... build/android/pylib/device/adb_wrapper.py:51: check_error: check that the command doesn't return an error message. Expand this comment to state that this doesn't work for shell commands failing. https://codereview.chromium.org/64553012/diff/20001/build/android/pylib/devic... build/android/pylib/device/adb_wrapper.py:56: cmd = ['adb'] + list(arg_list) same here, do you mean to assert it's a list? If so use an assert :) https://codereview.chromium.org/64553012/diff/20001/build/android/pylib/devic... build/android/pylib/device/adb_wrapper.py:113: def GetDevices(cls, timeout=_DEFAULT_TIMEOUT, retries=_DEFAULT_RETRIES): Take a look at current adb_commands. You want to get devices based on: - emulator versus hardware - various getprops https://codereview.chromium.org/64553012/diff/20001/build/android/pylib/devic... build/android/pylib/device/adb_wrapper.py:147: self._DeviceAdbCmd(['push', local, remote], timeout, retries) YOu need check preconditions for these operations by asserting whether the file exists, etc https://codereview.chromium.org/64553012/diff/20001/build/android/pylib/devic... build/android/pylib/device/adb_wrapper.py:161: def Shell(self, command, timeout=_DEFAULT_TIMEOUT, retries=_DEFAULT_RETRIES): how about returning the rc in the shell?
https://codereview.chromium.org/64553012/diff/20001/build/android/pylib/devic... File build/android/pylib/device/adb_wrapper.py (right): https://codereview.chromium.org/64553012/diff/20001/build/android/pylib/devic... build/android/pylib/device/adb_wrapper.py:5: """Module wrapping Android's Adb tool.""" On 2013/11/20 19:28:39, frankf wrote: > "adb" Done. https://codereview.chromium.org/64553012/diff/20001/build/android/pylib/devic... build/android/pylib/device/adb_wrapper.py:25: 'adb command \'%s\' failed with message: %s' % (cmd, msg)) On 2013/11/20 19:28:39, frankf wrote: > add quotation marks around msg Done. https://codereview.chromium.org/64553012/diff/20001/build/android/pylib/devic... build/android/pylib/device/adb_wrapper.py:28: class CommandTimeoutError(BaseError): On 2013/11/20 19:28:39, frankf wrote: > docstring, run pylint Done. https://codereview.chromium.org/64553012/diff/20001/build/android/pylib/devic... build/android/pylib/device/adb_wrapper.py:41: self._device_serial = str(device_serial) On 2013/11/20 19:28:39, frankf wrote: > hmm, why wouldn't this be a str? To be pythonic and support ducktyping with anything that implements __str__. For example you could pass an existing AdbWrapper. https://codereview.chromium.org/64553012/diff/20001/build/android/pylib/devic... build/android/pylib/device/adb_wrapper.py:51: check_error: check that the command doesn't return an error message. On 2013/11/20 19:28:39, frankf wrote: > Expand this comment to state that this doesn't work for shell commands failing. Clarified. https://codereview.chromium.org/64553012/diff/20001/build/android/pylib/devic... build/android/pylib/device/adb_wrapper.py:56: cmd = ['adb'] + list(arg_list) On 2013/11/20 19:28:39, frankf wrote: > same here, do you mean to assert it's a list? If so use an assert :) Not, list concatenation only works for lists but I see no reason why we shouldn't work if an iterator is passed. Again, being pythonic: ducktyping instead of hard type checks. https://codereview.chromium.org/64553012/diff/20001/build/android/pylib/devic... build/android/pylib/device/adb_wrapper.py:113: def GetDevices(cls, timeout=_DEFAULT_TIMEOUT, retries=_DEFAULT_RETRIES): On 2013/11/20 19:28:39, frankf wrote: > Take a look at current adb_commands. You want to get devices based on: > - emulator versus hardware > - various getprops Yes, that falls under the TODO. https://codereview.chromium.org/64553012/diff/20001/build/android/pylib/devic... build/android/pylib/device/adb_wrapper.py:147: self._DeviceAdbCmd(['push', local, remote], timeout, retries) On 2013/11/20 19:28:39, frankf wrote: > YOu need check preconditions for these operations by asserting whether the file > exists, etc Done. https://codereview.chromium.org/64553012/diff/20001/build/android/pylib/devic... build/android/pylib/device/adb_wrapper.py:147: self._DeviceAdbCmd(['push', local, remote], timeout, retries) On 2013/11/20 19:28:39, frankf wrote: > YOu need check preconditions for these operations by asserting whether the file > exists, etc Done. https://codereview.chromium.org/64553012/diff/20001/build/android/pylib/devic... build/android/pylib/device/adb_wrapper.py:161: def Shell(self, command, timeout=_DEFAULT_TIMEOUT, retries=_DEFAULT_RETRIES): On 2013/11/20 19:28:39, frankf wrote: > how about returning the rc in the shell? Done.
https://codereview.chromium.org/64553012/diff/80001/build/android/pylib/devic... File build/android/pylib/device/adb_wrapper.py (right): https://codereview.chromium.org/64553012/diff/80001/build/android/pylib/devic... build/android/pylib/device/adb_wrapper.py:70: check_error: check that the command doesn't return an error message. This actually, you'll only get 'error:' if device is offline right? So for install, you still need to look for success token https://codereview.chromium.org/64553012/diff/80001/build/android/pylib/devic... build/android/pylib/device/adb_wrapper.py:76: cmd = ['adb'] + list(arg_list) get rid of these conversions as discussed https://codereview.chromium.org/64553012/diff/80001/build/android/pylib/devic... build/android/pylib/device/adb_wrapper.py:158: def Push(self, local, remote, timeout=_DEFAULT_TIMEOUT, please add test cases for any method where feasable. In this case, check whether local/remote is file/directory/symlink, etc. https://codereview.chromium.org/64553012/diff/80001/build/android/pylib/devic... build/android/pylib/device/adb_wrapper.py:190: expect_rc: optional, if set checks that the command's return code matches (optional) if set, checks...
https://codereview.chromium.org/64553012/diff/80001/build/android/pylib/devic... File build/android/pylib/device/adb_wrapper.py (right): https://codereview.chromium.org/64553012/diff/80001/build/android/pylib/devic... build/android/pylib/device/adb_wrapper.py:70: check_error: check that the command doesn't return an error message. This On 2013/11/21 01:12:57, frankf wrote: > actually, you'll only get 'error:' if device is offline right? So for install, > you still need to look for success token Install already checks for 'Success' https://codereview.chromium.org/64553012/diff/80001/build/android/pylib/devic... build/android/pylib/device/adb_wrapper.py:76: cmd = ['adb'] + list(arg_list) On 2013/11/21 01:12:57, frankf wrote: > get rid of these conversions as discussed Done. https://codereview.chromium.org/64553012/diff/80001/build/android/pylib/devic... build/android/pylib/device/adb_wrapper.py:158: def Push(self, local, remote, timeout=_DEFAULT_TIMEOUT, On 2013/11/21 01:12:57, frankf wrote: > please add test cases for any method where feasable. In this case, check whether > local/remote is file/directory/symlink, etc. Done. I avoided testing all cases such as file/directory/symlink. Those are all handled the same from the point of view of AdbWrapper and I think it's fair to assume the behavior of adb itself is tested separately. https://codereview.chromium.org/64553012/diff/80001/build/android/pylib/devic... build/android/pylib/device/adb_wrapper.py:190: expect_rc: optional, if set checks that the command's return code matches On 2013/11/21 01:12:57, frankf wrote: > (optional) if set, checks... Done.
lgtm after fixes https://codereview.chromium.org/64553012/diff/150001/build/android/pylib/devi... File build/android/pylib/device/adb_wrapper.py (right): https://codereview.chromium.org/64553012/diff/150001/build/android/pylib/devi... build/android/pylib/device/adb_wrapper.py:5: """Module wrapping Android's adb tool.""" expand this description to mention that this is just a thin wrapper, more complicated logic just be done at a higher level https://codereview.chromium.org/64553012/diff/150001/build/android/pylib/devi... build/android/pylib/device/adb_wrapper.py:86: if check_error and output[:len('error:')] == 'error:': can you find out in which situations you get this? https://codereview.chromium.org/64553012/diff/150001/build/android/pylib/devi... build/android/pylib/device/adb_wrapper.py:132: return '%s(\'%s\')' % (self.__class__.__name__, str(self)) is str() necessary? https://codereview.chromium.org/64553012/diff/150001/build/android/pylib/devi... build/android/pylib/device/adb_wrapper.py:171: def Pull(self, remote, local, timeout=_DEFAULT_TIMEOUT, can you check that timeout default are set at least as long as existing logic? https://codereview.chromium.org/64553012/diff/150001/build/android/pylib/devi... build/android/pylib/device/adb_wrapper.py:223: filter_spec: (optional) spec to filter the logcat. be consistent with first letter capitalization wrt rest of codebase https://codereview.chromium.org/64553012/diff/150001/build/android/pylib/devi... build/android/pylib/device/adb_wrapper.py:231: if filter_spec is not None: if filter_spec: https://codereview.chromium.org/64553012/diff/150001/build/android/pylib/devi... build/android/pylib/device/adb_wrapper.py:253: self._DeviceAdbCmd(['forward', str(local), str(remote)], timeout, retries) remove str() as asked before https://codereview.chromium.org/64553012/diff/150001/build/android/pylib/devi... build/android/pylib/device/adb_wrapper.py:270: """Push a package file to the device and install it. just say 'Install apk' https://codereview.chromium.org/64553012/diff/150001/build/android/pylib/devi... File build/android/pylib/device/adb_wrapper_test.py (right): https://codereview.chromium.org/64553012/diff/150001/build/android/pylib/devi... build/android/pylib/device/adb_wrapper_test.py:39: output = self._adb.Shell('echo test', expect_rc=0) also test the failure case https://codereview.chromium.org/64553012/diff/150001/build/android/pylib/devi... build/android/pylib/device/adb_wrapper_test.py:62: self.assertRaises(adb_wrapper.CommandTimeoutError, self._adb.Logcat, I'd remove this test case
https://codereview.chromium.org/64553012/diff/150001/build/android/pylib/devi... File build/android/pylib/device/adb_wrapper.py (right): https://codereview.chromium.org/64553012/diff/150001/build/android/pylib/devi... build/android/pylib/device/adb_wrapper.py:5: """Module wrapping Android's adb tool.""" On 2013/11/27 23:35:03, frankf wrote: > expand this description to mention that this is just a thin wrapper, more > complicated logic just be done at a higher level Done. https://codereview.chromium.org/64553012/diff/150001/build/android/pylib/devi... build/android/pylib/device/adb_wrapper.py:86: if check_error and output[:len('error:')] == 'error:': On 2013/11/27 23:35:03, frankf wrote: > can you find out in which situations you get this? Done. https://codereview.chromium.org/64553012/diff/150001/build/android/pylib/devi... build/android/pylib/device/adb_wrapper.py:132: return '%s(\'%s\')' % (self.__class__.__name__, str(self)) On 2013/11/27 23:35:03, frankf wrote: > is str() necessary? Done. https://codereview.chromium.org/64553012/diff/150001/build/android/pylib/devi... build/android/pylib/device/adb_wrapper.py:171: def Pull(self, remote, local, timeout=_DEFAULT_TIMEOUT, On 2013/11/27 23:35:03, frankf wrote: > can you check that timeout default are set at least as long as existing logic? Generally true, discussed exceptions offline. https://codereview.chromium.org/64553012/diff/150001/build/android/pylib/devi... build/android/pylib/device/adb_wrapper.py:223: filter_spec: (optional) spec to filter the logcat. On 2013/11/27 23:35:03, frankf wrote: > be consistent with first letter capitalization wrt rest of codebase Done. https://codereview.chromium.org/64553012/diff/150001/build/android/pylib/devi... build/android/pylib/device/adb_wrapper.py:231: if filter_spec is not None: On 2013/11/27 23:35:03, frankf wrote: > if filter_spec: No, theoretically you could pass an empty filter string in which case we would be diverging from adb's behavior by not passing it along. https://codereview.chromium.org/64553012/diff/150001/build/android/pylib/devi... build/android/pylib/device/adb_wrapper.py:253: self._DeviceAdbCmd(['forward', str(local), str(remote)], timeout, retries) On 2013/11/27 23:35:03, frankf wrote: > remove str() as asked before Nope, these need str() so that local and remote can be passed as integers. They need converting because cmd_helper assumes all arguments are strings. https://codereview.chromium.org/64553012/diff/150001/build/android/pylib/devi... build/android/pylib/device/adb_wrapper.py:270: """Push a package file to the device and install it. On 2013/11/27 23:35:03, frankf wrote: > just say 'Install apk' Done. https://codereview.chromium.org/64553012/diff/150001/build/android/pylib/devi... File build/android/pylib/device/adb_wrapper_test.py (right): https://codereview.chromium.org/64553012/diff/150001/build/android/pylib/devi... build/android/pylib/device/adb_wrapper_test.py:39: output = self._adb.Shell('echo test', expect_rc=0) On 2013/11/27 23:35:03, frankf wrote: > also test the failure case Done. https://codereview.chromium.org/64553012/diff/150001/build/android/pylib/devi... build/android/pylib/device/adb_wrapper_test.py:62: self.assertRaises(adb_wrapper.CommandTimeoutError, self._adb.Logcat, On 2013/11/27 23:35:03, frankf wrote: > I'd remove this test case Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/craigdh@chromium.org/64553012/170001
Message was sent while issue was closed.
Change committed as 238123 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
