From 8a8dcb9479add8fefaa6ecf3beb74cfe9b73d193 Mon Sep 17 00:00:00 2001 From: Massimiliano Culpo Date: Tue, 14 Nov 2023 11:29:28 +0100 Subject: modules: unit-tests without polluted user scope (#41041) --- lib/spack/spack/modules/common.py | 34 ++++++++++----------- lib/spack/spack/test/cmd/env.py | 4 ++- lib/spack/spack/test/conftest.py | 12 ++++++++ lib/spack/spack/test/data/config/modules.yaml | 7 +---- lib/spack/spack/test/modules/common.py | 6 +++- lib/spack/spack/test/modules/conftest.py | 43 +++++++++------------------ lib/spack/spack/test/modules/lmod.py | 5 +++- lib/spack/spack/test/modules/tcl.py | 9 ++++-- 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") -- cgit v1.2.3-60-g2f50