Test-Builder refactor: move do_test from PackageBase to PackageTest

These changes allow removing references to test_suite from builder.py,
deferring them to PackageTest, which ensures the test_suite is set
for the builder's phase tests.

Also added comments to explain the need for test-related attributes
to the builder and package_base.

lib/spack/llnl/util/tty/log.py

@@ -21,7 +21,7 @@
 from multiprocessing.connection import Connection
 from threading import Thread
 from types import ModuleType
-from typing import Callable, Optional
+from typing import Callable, Optional, Union
 
 import llnl.util.tty as tty
 
@@ -1022,3 +1022,22 @@ def wrapped(*args, **kwargs):
 
 def _input_available(f):
     return f in select.select([f], [], [], 0)[0]
+
+
+LogType = Union[nixlog, winlog]
+
+
+def print_message(logger: LogType, msg: str, verbose: bool = False):
+    """Print the message to the log, optionally echoing.
+
+    Args:
+        logger: instance of the output logger (e.g. nixlog or winlog)
+        msg: message being output
+        verbose: ``True`` displays verbose output, ``False`` suppresses
+            it (``False`` is default)
+    """
+    if verbose:
+        with logger.force_echo():
+            tty.info(msg, format="g")
+    else:
+        tty.info(msg, format="g")

lib/spack/spack/build_systems/_checks.py

@@ -112,7 +112,7 @@ def execute_build_time_tests(builder: spack.builder.Builder):
     if not builder.pkg.run_tests or not builder.build_time_test_callbacks:
         return
 
-    builder.pkg.tester.phase_tests(builder, "build", builder.build_time_test_callbacks)
+    builder.phase_tests("build", builder.build_time_test_callbacks)
 
 
 def execute_install_time_tests(builder: spack.builder.Builder):
@@ -125,7 +125,7 @@ def execute_install_time_tests(builder: spack.builder.Builder):
     if not builder.pkg.run_tests or not builder.install_time_test_callbacks:
         return
 
-    builder.pkg.tester.phase_tests(builder, "install", builder.install_time_test_callbacks)
+    builder.phase_tests("install", builder.install_time_test_callbacks)
 
 
 class BuilderWithDefaults(spack.builder.Builder):

lib/spack/spack/builder.py

@@ -8,7 +8,12 @@
 import functools
 from typing import Dict, List, Optional, Tuple, Type
 
+import llnl.util.tty as tty
+import llnl.util.tty.log as log
+
+import spack.config
 import spack.error
+import spack.install_test
 import spack.multimethod
 import spack.package_base
 import spack.phase_callbacks
@@ -122,11 +127,16 @@ def __init__(self, wrapped_pkg_object, root_builder):
                 new_cls_name,
                 bases,
                 {
+                    # boolean to indicate whether install-time tests are run
                     "run_tests": property(lambda x: x.wrapped_package_object.run_tests),
+                    # boolean to indicate whether the package's stand-alone tests
+                    # require a compiler
                     "test_requires_compiler": property(
                         lambda x: x.wrapped_package_object.test_requires_compiler
                     ),
+                    # TestSuite instance the spec is a part of
                     "test_suite": property(lambda x: x.wrapped_package_object.test_suite),
+                    # PackageTest instance to manage the spec's testing
                     "tester": property(lambda x: x.wrapped_package_object.tester),
                 },
             )
