From a7f3488d1bf1c5f5a98c20466cade1d154bcf908 Mon Sep 17 00:00:00 2001 From: Sam Liu Date: Thu, 15 Jul 2021 15:24:51 -0700 Subject: [PATCH 1/2] ci: Speed up unit test by caching the git clone --- samcli/lib/utils/git_repo.py | 3 +- tests/unit/commands/init/test_cli.py | 37 ++++++++++++++++++++++ tests/unit/commands/init/test_templates.py | 8 ++--- tests/unit/lib/utils/test_git_repo.py | 20 ++++++------ 4 files changed, 53 insertions(+), 15 deletions(-) diff --git a/samcli/lib/utils/git_repo.py b/samcli/lib/utils/git_repo.py index 33e4597726..b428e62ef0 100644 --- a/samcli/lib/utils/git_repo.py +++ b/samcli/lib/utils/git_repo.py @@ -6,6 +6,7 @@ import shutil import subprocess from pathlib import Path +from subprocess import check_output from typing import Optional from samcli.lib.utils import osutils @@ -118,7 +119,7 @@ def clone(self, clone_dir: Path, clone_name: str, replace_existing: bool = False temp_path = os.path.normpath(os.path.join(tempdir, clone_name)) git_executable: str = GitRepo._git_executable() LOG.info("\nCloning from %s", self.url) - subprocess.check_output( + check_output( [git_executable, "clone", self.url, clone_name], cwd=tempdir, stderr=subprocess.STDOUT, diff --git a/tests/unit/commands/init/test_cli.py b/tests/unit/commands/init/test_cli.py index 5d61386acc..2b69356b14 100644 --- a/tests/unit/commands/init/test_cli.py +++ b/tests/unit/commands/init/test_cli.py @@ -1,5 +1,9 @@ import os +import shutil +import subprocess +import tempfile from pathlib import Path +from typing import Dict, Any from unittest import TestCase from unittest.mock import patch, ANY @@ -12,6 +16,7 @@ from samcli.commands.init import do_cli as init_cli from samcli.commands.init.init_templates import InitTemplates, APP_TEMPLATES_REPO_URL from samcli.lib.init import GenerateProjectFailedError +from samcli.lib.utils import osutils from samcli.lib.utils.git_repo import GitRepo from samcli.lib.utils.packagetype import IMAGE, ZIP @@ -43,6 +48,38 @@ def setUp(self): self.extra_context = '{"project_name": "testing project", "runtime": "python3.6"}' self.extra_context_as_json = {"project_name": "testing project", "runtime": "python3.6"} + # setup cache for clone, so that if `git clone` is called multiple times on the same URL, + # only one clone will happen. + clone_cache: Dict[str, Path] + patcher: Any + + @classmethod + def setUpClass(cls) -> None: + # Make (url -> directory) cache to avoid cloning the same thing twice + cls.clone_cache = {} + cls.patcher = patch("samcli.lib.utils.git_repo.check_output", side_effect=cls.check_output_mock) + cls.patcher.start() + + @classmethod + def tearDownClass(cls) -> None: + cls.patcher.stop() + for _, directory in cls.clone_cache.items(): + shutil.rmtree(directory.parent) + + @classmethod + def check_output_mock(cls, commands, cwd, stderr): + git_executable, _, url, clone_name = commands + if url not in cls.clone_cache: + tempdir = tempfile.NamedTemporaryFile(delete=False).name + subprocess.check_output( + [git_executable, "clone", url, clone_name], + cwd=tempdir, + stderr=stderr, + ) + cls.clone_cache[url] = Path(tempdir, clone_name) + + osutils.copytree(str(cls.clone_cache[url]), str(Path(cwd, clone_name))) + @patch("samcli.lib.utils.git_repo.GitRepo.clone") @patch("samcli.commands.init.init_generator.generate_project") def test_init_cli(self, generate_project_patch, git_repo_clone_mock): diff --git a/tests/unit/commands/init/test_templates.py b/tests/unit/commands/init/test_templates.py index 0e11d6aed9..0492905a6a 100644 --- a/tests/unit/commands/init/test_templates.py +++ b/tests/unit/commands/init/test_templates.py @@ -10,7 +10,7 @@ class TestTemplates(TestCase): - @patch("subprocess.check_output") + @patch("samcli.lib.utils.git_repo.check_output") @patch("samcli.lib.utils.git_repo.GitRepo._git_executable") @patch("samcli.lib.utils.git_repo.GitRepo._ensure_clone_directory_exists") @patch("shutil.copytree") @@ -37,7 +37,7 @@ def test_location_from_app_template_zip(self, subprocess_mock, git_exec_mock, cd location = it.location_from_app_template(ZIP, "ruby2.5", None, "bundler", "hello-world") self.assertTrue(search("mock-ruby-template", location)) - @patch("subprocess.check_output") + @patch("samcli.lib.utils.git_repo.check_output") @patch("samcli.lib.utils.git_repo.GitRepo._git_executable") @patch("samcli.lib.utils.git_repo.GitRepo._ensure_clone_directory_exists") @patch("shutil.copytree") @@ -71,7 +71,7 @@ def test_location_from_app_template_image(self, subprocess_mock, git_exec_mock, @patch("samcli.lib.utils.git_repo.GitRepo._ensure_clone_directory_exists") def test_fallback_options(self, git_exec_mock, prompt_mock, cd_mock): prompt_mock.return_value = "1" - with patch("subprocess.check_output", new_callable=MagicMock) as mock_sub: + with patch("samcli.lib.utils.git_repo.check_output", new_callable=MagicMock) as mock_sub: with patch("samcli.cli.global_config.GlobalConfig.config_dir", new_callable=PropertyMock) as mock_cfg: mock_sub.side_effect = OSError("Fail") mock_cfg.return_value = Path("/tmp/test-sam") @@ -85,7 +85,7 @@ def test_fallback_options(self, git_exec_mock, prompt_mock, cd_mock): @patch("samcli.lib.utils.git_repo.GitRepo._ensure_clone_directory_exists") def test_fallback_process_error(self, git_exec_mock, prompt_mock, cd_mock): prompt_mock.return_value = "1" - with patch("subprocess.check_output", new_callable=MagicMock) as mock_sub: + with patch("samcli.lib.utils.git_repo.check_output", new_callable=MagicMock) as mock_sub: with patch("samcli.cli.global_config.GlobalConfig.config_dir", new_callable=PropertyMock) as mock_cfg: mock_sub.side_effect = subprocess.CalledProcessError("fail", "fail", "not found".encode("utf-8")) mock_cfg.return_value = Path("/tmp/test-sam") diff --git a/tests/unit/lib/utils/test_git_repo.py b/tests/unit/lib/utils/test_git_repo.py index 645dc5c2de..5030ffaec1 100644 --- a/tests/unit/lib/utils/test_git_repo.py +++ b/tests/unit/lib/utils/test_git_repo.py @@ -48,7 +48,7 @@ def test_git_executable_fails(self, mock_popen): @patch("samcli.lib.utils.git_repo.Path.exists") @patch("samcli.lib.utils.git_repo.shutil") - @patch("samcli.lib.utils.git_repo.subprocess.check_output") + @patch("samcli.lib.utils.git_repo.check_output") @patch("samcli.lib.utils.git_repo.subprocess.Popen") @patch("samcli.lib.utils.git_repo.platform.system") def test_clone_happy_case(self, platform_mock, popen_mock, check_output_mock, shutil_mock, path_exist_mock): @@ -65,7 +65,7 @@ def test_clone_happy_case(self, platform_mock, popen_mock, check_output_mock, sh @patch("samcli.lib.utils.git_repo.Path.exists") @patch("samcli.lib.utils.git_repo.shutil") - @patch("samcli.lib.utils.git_repo.subprocess.check_output") + @patch("samcli.lib.utils.git_repo.check_output") @patch("samcli.lib.utils.git_repo.subprocess.Popen") @patch("samcli.lib.utils.git_repo.platform.system") def test_clone_create_new_local_repo( @@ -79,7 +79,7 @@ def test_clone_create_new_local_repo( @patch("samcli.lib.utils.git_repo.Path.exists") @patch("samcli.lib.utils.git_repo.shutil") - @patch("samcli.lib.utils.git_repo.subprocess.check_output") + @patch("samcli.lib.utils.git_repo.check_output") @patch("samcli.lib.utils.git_repo.subprocess.Popen") @patch("samcli.lib.utils.git_repo.platform.system") def test_clone_replace_current_local_repo_if_replace_existing_flag_is_set( @@ -93,7 +93,7 @@ def test_clone_replace_current_local_repo_if_replace_existing_flag_is_set( shutil_mock.ignore_patterns.assert_called_with("*.git") @patch("samcli.lib.utils.git_repo.Path.exists") - @patch("samcli.lib.utils.git_repo.subprocess.check_output") + @patch("samcli.lib.utils.git_repo.check_output") @patch("samcli.lib.utils.git_repo.subprocess.Popen") @patch("samcli.lib.utils.git_repo.platform.system") def test_clone_fail_if_current_local_repo_exists_and_replace_existing_flag_is_not_set( @@ -104,7 +104,7 @@ def test_clone_fail_if_current_local_repo_exists_and_replace_existing_flag_is_no self.repo.clone(clone_dir=self.local_clone_dir, clone_name=REPO_NAME) # replace_existing=False by default @patch("samcli.lib.utils.git_repo.shutil") - @patch("samcli.lib.utils.git_repo.subprocess.check_output") + @patch("samcli.lib.utils.git_repo.check_output") @patch("samcli.lib.utils.git_repo.subprocess.Popen") @patch("samcli.lib.utils.git_repo.platform.system") def test_clone_attempt_is_set_to_true_after_clone(self, platform_mock, popen_mock, check_output_mock, shutil_mock): @@ -113,7 +113,7 @@ def test_clone_attempt_is_set_to_true_after_clone(self, platform_mock, popen_moc self.assertTrue(self.repo.clone_attempted) @patch("samcli.lib.utils.git_repo.shutil") - @patch("samcli.lib.utils.git_repo.subprocess.check_output") + @patch("samcli.lib.utils.git_repo.check_output") @patch("samcli.lib.utils.git_repo.subprocess.Popen") @patch("samcli.lib.utils.git_repo.platform.system") def test_clone_attempt_is_set_to_true_even_if_clone_failed( @@ -129,7 +129,7 @@ def test_clone_attempt_is_set_to_true_even_if_clone_failed( self.assertTrue(self.repo.clone_attempted) @patch("samcli.lib.utils.git_repo.shutil") - @patch("samcli.lib.utils.git_repo.subprocess.check_output") + @patch("samcli.lib.utils.git_repo.check_output") @patch("samcli.lib.utils.git_repo.subprocess.Popen") @patch("samcli.lib.utils.git_repo.platform.system") def test_clone_failed_to_create_the_clone_directory( @@ -147,7 +147,7 @@ def test_clone_failed_to_create_the_clone_directory( shutil_mock.assert_not_called() @patch("samcli.lib.utils.git_repo.shutil") - @patch("samcli.lib.utils.git_repo.subprocess.check_output") + @patch("samcli.lib.utils.git_repo.check_output") @patch("samcli.lib.utils.git_repo.subprocess.Popen") @patch("samcli.lib.utils.git_repo.platform.system") def test_clone_when_the_subprocess_fail(self, platform_mock, popen_mock, check_output_mock, shutil_mock): @@ -156,7 +156,7 @@ def test_clone_when_the_subprocess_fail(self, platform_mock, popen_mock, check_o self.repo.clone(clone_dir=self.local_clone_dir, clone_name=REPO_NAME) @patch("samcli.lib.utils.git_repo.LOG") - @patch("samcli.lib.utils.git_repo.subprocess.check_output") + @patch("samcli.lib.utils.git_repo.check_output") @patch("samcli.lib.utils.git_repo.subprocess.Popen") @patch("samcli.lib.utils.git_repo.platform.system") def test_clone_when_the_git_repo_not_found(self, platform_mock, popen_mock, check_output_mock, log_mock): @@ -170,7 +170,7 @@ def test_clone_when_the_git_repo_not_found(self, platform_mock, popen_mock, chec @patch("samcli.lib.utils.git_repo.Path.exists") @patch("samcli.lib.utils.git_repo.shutil") - @patch("samcli.lib.utils.git_repo.subprocess.check_output") + @patch("samcli.lib.utils.git_repo.check_output") @patch("samcli.lib.utils.git_repo.subprocess.Popen") @patch("samcli.lib.utils.git_repo.platform.system") def test_clone_when_failed_to_move_cloned_repo_from_temp_to_final_destination( From 2ee9f23259dd850e6960e94e2e42da8846381b1c Mon Sep 17 00:00:00 2001 From: Sam Liu Date: Thu, 15 Jul 2021 15:49:09 -0700 Subject: [PATCH 2/2] Add comments --- samcli/lib/utils/git_repo.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/samcli/lib/utils/git_repo.py b/samcli/lib/utils/git_repo.py index b428e62ef0..c29b82a5e8 100644 --- a/samcli/lib/utils/git_repo.py +++ b/samcli/lib/utils/git_repo.py @@ -6,6 +6,9 @@ import shutil import subprocess from pathlib import Path + +# import check_output alone so that it can be patched without affecting +# other parts of subprocess. from subprocess import check_output from typing import Optional