crosperf: RunCommand should return one type of object.

Cleaned up the interfaces for the RunCommand routines.
These were returning different types (int or tuple) depending on
the value of the return_ouput parameter.
Returning different unrelated types from a routine is bad practice.
Linter complains about this with several warnings like this:

"Attempting to unpack a non-sequence defined at line XY of
utils.command_executer"

BUG=chromium:566256
TEST=ran crosperf with a example experiment file
     Ran run_tests.

Change-Id: Ibb83ab9322c87558077fc4937ef5c0686bbe5417
Reviewed-on: https://chrome-internal-review.googlesource.com/241459
Commit-Ready: Luis Lozano <llozano@chromium.org>
Tested-by: Luis Lozano <llozano@chromium.org>
Reviewed-by: Han Shen <shenhan@google.com>

8 files changed, 48 insertions, 69 deletions

diff --git a/crosperf/download_images.py b/crosperf/download_images.py
index 72f3fb04..8fecf8b3 100644
--- a/crosperf/download_images.py
+++ b/crosperf/download_images.py
@@ -28,8 +28,8 @@ class ImageDownloader(object):
     # image name.
     command = ("cd ~/trunk/src/third_party/toolchain-utils/crosperf; "
                "python translate_xbuddy.py '%s'" % xbuddy_label)
-    retval, build_id_tuple_str, _ = self._ce.ChrootRunCommand(chromeos_root,
-                                                          command, True)
+    retval, build_id_tuple_str, _ = self._ce.ChrootRunCommandWOutput(
+        chromeos_root, command)
     if not build_id_tuple_str:
       raise MissingImage ("Unable to find image for '%s'" % xbuddy_label)
 
diff --git a/crosperf/machine_manager.py b/crosperf/machine_manager.py
index f8834d73..7bada0d1 100644
--- a/crosperf/machine_manager.py
+++ b/crosperf/machine_manager.py
@@ -114,20 +114,16 @@ class CrosMachine(object):
     #TODO yunlian: when the machine in rebooting, it will not return
     #meminfo, the assert does not catch it either
     command = "cat /proc/meminfo"
-    ret, self.meminfo, _ = self.ce.CrosRunCommand(
-        command, return_output=True,
-        machine=self.name,
-        chromeos_root=self.chromeos_root)
+    ret, self.meminfo, _ = self.ce.CrosRunCommandWOutput(
+        command, machine=self.name, chromeos_root=self.chromeos_root)
     assert ret == 0, "Could not get meminfo from machine: %s" % self.name
     if ret == 0:
       self._ParseMemoryInfo()
 
   def _GetCPUInfo(self):
     command = "cat /proc/cpuinfo"
-    ret, self.cpuinfo, _ = self.ce.CrosRunCommand(
-        command, return_output=True,
-        machine=self.name,
-        chromeos_root=self.chromeos_root)
+    ret, self.cpuinfo, _ = self.ce.CrosRunCommandWOutput(
+        command, machine=self.name, chromeos_root=self.chromeos_root)
     assert ret == 0, "Could not get cpuinfo from machine: %s" % self.name
 
   def _ComputeMachineChecksumString(self):
@@ -146,18 +142,16 @@ class CrosMachine(object):
 
   def _GetMachineID(self):
     command = "dump_vpd_log --full --stdout"
-    _, if_out, _ = self.ce.CrosRunCommand(command, return_output=True,
-                                          machine=self.name,
-                                          chromeos_root=self.chromeos_root)
+    _, if_out, _ = self.ce.CrosRunCommandWOutput(
+        command, machine=self.name, chromeos_root=self.chromeos_root)
     b = if_out.splitlines()
     a = [l for l in b if "Product" in l]
     if len(a):
       self.machine_id = a[0]
       return
     command = "ifconfig"
-    _, if_out, _ = self.ce.CrosRunCommand(command, return_output=True,
-                                          machine=self.name,
-                                          chromeos_root=self.chromeos_root)
+    _, if_out, _ = self.ce.CrosRunCommandWOutput(
+        command, machine=self.name, chromeos_root=self.chromeos_root)
     b = if_out.splitlines()
     a = [l for l in b if "HWaddr" in l]
     if len(a):