@@ -481,7 +491,7 @@ def __str__(self):
 
 class Builder(BaseBuilder, collections.abc.Sequence):
     """A builder is a class that, given a package object (i.e. associated with concrete spec),
-    knows how to install it.
+    knows how to install it and perform install-time checks.
 
     The builder behaves like a sequence, and when iterated over return the "phases" of the
     installation in the correct order.
@@ -518,3 +528,52 @@ def __getitem__(self, idx):
 
     def __len__(self):
         return len(self.phases)
+
+    def phase_tests(self, phase_name: str, method_names: List[str]):
+        """Execute the package's phase-time tests.
+
+        This process uses the same test setup and logging used for
+        stand-alone tests for consistency.
+
+        Args:
+            phase_name: the name of the build-time phase (e.g., ``build``, ``install``)
+            method_names: phase-specific callback method names
+        """
+        verbose = tty.is_verbose()
+        fail_fast = spack.config.get("config:fail_fast", False)
+
+        tester = self.pkg.tester
+        with tester.test_logger(verbose=verbose, externals=False) as logger:
+            # Report running each of the methods in the build log
+            log.print_message(logger, f"Running {phase_name}-time tests", verbose)
+            tester.set_current_specs(self.pkg.spec, self.pkg.spec)
+
+            have_tests = any(name.startswith("test_") for name in method_names)
+            if have_tests:
+                spack.install_test.copy_test_files(self.pkg, self.pkg.spec)
+
+            for name in method_names:
+                try:
+                    # Prefer the method in the package over the builder's.
+                    # We need this primarily to pick up arbitrarily named test
+                    # methods but also some build-time checks.
+                    fn = getattr(self.pkg, name, getattr(self, name))
+
+                    msg = f"RUN-TESTS: {phase_name}-time tests [{name}]"
+                    log.print_message(logger, msg, verbose)
+
+                    fn()
+
+                except AttributeError as e:
+                    msg = f"RUN-TESTS: method not implemented [{name}]"
+                    log.print_message(logger, msg, verbose)
+
+                    tester.add_failure(e, msg)
+                    if fail_fast:
+                        break
+
+            if have_tests:
+                log.print_message(logger, "Completed testing", verbose)
+
+            # Raise exception if any failures encountered
+            tester.handle_failures()

lib/spack/spack/install_test.py

@@ -17,12 +17,13 @@
 
 import llnl.util.filesystem as fs
 import llnl.util.tty as tty
-import llnl.util.tty.log
+import llnl.util.tty.log as log
 from llnl.string import plural
 from llnl.util.lang import nullcontext
 from llnl.util.tty.color import colorize
 
 import spack.build_environment
+import spack.compilers
 import spack.config
 import spack.error
 import spack.package_base
@@ -50,7 +51,6 @@
 
 
 ListOrStringType = Union[str, List[str]]
-LogType = Union[llnl.util.tty.log.nixlog, llnl.util.tty.log.winlog]
 
 Pb = TypeVar("Pb", bound="spack.package_base.PackageBase")
 PackageObjectOrClass = Union[Pb, Type[Pb]]
@@ -207,22 +207,6 @@ def install_test_root(pkg: Pb):
     return os.path.join(pkg.metadata_dir, "test")
 
 
-def print_message(logger: LogType, msg: str, verbose: bool = False):
-    """Print the message to the log, optionally echoing.
-
-    Args:
-        logger: instance of the output logger (e.g. nixlog or winlog)
-        msg: message being output
-        verbose: ``True`` displays verbose output, ``False`` suppresses
-            it (``False`` is default)
-    """
-    if verbose:
-        with logger.force_echo():
-            tty.info(msg, format="g")
-    else:
-        tty.info(msg, format="g")
-
-
 def overall_status(current_status: "TestStatus", substatuses: List["TestStatus"]) -> "TestStatus":
     """Determine the overall status based on the current and associated sub status values.
 
@@ -269,15 +253,16 @@ def __init__(self, pkg: Pb):
         self.test_log_file: str
         self.pkg_id: str
 
-        if pkg.test_suite:
+        if self.pkg.test_suite is not None:
             # Running stand-alone tests
-            self.test_log_file = pkg.test_suite.log_file_for_spec(pkg.spec)
-            self.tested_file = pkg.test_suite.tested_file_for_spec(pkg.spec)
-            self.pkg_id = pkg.test_suite.test_pkg_id(pkg.spec)
+            suite = self.pkg.test_suite
+            self.test_log_file = suite.log_file_for_spec(pkg.spec)  # type: ignore[union-attr]
+            self.tested_file = suite.tested_file_for_spec(pkg.spec)  # type: ignore[union-attr]
+            self.pkg_id = suite.test_pkg_id(pkg.spec)  # type: ignore[union-attr]
         else:
             # Running phase-time tests for a single package whose results are
             # retained in the package's stage directory.
-            pkg.test_suite = TestSuite([pkg.spec])
+            self.pkg.test_suite = TestSuite([pkg.spec])
             self.test_log_file = fs.join_path(pkg.stage.path, spack_install_test_log)
             self.pkg_id = pkg.spec.format("{name}-{version}-{hash:7}")
 
@@ -285,10 +270,10 @@ def __init__(self, pkg: Pb):
         self._logger = None
 
     @property
-    def logger(self) -> Optional[LogType]:
+    def logger(self) -> Optional[log.LogType]:
         """The current logger or, if none, sets to one."""
         if not self._logger:
-            self._logger = llnl.util.tty.log.log_output(self.test_log_file)
+            self._logger = log.log_output(self.test_log_file)
 
         return self._logger
 
