refactor: move all process.wait to process.communicate (#1590)

* refactor: move all process.wait to process.communicate

* Fix layer integ tests

* Strip before spliting string

tests/integration/deploy/test_deploy_command.py

@@ -2,7 +2,7 @@
 import tempfile
 import uuid
 import time
-from subprocess import Popen, PIPE
+from subprocess import Popen, PIPE, TimeoutExpired
 from unittest import skipIf
 
 import boto3
@@ -18,6 +18,7 @@
 # This is to restrict package tests to run outside of CI/CD and when the branch is not master.
 SKIP_DEPLOY_TESTS = RUNNING_ON_CI and RUNNING_TEST_FOR_MASTER_ON_CI
 CFN_SLEEP = 3
+TIMEOUT = 300
 
 
 @skipIf(SKIP_DEPLOY_TESTS, "Skip deploy tests in CI/CD only")
@@ -44,7 +45,11 @@ def test_package_and_deploy_no_s3_bucket_all_args(self, template_file):
             )
 
             package_process = Popen(package_command_list, stdout=PIPE)
-            package_process.wait()
+            try:
+                package_process.communicate(timeout=TIMEOUT)
+            except TimeoutExpired:
+                package_process.kill()
+                raise
 
             self.assertEqual(package_process.returncode, 0)
 
@@ -67,7 +72,11 @@ def test_package_and_deploy_no_s3_bucket_all_args(self, template_file):
             )
 
             deploy_process_no_execute = Popen(deploy_command_list_no_execute, stdout=PIPE)
-            deploy_process_no_execute.wait()
+            try:
+                deploy_process_no_execute.communicate(timeout=TIMEOUT)
+            except TimeoutExpired:
+                deploy_process_no_execute.kill()
+                raise
             self.assertEqual(deploy_process_no_execute.returncode, 0)
 
             # Deploy the given stack with the changeset.
@@ -84,7 +93,11 @@ def test_package_and_deploy_no_s3_bucket_all_args(self, template_file):
             )
 
             deploy_process = Popen(deploy_command_list_execute, stdout=PIPE)
-            deploy_process.wait()
+            try:
+                deploy_process.communicate(timeout=TIMEOUT)
+            except TimeoutExpired:
+                deploy_process.kill()
+                raise
             self.assertEqual(deploy_process.returncode, 0)
 
     @parameterized.expand(["aws-serverless-function.yaml"])
@@ -111,7 +124,11 @@ def test_no_package_and_deploy_with_s3_bucket_all_args(self, template_file):
         )
 
         deploy_process_execute = Popen(deploy_command_list, stdout=PIPE)
-        deploy_process_execute.wait()
+        try:
+            deploy_process_execute.communicate(timeout=TIMEOUT)
+        except TimeoutExpired:
+            deploy_process_execute.kill()
+            raise
         self.assertEqual(deploy_process_execute.returncode, 0)
 
     @parameterized.expand(["aws-serverless-function.yaml"])
@@ -138,7 +155,7 @@ def test_no_package_and_deploy_with_s3_bucket_all_args_confirm_changeset(self, t
         )
 
         deploy_process_execute = Popen(deploy_command_list, stdout=PIPE, stderr=PIPE, stdin=PIPE)
-        deploy_process_execute.communicate("Y".encode())
+        deploy_process_execute.communicate("Y".encode(), timeout=TIMEOUT)
         self.assertEqual(deploy_process_execute.returncode, 0)
 
     @parameterized.expand(["aws-serverless-function.yaml"])
@@ -163,10 +180,14 @@ def test_deploy_without_s3_bucket(self, template_file):
         )
 
         deploy_process_execute = Popen(deploy_command_list, stdout=PIPE, stderr=PIPE)
-        deploy_process_execute.wait()
+        try:
+            _, stderr = deploy_process_execute.communicate(timeout=TIMEOUT)
+        except TimeoutExpired:
+            deploy_process_execute.kill()
+            raise
         # Error asking for s3 bucket
         self.assertEqual(deploy_process_execute.returncode, 1)
-        stderr = b"".join(deploy_process_execute.stderr.readlines()).strip()
+        stderr = stderr.strip()
         self.assertIn(
             bytes(
                 f"S3 Bucket not specified, use --s3-bucket to specify a bucket name or run sam deploy --guided",
@@ -194,7 +215,11 @@ def test_deploy_without_stack_name(self, template_file):
         )
 
         deploy_process_execute = Popen(deploy_command_list, stdout=PIPE, stderr=PIPE)
-        deploy_process_execute.wait()
+        try:
+            deploy_process_execute.communicate(timeout=TIMEOUT)
+        except TimeoutExpired:
+            deploy_process_execute.kill()
+            raise
         # Error no stack name present
         self.assertEqual(deploy_process_execute.returncode, 2)
 
@@ -219,7 +244,11 @@ def test_deploy_without_capabilities(self, template_file):
         )
 
         deploy_process_execute = Popen(deploy_command_list, stdout=PIPE, stderr=PIPE)
