Repository: google/mobly

Issue:
Reduce the number of adb calls made for `ad.getprop` 
Right now, each getprop requires a separate adb call.
In large scale labs, this could create a huge amount of adb calls.
We should provide a new API to get multiple props in one call.

Discussion:

Patch:
diff --git a/mobly/config_parser.py b/mobly/config_parser.py
index 2278ca6..d26f34b 100644
--- a/mobly/config_parser.py
+++ b/mobly/config_parser.py
@@ -152,7 +152,7 @@ def _load_config_file(path):
         A dict that represents info in the config file.
     """
     with io.open(utils.abs_path(path), 'r', encoding='utf-8') as f:
-        conf = yaml.load(f)
+        conf = yaml.safe_load(f)
         return conf
 
 
diff --git a/mobly/controllers/android_device.py b/mobly/controllers/android_device.py
index 331d2c1..20204be 100644
--- a/mobly/controllers/android_device.py
+++ b/mobly/controllers/android_device.py
@@ -711,8 +711,9 @@ class AndroidDevice(object):
                            'info.')
             return
         info = {}
-        info['build_id'] = self.adb.getprop('ro.build.id')
-        info['build_type'] = self.adb.getprop('ro.build.type')
+        build_info = self.adb.getprops(['ro.build.id', 'ro.build.type'])
+        info['build_id'] = build_info['ro.build.id']
+        info['build_type'] = build_info['ro.build.type']
         return info
 
     @property
diff --git a/mobly/controllers/android_device_lib/adb.py b/mobly/controllers/android_device_lib/adb.py
index 86b3112..885539a 100644
--- a/mobly/controllers/android_device_lib/adb.py
+++ b/mobly/controllers/android_device_lib/adb.py
@@ -62,8 +62,8 @@ class AdbError(Error):
 
     def __str__(self):
         return ('Error executing adb cmd "%s". ret: %d, stdout: %s, stderr: %s'
-                ) % (utils.cli_cmd_to_string(self.cmd), self.ret_code, self.stdout,
-                     self.stderr)
+                ) % (utils.cli_cmd_to_string(self.cmd), self.ret_code,
+                     self.stdout, self.stderr)
 
 
 class AdbTimeoutError(Error):
@@ -277,10 +277,28 @@ class AdbProxy(object):
             adb_cmd, shell=shell, handler=handler)
         return out
 
+    def _parse_getprop_output(self, output):
+        """Parses the raw output of `adb shell getprop` into a dictionary.
+
+        Args:
+            output: byte str, the raw output of the `adb shell getprop` call.
+
+        Returns:
+            dict, name-value pairs of the properties.
+        """
+        output = output.decode('utf-8').strip()
+        clean_output = output.replace('[', '').replace(']', '')
+        results = {}
+        for line in clean_output.split('\n'):
+            if line:
+                name, value = line.split(': ')
+                results[name] = value
+        return results
+
     def getprop(self, prop_name):
         """Get a property of the device.
 
-        This is a convenience wrapper for "adb shell getprop xxx".
+        This is a convenience wrapper for `adb shell getprop xxx`.
 
         Args:
             prop_name: A string that is the name of the property to get.
@@ -293,6 +311,28 @@ class AdbProxy(object):
             ['getprop', prop_name],
             timeout=DEFAULT_GETPROP_TIMEOUT_SEC).decode('utf-8').strip()
 