@@ -227,9 +221,8 @@ class MachineManager(object):
     """Get the version of Chrome running on the DUT."""
 
     cmd = "/opt/google/chrome/chrome --version"
-    ret, version, _ = self.ce.CrosRunCommand(cmd, return_output=True,
-                                             machine=machine.name,
-                                             chromeos_root=self.chromeos_root)
+    ret, version, _ = self.ce.CrosRunCommandWOutput(
+        cmd, machine=machine.name, chromeos_root=self.chromeos_root)
     if ret != 0:
       raise CrosCommandError("Couldn't get Chrome version from %s."
                              % machine.name)
@@ -340,8 +333,8 @@ class MachineManager(object):
       if locked:
         self._machines.append(cros_machine)
         command = "cat %s" % CHECKSUM_FILE
-        ret, out, _ = self.ce.CrosRunCommand(
-            command, return_output=True, chromeos_root=self.chromeos_root,
+        ret, out, _ = self.ce.CrosRunCommandWOutput(
+            command, chromeos_root=self.chromeos_root,
             machine=cros_machine.name)
         if ret == 0:
           cros_machine.checksum = out.strip()
diff --git a/crosperf/machine_manager_unittest.py b/crosperf/machine_manager_unittest.py
index bf04ee4c..7aed09d4 100755
--- a/crosperf/machine_manager_unittest.py
+++ b/crosperf/machine_manager_unittest.py
@@ -236,7 +236,7 @@ class MachineManagerTest(unittest.TestCase):
                      'daisy_checksum_str')
 
 
-  @mock.patch.object(command_executer.CommandExecuter, 'CrosRunCommand')
+  @mock.patch.object(command_executer.CommandExecuter, 'CrosRunCommandWOutput')
   def test_try_to_lock_machine(self, mock_cros_runcmd):
 
     self.assertRaises(self.mm._TryToLockMachine, None)
@@ -252,10 +252,9 @@ class MachineManagerTest(unittest.TestCase):
     cmd_str = mock_cros_runcmd.call_args[0][0]
     self.assertEqual(cmd_str, 'cat /usr/local/osimage_checksum_file')
     args_dict = mock_cros_runcmd.call_args[1]
-    self.assertEqual(len(args_dict), 3)
+    self.assertEqual(len(args_dict), 2)
     self.assertEqual(args_dict['machine'], self.mock_lumpy1.name)
     self.assertEqual(args_dict['chromeos_root'], '/usr/local/chromeos')
-    self.assertTrue(args_dict['return_output'])
 
 
   @mock.patch.object(machine_manager, 'CrosMachine')
@@ -730,7 +729,7 @@ class CrosMachineTest(unittest.TestCase):
     self.assertEqual(cm.phys_kbytes, 4194304)
 
 