-        deploy_process_execute.wait()
+        try:
+            deploy_process_execute.communicate(timeout=TIMEOUT)
+        except TimeoutExpired:
+            deploy_process_execute.kill()
+            raise
         # Error capabilities not specified
         self.assertEqual(deploy_process_execute.returncode, 1)
 
@@ -241,7 +270,11 @@ def test_deploy_without_template_file(self, template_file):
         )
 
         deploy_process_execute = Popen(deploy_command_list, stdout=PIPE, stderr=PIPE)
-        deploy_process_execute.wait()
+        try:
+            deploy_process_execute.communicate(timeout=TIMEOUT)
+        except TimeoutExpired:
+            deploy_process_execute.kill()
+            raise
         # Error template file not specified
         self.assertEqual(deploy_process_execute.returncode, 1)
 
@@ -268,7 +301,11 @@ def test_deploy_with_s3_bucket_switch_region(self, template_file):
         )
 
         deploy_process_execute = Popen(deploy_command_list, stdout=PIPE)
-        deploy_process_execute.wait()
+        try:
+            deploy_process_execute.communicate(timeout=TIMEOUT)
+        except TimeoutExpired:
+            deploy_process_execute.kill()
+            raise
         # Deploy should succeed
         self.assertEqual(deploy_process_execute.returncode, 0)
 
@@ -290,10 +327,14 @@ def test_deploy_with_s3_bucket_switch_region(self, template_file):
         )
 
         deploy_process_execute = Popen(deploy_command_list, stdout=PIPE, stderr=PIPE)
-        deploy_process_execute.wait()
+        try:
+            _, stderr = deploy_process_execute.communicate(timeout=TIMEOUT)
+        except TimeoutExpired:
+            deploy_process_execute.kill()
+            raise
         # Deploy should fail, asking for s3 bucket
         self.assertEqual(deploy_process_execute.returncode, 1)
-        stderr = b"".join(deploy_process_execute.stderr.readlines()).strip()
+        stderr = stderr.strip()
         self.assertIn(
             bytes(
                 f"Error: Failed to create/update stack {stack_name} : "

tests/integration/init/test_init_command.py

@@ -1,8 +1,10 @@
 from unittest import TestCase
-from subprocess import Popen
+from subprocess import Popen, TimeoutExpired
 import os
 import tempfile
 
+TIMEOUT = 300
+
 
 class TestBasicInitCommand(TestCase):
     def test_init_command_passes_and_dir_created(self):
@@ -24,9 +26,13 @@ def test_init_command_passes_and_dir_created(self):
                     temp,
                 ]
             )
-            return_code = process.wait()
+            try:
+                process.communicate(timeout=TIMEOUT)
+            except TimeoutExpired:
+                process.kill()
+                raise
 
-            self.assertEqual(return_code, 0)
+            self.assertEqual(process.returncode, 0)
             self.assertTrue(os.path.isdir(temp + "/sam-app"))
 
     def test_init_new_app_template(self):
@@ -48,9 +54,13 @@ def test_init_new_app_template(self):
                     temp,
                 ]
             )
-            return_code = process.wait()
+            try:
+                process.communicate(timeout=TIMEOUT)
+            except TimeoutExpired:
+                process.kill()
+                raise
 
-            self.assertEqual(return_code, 0)
+            self.assertEqual(process.returncode, 0)
             self.assertTrue(os.path.isdir(temp + "/qs-scratch"))
 
     def test_init_command_java_maven(self):
@@ -72,9 +82,13 @@ def test_init_command_java_maven(self):
                     temp,
                 ]
             )
-            return_code = process.wait()
+            try:
+                process.communicate(timeout=TIMEOUT)
+            except TimeoutExpired:
+                process.kill()
+                raise
 
-            self.assertEqual(return_code, 0)
+            self.assertEqual(process.returncode, 0)
             self.assertTrue(os.path.isdir(temp + "/sam-app-maven"))
 
     def test_init_command_java_gradle(self):
@@ -96,9 +110,13 @@ def test_init_command_java_gradle(self):
                     temp,
                 ]
             )
-            return_code = process.wait()
+            try:
+                process.communicate(timeout=TIMEOUT)
+            except TimeoutExpired:
+                process.kill()
+                raise
 
-            self.assertEqual(return_code, 0)
+            self.assertEqual(process.returncode, 0)
             self.assertTrue(os.path.isdir(temp + "/sam-app-gradle"))
 
     def test_init_command_with_extra_context_parameter(self):
@@ -122,9 +140,13 @@ def test_init_command_with_extra_context_parameter(self):
                     temp,
                 ]
             )
-            return_code = process.wait()
+            try:
+                process.communicate(timeout=TIMEOUT)
+            except TimeoutExpired:
+                process.kill()
+                raise
 
-            self.assertEqual(return_code, 0)
+            self.assertEqual(process.returncode, 0)
             self.assertTrue(os.path.isdir(temp + "/sam-app-maven"))
 
     @staticmethod

