[Android] Switch to DeviceUtils versions of Ls, SetJavaAsserts, GetProp, and SetProp.

[Android] Switch to DeviceUtils versions of Ls, SetJavaAsserts, GetProp, and SetProp.

BUG=267773
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=281794

[tonyg, 2014-07-07 15:22:56]

https://codereview.chromium.org/371813005/diff/20001/build/android/pylib/devi...
File build/android/pylib/device/device_utils.py (right):

https://codereview.chromium.org/371813005/diff/20001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:589: return
self.old_interface.system_properties[property_name]
Really up to you, but I thought it'd be more pythonic to expose the
system_properties dict directly and let users use it like a dict.

https://codereview.chromium.org/371813005/diff/20001/build/android/pylib/devi...
File build/android/pylib/device/device_utils_test.py (right):

https://codereview.chromium.org/371813005/diff/20001/build/android/pylib/devi...
build/android/pylib/device/device_utils_test.py:1306: def
testGetProp_exists(self):
The system_properties interface caches certain ro properties so we don't have to
go to the device every time to get them. Can we add a unittest that verifies the
caching is working by requesting the same ro property twice?

https://codereview.chromium.org/371813005/diff/20001/tools/telemetry/telemetr...
File
tools/telemetry/telemetry/core/platform/android_platform_backend_unittest.py
(right):

https://codereview.chromium.org/371813005/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/android_platform_backend_unittest.py:15:
self.system_properties = system_properties
if this still works, it seems brittle.

[jbudorick, 2014-07-07 16:12:45]

https://codereview.chromium.org/371813005/diff/20001/build/android/pylib/devi...
File build/android/pylib/device/device_utils.py (right):

https://codereview.chromium.org/371813005/diff/20001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:589: return
self.old_interface.system_properties[property_name]
On 2014/07/07 15:22:56, tonyg wrote:
> Really up to you, but I thought it'd be more pythonic to expose the
> system_properties dict directly and let users use it like a dict.

I think I prefer GetProp / SetProp over the dict interface for two reasons:
 1) they mirror the actual adb shell commands
 2) the dict interface seems likely to imply to the user that we aren't hitting
the device

That said, I'm interested in hearing other opinions on this.

https://codereview.chromium.org/371813005/diff/20001/build/android/pylib/devi...
File build/android/pylib/device/device_utils_test.py (right):

https://codereview.chromium.org/371813005/diff/20001/build/android/pylib/devi...
build/android/pylib/device/device_utils_test.py:1306: def
testGetProp_exists(self):
On 2014/07/07 15:22:56, tonyg wrote:
> The system_properties interface caches certain ro properties so we don't have
to
> go to the device every time to get them. Can we add a unittest that verifies
the
> caching is working by requesting the same ro property twice?

Done.

https://codereview.chromium.org/371813005/diff/20001/tools/telemetry/telemetr...
File
tools/telemetry/telemetry/core/platform/android_platform_backend_unittest.py
(right):

https://codereview.chromium.org/371813005/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/android_platform_backend_unittest.py:15:
self.system_properties = system_properties
On 2014/07/07 15:22:56, tonyg wrote:
> if this still works, it seems brittle.

It does. I had been planning to clean up this test case in a future CL, but I'd
be happy to do it now if you prefer.

[frankf, 2014-07-07 17:29:03]

https://codereview.chromium.org/371813005/diff/20001/build/android/pylib/devi...
File build/android/pylib/device/device_utils.py (right):

https://codereview.chromium.org/371813005/diff/20001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:589: return
self.old_interface.system_properties[property_name]
I agree with John, explicit setter/getter in this case seems more appropriate.

On 2014/07/07 16:12:45, jbudorick wrote:
> On 2014/07/07 15:22:56, tonyg wrote:
> > Really up to you, but I thought it'd be more pythonic to expose the
> > system_properties dict directly and let users use it like a dict.
> 
> I think I prefer GetProp / SetProp over the dict interface for two reasons:
>  1) they mirror the actual adb shell commands
>  2) the dict interface seems likely to imply to the user that we aren't
hitting
> the device
> 
> That said, I'm interested in hearing other opinions on this.

https://codereview.chromium.org/371813005/diff/40001/build/android/pylib/devi...
File build/android/pylib/device/device_utils.py (right):

