New run shell implementation for DeviceUtils

build/android/pylib/device/device_utils_test.py

Index: build/android/pylib/device/device_utils_test.py
diff --git a/build/android/pylib/device/device_utils_test.py b/build/android/pylib/device/device_utils_test.py
index 52aef0ac3f7eba3d2cca87788efd579ad28913e8..4aa9ab14dea84c73218ab801c6e63c106b6df8f5 100755
--- a/build/android/pylib/device/device_utils_test.py
+++ b/build/android/pylib/device/device_utils_test.py
@@ -288,19 +288,18 @@ class DeviceUtilsEnableRootTest(DeviceUtilsOldImplTest):
         self.device.EnableRoot()
 
 
-class DeviceUtilsGetExternalStoragePathTest(DeviceUtilsOldImplTest):
+class DeviceUtilsGetExternalStoragePathTest(DeviceUtilsNewImplTest):
 
   def testGetExternalStoragePath_succeeds(self):
     fakeStoragePath = '/fake/storage/path'
-    with self.assertCalls(
-        "adb -s 0123456789abcdef shell 'echo $EXTERNAL_STORAGE'",
-        '%s\r\n' % fakeStoragePath):
+    with mock.patch.object(self.device, '_NewRunShellImpl',
+                           return_value='%s\r\n' % fakeStoragePath):
       self.assertEquals(fakeStoragePath,
                         self.device.GetExternalStoragePath())
 
   def testGetExternalStoragePath_fails(self):
-    with self.assertCalls(
-        "adb -s 0123456789abcdef shell 'echo $EXTERNAL_STORAGE'", '\r\n'):
+    with mock.patch.object(self.device, '_NewRunShellImpl',

[jbudorick, 2014/10/14 16:55:19]

I'm not sure I like mocking here vs mocking between DeviceUtils and AdbWrapper -
in this case, mocking AdbWrapper.Shell.

+                           return_value='\r\n'):
       with self.assertRaises(device_errors.CommandFailedError):
         self.device.GetExternalStoragePath()
 
@@ -570,6 +569,55 @@ class DeviceUtilsRunShellCommandTest(DeviceUtilsOldImplTest):
         self.device.RunShellCommand('echo $ANDROID_DATA', check_return=True)
 
 
+class DeviceUtilsNewRunShellImplTest(DeviceUtilsNewImplTest):
+  def assert_called_shell_with(self, cmd):
+    self.adb.Shell.assert_called_once_with(cmd, expect_rc=0,
+      timeout=device_utils._DEFAULT_TIMEOUT,
+      retries=device_utils._DEFAULT_RETRIES)
+
+  def testNewRunShellImpl_simple(self):
+    self.device._NewRunShellImpl('echo hello')
+    self.assert_called_shell_with('echo hello')
+
+  def testNewRunShellImpl_withlist(self):
+    self.device._NewRunShellImpl(['echo', 'hello'])
+    self.assert_called_shell_with('echo hello')
+
+  def testNewRunShellImpl_withfancylist(self):
+    self.device._NewRunShellImpl(['echo', 'hello world'])
+    self.assert_called_shell_with("echo 'hello world'")
+
+  def testNewRunShellImpl_withenv(self):
+    self.device._NewRunShellImpl('echo "$VAR"', env={'VAR': 'some_string'})
+    self.assert_called_shell_with('VAR=some_string echo "$VAR"')
+
+  def testNewRunShellImpl_withfancyenv(self):
+    self.device._NewRunShellImpl('echo "$VAR"', env={'VAR': 'hello world'})
+    self.assert_called_shell_with('''VAR='hello world' echo "$VAR"''')
+
+  def testNewRunShellImpl_withenv_invalid(self):
+    with self.assertRaises(KeyError):
+      self.device._NewRunShellImpl('some_cmd', env={'INVALID NAME': 'value'})
+
+  def testNewRunShellImpl_withcwd(self):
+    self.device._NewRunShellImpl('ls', cwd='/some/test/path')
+    self.assert_called_shell_with('cd /some/test/path && ls')
+
+  def testNewRunShellImpl_withfancycwd(self):
+    self.device._NewRunShellImpl('ls', cwd='/some test/path with/spaces')
+    self.assert_called_shell_with("cd '/some test/path with/spaces' && ls")
+
+  def testNewRunShellImpl_asroot_withoutroot(self):
+    with mock.patch.object(self.device, '_HasRootImpl', return_value=False):
+      self.device._NewRunShellImpl('ls /path/to/protected', as_root=True)
+      self.assert_called_shell_with('su -c ls /path/to/protected')
+
+  def testNewRunShellImpl_asroot_withroot(self):
+    with mock.patch.object(self.device, '_HasRootImpl', return_value=True):
+      self.device._NewRunShellImpl('ls /path/to/protected', as_root=True)
+      self.assert_called_shell_with('ls /path/to/protected')
+
+
 class DeviceUtilsKillAllTest(DeviceUtilsOldImplTest):
 
   def testKillAll_noMatchingProcesses(self):
@@ -911,7 +959,7 @@ class DeviceUtilsPushChangedFilesZippedTest(DeviceUtilsHybridImplTest):
   def testPushChangedFilesZipped_single(self):
     test_files = [('/test/host/path/file1', '/test/device/path/file1')]
 
-    self.device._GetExternalStoragePathImpl = mock.Mock(
+    self.device.GetExternalStoragePath = mock.Mock(
         return_value='/test/device/external_dir')
     self.device._IsOnlineImpl = mock.Mock(return_value=True)
     self.device._RunShellCommandImpl = mock.Mock()
@@ -938,7 +986,7 @@ class DeviceUtilsPushChangedFilesZippedTest(DeviceUtilsHybridImplTest):
     test_files = [('/test/host/path/file1', '/test/device/path/file1'),
                   ('/test/host/path/file2', '/test/device/path/file2')]
 
-    self.device._GetExternalStoragePathImpl = mock.Mock(
+    self.device.GetExternalStoragePath = mock.Mock(
         return_value='/test/device/external_dir')
     self.device._IsOnlineImpl = mock.Mock(return_value=True)
     self.device._RunShellCommandImpl = mock.Mock()