From e7dc8a2bea23b013a8af4938a5eee298a1f59141 Mon Sep 17 00:00:00 2001
From: Todd Gamblin <tgamblin@llnl.gov>
Date: Sun, 29 Dec 2019 02:04:28 -0800
Subject: [PATCH] tests: refactor tests to avoid persistent global state

Previously, fixtures like `config`, `database`, and `store` were
module-scoped, but frequently used as test function arguments.  These
fixtures swap out global on setup and restore them on teardown.  As
function arguments, they would do the right set-up, but they'd leave the
global changes in place for the whole module the function lived in.  This
meant that if you use `config` once, other functions in the same module
would inadvertently inherit the mock Spack configuration, as it would
only be torn down once all tests in the module were complete.

In general, we should module- or session-scope the *STATE* required for
these global objects (as it's expensive to create0, but we shouldn't
module-or session scope the activation/use of them, or things can get
really confusing.

- [x] Make generic context managers for global-modifying fixtures.

- [x] Make session- and module-scoped fixtures that ONLY build filesystem
  state and create objects, but do not swap out any variables.

- [x] Make seeparate function-scoped fixtures that *use* the session
  scoped fixtures and actually swap out (and back in) the global
  variables like `config`, `database`, and `store`.

These changes make it so that global changes are *only* ever alive for a
singlee test function, and we don't get weird dependencies because a
global fixture hasn't been destroyed.
---
 lib/spack/spack/test/cmd/env.py               |   2 +-
 lib/spack/spack/test/cmd/module.py            |  17 ++-
 lib/spack/spack/test/cmd/spec.py              |   2 +-
 lib/spack/spack/test/concretize.py            |   2 +-
 .../spack/test/concretize_preferences.py      |   4 +-
 lib/spack/spack/test/conftest.py              | 142 ++++++++++++------
 lib/spack/spack/test/git_fetch.py             |   6 +-
 lib/spack/spack/test/hg_fetch.py              |   2 +-
 lib/spack/spack/test/install.py               |   6 +-
 lib/spack/spack/test/mirror.py                |   2 +-
 lib/spack/spack/test/packages.py              |   2 +-
 lib/spack/spack/test/repo.py                  |  40 ++---
 lib/spack/spack/test/spec_dag.py              |   2 +-
 lib/spack/spack/test/spec_yaml.py             |   2 +-
 lib/spack/spack/test/svn_fetch.py             |   2 +-
 lib/spack/spack/test/url_fetch.py             |   2 +-
 16 files changed, 143 insertions(+), 92 deletions(-)

diff --git a/lib/spack/spack/test/cmd/env.py b/lib/spack/spack/test/cmd/env.py
index fa7f9a640e9..9d6facfeeb1 100644
--- a/lib/spack/spack/test/cmd/env.py
+++ b/lib/spack/spack/test/cmd/env.py
@@ -26,7 +26,7 @@
 
 # everything here uses the mock_env_path
 pytestmark = pytest.mark.usefixtures(
-    'mutable_mock_env_path', 'config', 'mutable_mock_packages')
+    'mutable_mock_env_path', 'config', 'mutable_mock_repo')
 
 env        = SpackCommand('env')
 install    = SpackCommand('install')
diff --git a/lib/spack/spack/test/cmd/module.py b/lib/spack/spack/test/cmd/module.py
index 6f2ffcbf85c..fa23d7d5aa7 100644
--- a/lib/spack/spack/test/cmd/module.py
+++ b/lib/spack/spack/test/cmd/module.py
@@ -10,9 +10,21 @@
 
 import spack.main
 import spack.modules
+from spack.test.conftest import use_store, use_configuration, use_repo
 
 module = spack.main.SpackCommand('module')
 
+#: make sure module files are generated for all the tests here
+@pytest.fixture(scope='module', autouse=True)
+def ensure_module_files_are_there(
+        mock_repo_path, mock_store, mock_configuration):
+    """Generate module files for module tests."""
+    module = spack.main.SpackCommand('module')
+    with use_store(mock_store):
+        with use_configuration(mock_configuration):
+            with use_repo(mock_repo_path):
+                module('tcl', 'refresh', '-y')
+
 
 def _module_files(module_type, *specs):
     specs = [spack.spec.Spec(x).concretized() for x in specs]
