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 5e0c36f..15e2a5c 100644
--- a/mobly/controllers/android_device_lib/adb.py
+++ b/mobly/controllers/android_device_lib/adb.py
@@ -49,7 +49,7 @@ class AdbError(Error):
             device.
     """
 
-    def __init__(self, cmd, stdout, stderr, ret_code, serial=None):
+    def __init__(self, cmd, stdout, stderr, ret_code, serial=''):
         self.cmd = cmd
         self.stdout = stdout
         self.stderr = stderr
@@ -68,11 +68,15 @@ class AdbTimeoutError(Error):
     Args:
         cmd: list of strings, the adb command that timed out
         timeout: float, the number of seconds passed before timing out.
+        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, timeout):
+    def __init__(self, cmd, timeout, serial=''):
         self.cmd = cmd
         self.timeout = timeout
+        self.serial = serial
 
     def __str__(self):
         return 'Timed out executing command "%s" after %ss.' % (
@@ -159,6 +163,7 @@ class AdbProxy(object):
             The output of the adb command run if exit code is 0.
 
         Raises:
+            ValueError: timeout value is invalid.
             AdbError: The adb command exit code is not 0.
             AdbTimeoutError: The adb command timed out.
         """
@@ -166,13 +171,14 @@ class AdbProxy(object):
             args, stdout=subprocess.PIPE, stderr=subprocess.PIPE, shell=shell)
         process = psutil.Process(proc.pid)
         if timeout and timeout <= 0:
-            raise Error('Timeout is not a positive value: %s' % timeout)
+            raise ValueError('Timeout is not a positive value: %s' % timeout)
         if timeout and timeout > 0:
             try:
                 process.wait(timeout=timeout)
             except psutil.TimeoutExpired:
                 process.terminate()
-                raise AdbTimeoutError(cmd=args, timeout=timeout)
+                raise AdbTimeoutError(
+                    cmd=args, timeout=timeout, serial=self.serial)
 
         (out, err) = proc.communicate()
         if stderr:


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 63ae285..9430795 100755
--- a/tests/mobly/controllers/android_device_lib/adb_test.py
+++ b/tests/mobly/controllers/android_device_lib/adb_test.py
@@ -98,9 +98,9 @@ class AdbTest(unittest.TestCase):
         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)
+            adb.AdbProxy(mock_serial).fake_cmd()
         self.assertEqual(context.exception.serial, mock_serial)
+        self.assertIn(mock_serial, context.exception.cmd)
 
     @mock.patch('mobly.controllers.android_device_lib.adb.subprocess.Popen')
     @mock.patch('mobly.controllers.android_device_lib.adb.psutil.Process')
@@ -129,23 +129,34 @@ class AdbTest(unittest.TestCase):
     @mock.patch('mobly.controllers.android_device_lib.adb.psutil.Process')
     def test_exec_cmd_timed_out(self, mock_psutil_process, mock_popen):
         self._mock_process(mock_psutil_process, mock_popen)
-        # mock process.wait(timeout=timeout) to
-        # throw psutil.TimeoutExpired exception
         mock_psutil_process.return_value.wait.side_effect = (
             adb.psutil.TimeoutExpired('Timed out'))
+        mock_serial = '1234Abcd'
+        with self.assertRaisesRegex(
+                adb.AdbTimeoutError, 'Timed out executing command "adb -s '
+                '1234Abcd fake-cmd" after 0.01s.') as context:
+            adb.AdbProxy(mock_serial).fake_cmd(timeout=0.01)
+        self.assertEqual(context.exception.serial, mock_serial)
+        self.assertIn(mock_serial, context.exception.cmd)
 
+    @mock.patch('mobly.controllers.android_device_lib.adb.subprocess.Popen')
+    @mock.patch('mobly.controllers.android_device_lib.adb.psutil.Process')
+    def test_exec_cmd_timed_out_without_serial(self, mock_psutil_process,
+                                               mock_popen):
+        self._mock_process(mock_psutil_process, mock_popen)
+        mock_psutil_process.return_value.wait.side_effect = (
+            adb.psutil.TimeoutExpired('Timed out'))
         with self.assertRaisesRegex(adb.AdbTimeoutError,
-                                    'Timed out executing command "fake_cmd" '
-                                    'after 0.1s.'):
-            adb.AdbProxy()._exec_cmd(
-                ['fake_cmd'], shell=False, timeout=0.1, stderr=None)
+                                    'Timed out executing command "adb '
+                                    'fake-cmd" after 0.01s.') as context:
+            adb.AdbProxy().fake_cmd(timeout=0.01)
 
     @mock.patch('mobly.controllers.android_device_lib.adb.subprocess.Popen')
     @mock.patch('mobly.controllers.android_device_lib.adb.psutil.Process')
     def test_exec_cmd_with_negative_timeout_value(self, mock_psutil_process,
                                                   mock_popen):
         self._mock_process(mock_psutil_process, mock_popen)
-        with self.assertRaisesRegex(adb.Error,
+        with self.assertRaisesRegex(ValueError,
                                     'Timeout is not a positive value: -1'):
             adb.AdbProxy()._exec_cmd(
                 ['fake_cmd'], shell=False, timeout=-1, stderr=None)