+    def getprops(self, prop_names):
+        """Get multiple properties of the device.
+
+        This is a convenience wrapper for `adb shell getprop`. Use this to
+        reduce the number of adb calls when getting multiple properties.
+
+        Args:
+            prop_names: list of strings, the names of the properties to get.
+
+        Returns:
+            A dict containing name-value pairs of the properties requested, if
+            they exist.
+        """
+        raw_output = self.shell(
+            ['getprop'], timeout=DEFAULT_GETPROP_TIMEOUT_SEC)
+        properties = self._parse_getprop_output(raw_output)
+        results = {}
+        for name in prop_names:
+            if name in properties:
+                results[name] = properties[name]
+        return results
+
     def has_shell_command(self, command):
         """Checks to see if a given check command exists on the device.
 


Test patch:
diff --git a/tests/lib/mock_android_device.py b/tests/lib/mock_android_device.py
index 3745b1a..443f838 100755
--- a/tests/lib/mock_android_device.py
+++ b/tests/lib/mock_android_device.py
@@ -1,11 +1,11 @@
 # Copyright 2016 Google Inc.
-# 
+#
 # Licensed under the Apache License, Version 2.0 (the "License");
 # you may not use this file except in compliance with the License.
 # You may obtain a copy of the License at
-# 
+#
 #     http://www.apache.org/licenses/LICENSE-2.0
-# 
+#
 # Unless required by applicable law or agreed to in writing, software
 # distributed under the License is distributed on an "AS IS" BASIS,
 # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
@@ -68,6 +68,13 @@ class MockAdbProxy(object):
         self.serial = serial
         self.fail_br = fail_br
         self.fail_br_before_N = fail_br_before_N
+        self.mock_properties = {
+            "ro.build.id": "AB42",
+            "ro.build.type": "userdebug",
+            "ro.build.product": "FakeModel",
+            "ro.product.name": "FakeModel",
+            "sys.boot_completed": "1"
+        }
 
     def shell(self, params, timeout=None):
         if params == "id -u":
@@ -82,14 +89,11 @@ class MockAdbProxy(object):
             return b'1.1'
 
     def getprop(self, params):
-        if params == "ro.build.id":
-            return "AB42"
-        elif params == "ro.build.type":
-            return "userdebug"
-        elif params == "ro.build.product" or params == "ro.product.name":
-            return "FakeModel"
-        elif params == "sys.boot_completed":
-            return "1"
+        if params in self.mock_properties:
+            return self.mock_properties[params]
+
+    def getprops(self, params):
+        return self.mock_properties
 
     def bugreport(self, args, shell=False, timeout=None):
         expected = os.path.join(logging.log_path,
diff --git a/tests/mobly/base_test_test.py b/tests/mobly/base_test_test.py
index 43951f4..abe3a13 100755
--- a/tests/mobly/base_test_test.py
+++ b/tests/mobly/base_test_test.py
@@ -148,8 +148,8 @@ class BaseTestTest(unittest.TestCase):
 
         bt_cls = MockBaseTest(self.mock_test_cls_configs)
         expected_msg = (
-            'Test method name not_a_test_something does not follow '
-            'naming convention test_\*, abort.')
+            r'Test method name not_a_test_something does not follow '
+            r'naming convention test_\*, abort.')
         with self.assertRaisesRegex(base_test.Error, expected_msg):
             bt_cls.run()
 
@@ -186,8 +186,8 @@ class BaseTestTest(unittest.TestCase):
 
         bt_cls = MockBaseTest(self.mock_test_cls_configs)
         expected_msg = (
-            'Test method name not_a_test_something does not follow '
-            'naming convention test_\*, abort.')
+            r'Test method name not_a_test_something does not follow '
+            r'naming convention test_\*, abort.')
         with self.assertRaisesRegex(base_test.Error, expected_msg):
             bt_cls.run(test_names=["not_a_test_something"])
 
@@ -1994,7 +1994,7 @@ class BaseTestTest(unittest.TestCase):
         self.assertEqual(actual_record.test_name, "test_something")
         hit = False
         with io.open(self.summary_file, 'r', encoding='utf-8') as f:
-            for c in yaml.load_all(f):
+            for c in yaml.safe_load_all(f):
                 if c['Type'] != records.TestSummaryEntryType.USER_DATA.value:
                     continue
                 hit = True
diff --git a/tests/mobly/controllers/android_device_lib/adb_test.py b/tests/mobly/controllers/android_device_lib/adb_test.py
index be3db43..911c36f 100755
--- a/tests/mobly/controllers/android_device_lib/adb_test.py
+++ b/tests/mobly/controllers/android_device_lib/adb_test.py
@@ -452,7 +452,48 @@ class AdbTest(unittest.TestCase):
                 ['adb', 'shell', 'getprop', 'haha'],
                 shell=False,
                 stderr=None,
-                timeout=5)
+                timeout=adb.DEFAULT_GETPROP_TIMEOUT_SEC)
+
+    def test_getprops(self):
+        with mock.patch.object(adb.AdbProxy, '_exec_cmd') as mock_exec_cmd:
+            mock_exec_cmd.return_value = b'''
+[selinux.restorecon_recursive]: [/data/misc_ce/10]
+[sendbug.preferred.domain]: [google.com]
+[service.bootanim.exit]: [1]
+[sys.boot_completed]: [1]
+[sys.dvr.performance]: [idle]
+[sys.logbootcomplete]: [1]
+[sys.oem_unlock_allowed]: [1]
+[sys.rescue_boot_count]: [1]
+[sys.retaildemo.enabled]: [0]
+[sys.sysctl.extra_free_kbytes]: [27337]
+[sys.sysctl.tcp_def_init_rwnd]: [60]
+[sys.uidcpupower]: []
+[sys.usb.config]: [adb]
+[sys.usb.configfs]: [2]
+[sys.usb.controller]: [a600000.dwc3]
+[sys.usb.ffs.ready]: [1]
+[sys.usb.mtp.device_type]: [3]
+[sys.user.0.ce_available]: [true]
+[sys.wifitracing.started]: [1]
+[telephony.lteOnCdmaDevice]: [1]'''
+            actual_output = adb.AdbProxy().getprops([
+                'sys.wifitracing.started',  # "numeric" value
+                'sys.uidcpupower',  # empty value
+                'sendbug.preferred.domain',  # string value
+                'nonExistentProp'
+            ])
+            self.assertEqual(
+                actual_output, {
+                    'sys.wifitracing.started': '1',
+                    'sys.uidcpupower': '',
+                    'sendbug.preferred.domain': 'google.com'
+                })
+            mock_exec_cmd.assert_called_once_with(
+                ['adb', 'shell', 'getprop'],
+                shell=False,
+                stderr=None,
+                timeout=adb.DEFAULT_GETPROP_TIMEOUT_SEC)
 
     def test_forward(self):
         with mock.patch.object(adb.AdbProxy, '_exec_cmd') as mock_exec_cmd:
diff --git a/tests/mobly/output_test.py b/tests/mobly/output_test.py
index 8a87902..6caf9b8 100755
--- a/tests/mobly/output_test.py
+++ b/tests/mobly/output_test.py
@@ -232,7 +232,7 @@ class OutputTest(unittest.TestCase):
          info_log_path) = self.assert_output_logs_exist(output_dir)
         summary_entries = []
         with io.open(summary_file_path, 'r', encoding='utf-8') as f:
-            for entry in yaml.load_all(f):
+            for entry in yaml.safe_load_all(f):
                 self.assertTrue(entry['Type'])
                 summary_entries.append(entry)
         self.assert_log_contents(debug_log_path, whitelist=['DEBUG', 'INFO'])
@@ -255,7 +255,7 @@ class OutputTest(unittest.TestCase):
         with io.open(summary_file_path, 'r', encoding='utf-8') as f:
             raw_content = f.read()
             f.seek(0)
-            for entry in yaml.load_all(f):
+            for entry in yaml.safe_load_all(f):
                 if (entry['Type'] == 'Record'
                         and entry[records.TestResultEnums.RECORD_NAME] ==
                         'teardown_class'):
diff --git a/tests/mobly/records_test.py b/tests/mobly/records_test.py
index ba25a69..a374347 100755
--- a/tests/mobly/records_test.py
+++ b/tests/mobly/records_test.py
@@ -361,7 +361,7 @@ class RecordsTest(unittest.TestCase):
         writer = records.TestSummaryWriter(dump_path)
         writer.dump(record1.to_dict(), records.TestSummaryEntryType.RECORD)
         with io.open(dump_path, 'r', encoding='utf-8') as f:
-            content = yaml.load(f)
+            content = yaml.safe_load(f)
             self.assertEqual(content['Type'],
                              records.TestSummaryEntryType.RECORD.value)
             self.assertEqual(content[records.TestResultEnums.RECORD_DETAILS],
@@ -380,7 +380,7 @@ class RecordsTest(unittest.TestCase):
         writer = records.TestSummaryWriter(dump_path)
         writer.dump(record1.to_dict(), records.TestSummaryEntryType.RECORD)
         with io.open(dump_path, 'r', encoding='utf-8') as f:
-            content = yaml.load(f)
+            content = yaml.safe_load(f)
             self.assertEqual(content['Type'],
                              records.TestSummaryEntryType.RECORD.value)
             self.assertEqual(content[records.TestResultEnums.RECORD_DETAILS],
@@ -398,7 +398,7 @@ class RecordsTest(unittest.TestCase):
             writer.dump(data, records.TestSummaryEntryType.USER_DATA)
         with io.open(dump_path, 'r', encoding='utf-8') as f:
             contents = []
-            for c in yaml.load_all(f):
+            for c in yaml.safe_load_all(f):
                 contents.append(c)
         for content in contents:
             self.assertEqual(content['Type'],
diff --git a/tests/mobly/test_runner_test.py b/tests/mobly/test_runner_test.py
index 0d282ba..26af2ee 100755
--- a/tests/mobly/test_runner_test.py
+++ b/tests/mobly/test_runner_test.py
@@ -132,7 +132,7 @@ class TestRunnerTest(unittest.TestCase):
         summary_path = os.path.join(logging.log_path,
                                     records.OUTPUT_FILE_SUMMARY)
         with io.open(summary_path, 'r', encoding='utf-8') as f:
-            summary_entries = list(yaml.load_all(f))
+            summary_entries = list(yaml.safe_load_all(f))
         self.assertEqual(len(summary_entries), 4)
         # Verify the first entry is the list of test names.
         self.assertEqual(summary_entries[0]['Type'],