Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2899093002:
[devil] Allow instantiation of AdbWrapper with USB bus identifer.
Description[devil] Allow instantiation of AdbWrapper with USB bus identifier.
Adb allows selection of a device using both a serial number and the
device's USB bus identifier. This change makes it possible to create an AdbWrapper instance using a USB bus identifier in addition to a serial number. This option is required when multiple connected devices present the same serial number.
Bug: catapult:#3595
Patch Set 1 : [devil] Refactor AdbWrapper.GetState() to support USB ids #
Total comments: 12
Patch Set 2 : [devil] Reorganize comments and add warning when instantiating an AdbWrapper from USB id. #
Total comments: 1
Patch Set 3 : Allow instantiation of AdbWrapper with USB ID, with revised tests #
Total comments: 9
Patch Set 4 : Allow AdbWrapper to specify devices using either serial number or USB id. serial number #
Total comments: 4
Messages
Total messages: 44 (18 generated)
The CQ bit was checked by chris@kavefish.net to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-catapult-committers". Note that this has nothing to do with OWNERS files.
Description was changed from ========== [devil] Allow instantiation of AdbWrapper with USB bus identifer. Adb allows selection of a device using both a serial number and the device's USB bus identifier. This change makes it possible to create an AdbWrapper instance using a USB bus identifier in addition to a serial number. This option is required when multiple connected devices present the same serial number. BUG=catapult:# ========== to ========== [devil] Allow instantiation of AdbWrapper with USB bus identifer. Adb allows selection of a device using both a serial number and the device's USB bus identifier. This change makes it possible to create an AdbWrapper instance using a USB bus identifier in addition to a serial number. This option is required when multiple connected devices present the same serial number. BUG=catapult:# ==========
chris@kavefish.net changed reviewers: + perezju@chromium.org, rnephew@chromium.org
rnephew@chromium.org changed reviewers: + jbudorick@chromium.org
John, is this scenario one we care about supporting in devil? https://codereview.chromium.org/2899093002/diff/1/devil/devil/android/sdk/adb... File devil/devil/android/sdk/adb_wrapper.py (right): https://codereview.chromium.org/2899093002/diff/1/devil/devil/android/sdk/adb... devil/devil/android/sdk/adb_wrapper.py:835: if len(line) >= 3 and (line[0] == self._device_serial or line[2] == What does the output look like when there are 2 devices with the same serial? Cant say I have ever seen that happen. The output looks like this for me with only one device attached: >adb devices List of devices attached ZX1G22LCMZ device It looks like your change would break this, since the line with the device and its status is only 2 long.
This CL does not do anything for how we instantiate AdbWrapper objects. It only changes the GetState() method. Is this not complete? If so, can you remove me, perezju, and jbudorick and you can add us back when it is finished. I see that you have forked catapult, was this intended for your forked repo?
On 2017/05/24 22:53:19, rnephew (Reviews Here) wrote: > It looks like your change would break this, since the line with the device and > its status is only 2 long. It doesn't break in my admittedly limited testing because the change also adds "long_list=True" to the preceding invocation of self._RawDevices(), which guarantees the line variable has >= 3 elements. More thorough testing is greatly appreciated. I tried to invoke a CQ test, but it seems I don't have permission.
On 2017/05/24 23:04:53, rnephew (Reviews Here) wrote: > This CL does not do anything for how we instantiate AdbWrapper objects. It only > changes the GetState() method. I suppose technically it's true that you can instantiate an AdbWrapper object using whatever string you want. However, unless the instance was created with a valid serial number, the current implementation of GetState() will indicate that the device is offline which makes the resulting instance meaningless. This change modifies GetState() to correctly indicate that a device is available when it's instantiated using either a serial number or a USB bus ID. > I see that you have forked catapult, was this intended for your forked repo? I was hoping this small change would be mainlined unless it causes problems elsewhere. I forked assuming the contribution process involved a pull request, but that was before I read the contributor's guide. Thanks for the consideration.
On 2017/05/25 04:48:12, kavefish wrote: > On 2017/05/24 23:04:53, rnephew (Reviews Here) wrote: > > This CL does not do anything for how we instantiate AdbWrapper objects. It > only > > changes the GetState() method. > > I suppose technically it's true that you can instantiate an AdbWrapper object > using whatever string you want. However, unless the instance was created with a > valid serial number, the current implementation of GetState() will indicate that > > the device is offline which makes the resulting instance meaningless. > > This change modifies GetState() to correctly indicate that a device is available > when it's instantiated using either a serial number or a USB bus ID. Agree this change is interesting (allowing to talk to devices by USB bus ID), and with a little testing I did it looks like this implementation will actually do the right thing. My worry is in many other places we're probably assuming that "device_serial" actually *is* a device serial, so I'm not sure if we want to officially support the usage of AdbWrapper's instantiated with anything other than a device serial. Although maybe it's fine as long as these changes don't affect the instances with proper serials? What do others think?
Ah. I missed the -l being passed. Thats where my misconceptions came from. At the very least we need a unittest to test this change. I would also assume this change would break the already existing unittests for this method. Also like Juan said, It seems highly likely that there might be a corner condition where something expects a device serial number, and gets the USB bus ID instead. I did a quick pass in device_utils and adb_wrapper and nothing sticks out to me that might cause problems. There is the problem that this would mean _device_serial would not be a device serial and would be a usb address. Thats not something I'm exactly excited about. I think a better solution would be to refactor the code to better support this use case; but thats a big project so I would settle for a TODO() for doing that work with an explanation why. I ran adb devices -l and the output is below, I assume for the scenario you are talking about with both devices have the same serial the only difference would be both serials would be identical. Still seems like a weird scenario to come up. List of devices attached 00d0d567893340f4 device usb:3-4.2.3 product:bullhead model:Nexus_5X device:bullhead 0adbf8950390343f device usb:3-4.2.1 product:hammerhead model:Nexus_5 device:hammerhead I'll go ahead and start a try job for you to determine if the unittests fail.
The CQ bit was checked by rnephew@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Well shoot. There are no existing unittests for this. If you look at the existing ones in https://cs.chromium.org/chromium/src/third_party/catapult/devil/devil/android... it looks pretty easy to create one.
Description was changed from ========== [devil] Allow instantiation of AdbWrapper with USB bus identifer. Adb allows selection of a device using both a serial number and the device's USB bus identifier. This change makes it possible to create an AdbWrapper instance using a USB bus identifier in addition to a serial number. This option is required when multiple connected devices present the same serial number. BUG=catapult:# ========== to ========== [devil] Allow instantiation of AdbWrapper with USB bus identifer. Adb allows selection of a device using both a serial number and the device's USB bus identifier. This change makes it possible to create an AdbWrapper instance using a USB bus identifier in addition to a serial number. This option is required when multiple connected devices present the same serial number. ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Catapult Presubmit on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Pr...)
> At the very least we need a unittest to test this change. I'm thinking through how to do a meaningful unit test for this change and two things are keeping me from making a start: the existing convention that AdbWrapper can be instantiated with any string, and the use of pymock that abstracts away actual device connectivity from unit tests. The former convention means that checking device connectivity is the only way to determine whether an AdbWrapper instance is functional, but using pymock as the other unit tests do eliminates that option. I can see the utility of instantiating AdbWrapper for devices that aren't currently connected and also keeping instances for devices that fell over, so I'm not suggesting a change to that convention. I can make a test for this change using pymock, but it feels like ticking a box rather than a meaningful test. I'm happy to do that if that's what it takes, especially since it would be easy. Alternatively, I could make a more meaningful test that requires real devices, but I don't know whether that fits your usage model for unit tests. Incidentally, that's what I did to validate this change before submitting it: create AdbWrapper instances for connected devices using both their real serial numbers and USB bus IDs, then issuing commands to them in order to assess whether normal function was impaired. I assumed that's essentially what a CQ test did behind the scenes, but apparently not? I'd appreciate your suggestion for how to approach creating unit test(s) so I don't waste effort pursuing an implementation that doesn't fit your needs. > There is the problem that this would mean _device_serial would not be a device > serial and would be a usb address. Thats not something I'm exactly excited > about. I think a better solution would be to refactor the code to better support > this use case; but thats a big project so I would settle for a TODO() for doing > that work with an explanation why. I agree the _device_serial attribute becomes aesthetically displeasing when it can legitimately be used for values other than a serial number. In lieu of major refactoring, how do you feel about different approach: Rename _device_serial to _device_identifier, or equivalent. Its usage would be unchanged thereby eliminating the need for substantive refactoring and the attribute name would be accurate again. > I ran adb devices -l and the output is below, I assume for the scenario you are > talking about with both devices have the same serial the only difference would > be both serials would be identical. Agreed, it does seem odd this situation ever occurs given how easy it would be for OEMs to set serial numbers properly. However, my motto is: all things are possible with Android. FWIW, I'm not the only one [1] who's recognized this situation. Helpfully, a response to that SO post suggests using the -l option to select devices by USB bus ID instead of serial number. If you struggle to replicate in your environment, may I humbly postulate that the devices in your test rig are too nice. [1] https://stackoverflow.com/questions/14334656/same-serial-number-on-several-an...
On 2017/05/26 00:45:03, rnephew (Reviews Here) wrote: > Well shoot. There are no existing unittests for this. If you look at the > existing ones in > https://cs.chromium.org/chromium/src/third_party/catapult/devil/devil/android... > it looks pretty easy to create one. Thanks again for the test. As best I can tell, the reason the CQ run failed was the absence of a valid bug number in the commit message.
> > At the very least we need a unittest to test this change. > > I'm thinking through how to do a meaningful unit test for this change and two > things are keeping me from making a start: the existing convention that > AdbWrapper can be instantiated with any string, and the use of pymock that > abstracts away actual device connectivity from unit tests. OK, I'm seeing more options as I read more of the catapult code base. For instance devil/android/sdk/adb_devicetest.py could be extended to do on-device tests that could validate this change. I'll keep reading and come up with some unit tests for you to review.
I would suggest trying to keep this change small and self contained. For the unit tests: it would be fine to add two simple tests one creating an adb device with a serial, and one with a USB identifier; then on each check that GetStatus works as expected. It's ok for this to use some mock hard-coded output of adb devices. If you manage to also write a devicetest, that's fine too. But I would be OK leaving that one out for now if it get's tricky. Also, yes, please file an issue on https://github.com/catapult-project/catapult/issues/, and link to it from this CL description. https://codereview.chromium.org/2899093002/diff/1/devil/devil/android/sdk/adb... File devil/devil/android/sdk/adb_wrapper.py (right): https://codereview.chromium.org/2899093002/diff/1/devil/devil/android/sdk/adb... devil/devil/android/sdk/adb_wrapper.py:123: device_serial: The device serial number as a string. Add a note here to indicate that an USB identifier may also be provided, but with a warning stating that this is not fully tested. This would also be a good place, as Randy mentioned, to leave a TODO on improving support for that particular use case.
Description was changed from ========== [devil] Allow instantiation of AdbWrapper with USB bus identifer. Adb allows selection of a device using both a serial number and the device's USB bus identifier. This change makes it possible to create an AdbWrapper instance using a USB bus identifier in addition to a serial number. This option is required when multiple connected devices present the same serial number. ========== to ========== [devil] Allow instantiation of AdbWrapper with USB bus identifier. Adb allows selection of a device using both a serial number and the device's USB bus identifier. This change makes it possible to create an AdbWrapper instance using a USB bus identifier in addition to a serial number. This option is required when multiple connected devices present the same serial number. ==========
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
Thanks for the suggestions. I attempted to go small rather than get ambitious. In adb_wrapper.py I added a TODO and WARNING, as requested. In adb_wrapper_test.py I refactored the unit tests to use two AdbWrapper instances, one created with a serial number and another created with a USB id. Presubmit checks got a little cranky on my first pass. I appeased them through further refactoring that expanded the changes slightly more than I'd have liked, but hopefully you'll still find it manageable. I also filed an issue (3595) as requested, but I'm not sure it's linked from this side. For completeness and honesty's sake, I found an example of where this CL could lead to misbehaviour. In DeviceUtils.HealthyDevices(), devices are filtered against a blacklist by comparing the device's serial (retrieved using AdbWrapper.GetDeviceSerial()) against the blacklist. So, it's not possible to blacklist a device based on its USB id. Still, these changes do solve the problem I was having and I don't believe they will break any existing code. Thanks for the consideration.
I just tried running your patch locally, and the tests fail. You can run the
tests locally by going to 'catapult/devil/bin/' and running 'run_py_tests'
Here is the output from the last test that failed and the summary information:
[445/567]
devil.android.sdk.adb_wrapper_test.AdbWrapperTest.testGetStateUnauthorized
failed unexpectedly:
Traceback (most recent call last):
File
"/Users/rnephew/chromium/src/third_party/catapult/devil/devil/android/sdk/adb_wrapper_test.py",
line 89, in testGetStateUnauthorized
adb.GetState()
File
"/Users/rnephew/chromium/src/third_party/catapult/devil/devil/android/sdk/adb_wrapper.py",
line 839, in GetState
lines = self._RawDevices(timeout=timeout, retries=retries, long_list=True)
File
"/Users/rnephew/chromium/src/third_party/catapult/devil/devil/android/sdk/adb_wrapper.py",
line 392, in _RawDevices
output = cls._RunAdbCmd(cmd, timeout=timeout, retries=retries)
File
"/Users/rnephew/chromium/src/third_party/catapult/devil/devil/android/decorators.py",
line 57, in timeout_retry_wrapper
retry_if_func=retry_if_func)
File
"/Users/rnephew/chromium/src/third_party/catapult/devil/devil/utils/timeout_retry.py",
line 159, in Run
error_log_func=error_log_func)
File
"/Users/rnephew/chromium/src/third_party/catapult/devil/devil/utils/reraiser_thread.py",
line 186, in JoinAll
self._JoinAll(watcher, timeout)
File
"/Users/rnephew/chromium/src/third_party/catapult/devil/devil/utils/reraiser_thread.py",
line 158, in _JoinAll
thread.ReraiseIfException()
File
"/Users/rnephew/chromium/src/third_party/catapult/devil/devil/utils/reraiser_thread.py",
line 81, in run
self._ret = self._func(*self._args, **self._kwargs)
File
"/Users/rnephew/chromium/src/third_party/catapult/devil/devil/utils/timeout_retry.py",
line 152, in <lambda>
child_thread = reraiser_thread.ReraiserThread(lambda: func(*args,
**kwargs),
File
"/Users/rnephew/chromium/src/third_party/catapult/devil/devil/android/decorators.py",
line 47, in impl
return f(*args, **kwargs)
File
"/Users/rnephew/chromium/src/third_party/catapult/devil/devil/android/sdk/adb_wrapper.py",
line 254, in _RunAdbCmd
cls._BuildAdbCmd(args, device_serial, cpu_affinity=cpu_affinity),
File
"/Users/rnephew/chromium/src/third_party/catapult/devil/devil/android/sdk/adb_wrapper.py",
line 240, in _BuildAdbCmd
cmd.append(cls.GetAdbPath())
File
"/Users/rnephew/chromium/src/third_party/catapult/devil/devil/android/sdk/adb_wrapper.py",
line 228, in GetAdbPath
return cls._adb_path.read()
File
"/Users/rnephew/chromium/src/third_party/catapult/devil/devil/utils/lazy/weak_constant.py",
line 27, in read
self._val = self._initializer()
File
"/Users/rnephew/chromium/src/third_party/catapult/devil/devil/android/sdk/adb_wrapper.py",
line 74, in _FindAdb
raise device_errors.NoAdbError()
NoAdbError: Unable to find adb.
562 tests passed, 0 skipped, 5 failures.
https://codereview.chromium.org/2899093002/diff/60001/devil/devil/android/sdk...
File devil/devil/android/sdk/adb_wrapper.py (right):
https://codereview.chromium.org/2899093002/diff/60001/devil/devil/android/sdk...
devil/devil/android/sdk/adb_wrapper.py:115:
Warning should be moved up here, not in the args section.
https://codereview.chromium.org/2899093002/diff/60001/devil/devil/android/sdk...
devil/devil/android/sdk/adb_wrapper.py:125: # (*) WARNING: Not all devil
features are supported when using a USB ID.
I'm wondering if we can detect if we suspect that the user is using a USB Id and
output this warning there. Not necessarily in this CL, but just a thought.
https://codereview.chromium.org/2899093002/diff/60001/devil/devil/android/sdk...
devil/devil/android/sdk/adb_wrapper.py:130: if not device_serial:
TODO should be moved here outside the class level documentation.
https://codereview.chromium.org/2899093002/diff/60001/devil/devil/android/sdk...
File devil/devil/android/sdk/adb_wrapper_test.py (right):
https://codereview.chromium.org/2899093002/diff/60001/devil/devil/android/sdk...
devil/devil/android/sdk/adb_wrapper_test.py:20: def MockRunDeviceAdbCmd(adb,
return_value):
Any reason why this was moved to the module level from the AdbWrapperTest class?
https://codereview.chromium.org/2899093002/diff/60001/devil/devil/android/sdk...
devil/devil/android/sdk/adb_wrapper_test.py:74: return_value = 'device'
Is this output representative of what we would see in real use?
On 2017/05/30 18:21:12, rnephew (Reviews Here) wrote: > I just tried running your patch locally, and the tests fail. You can run the > tests locally by going to 'catapult/devil/bin/' and running 'run_py_tests' > ... > "/Users/rnephew/chromium/src/third_party/catapult/devil/devil/android/sdk/adb_wrapper.py", > line 74, in _FindAdb > raise device_errors.NoAdbError() > NoAdbError: Unable to find adb. It appears to be failing to locate adb. Is adb in your path or otherwise resolvable by adb_wrapper._FindAdb()? More importantly, does run_py_tests work in your local environment when you're on the master branch? I would be very surprised if I've changed something that influences that code path. Also, I did test locally with that script before submitting. I started using it upon noticing that's what the presubmit check uses.
Pushing responses to comments... https://codereview.chromium.org/2899093002/diff/1/devil/devil/android/sdk/adb... File devil/devil/android/sdk/adb_wrapper.py (right): https://codereview.chromium.org/2899093002/diff/1/devil/devil/android/sdk/adb... devil/devil/android/sdk/adb_wrapper.py:123: device_serial: The device serial number as a string. On 2017/05/30 11:11:58, perezju wrote: > Add a note here to indicate that an USB identifier may also be provided, but > with a warning stating that this is not fully tested. > > This would also be a good place, as Randy mentioned, to leave a TODO on > improving support for that particular use case. Done. https://codereview.chromium.org/2899093002/diff/60001/devil/devil/android/sdk... File devil/devil/android/sdk/adb_wrapper.py (right): https://codereview.chromium.org/2899093002/diff/60001/devil/devil/android/sdk... devil/devil/android/sdk/adb_wrapper.py:115: On 2017/05/30 18:21:12, rnephew (Reviews Here) wrote: > Warning should be moved up here, not in the args section. I was following perezju's suggestion here: https://codereview.chromium.org/2899093002/diff/1/devil/devil/android/sdk/adb... I'll relocate... https://codereview.chromium.org/2899093002/diff/60001/devil/devil/android/sdk... devil/devil/android/sdk/adb_wrapper.py:125: # (*) WARNING: Not all devil features are supported when using a USB ID. On 2017/05/30 18:21:12, rnephew (Reviews Here) wrote: > I'm wondering if we can detect if we suspect that the user is using a USB Id and > output this warning there. Not necessarily in this CL, but just a thought. That's interesting. Detection should be straightforward because all USB ids start with the string "usb:". I took a stab at a warning. https://codereview.chromium.org/2899093002/diff/60001/devil/devil/android/sdk... File devil/devil/android/sdk/adb_wrapper_test.py (right): https://codereview.chromium.org/2899093002/diff/60001/devil/devil/android/sdk... devil/devil/android/sdk/adb_wrapper_test.py:20: def MockRunDeviceAdbCmd(adb, return_value): On 2017/05/30 18:21:12, rnephew (Reviews Here) wrote: > Any reason why this was moved to the module level from the AdbWrapperTest class? Only one: after I refactored MockRunDeviceAdbCmd() to take an AdbWrapper as an argument rather than always using self.adb, a presubmit check complained that this method should be a function because it no longer referred to self. https://codereview.chromium.org/2899093002/diff/60001/devil/devil/android/sdk... devil/devil/android/sdk/adb_wrapper_test.py:74: return_value = 'device' On 2017/05/30 18:21:12, rnephew (Reviews Here) wrote: > Is this output representative of what we would see in real use? Not sure if you're asking me, but I'll do my best to answer. In short, I believe so. The possible return values of GetState() weren't changed by this CL. I just documented a couple more that I was aware of by noting them in the comments and then added test cases for them here. These aren't very interesting tests though, which is partly because GetState() isn't very interesting in isolation.
If its because its having problems finding adb, I wonder why only the 5 new tests are having problems and the existing tests in that file are passing. Interesting. I'll poke around with my machine and see if its something config'd wrong. https://codereview.chromium.org/2899093002/diff/60001/devil/devil/android/sdk... File devil/devil/android/sdk/adb_wrapper.py (right): https://codereview.chromium.org/2899093002/diff/60001/devil/devil/android/sdk... devil/devil/android/sdk/adb_wrapper.py:115: On 2017/05/30 19:27:20, kavefish wrote: > On 2017/05/30 18:21:12, rnephew (Reviews Here) wrote: > > Warning should be moved up here, not in the args section. > > I was following perezju's suggestion here: > https://codereview.chromium.org/2899093002/diff/1/devil/devil/android/sdk/adb... > > I'll relocate... If Juan said that is the correct place, its fine there then. I have high confidence in him being correct. The TODO should probably still be moved though. https://codereview.chromium.org/2899093002/diff/60001/devil/devil/android/sdk... File devil/devil/android/sdk/adb_wrapper_test.py (right): https://codereview.chromium.org/2899093002/diff/60001/devil/devil/android/sdk... devil/devil/android/sdk/adb_wrapper_test.py:20: def MockRunDeviceAdbCmd(adb, return_value): On 2017/05/30 19:27:20, kavefish wrote: > On 2017/05/30 18:21:12, rnephew (Reviews Here) wrote: > > Any reason why this was moved to the module level from the AdbWrapperTest > class? > > Only one: after I refactored MockRunDeviceAdbCmd() to take an AdbWrapper as an > argument rather than always using self.adb, a presubmit check complained that > this method should be a function because it no longer referred to self. Ah. In the future you can also just make it a static method. https://cs.chromium.org/chromium/src/third_party/catapult/devil/devil/android... But its fine as is. Was just more curious than anything else. https://codereview.chromium.org/2899093002/diff/60001/devil/devil/android/sdk... devil/devil/android/sdk/adb_wrapper_test.py:74: return_value = 'device' On 2017/05/30 19:27:20, kavefish wrote: > On 2017/05/30 18:21:12, rnephew (Reviews Here) wrote: > > Is this output representative of what we would see in real use? > > Not sure if you're asking me, but I'll do my best to answer. > > In short, I believe so. The possible return values of GetState() weren't changed > by this CL. I just documented a couple more that I was aware of by noting them > in the comments and then added test cases for them here. > > These aren't very interesting tests though, which is partly because GetState() > isn't very interesting in isolation. I just thought that the return value from adb we would want would be the output from this line: 839 lines = self._RawDevices(timeout=timeout, retries=retries, long_list=True) Which runs adb devices -l I might be misunderstanding what adb call this is mocking out though.
On 2017/05/30 19:40:33, rnephew (Reviews Here) wrote: > If its because its having problems finding adb, I wonder why only the 5 new > tests are having problems and the existing tests in that file are passing. That is indeed very puzzling. I'll have another look after some time away from the code for a fresh perspective. > On 2017/05/30 19:27:20, kavefish wrote: > > On 2017/05/30 18:21:12, rnephew (Reviews Here) wrote: > > > Any reason why this was moved to the module level from the AdbWrapperTest > > class? > > > > Only one: after I refactored MockRunDeviceAdbCmd() to take an AdbWrapper as an > > argument rather than always using self.adb, a presubmit check complained that > > this method should be a function because it no longer referred to self. > > Ah. In the future you can also just make it a static method. > https://cs.chromium.org/chromium/src/third_party/catapult/devil/devil/android... Ah, a static method would've been prettier. Oh well, that's what I get for blindly following the output of a script. > I just thought that the return value from adb we would want would be the output > from this line: > 839 lines = self._RawDevices(timeout=timeout, retries=retries, > long_list=True) > Which runs adb devices -l > > I might be misunderstanding what adb call this is mocking out though. I doubt you're misunderstanding, though I'm not quite grokking what it is you'd like to check that's not already covered. Since these changes were isolated to the GetState method I thought the cleanest way to test was to check the possible return values of that method. The method doesn't raise any exceptions and the closest thing to an error result is when it can't find the specified device, at which point it returns "offline" . From the caller's perspective the device-not-found case is indistinguishable from the device-offline case. It's quirky, but also nice because of course it allows you to keep instances around for devices that come and go during tests.
> > I just thought that the return value from adb we would want would be the > output > > from this line: > > 839 lines = self._RawDevices(timeout=timeout, retries=retries, > > long_list=True) > > Which runs adb devices -l > > > > I might be misunderstanding what adb call this is mocking out though. > > I doubt you're misunderstanding, though I'm not quite grokking what it is you'd > like to check that's not already covered. > > Since these changes were isolated to the GetState method I thought the cleanest > way to test was to check the possible return values of that method. > > The method doesn't raise any exceptions and the closest thing to an error result > is when it can't find the specified device, at which point it returns "offline" > . From the caller's perspective the device-not-found case is indistinguishable > from the device-offline case. It's quirky, but also nice because of course it > allows you to keep instances around for devices that come and go during tests. Ah. I think this is where the misunderstanding is coming from. The 'return_value' here isn't the return value of the adb.GetState() method. It is the return value for the call to RunDeviceAdbCmd. See comment for location. This should be the output from adb devices -l (for all tests). You also need to do self.assertEqual(adb.GetState(), EXPECTED_RETURN_VALUE) in order to test that the return value from adb.GetState() is as expected.
https://codereview.chromium.org/2899093002/diff/80001/devil/devil/android/sdk... File devil/devil/android/sdk/adb_wrapper_test.py (right): https://codereview.chromium.org/2899093002/diff/80001/devil/devil/android/sdk... devil/devil/android/sdk/adb_wrapper_test.py:23: '_RunDeviceAdbCmd', This is the method that has its return value mocked out.
On 2017/05/30 20:14:58, rnephew (Reviews Here) wrote: > Ah. I think this is where the misunderstanding is coming from. The > 'return_value' here isn't the return value of the adb.GetState() method. It is > the return value for the call to RunDeviceAdbCmd. See comment for location. This > should be the output from adb devices -l (for all tests). You also need to do > self.assertEqual(adb.GetState(), EXPECTED_RETURN_VALUE) in order to test that > the return value from adb.GetState() is as expected. Thanks, I see where I went astray. I've started on a fix and also seem to have found a useful thread to pull to understand that adb error from your last test. Alas I'll have to pick it up tomorrow. Thanks again.
Description was changed from ========== [devil] Allow instantiation of AdbWrapper with USB bus identifier. Adb allows selection of a device using both a serial number and the device's USB bus identifier. This change makes it possible to create an AdbWrapper instance using a USB bus identifier in addition to a serial number. This option is required when multiple connected devices present the same serial number. ========== to ========== [devil] Allow instantiation of AdbWrapper with USB bus identifier. Adb allows selection of a device using both a serial number and the device's USB bus identifier. This change makes it possible to create an AdbWrapper instance using a USB bus identifier in addition to a serial number. This option is required when multiple connected devices present the same serial number. catapult:#3595 ==========
Description was changed from ========== [devil] Allow instantiation of AdbWrapper with USB bus identifier. Adb allows selection of a device using both a serial number and the device's USB bus identifier. This change makes it possible to create an AdbWrapper instance using a USB bus identifier in addition to a serial number. This option is required when multiple connected devices present the same serial number. catapult:#3595 ========== to ========== [devil] Allow instantiation of AdbWrapper with USB bus identifier. Adb allows selection of a device using both a serial number and the device's USB bus identifier. This change makes it possible to create an AdbWrapper instance using a USB bus identifier in addition to a serial number. This option is required when multiple connected devices present the same serial number. Bug: catapult:#3595 ==========
On 2017/05/31 19:01:38, kavefish wrote: > On 2017/05/30 20:14:58, rnephew (Reviews Here) wrote: > > Ah. I think this is where the misunderstanding is coming from. The > > 'return_value' here isn't the return value of the adb.GetState() method. It is > > the return value for the call to RunDeviceAdbCmd. See comment for location. > This > > should be the output from adb devices -l (for all tests). You also need to do > > self.assertEqual(adb.GetState(), EXPECTED_RETURN_VALUE) in order to test that > > the return value from adb.GetState() is as expected. > > Thanks, I see where I went astray. I've started on a fix and also seem to have > found a useful thread to pull to understand that adb error from your last test. > > Alas I'll have to pick it up tomorrow. Thanks again. OK, so it wasn't quite "tomorrow" but please have a look at the latest patch set. In summary: - fixed the tests to mock up the correct adb command to exercise GetState - switched to a static method to reduce the size of the changes - pylint cleanups The new GetState tests feel a little brittle, particularly the part where each AdbWrapper's serial is injected into the mock output, but they work in my tests and perhaps that feeling comes more from the dirty business of parsing error strings. I would've liked to add a "bootloader" test case for GetState, but I couldn't get adb to recognise my local device in that state, so I opted to omit that case in lieu of guessing at the corresponding mock output. If you've got output then I'll add a test for that case, too. Cheers
Thanks for the changes! These are looking good. I've just added a few nits and comments/suggestions on the tests. In short: I think the new thing being tested is mainly the small change needed to parse the correct device status; for most cases, it's probably not needed to make each test for both device serial and USB id, only test for the later where it's really relevant. https://codereview.chromium.org/2899093002/diff/100001/devil/devil/android/sd... File devil/devil/android/sdk/adb_wrapper.py (right): https://codereview.chromium.org/2899093002/diff/100001/devil/devil/android/sd... devil/devil/android/sdk/adb_wrapper.py:116: WARNING: Not all features are supported when using a USB ID. nit: move this warning message to the constructor's docstring, after the Args section where you describe that a USB ID may be used to build the object. https://codereview.chromium.org/2899093002/diff/100001/devil/devil/android/sd... devil/devil/android/sdk/adb_wrapper.py:133: logger.warning("Not all features are supported when using a USB ID.") nit: are supported -> may be supported https://codereview.chromium.org/2899093002/diff/100001/devil/devil/android/sd... devil/devil/android/sdk/adb_wrapper.py:836: or 'device' nit: missing period at end of sentence. https://codereview.chromium.org/2899093002/diff/100001/devil/devil/android/sd... devil/devil/android/sdk/adb_wrapper.py:845: self._device_serial): nit: indentation seems off here, have a look at: https://google.github.io/styleguide/pyguide.html?showone=Indentation#Indentation https://google.github.io/styleguide/pyguide.html?showone=Line_length#Line_length https://codereview.chromium.org/2899093002/diff/100001/devil/devil/android/sd... File devil/devil/android/sdk/adb_wrapper_test.py (right): https://codereview.chromium.org/2899093002/diff/100001/devil/devil/android/sd... devil/devil/android/sdk/adb_wrapper_test.py:41: def testDisableVerityWhenDisabled(self): For most of these tests (particularly because we're only doing mocks), if the code paths tested do not depend on whether the adb object was built using a device serial or USB id, I think it's fine to leave them unchanged. https://codereview.chromium.org/2899093002/diff/100001/devil/devil/android/sd... devil/devil/android/sdk/adb_wrapper_test.py:81: def testGetStateEmpty(self): This and the tests below are good. Thanks for adding them! https://codereview.chromium.org/2899093002/diff/100001/devil/devil/android/sd... devil/devil/android/sdk/adb_wrapper_test.py:92: 'usb:1-2.3'], []] could you please mock at the _RunAdbCmd level (rather than _RawDevices), so that your mock_return_value's are essentially copy-pasted from the output of a real adb?
Hey there, I could be misunderstanding, but it appears to me that mocking at the _RunDeviceAdvCmd() level would not have the desired effect because GetState() uses _RawDevices and never calls _RunDeviceAdvCmd(). https://codereview.chromium.org/2899093002/diff/100001/devil/devil/android/sd... File devil/devil/android/sdk/adb_wrapper_test.py (right): https://codereview.chromium.org/2899093002/diff/100001/devil/devil/android/sd... devil/devil/android/sdk/adb_wrapper_test.py:92: 'usb:1-2.3'], []] On 2017/06/12 03:26:24, perezju wrote: > could you please mock at the _RunAdbCmd level (rather than _RawDevices), so that > your mock_return_value's are essentially copy-pasted from the output of a real adb? I opted to mock _RawDevices() because GetState() doesn't call _RunDeviceAdbCmd(). Am I missing something?
https://codereview.chromium.org/2899093002/diff/100001/devil/devil/android/sd... File devil/devil/android/sdk/adb_wrapper_test.py (right): https://codereview.chromium.org/2899093002/diff/100001/devil/devil/android/sd... devil/devil/android/sdk/adb_wrapper_test.py:92: 'usb:1-2.3'], []] On 2017/06/18 10:59:01, kavefish wrote: > On 2017/06/12 03:26:24, perezju wrote: > > could you please mock at the _RunAdbCmd level (rather than _RawDevices), so > that > > your mock_return_value's are essentially copy-pasted from the output of a real > adb? > > I opted to mock _RawDevices() because GetState() doesn't call > _RunDeviceAdbCmd(). Am I missing something? _RawDevices() calls _RunAdbCommand, thats what I would mock in a similar way to how RunDeviceAdbCmd() is mocked out: https://cs.chromium.org/chromium/src/third_party/catapult/devil/devil/android...
Patchset #1 (id:1) has been deleted
On 2017/06/19 15:30:03, rnephew (Reviews Here) wrote: > https://codereview.chromium.org/2899093002/diff/100001/devil/devil/android/sd... > File devil/devil/android/sdk/adb_wrapper_test.py (right): > > https://codereview.chromium.org/2899093002/diff/100001/devil/devil/android/sd... > devil/devil/android/sdk/adb_wrapper_test.py:92: 'usb:1-2.3'], []] > On 2017/06/18 10:59:01, kavefish wrote: > > On 2017/06/12 03:26:24, perezju wrote: > > > could you please mock at the _RunAdbCmd level (rather than _RawDevices), so > > that > > > your mock_return_value's are essentially copy-pasted from the output of a > real > > adb? > > > > I opted to mock _RawDevices() because GetState() doesn't call > > _RunDeviceAdbCmd(). Am I missing something? > > _RawDevices() calls _RunAdbCommand, thats what I would mock in a similar way to > how RunDeviceAdbCmd() is mocked out: > https://cs.chromium.org/chromium/src/third_party/catapult/devil/devil/android... Thanks for the pointer. I believe all the requested adjustments are now complete.
https://codereview.chromium.org/2899093002/diff/120001/devil/devil/android/sd... File devil/devil/android/sdk/adb_wrapper_test.py (right): https://codereview.chromium.org/2899093002/diff/120001/devil/devil/android/sd... devil/devil/android/sdk/adb_wrapper_test.py:46: 'for settings to take effect' nit: bad indent; try something like: mock_return_value = ( 'Verity disabled on /system\nNow reboot your device' 'for settings to take effect') https://codereview.chromium.org/2899093002/diff/120001/devil/devil/android/sd... devil/devil/android/sdk/adb_wrapper_test.py:57: 'for settings to take effect' nit: bad indent. https://codereview.chromium.org/2899093002/diff/120001/devil/devil/android/sd... devil/devil/android/sdk/adb_wrapper_test.py:82: mock_return_value = 'List of devices attached\n' \ nit: break long string using parenthesis, so you don't need to add the \ at the end of each of line. https://codereview.chromium.org/2899093002/diff/120001/devil/devil/android/sd... devil/devil/android/sdk/adb_wrapper_test.py:83: 'PLACEHOLDER no permissions (verify udev ' \ On the real adb output the PLACEHOLDER will always be a device serial and not an usb id. Right? Why not write directly "ABC12345678" and avoid doing the extra replace? |
