Repository: google/mobly

Issue:
`AdbError` should have a serial number attr
Our `AdbError` does not include an easily accessible serial number.

When handling an `AdbError`, it is sometimes useful to know which device the command was issued to, especially when you cannot change the code around the adb call directly and have to handle at an upper level.

Is this something worth adding?

Discussion:

Patch:
diff --git a/mobly/controllers/android_device_lib/adb.py b/mobly/controllers/android_device_lib/adb.py
index 41af20c..5e0c36f 100644
--- a/mobly/controllers/android_device_lib/adb.py
+++ b/mobly/controllers/android_device_lib/adb.py
@@ -44,13 +44,17 @@ class AdbError(Error):
         stdout: byte string, the raw stdout of the command.
         stderr: byte string, the raw stderr of the command.
         ret_code: int, the return code of the command.
+        serial: string, the serial of the device the command is executed on.
+            This is an empty string if the adb command is not specific to a
+            device.
     """
 
-    def __init__(self, cmd, stdout, stderr, ret_code):
+    def __init__(self, cmd, stdout, stderr, ret_code, serial=None):
         self.cmd = cmd
         self.stdout = stdout
         self.stderr = stderr
         self.ret_code = ret_code
+        self.serial = serial
 
     def __str__(self):
         return ('Error executing adb cmd "%s". ret: %d, stdout: %s, stderr: %s'
@@ -179,7 +183,12 @@ class AdbProxy(object):
         if ret == 0:
             return out
         else:
-            raise AdbError(cmd=args, stdout=out, stderr=err, ret_code=ret)
+            raise AdbError(
+                cmd=args,
+                stdout=out,
+                stderr=err,
+                ret_code=ret,
+                serial=self.serial)
 
     def _execute_and_process_stdout(self, args, shell, handler):
         """Executes adb commands and processes the stdout with a handler.
diff --git a/mobly/utils.py b/mobly/utils.py
index a9f065c..bb2f316 100644
--- a/mobly/utils.py
+++ b/mobly/utils.py
@@ -283,7 +283,7 @@ def concurrent_exec(func, param_list):
         return return_vals
 
 
-def start_standing_subprocess(cmd, shell=False):
+def start_standing_subprocess(cmd, shell=False, env=None):
     """Starts a long-running subprocess.
 
     This is not a blocking call and the subprocess started by it should be
@@ -296,6 +296,9 @@ def start_standing_subprocess(cmd, shell=False):
         cmd: string, the command to start the subprocess with.
         shell: bool, True to run this command through the system shell,
             False to invoke it directly. See subprocess.Proc() docs.
+        env: dict, a custom environment to run the standing subprocess. If not
+            specified, inherits the current environment. See subprocess.Popen()
+            docs.
 
     Returns:
         The subprocess that was started.
@@ -306,7 +309,8 @@ def start_standing_subprocess(cmd, shell=False):
         stdin=subprocess.PIPE,
         stdout=subprocess.PIPE,
         stderr=subprocess.PIPE,
-        shell=shell)
+        shell=shell,
+        env=env)
     # Leaving stdin open causes problems for input, e.g. breaking the
     # code.inspect() shell (http://stackoverflow.com/a/25512460/1612937), so
     # explicitly close it assuming it is not needed for standing subprocesses.


Test patch:
diff --git a/tests/mobly/controllers/android_device_lib/adb_test.py b/tests/mobly/controllers/android_device_lib/adb_test.py
index 71fab9c..63ae285 100755
--- a/tests/mobly/controllers/android_device_lib/adb_test.py
+++ b/tests/mobly/controllers/android_device_lib/adb_test.py
@@ -91,15 +91,29 @@ class AdbTest(unittest.TestCase):
 
     @mock.patch('mobly.controllers.android_device_lib.adb.subprocess.Popen')
     @mock.patch('mobly.controllers.android_device_lib.adb.psutil.Process')
-    def test_exec_cmd_error_no_timeout(self, mock_psutil_process, mock_popen):
+    def test_exec_cmd_error_with_serial(self, mock_psutil_process, mock_popen):
         self._mock_process(mock_psutil_process, mock_popen)
         # update return code to indicate command execution error
         mock_popen.return_value.returncode = 1
+        mock_serial = 'ABCD1234'
+        with self.assertRaisesRegex(adb.AdbError,
+                                    'Error executing adb cmd .*') as context:
+            adb.AdbProxy(mock_serial)._exec_cmd(
+                ['fake_cmd'], shell=False, timeout=None, stderr=None)
+        self.assertEqual(context.exception.serial, mock_serial)
 
+    @mock.patch('mobly.controllers.android_device_lib.adb.subprocess.Popen')
+    @mock.patch('mobly.controllers.android_device_lib.adb.psutil.Process')
+    def test_exec_cmd_error_without_serial(self, mock_psutil_process,
+                                           mock_popen):
+        self._mock_process(mock_psutil_process, mock_popen)
+        # update return code to indicate command execution error
+        mock_popen.return_value.returncode = 1
         with self.assertRaisesRegex(adb.AdbError,
-                                    'Error executing adb cmd .*'):
+                                    'Error executing adb cmd .*') as context:
             adb.AdbProxy()._exec_cmd(
                 ['fake_cmd'], shell=False, timeout=None, stderr=None)
+        self.assertFalse(context.exception.serial)
 
     @mock.patch('mobly.controllers.android_device_lib.adb.subprocess.Popen')
     @mock.patch('mobly.controllers.android_device_lib.adb.psutil.Process')
@@ -156,8 +170,8 @@ class AdbTest(unittest.TestCase):
         self._mock_execute_and_process_stdout_process(mock_popen)
         mock_handler = mock.MagicMock()
         mock_popen.return_value.communicate = mock.Mock(
-            return_value=(unexpected_stdout, MOCK_DEFAULT_STDERR.encode(
-                'utf-8')))
+            return_value=(unexpected_stdout,
+                          MOCK_DEFAULT_STDERR.encode('utf-8')))
 
         err = adb.AdbProxy()._execute_and_process_stdout(
             ['fake_cmd'], shell=False, handler=mock_handler)
diff --git a/tests/mobly/utils_test.py b/tests/mobly/utils_test.py
index 605dc8c..b9b0c22 100755
--- a/tests/mobly/utils_test.py
+++ b/tests/mobly/utils_test.py
@@ -53,6 +53,31 @@ class UtilsTest(unittest.TestCase):
             p.stderr.close()
             p.wait()
 
+    @mock.patch('subprocess.Popen')
+    def test_start_standing_subproc_without_env(self, mock_Popen):
+        p = utils.start_standing_subprocess(self.sleep_cmd)
+        mock_Popen.assert_called_with(
+            self.sleep_cmd,
+            stdin=subprocess.PIPE,
+            stdout=subprocess.PIPE,
+            stderr=subprocess.PIPE,
+            shell=False,
+            env=None,
+        )
+
+    @mock.patch('subprocess.Popen')
+    def test_start_standing_subproc_with_custom_env(self, mock_Popen):
+        mock_env = mock.MagicMock(spec=dict)
+        p = utils.start_standing_subprocess(self.sleep_cmd, env=mock_env)
+        mock_Popen.assert_called_with(
+            self.sleep_cmd,
+            stdin=subprocess.PIPE,
+            stdout=subprocess.PIPE,
+            stderr=subprocess.PIPE,
+            shell=False,
+            env=mock_env,
+        )
+
     def test_stop_standing_subproc(self):
         p = utils.start_standing_subprocess([self.sleep_cmd, '4'])
         p1 = psutil.Process(p.pid)