Repository: google/mobly

Issue:
controllers/android_device.py throws an exception on android configuration with just string
Seems like an earlier checkin to switch to using base64 string to load configuration might have introduced a bug in mobly/controllers/android_device.create(configs).  I was hitting an issue where I specified the android device serial in string format in my configuration file but was failing the isinstance check with str, so instead it tries to interpret my android config as a dict format and fails.  Changing the isinstance check with basestring instead of str seems to have solved the problem.

Here is the snippet of the exception thrown.
mobly/controllers/android_device.py", line 81, in create
mobly/controllers/android_device.py", line 219, in get_instances_with_configs
MoblyTest [ln]     serial = c.pop('serial')
MoblyTest [ln] AttributeError: 'str' object has no attribute 'pop'

Discussion:

Patch:
diff --git a/mobly/controllers/android_device.py b/mobly/controllers/android_device.py
index 79d3f16..5675e34 100644
--- a/mobly/controllers/android_device.py
+++ b/mobly/controllers/android_device.py
@@ -16,6 +16,7 @@
 
 from builtins import str
 from builtins import open
+from past.builtins import basestring
 
 import contextlib
 import logging
@@ -78,17 +79,19 @@ def create(configs):
         ads = get_all_instances()
     elif not isinstance(configs, list):
         raise Error(ANDROID_DEVICE_NOT_LIST_CONFIG_MSG)
-    elif isinstance(configs[0], str):
-        # Configs is a list of serials.
-        ads = get_instances(configs)
-    else:
+    elif isinstance(configs[0], dict):
         # Configs is a list of dicts.
         ads = get_instances_with_configs(configs)
+    elif isinstance(configs[0], basestring):
+        # Configs is a list of strings representing serials.
+        ads = get_instances(configs)
+    else:
+        raise Error("No valid config found in: %s" % configs)
     connected_ads = list_adb_devices()
 
     for ad in ads:
         if ad.serial not in connected_ads:
-            raise DeviceError(ad, 'Android device is specified in config but '
+            raise DeviceError(ad, 'Android device is specified in config but'
                               ' is not attached.')
     _start_services_on_ads(ads)
     return ads
diff --git a/mobly/controllers/android_device_lib/jsonrpc_client_base.py b/mobly/controllers/android_device_lib/jsonrpc_client_base.py
index 187ec1c..08237fb 100644
--- a/mobly/controllers/android_device_lib/jsonrpc_client_base.py
+++ b/mobly/controllers/android_device_lib/jsonrpc_client_base.py
@@ -188,7 +188,7 @@ class JsonRpcClientBase(object):
         for _ in range(wait_time):
             time.sleep(1)
             if self._is_app_running():
-                self.log.debug('Successfully started %s', self.app_name)
+                self._log.debug('Successfully started %s', self.app_name)
                 return
         raise AppStartError('%s failed to start on %s.' %
                             (self.app_name, self._adb.serial))


Test patch:
diff --git a/tests/lib/mock_android_device.py b/tests/lib/mock_android_device.py
index 5fd6fac..03b62a1 100755
--- a/tests/lib/mock_android_device.py
+++ b/tests/lib/mock_android_device.py
@@ -33,7 +33,7 @@ def get_mock_ads(num):
     """
     ads = []
     for i in range(num):
-        ad = mock.MagicMock(name="AndroidDevice", serial=i, h_port=None)
+        ad = mock.MagicMock(name="AndroidDevice", serial=str(i), h_port=None)
         ads.append(ad)
     return ads
 
@@ -42,6 +42,18 @@ def get_all_instances():
     return get_mock_ads(5)
 
 
+def get_instances(serials):
+    ads = []
+    for serial in serials:
+        ad = mock.MagicMock(name="AndroidDevice", serial=serial, h_port=None)
+        ads.append(ad)
+    return ads
+
+
+def get_instances_with_configs(dicts):
+    return get_instances([d['serial'] for d in dicts])
+
+
 def list_adb_devices():
     return [ad.serial for ad in get_mock_ads(5)]
 
diff --git a/tests/mobly/controllers/android_device_test.py b/tests/mobly/controllers/android_device_test.py
index 38247cb..c5b3733 100755
--- a/tests/mobly/controllers/android_device_test.py
+++ b/tests/mobly/controllers/android_device_test.py
@@ -79,6 +79,29 @@ class AndroidDeviceTest(unittest.TestCase):
         for actual, expected in zip(actual_ads, mock_android_device.get_mock_ads(5)):
             self.assertEqual(actual.serial, expected.serial)
 
+    @mock.patch.object(android_device,
+                       "get_instances",
+                       new=mock_android_device.get_instances)
+    @mock.patch.object(android_device,
+                       "list_adb_devices",
+                       new=mock_android_device.list_adb_devices)
+    def test_create_with_string_list(self):
+        string_list = [u'1', '2']
+        actual_ads = android_device.create(string_list)
+        for actual_ad, expected_serial in zip(actual_ads, ['1', '2']):
+            self.assertEqual(actual_ad.serial, expected_serial)
+
+    @mock.patch.object(android_device,
+                       "get_instances_with_configs",
+                       new=mock_android_device.get_instances_with_configs)
+    @mock.patch.object(android_device,
+                       "list_adb_devices",
+                       new=mock_android_device.list_adb_devices)
+    def test_create_with_dict_list(self):
+        string_list = [{'serial': '1'}, {'serial': '2'}]
+        actual_ads = android_device.create(string_list)
+        for actual_ad, expected_serial in zip(actual_ads, ['1', '2']):
+            self.assertEqual(actual_ad.serial, expected_serial)
     def test_create_with_empty_config(self):
         expected_msg = android_device.ANDROID_DEVICE_EMPTY_CONFIG_MSG
         with self.assertRaisesRegexp(android_device.Error,
@@ -91,15 +114,21 @@ class AndroidDeviceTest(unittest.TestCase):
                                      expected_msg):
             android_device.create("HAHA")
 
+    def test_create_with_no_valid_config(self):
+        expected_msg = "No valid config found in: .*"
+        with self.assertRaisesRegexp(android_device.Error,
+                                     expected_msg):
+            android_device.create([1])
+
     def test_get_device_success_with_serial(self):
         ads = mock_android_device.get_mock_ads(5)
-        expected_serial = 0
+        expected_serial = '0'
         ad = android_device.get_device(ads, serial=expected_serial)
         self.assertEqual(ad.serial, expected_serial)
 
     def test_get_device_success_with_serial_and_extra_field(self):
         ads = mock_android_device.get_mock_ads(5)
-        expected_serial = 1
+        expected_serial = '1'
         expected_h_port = 5555
         ads[1].h_port = expected_h_port
         ad = android_device.get_device(ads,
@@ -119,7 +148,7 @@ class AndroidDeviceTest(unittest.TestCase):
     def test_get_device_too_many_matches(self):
         ads = mock_android_device.get_mock_ads(5)
         target_serial = ads[1].serial = ads[0].serial
-        expected_msg = "More than one device matched: \[0, 0\]"
+        expected_msg = "More than one device matched: \['0', '0'\]"
         with self.assertRaisesRegexp(android_device.Error,
                                      expected_msg):
             android_device.get_device(ads, serial=target_serial)