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.

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 @@ class PackageBase(with_metaclass(PackageMeta, object)):
 
                     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 mock_fetch(mock_archive):
 
 
 @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
-
-        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)
+    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})
+    }
 
-        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 @@ class MockStage(object):
         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 @@ class TestStage(object):
         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 @@ class TestStage(object):
         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 @@ class TestStage(object):
         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'))