-  @mock.patch.object(command_executer.CommandExecuter, 'CrosRunCommand')
+  @mock.patch.object(command_executer.CommandExecuter, 'CrosRunCommandWOutput')
   @mock.patch.object(machine_manager.CrosMachine, "SetUpChecksumInfo")
   def test_get_memory_info(self, mock_setup, mock_run_cmd):
     cm = machine_manager.CrosMachine("daisy.cros", "/usr/local/chromeos",
@@ -744,7 +743,6 @@ class CrosMachineTest(unittest.TestCase):
     args_dict = call_args[1]
     self.assertEqual(args_dict['machine'], 'daisy.cros')
     self.assertEqual(args_dict['chromeos_root'], '/usr/local/chromeos')
-    self.assertEqual(args_dict['return_output'], True)
     self.assertEqual(cm.meminfo, MEMINFO_STRING)
     self.assertEqual(cm.phys_kbytes, 4194304)
 
@@ -752,7 +750,7 @@ class CrosMachineTest(unittest.TestCase):
     self.assertRaises (cm._GetMemoryInfo)
 
 
-  @mock.patch.object(command_executer.CommandExecuter, 'CrosRunCommand')
+  @mock.patch.object(command_executer.CommandExecuter, 'CrosRunCommandWOutput')
   @mock.patch.object(machine_manager.CrosMachine, "SetUpChecksumInfo")
   def test_get_cpu_info(self, mock_setup, mock_run_cmd):
     cm = machine_manager.CrosMachine("daisy.cros", "/usr/local/chromeos",
@@ -766,7 +764,6 @@ class CrosMachineTest(unittest.TestCase):
     args_dict = call_args[1]
     self.assertEqual(args_dict['machine'], 'daisy.cros')
     self.assertEqual(args_dict['chromeos_root'], '/usr/local/chromeos')
-    self.assertEqual(args_dict['return_output'], True)
     self.assertEqual(cm.cpuinfo, CPUINFO_STRING)
 
 
diff --git a/crosperf/results_cache.py b/crosperf/results_cache.py
index 9431e88a..cdf14315 100644
--- a/crosperf/results_cache.py
+++ b/crosperf/results_cache.py
@@ -137,10 +137,8 @@ class Result(object):
 
     command = ("python generate_test_report --no-color --csv %s" %
                (os.path.join("/tmp", os.path.basename(self._temp_dir))))
-    [_, out, _] = self._ce.ChrootRunCommand(self._chromeos_root,
-                                            command,
-                                            return_output=True,
-                                            print_to_console=False)
+    _, out, _ = self._ce.ChrootRunCommandWOutput(
+        self._chromeos_root, command, print_to_console=False)
     keyvals_dict = {}
     tmp_dir_in_chroot = misc.GetInsideChrootPath(self._chromeos_root,
                                                  self._temp_dir)
@@ -175,8 +173,7 @@ class Result(object):
 
     command = "find %s %s" % (self.results_dir,
                               find_args)
-    ret, out, _ = self._ce.RunCommand(command, return_output=True,
-                                      print_to_console=False)
+    ret, out, _ = self._ce.RunCommandWOutput(command, print_to_console=False)
     if ret:
       raise Exception("Could not run find command!")
     return out
@@ -239,8 +236,7 @@ class Result(object):
                   self._board,
                   chroot_perf_data_file,
                   chroot_perf_report_file))