https://codereview.chromium.org/371813005/diff/40001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:556: as_root: A boolean inidcating
whether the ls should be executed with root
where's this used?

[jbudorick, 2014-07-07 17:43:16]

https://codereview.chromium.org/371813005/diff/40001/build/android/pylib/devi...
File build/android/pylib/device/device_utils.py (right):

https://codereview.chromium.org/371813005/diff/40001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:556: as_root: A boolean inidcating
whether the ls should be executed with root
On 2014/07/07 17:29:03, frankf wrote:
> where's this used?

It currently isn't used by any caller, and obviously it isn't currently used in
the implementation.

Given that there are some directories on devices that require root permissions
to list, I think it's an option worth providing, and I'm planning to use it in
the final implementation.

[frankf, 2014-07-07 18:00:52]

https://codereview.chromium.org/371813005/diff/40001/build/android/pylib/devi...
File build/android/pylib/device/device_utils.py (right):

https://codereview.chromium.org/371813005/diff/40001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:556: as_root: A boolean inidcating
whether the ls should be executed with root
Just leave it out for now then. You can always add it back.

On 2014/07/07 17:43:15, jbudorick wrote:
> On 2014/07/07 17:29:03, frankf wrote:
> > where's this used?
> 
> It currently isn't used by any caller, and obviously it isn't currently used
in
> the implementation.
> 
> Given that there are some directories on devices that require root permissions
> to list, I think it's an option worth providing, and I'm planning to use it in
> the final implementation.

[jbudorick, 2014-07-07 18:13:49]

https://codereview.chromium.org/371813005/diff/40001/build/android/pylib/devi...
File build/android/pylib/device/device_utils.py (right):

https://codereview.chromium.org/371813005/diff/40001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:556: as_root: A boolean inidcating
whether the ls should be executed with root
On 2014/07/07 18:00:52, frankf wrote:
> Just leave it out for now then. You can always add it back.
> 
> On 2014/07/07 17:43:15, jbudorick wrote:
> > On 2014/07/07 17:29:03, frankf wrote:
> > > where's this used?
> > 
> > It currently isn't used by any caller, and obviously it isn't currently used
> in
> > the implementation.
> > 
> > Given that there are some directories on devices that require root
permissions
> > to list, I think it's an option worth providing, and I'm planning to use it
in
> > the final implementation.
> 

Done.

[frankf, 2014-07-07 21:36:54]

lgtm after fix

https://codereview.chromium.org/371813005/diff/60001/build/android/pylib/devi...
File build/android/pylib/device/device_utils_test.py (right):

https://codereview.chromium.org/371813005/diff/60001/build/android/pylib/devi...
build/android/pylib/device/device_utils_test.py:1237:
mock_file.__enter__.return_value = mock_file
you can factor all these boilerplate into a method. Alternatively, each method
in device_utils.py can have its own test class.

[tonyg, 2014-07-07 22:14:59]

lgtm

[jbudorick, 2014-07-07 22:48:46]

On 2014/07/07 21:36:54, frankf wrote:
> lgtm after fix
> 
>
https://codereview.chromium.org/371813005/diff/60001/build/android/pylib/devi...
> File build/android/pylib/device/device_utils_test.py (right):
> 
>
https://codereview.chromium.org/371813005/diff/60001/build/android/pylib/devi...
> build/android/pylib/device/device_utils_test.py:1237:
> mock_file.__enter__.return_value = mock_file
> you can factor all these boilerplate into a method. Alternatively, each method
> in device_utils.py can have its own test class.

I did the former for now. I think that the latter is the better solution in the
long term, though. I've added a TODO for it and will address it in a separate CL
along with running the tests in presubmit.

[jbudorick, 2014-07-07 23:42:11]

The CQ bit was checked by jbudorick@chromium.org

[commit-bot: I haz the power, 2014-07-07 23:43:47]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jbudorick@chromium.org/371813005/100001

[commit-bot: I haz the power, 2014-07-08 03:12:57]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/9...)

[commit-bot: I haz the power, 2014-07-08 03:38:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-08 03:38:24]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/9...)

[jbudorick, 2014-07-08 16:03:58]

The CQ bit was checked by jbudorick@chromium.org

[commit-bot: I haz the power, 2014-07-08 16:04:14]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jbudorick@chromium.org/371813005/100001

[commit-bot: I haz the power, 2014-07-08 19:30:48]

Change committed as 281794