From b50619b06df82a426d431e6bbe7a384bf1312146 Mon Sep 17 00:00:00 2001 From: Felipe Monteiro Date: Tue, 23 Oct 2018 12:06:08 -0400 Subject: [PATCH] refactor: Use temp path fixture to automatically clean up This patch set refactors uses of tempfile.mkdtemp() used throughout many tests in Pegleg which leaves lingering temporary directories around. Recently a fixture was introduced in [0] which automatically cleans up after itself. This patch set applies the fixture everywhere possible to minimize the testing footprint. [0] https://review.openstack.org/#/c/609818/20/tests/unit/fixtures.py Change-Id: Id4c1195c4f248b974a5396a429d651943a84ee83 --- tests/unit/conftest.py | 2 +- tests/unit/engine/test_util_files.py | 2 -- tests/unit/engine/util/test_git.py | 7 ++-- tests/unit/fixtures.py | 2 +- tests/unit/test_cli.py | 51 +++++++++++----------------- 5 files changed, 25 insertions(+), 39 deletions(-) diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py index fc0b171e..6dab893e 100644 --- a/tests/unit/conftest.py +++ b/tests/unit/conftest.py @@ -35,7 +35,7 @@ def restore_config(): config.GLOBAL_CONTEXT = original_global_context -@pytest.fixture(scope="class", autouse=True) +@pytest.fixture(scope="module", autouse=True) def clean_temporary_git_repos(): """Iterates through all temporarily created directories and deletes each one that was created for testing. diff --git a/tests/unit/engine/test_util_files.py b/tests/unit/engine/test_util_files.py index ea4fffdb..bf18a8f3 100644 --- a/tests/unit/engine/test_util_files.py +++ b/tests/unit/engine/test_util_files.py @@ -12,8 +12,6 @@ # See the License for the specific language governing permissions and # limitations under the License. -import tempfile - from pegleg import config from pegleg.engine.util import files from tests.unit.fixtures import create_tmp_deployment_files diff --git a/tests/unit/engine/util/test_git.py b/tests/unit/engine/util/test_git.py index e732814f..d86d0766 100644 --- a/tests/unit/engine/util/test_git.py +++ b/tests/unit/engine/util/test_git.py @@ -14,15 +14,14 @@ import os import shutil -import tempfile -import fixtures from git import Repo import mock import pytest from pegleg.engine import exceptions from pegleg.engine.util import git +from tests.unit.fixtures import temp_path from tests.unit import test_utils @@ -509,8 +508,8 @@ def test_is_repository(): subpath='deployment_files/site') -def test_is_repository_negative(): - assert not git.is_repository(tempfile.mkdtemp()) +def test_is_repository_negative(temp_path): + assert not git.is_repository(temp_path) @pytest.mark.skipif( diff --git a/tests/unit/fixtures.py b/tests/unit/fixtures.py index e9b0c605..b47c4e0e 100644 --- a/tests/unit/fixtures.py +++ b/tests/unit/fixtures.py @@ -158,7 +158,7 @@ schema: pegleg/SiteDefinition/v1 @pytest.fixture() -def temp_clone_path(): +def temp_path(): temp_folder = tempfile.mkdtemp() try: yield temp_folder diff --git a/tests/unit/test_cli.py b/tests/unit/test_cli.py index 79a6ff6a..4f730548 100644 --- a/tests/unit/test_cli.py +++ b/tests/unit/test_cli.py @@ -14,7 +14,6 @@ import os import shutil -import tempfile from click.testing import CliRunner import mock @@ -24,7 +23,7 @@ from pegleg import cli from pegleg.engine import errorcodes from pegleg.engine.util import git from tests.unit import test_utils -from tests.unit.fixtures import temp_clone_path +from tests.unit.fixtures import temp_path @pytest.mark.skipif( @@ -65,7 +64,7 @@ class TestSiteCLIOptions(BaseCLIActionTest): ### clone_path tests ### def test_list_sites_using_remote_repo_and_clone_path_option( - self, temp_clone_path): + self, temp_path): """Validates clone_path (-p) option is working properly with site list action when using remote repo. Verify that the repo was cloned in the clone_path @@ -80,15 +79,15 @@ class TestSiteCLIOptions(BaseCLIActionTest): # Note that the -p option is used to specify the clone_folder site_list = self.runner.invoke( - cli.site, ['-p', temp_clone_path, '-r', repo_url, 'list']) + cli.site, ['-p', temp_path, '-r', repo_url, 'list']) assert site_list.exit_code == 0 # Verify that the repo was cloned into the clone_path - assert os.path.exists(os.path.join(temp_clone_path, self.repo_name)) - assert git.is_repository(os.path.join(temp_clone_path, self.repo_name)) + assert os.path.exists(os.path.join(temp_path, self.repo_name)) + assert git.is_repository(os.path.join(temp_path, self.repo_name)) def test_list_sites_using_local_repo_and_clone_path_option( - self, temp_clone_path): + self, temp_path): """Validates clone_path (-p) option is working properly with site list action when using a local repo. Verify that the clone_path has NO effect when using a local repo @@ -102,12 +101,11 @@ class TestSiteCLIOptions(BaseCLIActionTest): # Note that the -p option is used to specify the clone_folder site_list = self.runner.invoke( - cli.site, ['-p', temp_clone_path, '-r', repo_path, 'list']) + cli.site, ['-p', temp_path, '-r', repo_path, 'list']) assert site_list.exit_code == 0 # Verify that passing in clone_path when using local repo has no effect - assert not os.path.exists( - os.path.join(temp_clone_path, self.repo_name)) + assert not os.path.exists(os.path.join(temp_path, self.repo_name)) class TestSiteCLIOptionsNegative(BaseCLIActionTest): @@ -116,7 +114,7 @@ class TestSiteCLIOptionsNegative(BaseCLIActionTest): ### Negative clone_path tests ### def test_list_sites_using_remote_repo_and_reuse_clone_path_option( - self, temp_clone_path): + self, temp_path): """Validates clone_path (-p) option is working properly with site list action when using remote repo. Verify that the same repo can't be cloned in the same clone_path if it already exists @@ -131,14 +129,14 @@ class TestSiteCLIOptionsNegative(BaseCLIActionTest): # Note that the -p option is used to specify the clone_folder site_list = self.runner.invoke( - cli.site, ['-p', temp_clone_path, '-r', repo_url, 'list']) + cli.site, ['-p', temp_path, '-r', repo_url, 'list']) - assert git.is_repository(os.path.join(temp_clone_path, self.repo_name)) + assert git.is_repository(os.path.join(temp_path, self.repo_name)) # Run site list for a second time to validate that the repo can't be # cloned twice in the same clone_path site_list = self.runner.invoke( - cli.site, ['-p', temp_clone_path, '-r', repo_url, 'list']) + cli.site, ['-p', temp_path, '-r', repo_url, 'list']) assert site_list.exit_code == 1 msg = "The repository already exists in the given path. Either " \ @@ -146,23 +144,13 @@ class TestSiteCLIOptionsNegative(BaseCLIActionTest): "repository as the site repository (-r)." assert msg in site_list.output - @classmethod - def teardown_class(cls): - # Cleanup temporary Git repos. - root_tempdir = tempfile.gettempdir() - for tempdir in os.listdir(root_tempdir): - path = os.path.join(root_tempdir, tempdir) - if git.is_repository(path): - shutil.rmtree(path, ignore_errors=True) - class TestSiteCliActions(BaseCLIActionTest): """Tests site-level CLI actions.""" ### Collect tests ### - def _validate_collect_site_action(self, repo_path_or_url): - save_location = tempfile.mkdtemp() + def _validate_collect_site_action(self, repo_path_or_url, save_location): result = self.runner.invoke(cli.site, [ '-r', repo_path_or_url, 'collect', self.site_name, '-s', save_location @@ -176,7 +164,7 @@ class TestSiteCliActions(BaseCLIActionTest): # are written out to sensibly named files like airship-treasuremap.yaml assert collected_files[0] == ("%s.yaml" % self.repo_name) - def test_collect_using_remote_repo_url(self): + def test_collect_using_remote_repo_url(self, temp_path): """Validates collect action using a remote URL.""" # Scenario: # @@ -186,9 +174,10 @@ class TestSiteCliActions(BaseCLIActionTest): repo_url = 'https://github.com/openstack/%s@%s' % (self.repo_name, self.repo_rev) - self._validate_collect_site_action(repo_url) + self._validate_collect_site_action(repo_url, temp_path) - def test_collect_using_remote_repo_url_ending_with_dot_git(self): + def test_collect_using_remote_repo_url_ending_with_dot_git( + self, temp_path): """Validates collect action using a remote URL ending in .git.""" # Scenario: # @@ -198,9 +187,9 @@ class TestSiteCliActions(BaseCLIActionTest): repo_url = 'https://github.com/openstack/%s@%s.git' % (self.repo_name, self.repo_rev) - self._validate_collect_site_action(repo_url) + self._validate_collect_site_action(repo_url, temp_path) - def test_collect_using_local_path(self): + def test_collect_using_local_path(self, temp_path): """Validates collect action using a path to a local repo.""" # Scenario: # @@ -209,7 +198,7 @@ class TestSiteCliActions(BaseCLIActionTest): # 3) Check that expected file name is there repo_path = self.treasuremap_path - self._validate_collect_site_action(repo_path) + self._validate_collect_site_action(repo_path, temp_path) ### Lint tests ###