tests/integration/local/generate_event/test_cli_integ.py

@@ -6,8 +6,8 @@
 class Test_EventGeneration_Integ(TestCase):
     def test_generate_event_substitution(self):
         process = Popen([Test_EventGeneration_Integ._get_command(), "local", "generate-event", "s3", "put"])
-        return_code = process.wait()
-        self.assertEqual(return_code, 0)
+        process.communicate()
+        self.assertEqual(process.returncode, 0)
 
     @staticmethod
     def _get_command():

tests/integration/local/invoke/runtimes/test_with_runtime_zips.py

@@ -3,13 +3,15 @@
 import os
 import tempfile
 
-from subprocess import Popen, PIPE
+from subprocess import Popen, PIPE, TimeoutExpired
 from nose_parameterized import parameterized, param
 import pytest
 
 from tests.integration.local.invoke.invoke_integ_base import InvokeIntegBase
 from pathlib import Path
 
+TIMEOUT = 300
+
 
 class TestWithDifferentLambdaRuntimeZips(InvokeIntegBase):
     template = Path("runtimes", "template.yaml")
@@ -35,10 +37,14 @@ def test_runtime_zip(self, function_name):
         )
 
         process = Popen(command_list, stdout=PIPE)
-        return_code = process.wait()
-
-        self.assertEqual(return_code, 0)
-        process_stdout = b"".join(process.stdout.readlines()).strip()
+        try:
+            stdout, _ = process.communicate(timeout=TIMEOUT)
+        except TimeoutExpired:
+            process.kill()
+            raise
+
+        self.assertEqual(process.returncode, 0)
+        process_stdout = stdout.strip()
         self.assertEqual(process_stdout.decode("utf-8"), '"Hello World"')
 
     @pytest.mark.timeout(timeout=300, method="thread")
@@ -50,8 +56,12 @@ def test_custom_provided_runtime(self):
         command_list = command_list + ["--skip-pull-image"]
 
         process = Popen(command_list, stdout=PIPE)
-        return_code = process.wait()
-
-        self.assertEqual(return_code, 0)
-        process_stdout = b"".join(process.stdout.readlines()).strip()
+        try:
+            stdout, _ = process.communicate(timeout=TIMEOUT)
+        except TimeoutExpired:
+            process.kill()
+            raise
+
+        self.assertEqual(process.returncode, 0)
+        process_stdout = stdout.strip()
         self.assertEqual(process_stdout.decode("utf-8"), '{"body":"hello 曰有冥 world 🐿","statusCode":200,"headers":{}}')

tests/integration/local/invoke/test_integrations_cli.py

@@ -630,10 +630,10 @@ def test_download_one_layer(self, function_logical_id):
             process.kill()
             raise
 
-        process_stdout = stdout.split(os.linesep)[-1:].strip()
+        process_stdout = stdout.decode("utf-8").strip().split(os.linesep)[-1]
         expected_output = '"Layer1"'
 
-        self.assertEqual(process_stdout.decode("utf-8"), expected_output)
+        self.assertEqual(process_stdout, expected_output)
 
     @parameterized.expand([("ChangedLayerVersionServerlessFunction"), ("ChangedLayerVersionLambdaFunction")])
     def test_publish_changed_download_layer(self, function_logical_id):
@@ -656,10 +656,10 @@ def test_publish_changed_download_layer(self, function_logical_id):
             process.kill()
             raise
 
-        process_stdout = stdout.split(os.linesep).strip()
+        process_stdout = stdout.decode("utf-8").strip().split(os.linesep)[-1]
         expected_output = '"Layer1"'
 
-        self.assertEqual(process_stdout.decode("utf-8"), expected_output)
+        self.assertEqual(process_stdout, expected_output)
 
         self.layer_utils.upsert_layer(
             layer_name=layer_name, ref_layer_name="ChangedLayerArn", layer_zip="changedlayer1.zip"
@@ -681,10 +681,10 @@ def test_publish_changed_download_layer(self, function_logical_id):
             process.kill()
             raise
 
-        process_stdout = stdout.split(os.linesep).strip()
+        process_stdout = stdout.decode("utf-8").strip().split(os.linesep)[-1]
         expected_output = '"Changed_Layer_1"'
 
-        self.assertEqual(process_stdout.decode("utf-8"), expected_output)
+        self.assertEqual(process_stdout, expected_output)
 
     @parameterized.expand([("TwoLayerVersionServerlessFunction"), ("TwoLayerVersionLambdaFunction")])
     def test_download_two_layers(self, function_logical_id):
@@ -707,10 +707,10 @@ def test_download_two_layers(self, function_logical_id):
 
         stdout = stdout
 
-        process_stdout = stdout.split(os.linesep).strip()
+        process_stdout = stdout.decode("utf-8").strip().split(os.linesep)[-1]
         expected_output = '"Layer2"'
 
-        self.assertEqual(process_stdout.decode("utf-8"), expected_output)
+        self.assertEqual(process_stdout, expected_output)
 
     def test_caching_two_layers(self):