Merge "Distinguish launch_cvd timeout and non-zero return code"
diff --git a/create/local_image_local_instance.py b/create/local_image_local_instance.py
index 2471a3a..f7515d3 100644
--- a/create/local_image_local_instance.py
+++ b/create/local_image_local_instance.py
@@ -53,7 +53,6 @@
 import logging
 import os
 import subprocess
-import threading
 import sys
 
 from acloud import errors
@@ -496,7 +495,7 @@
     @staticmethod
     @utils.TimeExecute(function_description="Waiting for AVD(s) to boot up")
     def _LaunchCvd(cmd, local_instance_id, host_bins_path, cvd_home_dir,
-                   timeout=None):
+                   timeout):
         """Execute Launch CVD.
 
         Kick off the launch_cvd command and log the output.
@@ -519,15 +518,12 @@
         cvd_env[constants.ENV_CUTTLEFISH_INSTANCE] = str(local_instance_id)
         # Check the result of launch_cvd command.
         # An exit code of 0 is equivalent to VIRTUAL_DEVICE_BOOT_COMPLETED
-        process = subprocess.Popen(cmd, shell=True, stderr=subprocess.STDOUT,
-                                   env=cvd_env)
-        if timeout:
-            timer = threading.Timer(timeout, process.kill)
-            timer.start()
-        process.wait()
-        if timeout:
-            timer.cancel()
-        if process.returncode == 0:
-            return
-        raise errors.LaunchCVDFail("launch_cvd returned %s" %
-                                   process.returncode)
+        try:
+            subprocess.check_call(cmd, shell=True, stderr=subprocess.STDOUT,
+                                  env=cvd_env, timeout=timeout)
+        except subprocess.TimeoutExpired as e:
+            raise errors.LaunchCVDFail("Device did not boot within %d secs." %
+                                       timeout) from e
+        except subprocess.CalledProcessError as e:
+            raise errors.LaunchCVDFail("launch_cvd returned %s." %
+                                       e.returncode) from e
diff --git a/create/local_image_local_instance_test.py b/create/local_image_local_instance_test.py
index 0b893f2..24eb5f8 100644
--- a/create/local_image_local_instance_test.py
+++ b/create/local_image_local_instance_test.py
@@ -392,32 +392,55 @@
         self.assertTrue(answer)
 
     # pylint: disable=protected-access
+    @mock.patch("acloud.create.local_image_local_instance.subprocess."
+                "check_call")
     @mock.patch.dict("os.environ", clear=True)
-    def testLaunchCVD(self):
-        """test _LaunchCvd should call subprocess.Popen with the specific env"""
+    def testLaunchCVD(self, mock_check_call):
+        """test _LaunchCvd should call subprocess.check_call with the env."""
         local_instance_id = 3
         launch_cvd_cmd = "launch_cvd"
         host_bins_path = "host_bins_path"
         cvd_home_dir = "fake_home"
+        timeout = 100
         cvd_env = {}
         cvd_env[constants.ENV_CVD_HOME] = cvd_home_dir
         cvd_env[constants.ENV_CUTTLEFISH_INSTANCE] = str(local_instance_id)
         cvd_env[constants.ENV_ANDROID_SOONG_HOST_OUT] = host_bins_path
         cvd_env[constants.ENV_ANDROID_HOST_OUT] = host_bins_path
-        process = mock.MagicMock()
-        process.wait.return_value = True
-        process.returncode = 0
-        self.Patch(subprocess, "Popen", return_value=process)
 
         self.local_image_local_instance._LaunchCvd(launch_cvd_cmd,
                                                    local_instance_id,
                                                    host_bins_path,
-                                                   cvd_home_dir)
-        # pylint: disable=no-member
-        subprocess.Popen.assert_called_once_with(launch_cvd_cmd,
-                                                 shell=True,
-                                                 stderr=subprocess.STDOUT,
-                                                 env=cvd_env)
+                                                   cvd_home_dir,
+                                                   timeout)
+
+        mock_check_call.assert_called_once_with(launch_cvd_cmd,
+                                                shell=True,
+                                                stderr=subprocess.STDOUT,
+                                                env=cvd_env,
+                                                timeout=timeout)
+
+    @mock.patch("acloud.create.local_image_local_instance.subprocess."
+                "check_call")
+    def testLaunchCVDTimeout(self, mock_check_call):
+        """test _LaunchCvd with subprocess errors."""
+        mock_check_call.side_effect = subprocess.TimeoutExpired(
+            cmd="launch_cvd", timeout=100)
+        with self.assertRaises(errors.LaunchCVDFail):
+            self.local_image_local_instance._LaunchCvd("launch_cvd",
+                                                       3,
+                                                       "host_bins_path",
+                                                       "cvd_home_dir",
+                                                       100)
+
+        mock_check_call.side_effect = subprocess.CalledProcessError(
+            cmd="launch_cvd", returncode=1)
+        with self.assertRaises(errors.LaunchCVDFail):
+            self.local_image_local_instance._LaunchCvd("launch_cvd",
+                                                       3,
+                                                       "host_bins_path",
+                                                       "cvd_home_dir",
+                                                       100)
 
 
 if __name__ == "__main__":