modules: unit-tests without polluted user scope (#41041)

8 files changed, 62 insertions, 58 deletions

diff --git a/lib/spack/spack/modules/common.py b/lib/spack/spack/modules/common.py
index d1afdd22fd..bccc6805cb 100644
--- a/lib/spack/spack/modules/common.py
+++ b/lib/spack/spack/modules/common.py
@@ -62,7 +62,7 @@ from spack.context import Context
 
 #: config section for this file
 def configuration(module_set_name):
-    config_path = "modules:%s" % module_set_name
+    config_path = f"modules:{module_set_name}"
     return spack.config.get(config_path, {})
 
 
@@ -96,10 +96,10 @@ def _check_tokens_are_valid(format_string, message):
     named_tokens = re.findall(r"{(\w*)}", format_string)
     invalid_tokens = [x for x in named_tokens if x.lower() not in _valid_tokens]
     if invalid_tokens:
-        msg = message
-        msg += " [{0}]. ".format(", ".join(invalid_tokens))
-        msg += 'Did you check your "modules.yaml" configuration?'
-        raise RuntimeError(msg)
+        raise RuntimeError(
+            f"{message} [{', '.join(invalid_tokens)}]. "
+            f"Did you check your 'modules.yaml' configuration?"
+        )
 
 
 def update_dictionary_extending_lists(target, update):
@@ -219,7 +219,7 @@ def root_path(name, module_set_name):
     """
     defaults = {"lmod": "$spack/share/spack/lmod", "tcl": "$spack/share/spack/modules"}
     # Root folders where the various module files should be written
-    roots = spack.config.get("modules:%s:roots" % module_set_name, {})
+    roots = spack.config.get(f"modules:{module_set_name}:roots", {})
 
     # Merge config values into the defaults so we prefer configured values
     roots = spack.config.merge_yaml(defaults, roots)
@@ -262,7 +262,7 @@ def read_module_index(root):
     index_path = os.path.join(root, "module-index.yaml")
     if not os.path.exists(index_path):
         return {}
-    with open(index_path, "r") as index_file:
+    with open(index_path) as index_file:
         return _read_module_index(index_file)
 
 
@@ -310,21 +310,21 @@ class UpstreamModuleIndex:
         if db_for_spec in self.upstream_dbs:
             db_index = self.upstream_dbs.index(db_for_spec)
         elif db_for_spec:
-            raise spack.error.SpackError("Unexpected: {0} is installed locally".format(spec))
+            raise spack.error.SpackError(f"Unexpected: {spec} is installed locally")
         else:
-            raise spack.error.SpackError("Unexpected: no install DB found for {0}".format(spec))
+            raise spack.error.SpackError(f"Unexpected: no install DB found for {spec}")
         module_index = self.module_indices[db_index]
         module_type_index = module_index.get(module_type, {})
         if not module_type_index:
             tty.debug(
-                "No {0} modules associated with the Spack instance where"
-                " {1} is installed".format(module_type, spec)
+                f"No {module_type} modules associated with the Spack instance "
+                f"where {spec} is installed"
             )
             return None
         if spec.dag_hash() in module_type_index:
             return module_type_index[spec.dag_hash()]
         else:
-            tty.debug("No module is available for upstream package {0}".format(spec))
+            tty.debug(f"No module is available for upstream package {spec}")
             return None
 
 
@@ -603,7 +603,7 @@ class BaseFileLayout:
         # Just the name of the file
         filename = self.use_name
         if self.extension:
-            filename = "{0}.{1}".format(self.use_name, self.extension)
+            filename = f"{self.use_name}.{self.extension}"
         # Architecture sub-folder
         arch_folder_conf = spack.config.get("modules:%s:arch_folder" % self.conf.name, True)
         if arch_folder_conf:
@@ -671,7 +671,7 @@ class BaseContext(tengine.Context):
             return msg
 
         if os.path.exists(pkg.install_configure_args_path):
-            with open(pkg.install_configure_args_path, "r") as args_file:
+            with open(pkg.install_configure_args_path) as args_file:
                 return spack.util.path.padding_filter(args_file.read())
 
         # Returning a false-like value makes the default templates skip
@@ -886,7 +886,7 @@ class BaseModuleFileWriter:
         # 2. template specified in a package directly
         # 3. default template (must be defined, check in __init__)
         module_system_name = str(self.module.__name__).split(".")[-1]
-        package_attribute = "{0}_template".format(module_system_name)
+        package_attribute = f"{module_system_name}_template"
         choices = [
             self.conf.template,
             getattr(self.spec.package, package_attribute, None),
@@ -952,7 +952,7 @@ class BaseModuleFileWriter:
 
         # Attribute from package
         module_name = str(self.module.__name__).split(".")[-1]
-        attr_name = "{0}_context".format(module_name)
+        attr_name = f"{module_name}_context"
         pkg_update = getattr(self.spec.package, attr_name, {})
         context.update(pkg_update)
 
@@ -1002,7 +1002,7 @@ class BaseModuleFileWriter:
 
         if modulerc_exists:
             # retrieve modulerc content
-            with open(modulerc_path, "r") as f:
+            with open(modulerc_path) as f:
                 content = f.readlines()
                 content = "".join(content).split("\n")
                 # remove last empty item if any
diff --git a/lib/spack/spack/test/cmd/env.py b/lib/spack/spack/test/cmd/env.py
index c3a7551e94..3fd40867eb 100644
--- a/lib/spack/spack/test/cmd/env.py
+++ b/lib/spack/spack/test/cmd/env.py
@@ -53,6 +53,7 @@ concretize = SpackCommand("concretize")
 stage = SpackCommand("stage")
 uninstall = SpackCommand("uninstall")
 find = SpackCommand("find")
+module = SpackCommand("module")
 
 sep = os.sep
 
@@ -1105,13 +1106,14 @@ def test_multi_env_remove(mutable_mock_env_path, monkeypatch, answer):
         assert all(e in env("list") for e in environments)
 
 
-def test_env_loads(install_mockery, mock_fetch):
+def test_env_loads(install_mockery, mock_fetch, mock_modules_root):
     env("create", "test")
 
     with ev.read("test"):
         add("mpileaks")
         concretize()
         install("--fake")
+        module("tcl", "refresh", "-y")
 
     with ev.read("test"):
         env("loads")
diff --git a/lib/spack/spack/test/conftest.py b/lib/spack/spack/test/conftest.py
index 514b1e9154..fb7608a56b 100644
--- a/lib/spack/spack/test/conftest.py
+++ b/lib/spack/spack/test/conftest.py
@@ -6,6 +6,7 @@
 import collections
 import datetime
 import errno
+import functools
 import inspect
 import itertools
 import json
@@ -1967,3 +1968,14 @@ def disable_parallel_buildcache_push(monkeypatch):
             pass
 
     monkeypatch.setattr(spack.cmd.buildcache, "_make_pool", MockPool)
+
+
+def _root_path(x, y, *, path):
+    return path
+
+
+@pytest.fixture
+def mock_modules_root(tmp_path, monkeypatch):
+    """Sets the modules root to a temporary directory, to avoid polluting configuration scopes."""
+    fn = functools.partial(_root_path, path=str(tmp_path))
+    monkeypatch.setattr(spack.modules.common, "root_path", fn)
diff --git a/lib/spack/spack/test/data/config/modules.yaml b/lib/spack/spack/test/data/config/modules.yaml
index 28e2ec91b3..f217dd7eaf 100644
--- a/lib/spack/spack/test/data/config/modules.yaml
+++ b/lib/spack/spack/test/data/config/modules.yaml
@@ -14,12 +14,7 @@
 #   ~/.spack/modules.yaml
 # -------------------------------------------------------------------------
 modules:
-  default:
-    enable:
-      - tcl
-    roots:
-      tcl: $user_cache_path/tcl
-      lmod: $user_cache_path/lmod
+  default: {}
   prefix_inspections:
     bin:
       - PATH
diff --git a/lib/spack/spack/test/modules/common.py b/lib/spack/spack/test/modules/common.py
index 11b4305b48..906c1d5c2a 100644
--- a/lib/spack/spack/test/modules/common.py
+++ b/lib/spack/spack/test/modules/common.py
@@ -17,7 +17,10 @@ import spack.spec
 from spack.modules.common import UpstreamModuleIndex
 from spack.spec import Spec
 
-pytestmark = pytest.mark.not_on_windows("does not run on windows")
+pytestmark = [
+    pytest.mark.not_on_windows("does not run on windows"),
+    pytest.mark.usefixtures("mock_modules_root"),
+]
 
 
 def test_update_dictionary_extending_list():
@@ -174,6 +177,7 @@ def test_load_installed_package_not_in_repo(install_mockery, mock_fetch, monkeyp
     """Test that installed packages that have been removed are still loadable"""
     spec = Spec("trivial-install-test-package").concretized()
     spec.package.do_install()
+    spack.modules.module_types["tcl"](spec, "default", True).write()
 
     def find_nothing(*args):
         raise spack.repo.UnknownPackageError("Repo package access is disabled for test")
diff --git a/lib/spack/spack/test/modules/conftest.py b/lib/spack/spack/test/modules/conftest.py
index 210a88a65f..12ee5c1fcd 100644
--- a/lib/spack/spack/test/modules/conftest.py
+++ b/lib/spack/spack/test/modules/conftest.py
@@ -2,6 +2,8 @@
 # Spack Project Developers. See the top-level COPYRIGHT file for details.
 #
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
+import pathlib
+
 import pytest
 
 import spack.config
@@ -13,26 +15,15 @@ import spack.util.path
 
 @pytest.fixture()
 def modulefile_content(request):
-    """Returns a function that generates the content of a module file
-    as a list of lines.
-    """
-
+    """Returns a function that generates the content of a module file as a list of lines."""
     writer_cls = getattr(request.module, "writer_cls")
 
     def _impl(spec_str, module_set_name="default", explicit=True):
-        # Write the module file
-        spec = spack.spec.Spec(spec_str)
-        spec.concretize()
+        spec = spack.spec.Spec(spec_str).concretized()
         generator = writer_cls(spec, module_set_name, explicit)
         generator.write(overwrite=True)
-
-        # Get its filename
-        filename = generator.layout.filename
-
-        # Retrieve the content
-        with open(filename) as f:
-            content = f.readlines()
-            content = "".join(content).split("\n")
+        written_module = pathlib.Path(generator.layout.filename)
+        content = written_module.read_text().splitlines()
         generator.remove()
         return content
 
@@ -40,27 +31,21 @@ def modulefile_content(request):
 
 
 @pytest.fixture()
-def factory(request):
-    """Function that, given a spec string, returns an instance of the writer
-    and the corresponding spec.
-    """
-
-    # Class of the module file writer
+def factory(request, mock_modules_root):
+    """Given a spec string, returns an instance of the writer and the corresponding spec."""
     writer_cls = getattr(request.module, "writer_cls")
 
     def _mock(spec_string, module_set_name="default", explicit=True):
-        spec = spack.spec.Spec(spec_string)
-        spec.concretize()
+        spec = spack.spec.Spec(spec_string).concretized()
         return writer_cls(spec, module_set_name, explicit), spec
 
     return _mock
 
 
 @pytest.fixture()
-def mock_module_filename(monkeypatch, tmpdir):
-    filename = str(tmpdir.join("module"))
+def mock_module_filename(monkeypatch, tmp_path):
+    filename = tmp_path / "module"
     # Set for both module types so we can test both
-    monkeypatch.setattr(spack.modules.lmod.LmodFileLayout, "filename", filename)
-    monkeypatch.setattr(spack.modules.tcl.TclFileLayout, "filename", filename)
-
-    yield filename
+    monkeypatch.setattr(spack.modules.lmod.LmodFileLayout, "filename", str(filename))
+    monkeypatch.setattr(spack.modules.tcl.TclFileLayout, "filename", str(filename))
+    yield str(filename)
diff --git a/lib/spack/spack/test/modules/lmod.py b/lib/spack/spack/test/modules/lmod.py
index acaae90f69..35c3f3cd97 100644
--- a/lib/spack/spack/test/modules/lmod.py
+++ b/lib/spack/spack/test/modules/lmod.py
@@ -21,7 +21,10 @@ install = spack.main.SpackCommand("install")
 #: Class of the writer tested in this module
 writer_cls = spack.modules.lmod.LmodModulefileWriter
 
-pytestmark = pytest.mark.not_on_windows("does not run on windows")
+pytestmark = [
+    pytest.mark.not_on_windows("does not run on windows"),
+    pytest.mark.usefixtures("mock_modules_root"),
+]
 
 
 @pytest.fixture(params=["clang@=12.0.0", "gcc@=10.2.1"])
diff --git a/lib/spack/spack/test/modules/tcl.py b/lib/spack/spack/test/modules/tcl.py
index f43f3d041e..e2f1235db0 100644
--- a/lib/spack/spack/test/modules/tcl.py
+++ b/lib/spack/spack/test/modules/tcl.py
@@ -18,7 +18,10 @@ libdwarf_spec_string = "libdwarf target=x86_64"
 #: Class of the writer tested in this module
 writer_cls = spack.modules.tcl.TclModulefileWriter
 
-pytestmark = pytest.mark.not_on_windows("does not run on windows")
+pytestmark = [
+    pytest.mark.not_on_windows("does not run on windows"),
+    pytest.mark.usefixtures("mock_modules_root"),
+]
 
 
 @pytest.mark.usefixtures("config", "mock_packages", "mock_module_filename")
@@ -279,7 +282,7 @@ class TestTcl:
         projection = writer.spec.format(writer.conf.projections["all"])
         assert projection in writer.layout.use_name
 
-    def test_invalid_naming_scheme(self, factory, module_configuration, mock_module_filename):
+    def test_invalid_naming_scheme(self, factory, module_configuration):
         """Tests the evaluation of an invalid naming scheme."""
 
         module_configuration("invalid_naming_scheme")
@@ -290,7 +293,7 @@ class TestTcl:
         with pytest.raises(RuntimeError):
             writer.layout.use_name
 
-    def test_invalid_token_in_env_name(self, factory, module_configuration, mock_module_filename):
+    def test_invalid_token_in_env_name(self, factory, module_configuration):
         """Tests setting environment variables with an invalid name."""
 
         module_configuration("invalid_token_in_env_var_name")