From f2e3edf6db08fc2bc43ee73c10d2da0fd06febd4 Mon Sep 17 00:00:00 2001
From: Massimiliano Culpo <massimiliano.culpo@gmail.com>
Date: Thu, 18 Feb 2021 22:22:49 +0100
Subject: Testing: use spack.store.use_store everywhere (#21656)

Keep spack.store.store and spack.store.db consistent in unit tests

* Remove calls to monkeypatch for spack.store.store and spack.store.db:
  tests that used these called one or the other, which lead to
  inconsistencies (the tests passed regardless but were fragile as a
  result)
* Fixtures making use of monkeypatch with mock_store now use the
  updated use_store function, which sets store.store and store.db
  consistently
* subprocess_context.TestState now transfers the serializes and
  restores spack.store.store (without the monkeypatch changes this
  would have created inconsistencies)
---
 lib/spack/spack/binary_distribution.py             |  6 +-
 lib/spack/spack/store.py                           | 31 ++++++-
 lib/spack/spack/subprocess_context.py              | 15 ++--
 lib/spack/spack/test/bindist.py                    | 53 ++++++------
 lib/spack/spack/test/cmd/module.py                 |  2 +-
 lib/spack/spack/test/config_values.py              | 60 +++++++-------
 lib/spack/spack/test/conftest.py                   | 45 +++++-----
 lib/spack/spack/test/database.py                   | 18 +---
 lib/spack/spack/test/directory_layout.py           | 25 ++----
 lib/spack/spack/test/install.py                    | 96 +++++++++++-----------
 .../builtin.mock/packages/old-sbang/package.py     |  9 +-
 11 files changed, 181 insertions(+), 179 deletions(-)

diff --git a/lib/spack/spack/binary_distribution.py b/lib/spack/spack/binary_distribution.py
index e477c3104a..99dfcd9292 100644
--- a/lib/spack/spack/binary_distribution.py
+++ b/lib/spack/spack/binary_distribution.py
@@ -1089,6 +1089,8 @@ def relocate_package(spec, allow_root):
     """
     Relocate the given package
     """
+    import spack.hooks.sbang as sbang
+
     workdir = str(spec.prefix)
     buildinfo = read_buildinfo_file(workdir)
     new_layout_root = str(spack.store.layout.root)
@@ -1127,7 +1129,6 @@ def relocate_package(spec, allow_root):
     prefix_to_prefix_bin = OrderedDict({})
 
     if old_sbang_install_path:
-        import spack.hooks.sbang as sbang
         prefix_to_prefix_text[old_sbang_install_path] = sbang.sbang_install_path()
 
     prefix_to_prefix_text[old_prefix] = new_prefix
@@ -1141,7 +1142,6 @@ def relocate_package(spec, allow_root):
     # sbang was a bash script, and it lived in the spack prefix. It is
     # now a POSIX script that lives in the install prefix. Old packages
     # will have the old sbang location in their shebangs.
-    import spack.hooks.sbang as sbang
     orig_sbang = '#!/bin/bash {0}/bin/sbang'.format(old_spack_prefix)
     new_sbang = sbang.sbang_shebang_line()
     prefix_to_prefix_text[orig_sbang] = new_sbang
@@ -1160,7 +1160,7 @@ def relocate_package(spec, allow_root):
         if not is_backup_file(text_name):
             text_names.append(text_name)
 
-# If we are not installing back to the same install tree do the relocation
+    # If we are not installing back to the same install tree do the relocation
     if old_layout_root != new_layout_root:
         files_to_relocate = [os.path.join(workdir, filename)
                              for filename in buildinfo.get('relocate_binaries')
diff --git a/lib/spack/spack/store.py b/lib/spack/spack/store.py
index f53183d323..80a7ae0831 100644
--- a/lib/spack/spack/store.py
+++ b/lib/spack/spack/store.py
@@ -158,6 +158,8 @@ class Store(object):
     ):
         self.root = root
         self.unpadded_root = unpadded_root or root
+        self.projections = projections
+        self.hash_length = hash_length
         self.db = spack.database.Database(
             root, upstream_dbs=retrieve_upstream_dbs())
         self.layout = spack.directory_layout.YamlDirectoryLayout(
@@ -167,6 +169,27 @@ class Store(object):
         """Convenience function to reindex the store DB with its own layout."""
         return self.db.reindex(self.layout)
 
+    def serialize(self):
+        """Return a pickle-able object that can be used to reconstruct
+        a store.
+        """
+        return (
+            self.root, self.unpadded_root, self.projections, self.hash_length
+        )
+
+    @staticmethod
+    def deserialize(token):
+        """Return a store reconstructed from a token created by
+        the serialize method.
+
+        Args:
+            token: return value of the serialize method
+
+        Returns:
+            Store object reconstructed from the token
+        """
+        return Store(*token)
+
 
 def _store():
     """Get the singleton store instance."""
@@ -240,7 +263,7 @@ def use_store(store_or_path):
     Returns:
         Store object associated with the context manager's store
     """
-    global store
+    global store, db, layout, root, unpadded_root
 
     # Normalize input arguments
     temporary_store = store_or_path
@@ -248,8 +271,14 @@ def use_store(store_or_path):
         temporary_store = Store(store_or_path)
 
     # Swap the store with the one just constructed and return it
+    _ = store.db
     original_store, store = store, temporary_store
+    db, layout = store.db, store.layout
+    root, unpadded_root = store.root, store.unpadded_root
+
     yield temporary_store
 
     # Restore the original store
     store = original_store
+    db, layout = original_store.db, original_store.layout
+    root, unpadded_root = original_store.root, original_store.unpadded_root
diff --git a/lib/spack/spack/subprocess_context.py b/lib/spack/spack/subprocess_context.py
index 002fccad4f..3eee2125d2 100644
--- a/lib/spack/spack/subprocess_context.py
+++ b/lib/spack/spack/subprocess_context.py
@@ -93,18 +93,21 @@ class TestState(object):
             self.config = spack.config.config
             self.platform = spack.architecture.platform
             self.test_patches = store_patches()
-
-            # TODO: transfer spack.store.store? note that you should not
-            # transfer spack.store.store and spack.store.db: 'db' is a
-            # shortcut that accesses the store (so transferring both can
-            # create an inconsistency). Some tests set 'db' directly, and
-            # others set 'store'
+            self.store_token = spack.store.store.serialize()
 
     def restore(self):
         if _serialize:
             spack.repo.path = spack.repo._path(self.repo_dirs)
             spack.config.config = self.config
             spack.architecture.platform = self.platform
+
+            new_store = spack.store.Store.deserialize(self.store_token)
+            spack.store.store = new_store
+            spack.store.root = new_store.root
+            spack.store.unpadded_root = new_store.unpadded_root
+            spack.store.db = new_store.db
+            spack.store.layout = new_store.layout
+
             self.test_patches.restore()
 
 
diff --git a/lib/spack/spack/test/bindist.py b/lib/spack/spack/test/bindist.py
index 45dd931e8c..1868d74ccc 100644
--- a/lib/spack/spack/test/bindist.py
+++ b/lib/spack/spack/test/bindist.py
@@ -493,40 +493,39 @@ def test_update_sbang(tmpdir, test_mirror):
         '${name}', '${version}',
         '${architecture}-${compiler.name}-${compiler.version}-${hash}'
     )
-    # Save the original store and layout before we touch ANYTHING.
-    real_store, real_layout = spack.store.store, spack.store.layout
-
+    spec_str = 'old-sbang'
     # Concretize a package with some old-fashioned sbang lines.
-    sspec = Spec('old-sbang')
-    sspec.concretize()
+    old_spec = Spec(spec_str).concretized()
+    old_spec_hash_str = '/{0}'.format(old_spec.dag_hash())
 
     # Need a fake mirror with *function* scope.
     mirror_dir = test_mirror
+    mirror_url = 'file://{0}'.format(mirror_dir)
 
-    # Assumes all commands will concretize sspec the same way.
-    install_cmd('--no-cache', sspec.name)
+    # Assume all commands will concretize old_spec the same way.
+    install_cmd('--no-cache', old_spec.name)
 
     # Create a buildcache with the installed spec.
-    buildcache_cmd('create', '-u', '-a', '-d', mirror_dir,
-                   '/%s' % sspec.dag_hash())
+    buildcache_cmd('create', '-u', '-a', '-d', mirror_dir, old_spec_hash_str)
 
     # Need to force an update of the buildcache index
-    buildcache_cmd('update-index', '-d', 'file://%s' % mirror_dir)
+    buildcache_cmd('update-index', '-d', mirror_url)
 
     # Uninstall the original package.
-    uninstall_cmd('-y', '/%s' % sspec.dag_hash())
+    uninstall_cmd('-y', old_spec_hash_str)
 
-    try:
-        # New install tree locations...
-        # Too fine-grained to do be done in a fixture
-        newtree_dir = tmpdir.join('newtree')
-        spack.store.store = spack.store.Store(str(newtree_dir))
-        spack.store.layout = YamlDirectoryLayout(
-            str(newtree_dir), path_scheme=scheme
-        )
+    # Switch the store to the new install tree locations
+    newtree_dir = tmpdir.join('newtree')
+    s = spack.store.Store(str(newtree_dir))
+    s.layout = YamlDirectoryLayout(str(newtree_dir), path_scheme=scheme)
+
+    with spack.store.use_store(s):
+        new_spec = Spec('old-sbang')
+        new_spec.concretize()
+        assert new_spec.dag_hash() == old_spec.dag_hash()
 
         # Install package from buildcache
-        buildcache_cmd('install', '-a', '-u', '-f', sspec.name)
+        buildcache_cmd('install', '-a', '-u', '-f', new_spec.name)
 
         # Continue blowing away caches
         bindist.clear_spec_cache()
@@ -537,23 +536,19 @@ def test_update_sbang(tmpdir, test_mirror):
 #!/usr/bin/env python
 
 {1}
-        '''.format(sbang.sbang_shebang_line(), sspec.prefix.bin)
+'''.format(sbang.sbang_shebang_line(), new_spec.prefix.bin)
         sbang_style_2_expected = '''{0}
 #!/usr/bin/env python
 
 {1}
-        '''.format(sbang.sbang_shebang_line(), sspec.prefix.bin)
+'''.format(sbang.sbang_shebang_line(), new_spec.prefix.bin)
 
-        installed_script_style_1_path = sspec.prefix.bin.join('sbang-style-1.sh')
+        installed_script_style_1_path = new_spec.prefix.bin.join('sbang-style-1.sh')
         assert sbang_style_1_expected == \
             open(str(installed_script_style_1_path)).read()
 
-        installed_script_style_2_path = sspec.prefix.bin.join('sbang-style-2.sh')
+        installed_script_style_2_path = new_spec.prefix.bin.join('sbang-style-2.sh')
         assert sbang_style_2_expected == \
             open(str(installed_script_style_2_path)).read()
 
-        uninstall_cmd('-y', '/%s' % sspec.dag_hash())
-
-    finally:
-        spack.store.store = real_store
-        spack.store.layout = real_layout
+        uninstall_cmd('-y', '/%s' % new_spec.dag_hash())
diff --git a/lib/spack/spack/test/cmd/module.py b/lib/spack/spack/test/cmd/module.py
index 3917b391fc..9acb21fdef 100644
--- a/lib/spack/spack/test/cmd/module.py
+++ b/lib/spack/spack/test/cmd/module.py
@@ -23,7 +23,7 @@ def ensure_module_files_are_there(
 ):
     """Generate module files for module tests."""
     module = spack.main.SpackCommand('module')
-    with spack.store.use_store(mock_store):
+    with spack.store.use_store(str(mock_store)):
         with spack.config.use_configuration(*mock_configuration_scopes):
             with spack.repo.use_repositories(mock_repo_path):
                 module('tcl', 'refresh', '-y')
diff --git a/lib/spack/spack/test/config_values.py b/lib/spack/spack/test/config_values.py
index 4de5c4dee8..367138f098 100644
--- a/lib/spack/spack/test/config_values.py
+++ b/lib/spack/spack/test/config_values.py
@@ -2,36 +2,30 @@
 # Spack Project Developers. See the top-level COPYRIGHT file for details.
 #
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
+import pytest
 
 import spack.spec
+import spack.store
 
 
-def test_set_install_hash_length(mock_packages, mutable_config, monkeypatch,
-                                 tmpdir):
-    # spack.store.layout caches initial config values, so we monkeypatch
-    mutable_config.set('config:install_hash_length', 5)
+@pytest.mark.parametrize('hash_length', [1, 2, 3, 4, 5, 9])
+@pytest.mark.use_fixtures('mock_packages')
+def test_set_install_hash_length(hash_length, mutable_config, tmpdir):
+    mutable_config.set('config:install_hash_length', hash_length)
     mutable_config.set('config:install_tree', {'root': str(tmpdir)})
-    monkeypatch.setattr(spack.store, 'store', spack.store._store())
-
-    spec = spack.spec.Spec('libelf').concretized()
-    prefix = spec.prefix
-    hash = prefix.rsplit('-')[-1]
-
-    assert len(hash) == 5
-
-    mutable_config.set('config:install_hash_length', 9)
-    monkeypatch.setattr(spack.store, 'store', spack.store._store())
-
-    spec = spack.spec.Spec('libelf').concretized()
-    prefix = spec.prefix
-    hash = prefix.rsplit('-')[-1]
-
-    assert len(hash) == 9
-
-
-def test_set_install_hash_length_upper_case(mock_packages, mutable_config,
-                                            monkeypatch, tmpdir):
-    # spack.store.layout caches initial config values, so we monkeypatch
+    # The call below is to reinitialize the directory layout associated
+    # with the store according to the configuration changes above (i.e.
+    # with the shortened hash)
+    store = spack.store._store()
+    with spack.store.use_store(store):
+        spec = spack.spec.Spec('libelf').concretized()
+        prefix = spec.prefix
+        hash_str = prefix.rsplit('-')[-1]
+        assert len(hash_str) == hash_length
+
+
+@pytest.mark.use_fixtures('mock_packages')
+def test_set_install_hash_length_upper_case(mutable_config, tmpdir):
     mutable_config.set('config:install_hash_length', 5)
     mutable_config.set(
         'config:install_tree',
@@ -42,10 +36,12 @@ def test_set_install_hash_length_upper_case(mock_packages, mutable_config,
             }
         }
     )
-    monkeypatch.setattr(spack.store, 'store', spack.store._store())
-
-    spec = spack.spec.Spec('libelf').concretized()
-    prefix = spec.prefix
-    hash = prefix.rsplit('-')[-1]
-
-    assert len(hash) == 5
+    # The call below is to reinitialize the directory layout associated
+    # with the store according to the configuration changes above (i.e.
+    # with the shortened hash and projection)
+    store = spack.store._store()
+    with spack.store.use_store(store):
+        spec = spack.spec.Spec('libelf').concretized()
+        prefix = spec.prefix
+        hash_str = prefix.rsplit('-')[-1]
+        assert len(hash_str) == 5
diff --git a/lib/spack/spack/test/conftest.py b/lib/spack/spack/test/conftest.py
index fc48b7bfc6..6cc3ad16db 100644
--- a/lib/spack/spack/test/conftest.py
+++ b/lib/spack/spack/test/conftest.py
@@ -563,12 +563,10 @@ def _populate(mock_db):
         pkg = spack.repo.get(s)
         pkg.do_install(fake=True, explicit=True)
 
-    # Transaction used to avoid repeated writes.
-    with mock_db.write_transaction():
-        _install('mpileaks ^mpich')
-        _install('mpileaks ^mpich2')
-        _install('mpileaks ^zmpi')
-        _install('externaltest')
+    _install('mpileaks ^mpich')
+    _install('mpileaks ^mpich2')
+    _install('mpileaks ^zmpi')
+    _install('externaltest')
 
 
 @pytest.fixture(scope='session')
@@ -605,7 +603,7 @@ def mock_store(tmpdir_factory, mock_repo_path, mock_configuration_scopes,
     # Make the DB filesystem read-only to ensure we can't modify entries
     store_path.join('.spack-db').chmod(mode=0o555, rec=1)
 
-    yield store
+    yield store_path
 
     store_path.join('.spack-db').chmod(mode=0o755, rec=1)
 
@@ -613,10 +611,9 @@ def mock_store(tmpdir_factory, mock_repo_path, mock_configuration_scopes,
 @pytest.fixture(scope='function')
 def database(mock_store, mock_packages, config, monkeypatch):
     """This activates the mock store, packages, AND config."""
-    monkeypatch.setattr(spack.store, 'store', mock_store)
-    yield mock_store.db
-    # Force reading the database again between tests
-    mock_store.db.last_seen_verifier = ''
+    with spack.store.use_store(str(mock_store)) as store:
+        yield store.db
+        store.db.last_seen_verifier = ''
 
 
 @pytest.fixture(scope='function')
@@ -687,20 +684,15 @@ def disable_compiler_execution(monkeypatch, request):
 
 
 @pytest.fixture(scope='function')
-def install_mockery(tmpdir, config, mock_packages, monkeypatch):
+def install_mockery(temporary_store, config, mock_packages):
     """Hooks a fake install directory, DB, and stage directory into Spack."""
-    monkeypatch.setattr(
-        spack.store, 'store', spack.store.Store(str(tmpdir.join('opt'))))
-
     # We use a fake package, so temporarily disable checksumming
     with spack.config.override('config:checksum', False):
         yield
 
-    tmpdir.join('opt').remove()
-
     # Also wipe out any cached prefix failure locks (associated with
     # the session-scoped mock archive).
-    for pkg_id in list(spack.store.db._prefix_failures.keys()):
+    for pkg_id in list(temporary_store.db._prefix_failures.keys()):
         lock = spack.store.db._prefix_failures.pop(pkg_id, None)
         if lock:
             try:
@@ -709,24 +701,29 @@ def install_mockery(tmpdir, config, mock_packages, monkeypatch):
                 pass
 
 
+@pytest.fixture(scope='function')
+def temporary_store(tmpdir):
+    """Hooks a temporary empty store for the test function."""
+    temporary_store_path = tmpdir.join('opt')
+    with spack.store.use_store(str(temporary_store_path)) as s:
+        yield s
+    temporary_store_path.remove()
+
+
 @pytest.fixture(scope='function')
 def install_mockery_mutable_config(
-        tmpdir, mutable_config, mock_packages, monkeypatch):
+        temporary_store, mutable_config, mock_packages
+):
     """Hooks a fake install directory, DB, and stage directory into Spack.
 
     This is specifically for tests which want to use 'install_mockery' but
     also need to modify configuration (and hence would want to use
     'mutable config'): 'install_mockery' does not support this.
     """
-    monkeypatch.setattr(
-        spack.store, 'store', spack.store.Store(str(tmpdir.join('opt'))))
-
     # We use a fake package, so temporarily disable checksumming
     with spack.config.override('config:checksum', False):
         yield
 
-    tmpdir.join('opt').remove()
-
 
 @pytest.fixture()
 def mock_fetch(mock_archive, monkeypatch):
diff --git a/lib/spack/spack/test/database.py b/lib/spack/spack/test/database.py
index 9f1b669510..8d1a204ba9 100644
--- a/lib/spack/spack/test/database.py
+++ b/lib/spack/spack/test/database.py
@@ -37,16 +37,6 @@ from spack.schema.database_index import schema
 pytestmark = pytest.mark.db
 
 
-@pytest.fixture()
-def test_store(tmpdir):
-    real_store = spack.store.store
-    spack.store.store = spack.store.Store(str(tmpdir.join('test_store')))
-
-    yield
-
-    spack.store.store = real_store
-
-
 @pytest.fixture()
 def upstream_and_downstream_db(tmpdir_factory, gen_mock_layout):
     mock_db_root = str(tmpdir_factory.mktemp('mock_db_root'))
@@ -180,8 +170,8 @@ def test_add_to_upstream_after_downstream(upstream_and_downstream_db):
             spack.store.db = orig_db
 
 
-@pytest.mark.usefixtures('config')
-def test_cannot_write_upstream(tmpdir_factory, test_store, gen_mock_layout):
+@pytest.mark.usefixtures('config', 'temporary_store')
+def test_cannot_write_upstream(tmpdir_factory, gen_mock_layout):
     roots = [str(tmpdir_factory.mktemp(x)) for x in ['a', 'b']]
     layouts = [gen_mock_layout(x) for x in ['/ra/', '/rb/']]
 
@@ -205,8 +195,8 @@ def test_cannot_write_upstream(tmpdir_factory, test_store, gen_mock_layout):
             upstream_dbs[0].add(spec, layouts[1])
 
 
-@pytest.mark.usefixtures('config')
-def test_recursive_upstream_dbs(tmpdir_factory, test_store, gen_mock_layout):
+@pytest.mark.usefixtures('config', 'temporary_store')
+def test_recursive_upstream_dbs(tmpdir_factory, gen_mock_layout):
     roots = [str(tmpdir_factory.mktemp(x)) for x in ['a', 'b', 'c']]
     layouts = [gen_mock_layout(x) for x in ['/ra/', '/rb/', '/rc/']]
 
diff --git a/lib/spack/spack/test/directory_layout.py b/lib/spack/spack/test/directory_layout.py
index de2b76b679..04e42549d6 100644
--- a/lib/spack/spack/test/directory_layout.py
+++ b/lib/spack/spack/test/directory_layout.py
@@ -7,6 +7,7 @@
 This test verifies that the Spack directory layout works properly.
 """
 import os
+import os.path
 import pytest
 
 import spack.paths
@@ -19,16 +20,6 @@ from spack.spec import Spec
 max_packages = 10
 
 
-@pytest.fixture()
-def layout_and_dir(tmpdir):
-    """Returns a directory layout and the corresponding directory."""
-    layout = YamlDirectoryLayout(str(tmpdir))
-    old_layout = spack.store.layout
-    spack.store.layout = layout
-    yield layout, str(tmpdir)
-    spack.store.layout = old_layout
-
-
 def test_yaml_directory_layout_parameters(tmpdir, config):
     """This tests the various parameters that can be used to configure
     the install location """
@@ -84,14 +75,14 @@ def test_yaml_directory_layout_parameters(tmpdir, config):
                             projections=projections_package7)
 
 
-def test_read_and_write_spec(layout_and_dir, config, mock_packages):
+def test_read_and_write_spec(temporary_store, config, mock_packages):
     """This goes through each package in spack and creates a directory for
     it.  It then ensures that the spec for the directory's
     installed package can be read back in consistently, and
     finally that the directory can be removed by the directory
     layout.
     """
-    layout, tmpdir = layout_and_dir
+    layout = temporary_store.layout
     packages = list(spack.repo.path.all_packages())[:max_packages]
 
     for pkg in packages:
@@ -114,7 +105,7 @@ def test_read_and_write_spec(layout_and_dir, config, mock_packages):
 
         # Ensure directory has been created in right place.
         assert os.path.isdir(install_dir)
-        assert install_dir.startswith(str(tmpdir))
+        assert install_dir.startswith(temporary_store.root)
 
         # Ensure spec file exists when directory is created
         assert os.path.isfile(spec_path)
@@ -160,7 +151,7 @@ def test_read_and_write_spec(layout_and_dir, config, mock_packages):
         assert not os.path.exists(install_dir)
 
 
-def test_handle_unknown_package(layout_and_dir, config, mock_packages):
+def test_handle_unknown_package(temporary_store, config, mock_packages):
     """This test ensures that spack can at least do *some*
     operations with packages that are installed but that it
     does not know about.  This is actually not such an uncommon
@@ -171,7 +162,7 @@ def test_handle_unknown_package(layout_and_dir, config, mock_packages):
     information about installed packages' specs to uninstall
     or query them again if the package goes away.
     """
-    layout, _ = layout_and_dir
+    layout = temporary_store.layout
     mock_db = spack.repo.RepoPath(spack.paths.mock_packages_path)
 
     not_in_mock = set.difference(
@@ -209,9 +200,9 @@ def test_handle_unknown_package(layout_and_dir, config, mock_packages):
             assert spec.dag_hash() == spec_from_file.dag_hash()
 
 
-def test_find(layout_and_dir, config, mock_packages):
+def test_find(temporary_store, config, mock_packages):
     """Test that finding specs within an install layout works."""
-    layout, _ = layout_and_dir
+    layout = temporary_store.layout
     packages = list(spack.repo.path.all_packages())[:max_packages]
 
     # Create install prefixes for all packages in the list
diff --git a/lib/spack/spack/test/install.py b/lib/spack/spack/test/install.py
index 059fe5bd3f..77810ff0e3 100644
--- a/lib/spack/spack/test/install.py
+++ b/lib/spack/spack/test/install.py
@@ -183,70 +183,70 @@ def test_flatten_deps(
     assert os.path.isdir(dependency_dir)
 
 
-def test_installed_upstream_external(
-        tmpdir_factory, install_mockery, mock_fetch, gen_mock_layout,
-        monkeypatch):
-    """Check that when a dependency package is recorded as installed in
-       an upstream database that it is not reinstalled.
+@pytest.fixture()
+def install_upstream(tmpdir_factory, gen_mock_layout, install_mockery):
+    """Provides a function that installs a specified set of specs to an
+    upstream database. The function returns a store which points to the
+    upstream, as well as the upstream layout (for verifying that dependent
+    installs are using the upstream installs).
     """
     mock_db_root = str(tmpdir_factory.mktemp('mock_db_root'))
     prepared_db = spack.database.Database(mock_db_root)
-
     upstream_layout = gen_mock_layout('/a/')
 
-    dependency = spack.spec.Spec('externaltool')
-    dependency.concretize()
-    prepared_db.add(dependency, upstream_layout)
-
-    downstream_db_root = str(
-        tmpdir_factory.mktemp('mock_downstream_db_root'))
-    db_for_test = spack.database.Database(
-        downstream_db_root, upstream_dbs=[prepared_db])
-    monkeypatch.setattr(spack.store, 'db', db_for_test)
-    dependent = spack.spec.Spec('externaltest')
-    dependent.concretize()
-
-    new_dependency = dependent['externaltool']
-    assert new_dependency.external
-    assert new_dependency.prefix == '/path/to/external_tool'
+    def _install_upstream(*specs):
+        for spec_str in specs:
+            s = spack.spec.Spec(spec_str).concretized()
+            prepared_db.add(s, upstream_layout)
 
-    dependent.package.do_install()
+        downstream_root = str(tmpdir_factory.mktemp('mock_downstream_db_root'))
+        db_for_test = spack.database.Database(
+            downstream_root, upstream_dbs=[prepared_db]
+        )
+        store = spack.store.Store(downstream_root)
+        store.db = db_for_test
+        return store, upstream_layout
 
-    assert not os.path.exists(new_dependency.prefix)
-    assert os.path.exists(dependent.prefix)
+    return _install_upstream
 
 
-def test_installed_upstream(tmpdir_factory, install_mockery, mock_fetch,
-                            gen_mock_layout, monkeypatch):
+def test_installed_upstream_external(install_upstream, mock_fetch):
     """Check that when a dependency package is recorded as installed in
-       an upstream database that it is not reinstalled.
+    an upstream database that it is not reinstalled.
     """
-    mock_db_root = str(tmpdir_factory.mktemp('mock_db_root'))
-    prepared_db = spack.database.Database(mock_db_root)
+    s, _ = install_upstream('externaltool')
+    with spack.store.use_store(s):
+        dependent = spack.spec.Spec('externaltest')
+        dependent.concretize()
 
-    upstream_layout = gen_mock_layout('/a/')
+        new_dependency = dependent['externaltool']
+        assert new_dependency.external
+        assert new_dependency.prefix == '/path/to/external_tool'
 
-    dependency = spack.spec.Spec('dependency-install')
-    dependency.concretize()
-    prepared_db.add(dependency, upstream_layout)
-
-    downstream_db_root = str(
-        tmpdir_factory.mktemp('mock_downstream_db_root'))
-    db_for_test = spack.database.Database(
-        downstream_db_root, upstream_dbs=[prepared_db])
-    monkeypatch.setattr(spack.store, 'db', db_for_test)
-    dependent = spack.spec.Spec('dependent-install')
-    dependent.concretize()
+        dependent.package.do_install()
+
+        assert not os.path.exists(new_dependency.prefix)
+        assert os.path.exists(dependent.prefix)
+
+
+def test_installed_upstream(install_upstream, mock_fetch):
+    """Check that when a dependency package is recorded as installed in
+    an upstream database that it is not reinstalled.
+    """
+    s, upstream_layout = install_upstream('dependency-install')
+    with spack.store.use_store(s):
+        dependency = spack.spec.Spec('dependency-install').concretized()
+        dependent = spack.spec.Spec('dependent-install').concretized()
 
-    new_dependency = dependent['dependency-install']
-    assert new_dependency.package.installed_upstream
-    assert (new_dependency.prefix ==
-            upstream_layout.path_for_spec(dependency))
+        new_dependency = dependent['dependency-install']
+        assert new_dependency.package.installed_upstream
+        assert (new_dependency.prefix ==
+                upstream_layout.path_for_spec(dependency))
 
-    dependent.package.do_install()
+        dependent.package.do_install()
 
-    assert not os.path.exists(new_dependency.prefix)
-    assert os.path.exists(dependent.prefix)
+        assert not os.path.exists(new_dependency.prefix)
+        assert os.path.exists(dependent.prefix)
 
 
 @pytest.mark.disable_clean_stage_check
diff --git a/var/spack/repos/builtin.mock/packages/old-sbang/package.py b/var/spack/repos/builtin.mock/packages/old-sbang/package.py
index 3308f91611..305f693c5a 100644
--- a/var/spack/repos/builtin.mock/packages/old-sbang/package.py
+++ b/var/spack/repos/builtin.mock/packages/old-sbang/package.py
@@ -2,10 +2,11 @@
 # Spack Project Developers. See the top-level COPYRIGHT file for details.
 #
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
-
-
 from spack import *
 
+import spack.paths
+import spack.store
+
 
 class OldSbang(Package):
     """Toy package for testing the old sbang replacement problem"""
@@ -22,12 +23,12 @@ class OldSbang(Package):
 #!/usr/bin/env python
 
 {1}
-        '''.format(spack.paths.prefix, prefix.bin)
+'''.format(spack.paths.prefix, prefix.bin)
         sbang_style_2 = '''#!/bin/sh {0}/bin/sbang
 #!/usr/bin/env python
 
 {1}
-        '''.format(spack.store.unpadded_root, prefix.bin)
+'''.format(spack.store.unpadded_root, prefix.bin)
         with open('%s/sbang-style-1.sh' % self.prefix.bin, 'w') as f:
             f.write(sbang_style_1)
 
-- 
cgit v1.2.3-60-g2f50