@@ -20,11 +32,6 @@ def _module_files(module_type, *specs):
     return [writer_cls(spec).layout.filename for spec in specs]
 
 
-@pytest.fixture(scope='module', autouse=True)
-def ensure_module_files_are_there(database):
-    module('tcl', 'refresh', '-y')
-
-
 @pytest.fixture(
     params=[
         ['rm', 'doesnotexist'],  # Try to remove a non existing module
diff --git a/lib/spack/spack/test/cmd/spec.py b/lib/spack/spack/test/cmd/spec.py
index 4637e14a1f8..002e0aa1fe6 100644
--- a/lib/spack/spack/test/cmd/spec.py
+++ b/lib/spack/spack/test/cmd/spec.py
@@ -9,7 +9,7 @@
 import spack.spec
 from spack.main import SpackCommand
 
-pytestmark = pytest.mark.usefixtures('config', 'mutable_mock_packages')
+pytestmark = pytest.mark.usefixtures('config', 'mutable_mock_repo')
 
 spec = SpackCommand('spec')
 
diff --git a/lib/spack/spack/test/concretize.py b/lib/spack/spack/test/concretize.py
index 4b17e755ed9..483cae7809e 100644
--- a/lib/spack/spack/test/concretize.py
+++ b/lib/spack/spack/test/concretize.py
@@ -276,7 +276,7 @@ def test_concretize_two_virtuals(self):
         Spec('hypre').concretize()
 
     def test_concretize_two_virtuals_with_one_bound(
-            self, mutable_mock_packages
+            self, mutable_mock_repo
     ):
         """Test a package with multiple virtual dependencies and one preset."""
         Spec('hypre ^openblas').concretize()
diff --git a/lib/spack/spack/test/concretize_preferences.py b/lib/spack/spack/test/concretize_preferences.py
index e3055192cdf..13fc850c189 100644
--- a/lib/spack/spack/test/concretize_preferences.py
+++ b/lib/spack/spack/test/concretize_preferences.py
@@ -83,7 +83,7 @@ def test_preferred_variants(self):
             'mpileaks', debug=True, opt=True, shared=False, static=False
         )
 
-    def test_preferred_compilers(self, mutable_mock_packages):
+    def test_preferred_compilers(self, mutable_mock_repo):
         """Test preferred compilers are applied correctly
         """
         update_packages('mpileaks', 'compiler', ['clang@3.3'])
@@ -94,7 +94,7 @@ def test_preferred_compilers(self, mutable_mock_packages):
         spec = concretize('mpileaks')
         assert spec.compiler == spack.spec.CompilerSpec('gcc@4.5.0')
 
-    def test_preferred_target(self, mutable_mock_packages):
+    def test_preferred_target(self, mutable_mock_repo):
         """Test preferred compilers are applied correctly
         """
         spec = concretize('mpich')
diff --git a/lib/spack/spack/test/conftest.py b/lib/spack/spack/test/conftest.py
index b23c53c20eb..26cb0f453dd 100644
--- a/lib/spack/spack/test/conftest.py
+++ b/lib/spack/spack/test/conftest.py
@@ -4,7 +4,7 @@
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
 
 import collections
-import copy
+import contextlib
 import errno
 import inspect
 import itertools
@@ -270,31 +270,75 @@ def _skip_if_missing_executables(request):
 spack.architecture.real_platform = spack.architecture.platform
 spack.architecture.platform = lambda: spack.platforms.test.Test()
 
-##########
-# Test-specific fixtures
-##########
+
+#
+# Context managers used by fixtures
+#
+# Because these context managers modify global state, they should really
+# ONLY be used persistently (i.e., around yield statements) in
+# function-scoped fixtures, OR in autouse session- or module-scoped
+# fixtures.
+#
+# If they're used in regular tests or in module-scoped fixtures that are
+# then injected as function arguments, weird things can happen, because
+# the original state won't be restored until *after* the fixture is
+# destroyed.  This makes sense for an autouse fixture, where you know
+# everything in the module/session is going to need the modified
+# behavior, but modifying global state for one function in a way that
+# won't be restored until after the module or session is done essentially
+# leaves garbage behind for other tests.
+#
+# In general, we should module- or session-scope the *STATE* required for
+# these global objects, but we shouldn't module- or session-scope their
+# *USE*, or things can get really confusing.
+#
+
+@contextlib.contextmanager
+def use_configuration(config):
+    """Context manager to swap out the global Spack configuration."""
+    saved = spack.config.config
+    spack.config.config = config
+    yield
+    spack.config.config = saved
 
 
-@pytest.fixture(scope='session')
-def repo_path():
-    """Session scoped RepoPath object pointing to the mock repository"""
-    return spack.repo.RepoPath(spack.paths.mock_packages_path)
+@contextlib.contextmanager
+def use_store(store):
+    """Context manager to swap out the global Spack store."""
+    saved = spack.store.store
+    spack.store.store = store
+    yield
+    spack.store.store = saved
 
 
-@pytest.fixture(scope='module')
-def mock_packages(repo_path):
-    """Use the 'builtin.mock' repository instead of 'builtin'"""
-    mock_repo = copy.deepcopy(repo_path)
-    with spack.repo.swap(mock_repo):
+@contextlib.contextmanager
+def use_repo(repo):
+    """Context manager to swap out the global Spack repo path."""
+    with spack.repo.swap(repo):
         yield
 
 
+#
+# Test-specific fixtures
+#
+@pytest.fixture(scope='session')
+def mock_repo_path():
+    yield spack.repo.RepoPath(spack.paths.mock_packages_path)
+
+
 @pytest.fixture(scope='function')
-def mutable_mock_packages(mock_packages, repo_path):
+def mock_packages(mock_repo_path):
+    """Use the 'builtin.mock' repository instead of 'builtin'"""
+    with use_repo(mock_repo_path):
+        yield mock_repo_path
+
+
+@pytest.fixture(scope='function')
+def mutable_mock_repo(mock_repo_path):
     """Function-scoped mock packages, for tests that need to modify them."""
-    mock_repo = copy.deepcopy(repo_path)
-    with spack.repo.swap(mock_repo):
-        yield
+    mock_repo_path = spack.repo.RepoPath(spack.paths.mock_packages_path)
+    with use_repo(mock_repo_path):
+        yield mock_repo_path
 
 
 @pytest.fixture(scope='session')
@@ -345,12 +389,9 @@ def configuration_dir(tmpdir_factory, linux_os):
     shutil.rmtree(str(tmpdir))
 
 
-@pytest.fixture(scope='module')
-def config(configuration_dir):
-    """Hooks the mock configuration files into spack.config"""
-    # Set up a mock config scope
-    real_configuration = spack.config.config
-
+@pytest.fixture(scope='session')
+def mock_configuration(configuration_dir):
+    """Create a persistent Configuration object from the configuration_dir."""
     defaults = spack.config.InternalConfigScope(
         '_builtin', spack.config.config_defaults
     )
@@ -360,11 +401,14 @@ def config(configuration_dir):
         for name in ['site', 'system', 'user']]
     test_scopes.append(spack.config.InternalConfigScope('command_line'))
 