@@ -305,7 +290,7 @@ def test_logger(self, verbose: bool = False, externals: bool = False):
         fs.touch(self.test_log_file)  # Otherwise log_parse complains
         fs.set_install_permissions(self.test_log_file)
 
-        with llnl.util.tty.log.log_output(self.test_log_file, verbose) as self._logger:
+        with log.log_output(self.test_log_file, verbose) as self._logger:
             with self.logger.force_echo():  # type: ignore[union-attr]
                 tty.msg("Testing package " + colorize(r"@*g{" + self.pkg_id + r"}"))
 
@@ -331,6 +316,13 @@ def add_failure(self, exception: Exception, msg: str):
         """Add the failure details to the current list."""
         self.test_failures.append((exception, msg))
 
+    def set_current_specs(self, base_spec: spack.spec.Spec, test_spec: spack.spec.Spec):
+        # Ignore union-attr check for test_suite since the constructor of this
+        # class ensures it is always not None.
+        test_suite = self.pkg.test_suite
+        test_suite.current_base_spec = base_spec  # type: ignore[union-attr]
+        test_suite.current_test_spec = test_spec  # type: ignore[union-attr]
+
     def status(self, name: str, status: "TestStatus", msg: Optional[str] = None):
         """Track and print the test status for the test part name."""
         part_name = f"{self.pkg.__class__.__name__}::{name}"
@@ -352,63 +344,54 @@ def status(self, name: str, status: "TestStatus", msg: Optional[str] = None):
         self.test_parts[part_name] = status
         self.counts[status] += 1
 
-    def phase_tests(self, builder, phase_name: str, method_names: List[str]):
-        """Execute the builder's package phase-time tests.
+    def handle_failures(self):
+        """Raise exception if any failures were collected during testing
 
-        Args:
-            builder: builder for package being tested
-            phase_name: the name of the build-time phase (e.g., ``build``, ``install``)
-            method_names: phase-specific callback method names
+        Raises:
+            TestFailure: test failures were collected
         """
-        verbose = tty.is_verbose()
-        fail_fast = spack.config.get("config:fail_fast", False)
+        if self.test_failures:
+            raise TestFailure(self.test_failures)
 
-        with self.test_logger(verbose=verbose, externals=False) as logger:
-            # Report running each of the methods in the build log
-            print_message(logger, f"Running {phase_name}-time tests", verbose)
-            builder.pkg.test_suite.current_test_spec = builder.pkg.spec
-            builder.pkg.test_suite.current_base_spec = builder.pkg.spec
-
-            have_tests = any(name.startswith("test_") for name in method_names)
-            if have_tests:
-                copy_test_files(builder.pkg, builder.pkg.spec)
-
-            for name in method_names:
-                try:
-                    # Prefer the method in the package over the builder's.
-                    # We need this primarily to pick up arbitrarily named test
-                    # methods but also some build-time checks.
-                    fn = getattr(builder.pkg, name, getattr(builder, name))
-
-                    msg = f"RUN-TESTS: {phase_name}-time tests [{name}]"
-                    print_message(logger, msg, verbose)
-
-                    fn()
-
-                except AttributeError as e:
-                    msg = f"RUN-TESTS: method not implemented [{name}]"
-                    print_message(logger, msg, verbose)
-
-                    self.add_failure(e, msg)
-                    if fail_fast:
-                        break
-
-            if have_tests:
-                print_message(logger, "Completed testing", verbose)
-
-            # Raise any collected failures here
-            if self.test_failures:
-                raise TestFailure(self.test_failures)
-
-    def stand_alone_tests(self, kwargs):
+    def stand_alone_tests(self, dirty=False, externals=False):
         """Run the package's stand-alone tests.
 
         Args:
             kwargs (dict): arguments to be used by the test process
-        """
-        import spack.build_environment
 
-        spack.build_environment.start_build_process(self.pkg, test_process, kwargs)
+        Raises:
+            AttributeError: required test_requires_compiler attribute is missing
+        """
+        pkg = self.pkg
+        spec = pkg.spec
+        pkg_spec = spec.format("{name}-{version}-{hash:7}")
+
+        if not hasattr(pkg, "test_requires_compiler"):
+            raise AttributeError(
+                f"Cannot run tests for {pkg_spec}: missing required "
+                "test_requires_compiler attribute"
+            )
+
+        if pkg.test_requires_compiler:
+            compilers = spack.compilers.compilers_for_spec(
+                spec.compiler, arch_spec=spec.architecture
+            )
+            if not compilers:
+                tty.error(
+                    f"Skipping tests for package {pkg_spec}\n"
+                    f"Package test requires missing compiler {spec.compiler}"
+                )
+                return
+
+        kwargs = {
+            "dirty": dirty,
+            "fake": False,
+            "context": "test",
+            "externals": externals,
+            "verbose": tty.is_verbose(),
+        }
+
+        spack.build_environment.start_build_process(pkg, test_process, kwargs)
 
     def parts(self) -> int:
         """The total number of (checked) test parts."""