-      self._ce.ChrootRunCommand(self._chromeos_root,
-                                command)
+      self._ce.ChrootRunCommand(self._chromeos_root, command)
 
       # Add a keyval to the dictionary for the events captured.
       perf_report_files.append(
diff --git a/crosperf/results_cache_unittest.py b/crosperf/results_cache_unittest.py
index e858a4e4..790b4718 100755
--- a/crosperf/results_cache_unittest.py
+++ b/crosperf/results_cache_unittest.py
@@ -713,13 +713,13 @@ class ResultTest(unittest.TestCase):
     f1 = os.path.join(test_dir, 'machine.txt')
     f2 = os.path.join(base_dir, 'machine.txt')
     cmd = 'diff %s %s' % (f1, f2)
-    [_, out, _] = self.result._ce.RunCommand(cmd, return_output=True)
+    [_, out, _] = self.result._ce.RunCommandWOutput(cmd)
     self.assertEqual(len(out), 0)
 
     f1 = os.path.join(test_dir, 'results.txt')
     f2 = os.path.join(base_dir, 'results.txt')
     cmd = 'diff %s %s' % (f1, f2)
-    [_, out, _] = self.result._ce.RunCommand(cmd, return_output=True)
+    [_, out, _] = self.result._ce.RunCommandWOutput(cmd)
     self.assertEqual(len(out), 0)
 
     # Clean up after test.
diff --git a/crosperf/schedv2.py b/crosperf/schedv2.py
index 46d5657b..b73d384f 100644
--- a/crosperf/schedv2.py
+++ b/crosperf/schedv2.py
@@ -152,9 +152,8 @@ class DutWorker(Thread):
         checksum_file = "/usr/local/osimage_checksum_file"
         try:
             rv, checksum, _ = command_executer.GetCommandExecuter().\
-                CrosRunCommand(
+                CrosRunCommandWOutput(
                     "cat " + checksum_file,
-                    return_output=True,
                     chromeos_root=self._sched._labels[0].chromeos_root,
                     machine=self._dut.name,
                     print_to_console=False)
diff --git a/crosperf/suite_runner.py b/crosperf/suite_runner.py
index afec6c85..4c94de20 100644
--- a/crosperf/suite_runner.py
+++ b/crosperf/suite_runner.py
@@ -85,9 +85,8 @@ class SuiteRunner(object):
                        "else "
                        "  cat scaling_max_freq ; "
                        "fi")
-    ret, freqs_str, _ = self._ce.CrosRunCommand(
-        get_avail_freqs, return_output=True, machine=machine_name,
-        chromeos_root=chromeos_root)
+    ret, freqs_str, _ = self._ce.CrosRunCommandWOutput(
+        get_avail_freqs, machine=machine_name, chromeos_root=chromeos_root)
     self._logger.LogFatalIf(ret, "Could not get available frequencies "
                             "from machine: %s" % machine_name)
     freqs = freqs_str.split()
@@ -163,11 +162,9 @@ class SuiteRunner(object):
     # Use --no-ns-pid so that cros_sdk does not create a different
     # process namespace and we can kill process created easily by
     # their process group.
-    return self._ce.ChrootRunCommand(label.chromeos_root,
-                                     command,
-                                     True,
-                                     self._ct,
-                                     cros_sdk_options="--no-ns-pid")
+    return self._ce.ChrootRunCommandWOutput(
+        label.chromeos_root, command, command_terminator=self._ct,
+        cros_sdk_options="--no-ns-pid")
 
   def RemoveTelemetryTempFile (self, machine, chromeos_root):
     filename = "telemetry@%s" % machine
@@ -230,11 +227,9 @@ class SuiteRunner(object):
     if self.log_level != "verbose":
       self._logger.LogOutput("Running test.")
       self._logger.LogOutput("CMD: %s" % cmd)
-    return self._ce.ChrootRunCommand(label.chromeos_root,
-                                     cmd,
-                                     return_output=True,
-                                     command_terminator=self._ct,
-                                     cros_sdk_options=chrome_root_options)
+    return self._ce.ChrootRunCommandWOutput(
+        label.chromeos_root, cmd, command_terminator=self._ct,
+        cros_sdk_options=chrome_root_options)
 
 
   def Telemetry_Run(self, machine, label, benchmark, profiler_args):
@@ -272,8 +267,7 @@ class SuiteRunner(object):
     if self.log_level != "verbose":
       self._logger.LogOutput("Running test.")
       self._logger.LogOutput("CMD: %s" % cmd)
-    return self._ce.RunCommand(cmd, return_output=True,
-                               print_to_console=False)
+    return self._ce.RunCommandWOutput(cmd, print_to_console=False)
 
   def Terminate(self):
     self._ct.Terminate()
diff --git a/crosperf/suite_runner_unittest.py b/crosperf/suite_runner_unittest.py
index 41013f74..d534f3a8 100755
--- a/crosperf/suite_runner_unittest.py
+++ b/crosperf/suite_runner_unittest.py
@@ -150,10 +150,10 @@ class SuiteRunnerTest(unittest.TestCase):
 
 
 
-  @mock.patch.object (command_executer.CommandExecuter, 'CrosRunCommand')
+  @mock.patch.object (command_executer.CommandExecuter, 'CrosRunCommandWOutput')
   def test_get_highest_static_frequency(self, mock_cros_runcmd):
 
-    self.mock_cmd_exec.CrosRunCommand = mock_cros_runcmd
+    self.mock_cmd_exec.CrosRunCommandWOutput = mock_cros_runcmd
     mock_cros_runcmd.return_value = [ 0, '1666000 1333000 1000000', '']
     freq = self.runner.GetHighestStaticFrequency ('lumpy1.cros', '/tmp/chromeos')
     self.assertEqual(freq, '1666000')
@@ -199,7 +199,8 @@ class SuiteRunnerTest(unittest.TestCase):
 
 
   @mock.patch.object (command_executer.CommandExecuter, 'CrosRunCommand')
-  @mock.patch.object (command_executer.CommandExecuter, 'ChrootRunCommand')
+  @mock.patch.object (command_executer.CommandExecuter,
+                      'ChrootRunCommandWOutput')
   def test_test_that_run(self, mock_chroot_runcmd, mock_cros_runcmd):
 
     def FakeRebootMachine (machine, chroot):
@@ -223,7 +224,7 @@ class SuiteRunnerTest(unittest.TestCase):
     self.assertTrue(raised_exception)
 
     mock_chroot_runcmd.return_value = 0
-    self.mock_cmd_exec.ChrootRunCommand = mock_chroot_runcmd
+    self.mock_cmd_exec.ChrootRunCommandWOutput = mock_chroot_runcmd
     self.mock_cmd_exec.CrosRunCommand = mock_cros_runcmd
     res = self.runner.Test_That_Run ('lumpy1.cros', self.mock_label,
                                      self.test_that_bench, '--iterations=2',
@@ -234,25 +235,25 @@ class SuiteRunnerTest(unittest.TestCase):
     self.assertEqual(mock_cros_runcmd.call_args_list[0][0],
                      ('rm -rf /usr/local/autotest/results/*',))
     args_list = mock_chroot_runcmd.call_args_list[0][0]
-    self.assertEqual(len(args_list), 4)
+    args_dict = mock_chroot_runcmd.call_args_list[0][1]
+    self.assertEqual(len(args_list), 2)
     self.assertEqual(args_list[0], '/tmp/chromeos')
     self.assertEqual(args_list[1], ('/usr/bin/test_that --autotest_dir '
                                     '~/trunk/src/third_party/autotest/files '
                                     '--fast  --board=lumpy '
                                     '--iterations=2 lumpy1.cros octane'))
-    self.assertTrue(args_list[2])
-    self.assertEqual(args_list[3], self.mock_cmd_term)
-
+    self.assertEqual(args_dict['command_terminator'], self.mock_cmd_term)
     self.real_logger._LogMsg = save_log_msg
 
 
   @mock.patch.object (os.path, 'isdir')
-  @mock.patch.object (command_executer.CommandExecuter, 'ChrootRunCommand')
+  @mock.patch.object (command_executer.CommandExecuter,
+                      'ChrootRunCommandWOutput')
   def test_telemetry_crosperf_run(self, mock_chroot_runcmd, mock_isdir):
 
     mock_isdir.return_value = True
     mock_chroot_runcmd.return_value = 0
-    self.mock_cmd_exec.ChrootRunCommand = mock_chroot_runcmd
+    self.mock_cmd_exec.ChrootRunCommandWOutput = mock_chroot_runcmd
     profiler_args = ('--profiler=custom_perf --profiler_args=\'perf_options'
                      '="record -a -e cycles,instructions"\'')
     res = self.runner.Telemetry_Crosperf_Run ('lumpy1.cros', self.mock_label,
@@ -274,13 +275,12 @@ class SuiteRunnerTest(unittest.TestCase):
                       '--chrome_root_mount=/tmp/chrome_root '
                       'FEATURES="-usersandbox" CHROME_ROOT=/tmp/chrome_root'))
     self.assertEqual(args_dict['command_terminator'], self.mock_cmd_term)
-    self.assertTrue(args_dict['return_output'])
-    self.assertEqual(len(args_dict), 3)
+    self.assertEqual(len(args_dict), 2)
 
 
   @mock.patch.object (os.path, 'isdir')
   @mock.patch.object (os.path, 'exists')
-  @mock.patch.object (command_executer.CommandExecuter, 'RunCommand')
+  @mock.patch.object (command_executer.CommandExecuter, 'RunCommandWOutput')
   def test_telemetry_run(self, mock_runcmd, mock_exists, mock_isdir):
 
     def FakeLogMsg (fd, termfd, msg, flush):
@@ -290,7 +290,7 @@ class SuiteRunnerTest(unittest.TestCase):
     self.real_logger._LogMsg = FakeLogMsg
     mock_runcmd.return_value = 0
 
-    self.mock_cmd_exec.RunCommand = mock_runcmd
+    self.mock_cmd_exec.RunCommandWOutput = mock_runcmd
     self.runner._logger = self.real_logger
 
     profiler_args = ('--profiler=custom_perf --profiler_args=\'perf_options'