-    spack.config.config = spack.config.Configuration(*test_scopes)
+    yield spack.config.Configuration(*test_scopes)
 
-    yield spack.config.config
 
-    spack.config.config = real_configuration
+@pytest.fixture(scope='function')
+def config(mock_configuration):
+    """This fixture activates/deactivates the mock configuration."""
+    with use_configuration(mock_configuration):
+        yield mock_configuration
 
 
 @pytest.fixture(scope='function')
@@ -376,27 +420,24 @@ def mutable_config(tmpdir_factory, configuration_dir, monkeypatch):
     cfg = spack.config.Configuration(
         *[spack.config.ConfigScope(name, str(mutable_dir))
           for name in ['site', 'system', 'user']])
-    monkeypatch.setattr(spack.config, 'config', cfg)
 
     # This is essential, otherwise the cache will create weird side effects
     # that will compromise subsequent tests if compilers.yaml is modified
     monkeypatch.setattr(spack.compilers, '_cache_config_file', [])
 
-    yield spack.config.config
+    with use_configuration(cfg):
+        yield cfg
 
 
 @pytest.fixture()
 def mock_config(tmpdir):
     """Mocks two configuration scopes: 'low' and 'high'."""
-    real_configuration = spack.config.config
-
-    spack.config.config = spack.config.Configuration(
+    config = spack.config.Configuration(
         *[spack.config.ConfigScope(name, str(tmpdir.join(name)))
           for name in ['low', 'high']])
 
-    yield spack.config.config
-
-    spack.config.config = real_configuration
+    with use_configuration(config):
+        yield config
 
 
 def _populate(mock_db):
@@ -443,30 +484,41 @@ def _store_dir_and_cache(tmpdir_factory):
     return store, cache
 
 
-@pytest.fixture(scope='module')
-def database(tmpdir_factory, mock_packages, config, _store_dir_and_cache):
+@pytest.fixture(scope='session')
+def mock_store(tmpdir_factory, mock_repo_path, mock_configuration,
+               _store_dir_and_cache):
     """Creates a read-only mock database with some packages installed note
     that the ref count for dyninst here will be 3, as it's recycled
     across each install.
-    """
-    real_store = spack.store.store
-    store_path, store_cache = _store_dir_and_cache
 
-    mock_store = spack.store.Store(str(store_path))
-    spack.store.store = mock_store
+    This does not actually activate the store for use by Spack -- see the
+    ``database`` fixture for that.
+
+    """
+    store_path, store_cache = _store_dir_and_cache
+    store = spack.store.Store(str(store_path))
 
     # If the cache does not exist populate the store and create it
     if not os.path.exists(str(store_cache.join('.spack-db'))):
-        _populate(mock_store.db)
+        with use_configuration(mock_configuration):
+            with use_store(store):
+                with use_repo(mock_repo_path):
+                    _populate(store.db)
         store_path.copy(store_cache, mode=True, stat=True)
 
-    # Make the database read-only to ensure we can't modify entries
+    # Make the DB filesystem read-only to ensure we can't modify entries
     store_path.join('.spack-db').chmod(mode=0o555, rec=1)
 
-    yield mock_store.db
+    yield store
 
     store_path.join('.spack-db').chmod(mode=0o755, rec=1)
-    spack.store.store = real_store
+
+
+@pytest.fixture(scope='function')
+def database(mock_store, mock_packages, config):
+    """This activates the mock store, packages, AND config."""
+    with use_store(mock_store):
+        yield mock_store.db
 
 
 @pytest.fixture(scope='function')
diff --git a/lib/spack/spack/test/git_fetch.py b/lib/spack/spack/test/git_fetch.py
index ae1e0277521..d0c2ca7e3de 100644
--- a/lib/spack/spack/test/git_fetch.py
+++ b/lib/spack/spack/test/git_fetch.py
@@ -88,7 +88,7 @@ def test_fetch(type_of_test,
                secure,
                mock_git_repository,
                config,
-               mutable_mock_packages,
+               mutable_mock_repo,
                git_version):
     """Tries to:
 
@@ -137,7 +137,7 @@ def test_fetch(type_of_test,
 
 
 @pytest.mark.parametrize("type_of_test", ['branch', 'commit'])
-def test_debug_fetch(type_of_test, mock_git_repository, config):
+def test_debug_fetch(mock_packages, type_of_test, mock_git_repository, config):
     """Fetch the repo with debug enabled."""
     # Retrieve the right test parameters
     t = mock_git_repository.checks[type_of_test]
@@ -176,7 +176,7 @@ def test_needs_stage():
 
 @pytest.mark.parametrize("get_full_repo", [True, False])
 def test_get_full_repo(get_full_repo, git_version, mock_git_repository,
-                       config, mutable_mock_packages):
+                       config, mutable_mock_repo):
     """Ensure that we can clone a full repository."""
 
     if git_version < ver('1.7.1'):
diff --git a/lib/spack/spack/test/hg_fetch.py b/lib/spack/spack/test/hg_fetch.py
index 109e3d9d59d..5e4fde978b9 100644
--- a/lib/spack/spack/test/hg_fetch.py
+++ b/lib/spack/spack/test/hg_fetch.py
@@ -29,7 +29,7 @@ def test_fetch(
         secure,
         mock_hg_repository,
         config,
-        mutable_mock_packages
+        mutable_mock_repo
 ):
     """Tries to:
 
diff --git a/lib/spack/spack/test/install.py b/lib/spack/spack/test/install.py
index 4bed12456af..1fa892c7609 100644
--- a/lib/spack/spack/test/install.py
+++ b/lib/spack/spack/test/install.py
@@ -128,7 +128,7 @@ def test_dont_add_patches_to_installed_package(install_mockery, mock_fetch):
 
 
 def test_installed_dependency_request_conflicts(
-        install_mockery, mock_fetch, mutable_mock_packages):
+        install_mockery, mock_fetch, mutable_mock_repo):
     dependency = Spec('dependency-install')
     dependency.concretize()
     dependency.package.do_install()
@@ -141,7 +141,7 @@ def test_installed_dependency_request_conflicts(
 
 
 def test_install_dependency_symlinks_pkg(
-        install_mockery, mock_fetch, mutable_mock_packages):
+        install_mockery, mock_fetch, mutable_mock_repo):
     """Test dependency flattening/symlinks mock package."""
     spec = Spec('flatten-deps')
     spec.concretize()
@@ -154,7 +154,7 @@ def test_install_dependency_symlinks_pkg(
 
 
 def test_flatten_deps(
-        install_mockery, mock_fetch, mutable_mock_packages):
+        install_mockery, mock_fetch, mutable_mock_repo):
     """Explicitly test the flattening code for coverage purposes."""
     # Unfortunately, executing the 'flatten-deps' spec's installation does
     # not affect code coverage results, so be explicit here.
diff --git a/lib/spack/spack/test/mirror.py b/lib/spack/spack/test/mirror.py
index e1b31695e37..bc0c3ed0f7a 100644
--- a/lib/spack/spack/test/mirror.py
+++ b/lib/spack/spack/test/mirror.py
@@ -16,7 +16,7 @@
 
 from llnl.util.filesystem import resolve_link_target_relative_to_the_link
 
-pytestmark = pytest.mark.usefixtures('config', 'mutable_mock_packages')
+pytestmark = pytest.mark.usefixtures('config', 'mutable_mock_repo')
 
 # paths in repos that shouldn't be in the mirror tarballs.
 exclude = ['.hg', '.git', '.svn']
diff --git a/lib/spack/spack/test/packages.py b/lib/spack/spack/test/packages.py
index bd4ba95053a..4a6957cc5a7 100644
--- a/lib/spack/spack/test/packages.py
+++ b/lib/spack/spack/test/packages.py
@@ -182,7 +182,7 @@ def test_urls_for_versions(mock_packages, config):
         assert url == 'http://www.doesnotexist.org/url_override-0.8.1.tar.gz'
 
 
-def test_url_for_version_with_no_urls():
+def test_url_for_version_with_no_urls(mock_packages, config):
     pkg = spack.repo.get('git-test')
     with pytest.raises(spack.package.NoURLError):
         pkg.url_for_version('1.0')
diff --git a/lib/spack/spack/test/repo.py b/lib/spack/spack/test/repo.py
index e171dbca68d..35919f34623 100644
--- a/lib/spack/spack/test/repo.py
+++ b/lib/spack/spack/test/repo.py
@@ -10,14 +10,6 @@
 import spack.paths
 
 
-# Unlike the repo_path fixture defined in conftest, this has a test-level
-# scope rather than a session level scope, since we want to edit the
-# given RepoPath
-@pytest.fixture()
-def repo_for_test():
-    return spack.repo.RepoPath(spack.paths.mock_packages_path)
-
-
 @pytest.fixture()
 def extra_repo(tmpdir_factory):
     repo_namespace = 'extra_test_repo'
@@ -32,31 +24,31 @@ def extra_repo(tmpdir_factory):
     return spack.repo.Repo(str(repo_dir))
 
 
-def test_repo_getpkg(repo_for_test):
-    repo_for_test.get('a')
-    repo_for_test.get('builtin.mock.a')
+def test_repo_getpkg(mutable_mock_repo):
+    mutable_mock_repo.get('a')
+    mutable_mock_repo.get('builtin.mock.a')
 
 
-def test_repo_multi_getpkg(repo_for_test, extra_repo):
-    repo_for_test.put_first(extra_repo)
-    repo_for_test.get('a')
-    repo_for_test.get('builtin.mock.a')
+def test_repo_multi_getpkg(mutable_mock_repo, extra_repo):
+    mutable_mock_repo.put_first(extra_repo)
+    mutable_mock_repo.get('a')
+    mutable_mock_repo.get('builtin.mock.a')
 
 
-def test_repo_multi_getpkgclass(repo_for_test, extra_repo):
-    repo_for_test.put_first(extra_repo)
-    repo_for_test.get_pkg_class('a')
-    repo_for_test.get_pkg_class('builtin.mock.a')
+def test_repo_multi_getpkgclass(mutable_mock_repo, extra_repo):
+    mutable_mock_repo.put_first(extra_repo)
+    mutable_mock_repo.get_pkg_class('a')
+    mutable_mock_repo.get_pkg_class('builtin.mock.a')
 
 
-def test_repo_pkg_with_unknown_namespace(repo_for_test):
+def test_repo_pkg_with_unknown_namespace(mutable_mock_repo):
     with pytest.raises(spack.repo.UnknownNamespaceError):
-        repo_for_test.get('unknown.a')
+        mutable_mock_repo.get('unknown.a')
 
 
-def test_repo_unknown_pkg(repo_for_test):
+def test_repo_unknown_pkg(mutable_mock_repo):
     with pytest.raises(spack.repo.UnknownPackageError):
-        repo_for_test.get('builtin.mock.nonexistentpackage')
+        mutable_mock_repo.get('builtin.mock.nonexistentpackage')
 
 
 @pytest.mark.maybeslow
@@ -66,7 +58,7 @@ def test_repo_last_mtime():
     assert spack.repo.path.last_mtime() == latest_mtime
 
 
-def test_repo_invisibles(repo_for_test, extra_repo):
+def test_repo_invisibles(mutable_mock_repo, extra_repo):
     with open(os.path.join(extra_repo.root, 'packages', '.invisible'), 'w'):
         pass
     extra_repo.all_package_names()
diff --git a/lib/spack/spack/test/spec_dag.py b/lib/spack/spack/test/spec_dag.py
index 628f67948fd..e42cd601899 100644
--- a/lib/spack/spack/test/spec_dag.py
+++ b/lib/spack/spack/test/spec_dag.py
@@ -189,7 +189,7 @@ def test_conditional_dep_with_user_constraints():
         assert ('y@3' in spec)
 
 
-@pytest.mark.usefixtures('mutable_mock_packages')
+@pytest.mark.usefixtures('mutable_mock_repo')
 class TestSpecDag(object):
 
     def test_conflicting_package_constraints(self, set_dependency):
diff --git a/lib/spack/spack/test/spec_yaml.py b/lib/spack/spack/test/spec_yaml.py
index 7fd2a36469a..4f723988a31 100644
--- a/lib/spack/spack/test/spec_yaml.py
+++ b/lib/spack/spack/test/spec_yaml.py
@@ -61,7 +61,7 @@ def test_concrete_spec(config, mock_packages):
     check_yaml_round_trip(spec)
 
 
-def test_yaml_multivalue():
+def test_yaml_multivalue(config, mock_packages):
     spec = Spec('multivalue_variant foo="bar,baz"')
     spec.concretize()
     check_yaml_round_trip(spec)
diff --git a/lib/spack/spack/test/svn_fetch.py b/lib/spack/spack/test/svn_fetch.py
index eb0d22ee7cd..b065367cbee 100644
--- a/lib/spack/spack/test/svn_fetch.py
+++ b/lib/spack/spack/test/svn_fetch.py
@@ -30,7 +30,7 @@ def test_fetch(
         secure,
         mock_svn_repository,
         config,
-        mutable_mock_packages
+        mutable_mock_repo
 ):
     """Tries to:
 
diff --git a/lib/spack/spack/test/url_fetch.py b/lib/spack/spack/test/url_fetch.py
index b4df27336e4..1b65be0fd7e 100644
--- a/lib/spack/spack/test/url_fetch.py
+++ b/lib/spack/spack/test/url_fetch.py
@@ -75,7 +75,7 @@ def test_fetch(
         secure,
         checksum_type,
         config,
-        mutable_mock_packages
+        mutable_mock_repo
 ):
     """Fetch an archive and make sure we can checksum it."""
     mock_archive.url