@@ -700,10 +683,9 @@ def process_test_parts(pkg: Pb, test_specs: List[spack.spec.Spec], verbose: bool
                 ):
                     test_fn(pkg)
 
-        # If fail-fast was on, we error out above
-        # If we collect errors, raise them in batch here
-        if tester.test_failures:
-            raise TestFailure(tester.test_failures)
+        # If fail-fast was on, we errored out above
+        # If we collected errors, raise them in batch here
+        tester.handle_failures()
 
     finally:
         if tester.ran_tests():
@@ -729,12 +711,12 @@ def test_process(pkg: Pb, kwargs):
 
     with pkg.tester.test_logger(verbose, externals) as logger:
         if pkg.spec.external and not externals:
-            print_message(logger, "Skipped tests for external package", verbose)
+            log.print_message(logger, "Skipped tests for external package", verbose)
             pkg.tester.status(pkg.spec.name, TestStatus.SKIPPED)
             return
 
         if not pkg.spec.installed:
-            print_message(logger, "Skipped not installed package", verbose)
+            log.print_message(logger, "Skipped not installed package", verbose)
             pkg.tester.status(pkg.spec.name, TestStatus.SKIPPED)
             return
 
@@ -859,7 +841,7 @@ def __init__(self, specs, alias=None):
         # even if they contain the same spec
         self.specs = [spec.copy() for spec in specs]
         self.current_test_spec = None  # spec currently tested, can be virtual
-        self.current_base_spec = None  # spec currently running do_test
+        self.current_base_spec = None  # spec currently running tests
 
         self.alias = alias
         self._hash = None
@@ -883,6 +865,10 @@ def content_hash(self):
             self._hash = b32_hash
         return self._hash
 
+    def set_current_specs(self, base_spec: spack.spec.Spec, test_spec: spack.spec.Spec):
+        self.current_base_spec = base_spec
+        self.current_test_spec = test_spec
+
     def __call__(self, *args, **kwargs):
         self.write_reproducibility_data()
 
@@ -892,18 +878,16 @@ def __call__(self, *args, **kwargs):
         externals = kwargs.get("externals", False)
 
         for spec in self.specs:
+            pkg = spec.package
             try:
-                if spec.package.test_suite:
+                if pkg.test_suite:
                     raise TestSuiteSpecError(
-                        "Package {} cannot be run in two test suites at once".format(
-                            spec.package.name
-                        )
+                        f"Package {pkg.name} cannot be run in two test suites at once"
                     )
 
                 # Set up the test suite to know which test is running
-                spec.package.test_suite = self
-                self.current_base_spec = spec
-                self.current_test_spec = spec
+                pkg.test_suite = self
+                self.set_current_specs(spec, spec)
 
                 # setup per-test directory in the stage dir
                 test_dir = self.test_dir_for_spec(spec)
@@ -912,7 +896,7 @@ def __call__(self, *args, **kwargs):
                 fs.mkdirp(test_dir)
 
                 # run the package tests
-                spec.package.do_test(dirty=dirty, externals=externals)
+                pkg.tester.stand_alone_tests(dirty=dirty, externals=externals)
 
                 # Clean up on success
                 if remove_directory:
@@ -946,8 +930,7 @@ def __call__(self, *args, **kwargs):
 
             finally:
                 spec.package.test_suite = None
-                self.current_test_spec = None
-                self.current_base_spec = None
+                self.set_current_specs(None, None)
 
         write_test_summary(self.counts)
 

lib/spack/spack/package_base.py

@@ -712,7 +712,7 @@ class PackageBase(WindowsRPath, PackageViewMixin, metaclass=PackageMeta):
     #: are available to build a custom test code.
     test_requires_compiler: bool = False
 
-    #: TestSuite instance used to manage stand-alone tests for 1+ specs.
+    #: The spec's TestSuite instance, which is used to manage its testing.
     test_suite: Optional[Any] = None
 
     def __init__(self, spec):
