From d14816cbafdae7e78beee2910ec0827db16122ef Mon Sep 17 00:00:00 2001 From: Todd Gamblin Date: Sun, 15 Oct 2017 23:59:53 -0700 Subject: [PATCH] Spack tests no longer clutter var/spack/stage - Tests use a session-scoped mock stage directory so as not to interfere with the real install. - Every test is forced to clean up after itself with an additional check. We now automatically assert that no new files have been added to `spack.stage_path` during each test. - This means that tests that fail installs now need to clean up their stages, but in all other cases the check is useful. --- lib/spack/spack/package.py | 1 + lib/spack/spack/test/cmd/install.py | 4 + lib/spack/spack/test/conftest.py | 137 ++++++++++++------ lib/spack/spack/test/install.py | 54 +++---- lib/spack/spack/test/stage.py | 63 ++++---- .../builtin.mock/packages/canfail/package.py | 12 +- 6 files changed, 158 insertions(+), 113 deletions(-) diff --git a/lib/spack/spack/package.py b/lib/spack/spack/package.py index e941aff499..ebb96eb739 100644 --- a/lib/spack/spack/package.py +++ b/lib/spack/spack/package.py @@ -1405,6 +1405,7 @@ def build_process(): echo = logger.echo self.log() + # Run post install hooks before build stage is removed. spack.hooks.post_install(self.spec) diff --git a/lib/spack/spack/test/cmd/install.py b/lib/spack/spack/test/cmd/install.py index 7a04b59748..124836ef9a 100644 --- a/lib/spack/spack/test/cmd/install.py +++ b/lib/spack/spack/test/cmd/install.py @@ -150,6 +150,7 @@ def test_package_output(tmpdir, capsys, install_mockery, mock_fetch): assert "'install'\nAFTER INSTALL" in out +@pytest.mark.disable_clean_stage_check def test_install_output_on_build_error(builtin_mock, mock_archive, mock_fetch, config, install_mockery, capfd): # capfd interferes with Spack's capturing @@ -161,6 +162,7 @@ def test_install_output_on_build_error(builtin_mock, mock_archive, mock_fetch, assert 'configure: error: cannot run C compiled programs.' in out +@pytest.mark.disable_clean_stage_check def test_install_output_on_python_error(builtin_mock, mock_archive, mock_fetch, config, install_mockery): out = install('failing-build', fail_on_error=False) @@ -169,6 +171,7 @@ def test_install_output_on_python_error(builtin_mock, mock_archive, mock_fetch, assert 'raise InstallError("Expected failure.")' in out +@pytest.mark.disable_clean_stage_check def test_install_with_source( builtin_mock, mock_archive, mock_fetch, config, install_mockery): """Verify that source has been copied into place.""" @@ -180,6 +183,7 @@ def test_install_with_source( os.path.join(src, 'configure')) +@pytest.mark.disable_clean_stage_check def test_show_log_on_error(builtin_mock, mock_archive, mock_fetch, config, install_mockery, capfd): """Make sure --show-log-on-error works.""" diff --git a/lib/spack/spack/test/conftest.py b/lib/spack/spack/test/conftest.py index 519f54e6c9..657d809dba 100644 --- a/lib/spack/spack/test/conftest.py +++ b/lib/spack/spack/test/conftest.py @@ -25,14 +25,16 @@ import collections import copy import os -import re import shutil +import re import ordereddict_backport import py import pytest +from llnl.util.filesystem import remove_linked_tree + import spack import spack.architecture import spack.database @@ -71,6 +73,64 @@ def no_chdir(): assert os.getcwd() == original_wd +@pytest.fixture(scope='session', autouse=True) +def mock_stage(tmpdir_factory): + """Mocks up a fake stage directory for use by tests.""" + stage_path = spack.stage_path + new_stage = str(tmpdir_factory.mktemp('mock_stage')) + spack.stage_path = new_stage + yield new_stage + spack.stage_path = stage_path + + +@pytest.fixture(scope='session') +def _ignore_stage_files(): + """Session-scoped helper for check_for_leftover_stage_files. + + Used to track which leftover files in the stage have been seen. + """ + # to start with, ignore the .lock file at the stage root. + return set(['.lock']) + + +def remove_whatever_it_is(path): + """Type-agnostic remove.""" + if os.path.isfile(path): + os.remove(path) + elif os.path.islink(path): + remove_linked_tree(path) + else: + shutil.rmtree(path) + + +@pytest.fixture(scope='function', autouse=True) +def check_for_leftover_stage_files(request, mock_stage, _ignore_stage_files): + """Ensure that each test leaves a clean stage when done. + + This can be disabled for tests that are expected to dirty the stage + by adding:: + + @pytest.mark.disable_clean_stage_check + + to tests that need it. + """ + yield + + files_in_stage = set() + if os.path.exists(spack.stage_path): + files_in_stage = set( + os.listdir(spack.stage_path)) - _ignore_stage_files + + if 'disable_clean_stage_check' in request.keywords: + # clean up after tests that are expected to be dirty + for f in files_in_stage: + path = os.path.join(spack.stage_path, f) + remove_whatever_it_is(path) + else: + _ignore_stage_files |= files_in_stage + assert not files_in_stage + + @pytest.fixture(autouse=True) def mock_fetch_cache(monkeypatch): """Substitutes spack.fetch_cache with a mock object that does nothing @@ -193,7 +253,7 @@ def config(configuration_dir): @pytest.fixture(scope='module') -def database(mock_stage, tmpdir_factory, builtin_mock, config): +def database(tmpdir_factory, builtin_mock, config): """Creates a mock database with some packages installed note that the ref count for dyninst here will be 3, as it's recycled across each install. @@ -296,18 +356,8 @@ def refresh_db_on_exit(database): database.refresh() -@pytest.fixture(scope='session') -def mock_stage(tmpdir_factory): - """Mocks up a fake stage directory for use by tests.""" - stage_path = spack.stage_path - new_stage = str(tmpdir_factory.mktemp('mock_stage')) - spack.stage_path = new_stage - yield - spack.stage_path = stage_path - - @pytest.fixture() -def install_mockery(mock_stage, tmpdir, config, builtin_mock): +def install_mockery(tmpdir, config, builtin_mock): """Hooks a fake install directory, DB, and stage directory into Spack.""" layout = spack.store.layout db = spack.store.db @@ -350,14 +400,13 @@ def fake_fn(self): @pytest.fixture(scope='session') -def mock_archive(mock_stage): +def mock_archive(tmpdir_factory): """Creates a very simple archive directory with a configure script and a makefile that installs to a prefix. Tars it up into an archive. """ tar = spack.util.executable.which('tar', required=True) - stage = spack.stage.Stage('mock-archive-stage') - tmpdir = py.path.local(stage.path) + tmpdir = tmpdir_factory.mktemp('mock-archive-dir') repo_name = 'mock-archive-repo' tmpdir.ensure(repo_name, dir=True) repodir = tmpdir.join(repo_name) @@ -392,17 +441,16 @@ def mock_archive(mock_stage): url=('file://' + archive_file), archive_file=archive_file, path=str(repodir)) - stage.destroy() @pytest.fixture(scope='session') -def mock_git_repository(mock_stage): +def mock_git_repository(tmpdir_factory): """Creates a very simple git repository with two branches and two commits. """ git = spack.util.executable.which('git', required=True) - stage = spack.stage.Stage('mock-git-stage') - tmpdir = py.path.local(stage.path) + + tmpdir = tmpdir_factory.mktemp('mock-git-repo-dir') repo_name = 'mock-git-repo' tmpdir.ensure(repo_name, dir=True) repodir = tmpdir.join(repo_name) @@ -449,7 +497,6 @@ def mock_git_repository(mock_stage): r1_file = branch_file Bunch = spack.util.pattern.Bunch - checks = { 'master': Bunch( revision='master', file=r0_file, args={'git': str(repodir)} @@ -469,15 +516,14 @@ def mock_git_repository(mock_stage): t = Bunch(checks=checks, url=url, hash=rev_hash, path=str(repodir)) yield t - stage.destroy() @pytest.fixture(scope='session') -def mock_hg_repository(mock_stage): +def mock_hg_repository(tmpdir_factory): """Creates a very simple hg repository with two commits.""" hg = spack.util.executable.which('hg', required=True) - stage = spack.stage.Stage('mock-hg-stage') - tmpdir = py.path.local(stage.path) + + tmpdir = tmpdir_factory.mktemp('mock-hg-repo-dir') repo_name = 'mock-hg-repo' tmpdir.ensure(repo_name, dir=True) repodir = tmpdir.join(repo_name) @@ -504,7 +550,6 @@ def mock_hg_repository(mock_stage): r1 = get_rev() Bunch = spack.util.pattern.Bunch - checks = { 'default': Bunch( revision=r1, file=r1_file, args={'hg': str(repodir)} @@ -517,17 +562,15 @@ def mock_hg_repository(mock_stage): } t = Bunch(checks=checks, url=url, hash=get_rev, path=str(repodir)) yield t - stage.destroy() @pytest.fixture(scope='session') -def mock_svn_repository(mock_stage): +def mock_svn_repository(tmpdir_factory): """Creates a very simple svn repository with two commits.""" svn = spack.util.executable.which('svn', required=True) svnadmin = spack.util.executable.which('svnadmin', required=True) - stage = spack.stage.Stage('mock-svn-stage') - tmpdir = py.path.local(stage.path) + tmpdir = tmpdir_factory.mktemp('mock-svn-stage') repo_name = 'mock-svn-repo' tmpdir.ensure(repo_name, dir=True) repodir = tmpdir.join(repo_name) @@ -563,26 +606,24 @@ def mock_svn_repository(mock_stage): r0 = '1' r1 = '2' - Bunch = spack.util.pattern.Bunch + Bunch = spack.util.pattern.Bunch + checks = { + 'default': Bunch( + revision=r1, file=r1_file, args={'svn': url}), + 'rev0': Bunch( + revision=r0, file=r0_file, args={ + 'svn': url, 'revision': r0}) + } - checks = { - 'default': Bunch( - revision=r1, file=r1_file, args={'svn': url}), - 'rev0': Bunch( - revision=r0, file=r0_file, args={ - 'svn': url, 'revision': r0}) - } - - def get_rev(): - output = svn('info', output=str) - assert "Revision" in output - for line in output.split('\n'): - match = re.match(r'Revision: (\d+)', line) - if match: - return match.group(1) - - t = Bunch(checks=checks, url=url, hash=get_rev, path=str(repodir)) + def get_rev(): + output = svn('info', output=str) + assert "Revision" in output + for line in output.split('\n'): + match = re.match(r'Revision: (\d+)', line) + if match: + return match.group(1) + t = Bunch(checks=checks, url=url, hash=get_rev, path=str(repodir)) yield t diff --git a/lib/spack/spack/test/install.py b/lib/spack/spack/test/install.py index 454adbac5e..cf08642dfa 100644 --- a/lib/spack/spack/test/install.py +++ b/lib/spack/spack/test/install.py @@ -69,46 +69,49 @@ def __init__(self, wrapped_stage): self.test_destroyed = False def __enter__(self): + self.create() return self - def __exit__(self, *exc): - return True + def __exit__(self, exc_type, exc_val, exc_tb): + if exc_type is None: + self.destroy() def destroy(self): self.test_destroyed = True self.wrapped_stage.destroy() + def create(self): + self.wrapped_stage.create() + def __getattr__(self, attr): return getattr(self.wrapped_stage, attr) def test_partial_install_delete_prefix_and_stage(install_mockery, mock_fetch): - spec = Spec('canfail') - spec.concretize() + spec = Spec('canfail').concretized() pkg = spack.repo.get(spec) remove_prefix = spack.package.Package.remove_prefix instance_rm_prefix = pkg.remove_prefix try: + pkg.succeed = False spack.package.Package.remove_prefix = mock_remove_prefix with pytest.raises(MockInstallError): pkg.do_install() assert os.path.isdir(pkg.prefix) rm_prefix_checker = RemovePrefixChecker(instance_rm_prefix) spack.package.Package.remove_prefix = rm_prefix_checker.remove_prefix - setattr(pkg, 'succeed', True) + + pkg.succeed = True pkg.stage = MockStage(pkg.stage) + pkg.do_install(restage=True) assert rm_prefix_checker.removed assert pkg.stage.test_destroyed assert pkg.installed + finally: spack.package.Package.remove_prefix = remove_prefix - pkg._stage = None - try: - delattr(pkg, 'succeed') - except AttributeError: - pass def test_dont_add_patches_to_installed_package(install_mockery, mock_fetch): @@ -141,53 +144,50 @@ def test_installed_dependency_request_conflicts( dependent.concretize() +@pytest.mark.disable_clean_stage_check def test_partial_install_keep_prefix(install_mockery, mock_fetch): - spec = Spec('canfail') - spec.concretize() + spec = Spec('canfail').concretized() pkg = spack.repo.get(spec) + # Normally the stage should start unset, but other tests set it pkg._stage = None remove_prefix = spack.package.Package.remove_prefix try: # If remove_prefix is called at any point in this test, that is an # error + pkg.succeed = False # make the build fail spack.package.Package.remove_prefix = mock_remove_prefix with pytest.raises(spack.build_environment.ChildError): pkg.do_install(keep_prefix=True) assert os.path.exists(pkg.prefix) - setattr(pkg, 'succeed', True) + + pkg.succeed = True # make the build succeed pkg.stage = MockStage(pkg.stage) pkg.do_install(keep_prefix=True) assert pkg.installed assert not pkg.stage.test_destroyed + finally: spack.package.Package.remove_prefix = remove_prefix - pkg._stage = None - try: - delattr(pkg, 'succeed') - except AttributeError: - pass def test_second_install_no_overwrite_first(install_mockery, mock_fetch): - spec = Spec('canfail') - spec.concretize() + spec = Spec('canfail').concretized() pkg = spack.repo.get(spec) remove_prefix = spack.package.Package.remove_prefix try: spack.package.Package.remove_prefix = mock_remove_prefix - setattr(pkg, 'succeed', True) + + pkg.succeed = True pkg.do_install() assert pkg.installed + # If Package.install is called after this point, it will fail - delattr(pkg, 'succeed') + pkg.succeed = False pkg.do_install() + finally: spack.package.Package.remove_prefix = remove_prefix - try: - delattr(pkg, 'succeed') - except AttributeError: - pass def test_store(install_mockery, mock_fetch): @@ -196,9 +196,11 @@ def test_store(install_mockery, mock_fetch): pkg.do_install() +@pytest.mark.disable_clean_stage_check def test_failing_build(install_mockery, mock_fetch): spec = Spec('failing-build').concretized() pkg = spec.package + with pytest.raises(spack.build_environment.ChildError): pkg.do_install() diff --git a/lib/spack/spack/test/stage.py b/lib/spack/spack/test/stage.py index ffd2ed5afb..aca28ea8be 100644 --- a/lib/spack/spack/test/stage.py +++ b/lib/spack/spack/test/stage.py @@ -26,11 +26,12 @@ import collections import os +from llnl.util.filesystem import join_path, working_dir + import pytest import spack import spack.stage import spack.util.executable -from llnl.util.filesystem import join_path, working_dir from spack.stage import Stage @@ -236,24 +237,20 @@ def test_fetch(self, mock_archive): check_destroy(stage, self.stage_name) def test_no_search_if_default_succeeds( - self, mock_archive, failing_search_fn - ): - with Stage( - mock_archive.url, - name=self.stage_name, - search_fn=failing_search_fn - ) as stage: + self, mock_archive, failing_search_fn): + stage = Stage(mock_archive.url, + name=self.stage_name, + search_fn=failing_search_fn) + with stage: stage.fetch() check_destroy(stage, self.stage_name) def test_no_search_mirror_only( - self, failing_fetch_strategy, failing_search_fn - ): - with Stage( - failing_fetch_strategy, - name=self.stage_name, - search_fn=failing_search_fn - ) as stage: + self, failing_fetch_strategy, failing_search_fn): + stage = Stage(failing_fetch_strategy, + name=self.stage_name, + search_fn=failing_search_fn) + with stage: try: stage.fetch(mirror_only=True) except spack.fetch_strategy.FetchError: @@ -261,11 +258,10 @@ def test_no_search_mirror_only( check_destroy(stage, self.stage_name) def test_search_if_default_fails(self, failing_fetch_strategy, search_fn): - with Stage( - failing_fetch_strategy, - name=self.stage_name, - search_fn=search_fn - ) as stage: + stage = Stage(failing_fetch_strategy, + name=self.stage_name, + search_fn=search_fn) + with stage: try: stage.fetch(mirror_only=False) except spack.fetch_strategy.FetchError: @@ -303,40 +299,43 @@ def test_restage(self, mock_archive): check_destroy(stage, self.stage_name) def test_no_keep_without_exceptions(self, mock_archive): - with Stage( - mock_archive.url, name=self.stage_name, keep=False - ) as stage: + stage = Stage(mock_archive.url, name=self.stage_name, keep=False) + with stage: pass check_destroy(stage, self.stage_name) + @pytest.mark.disable_clean_stage_check def test_keep_without_exceptions(self, mock_archive): - with Stage( - mock_archive.url, name=self.stage_name, keep=True - ) as stage: + stage = Stage(mock_archive.url, name=self.stage_name, keep=True) + with stage: pass path = get_stage_path(stage, self.stage_name) assert os.path.isdir(path) + @pytest.mark.disable_clean_stage_check def test_no_keep_with_exceptions(self, mock_archive): class ThisMustFailHere(Exception): pass + + stage = Stage(mock_archive.url, name=self.stage_name, keep=False) try: - with Stage( - mock_archive.url, name=self.stage_name, keep=False - ) as stage: + with stage: raise ThisMustFailHere() + except ThisMustFailHere: path = get_stage_path(stage, self.stage_name) assert os.path.isdir(path) + @pytest.mark.disable_clean_stage_check def test_keep_exceptions(self, mock_archive): class ThisMustFailHere(Exception): pass + + stage = Stage(mock_archive.url, name=self.stage_name, keep=True) try: - with Stage( - mock_archive.url, name=self.stage_name, keep=True - ) as stage: + with stage: raise ThisMustFailHere() + except ThisMustFailHere: path = get_stage_path(stage, self.stage_name) assert os.path.isdir(path) diff --git a/var/spack/repos/builtin.mock/packages/canfail/package.py b/var/spack/repos/builtin.mock/packages/canfail/package.py index 60272a529d..965ade531e 100644 --- a/var/spack/repos/builtin.mock/packages/canfail/package.py +++ b/var/spack/repos/builtin.mock/packages/canfail/package.py @@ -33,11 +33,9 @@ class Canfail(Package): version('1.0', '0123456789abcdef0123456789abcdef') + succeed = False + def install(self, spec, prefix): - try: - succeed = getattr(self, 'succeed') - if not succeed: - raise InstallError("'succeed' was false") - touch(join_path(prefix, 'an_installation_file')) - except AttributeError: - raise InstallError("'succeed' attribute was not set") + if not self.succeed: + raise InstallError("'succeed' was false") + touch(join_path(prefix, 'an_installation_file'))