@@ -1943,29 +1943,6 @@ def _resource_stage(self, resource):
         resource_stage_folder = "-".join(pieces)
         return resource_stage_folder
 
-    def do_test(self, dirty=False, externals=False):
-        if self.test_requires_compiler:
-            compilers = spack.compilers.compilers_for_spec(
-                self.spec.compiler, arch_spec=self.spec.architecture
-            )
-            if not compilers:
-                tty.error(
-                    "Skipping tests for package %s\n"
-                    % self.spec.format("{name}-{version}-{hash:7}")
-                    + "Package test requires missing compiler %s" % self.spec.compiler
-                )
-                return
-
-        kwargs = {
-            "dirty": dirty,
-            "fake": False,
-            "context": "test",
-            "externals": externals,
-            "verbose": tty.is_verbose(),
-        }
-
-        self.tester.stand_alone_tests(kwargs)
-
     def unit_test_check(self):
         """Hook for unit tests to assert things about package internals.
 

lib/spack/spack/report.py

@@ -204,7 +204,7 @@ def extract_package_from_signature(self, instance, *args, **kwargs):
 
 
 class TestInfoCollector(InfoCollector):
-    """Collect information for the PackageBase.do_test method.
+    """Collect information for the PackageTest.stand_alone_tests method.
 
     Args:
         specs: specs whose install information will be recorded
@@ -214,7 +214,7 @@ class TestInfoCollector(InfoCollector):
     dir: str
 
     def __init__(self, specs: List[spack.spec.Spec], record_directory: str):
-        super().__init__(spack.package_base.PackageBase, "do_test", specs)
+        super().__init__(spack.install_test.PackageTest, "stand_alone_tests", specs)
         self.dir = record_directory
 
     def on_success(self, pkg, kwargs, package_record):
@@ -233,7 +233,7 @@ def fetch_log(self, pkg: spack.package_base.PackageBase):
             return f"Cannot open log for {pkg.spec.cshort_spec}"
 
     def extract_package_from_signature(self, instance, *args, **kwargs):
-        return instance
+        return instance.pkg
 
 
 @contextlib.contextmanager

lib/spack/spack/test/package_class.py

@@ -17,7 +17,6 @@
 
 import llnl.util.filesystem as fs
 
-import spack.compilers
 import spack.deptypes as dt
 import spack.error
 import spack.install_test
@@ -262,7 +261,7 @@ def test_package_tester_fails():
     s = spack.spec.Spec("pkg-a")
     pkg = BaseTestPackage(s)
     with pytest.raises(ValueError, match="without concrete version"):
-        pkg.tester()
+        pkg.tester
 
 
 def test_package_fetcher_fails():
@@ -270,18 +269,3 @@ def test_package_fetcher_fails():
     pkg = BaseTestPackage(s)
     with pytest.raises(ValueError, match="without concrete version"):
         pkg.fetcher
-
-
-def test_package_test_no_compilers(mock_packages, monkeypatch, capfd):
-    def compilers(compiler, arch_spec):
-        return None
-
-    monkeypatch.setattr(spack.compilers, "compilers_for_spec", compilers)
-
-    s = spack.spec.Spec("pkg-a")
-    pkg = BaseTestPackage(s)
-    pkg.test_requires_compiler = True
-    pkg.do_test()
-    error = capfd.readouterr()[1]
-    assert "Skipping tests for package" in error
-    assert "test requires missing compiler" in error

lib/spack/spack/test/test_suite.py

@@ -10,6 +10,7 @@
 
 from llnl.util.filesystem import join_path, mkdirp, touch
 
+import spack.compilers
 import spack.config
 import spack.install_test
 import spack.spec
@@ -525,3 +526,18 @@ def test_packagetest_fails(mock_packages):
     pkg = MyPackage(s)
     with pytest.raises(ValueError, match="require a concrete package"):
         spack.install_test.PackageTest(pkg)
+
+
+def test_package_test_no_compilers(mock_packages, config, monkeypatch, capfd):
+    s = spack.spec.Spec("libdwarf").concretized()
+
+    def compilers(compiler, arch_spec):
+        return None
+
+    monkeypatch.setattr(spack.compilers, "compilers_for_spec", compilers)
+
+    s.package.test_requires_compiler = True
+    s.package.tester.stand_alone_tests()
+    error = capfd.readouterr()[1]
+    assert "Skipping tests for package" in error
+    assert "test requires missing compiler" in error