From 2f7d8947ba9b66827c05446f7c7c382c840040de Mon Sep 17 00:00:00 2001
From: Tamara Dahlgren <35777542+tldahlgren@users.noreply.github.com>
Date: Mon, 18 Nov 2024 17:15:40 -0800
Subject: [PATCH] Test-Builder refactor: move do_test from PackageBase to
 PackageTest

---
 lib/spack/spack/builder.py            |  1 +
 lib/spack/spack/install_test.py       | 57 +++++++++++++++++++--------
 lib/spack/spack/package_base.py       | 23 -----------
 lib/spack/spack/report.py             |  6 +--
 lib/spack/spack/test/package_class.py | 18 +--------
 lib/spack/spack/test/test_suite.py    | 16 ++++++++
 6 files changed, 62 insertions(+), 59 deletions(-)

diff --git a/lib/spack/spack/builder.py b/lib/spack/spack/builder.py
index 30cf3102019..6122b843495 100644
--- a/lib/spack/spack/builder.py
+++ b/lib/spack/spack/builder.py
@@ -13,6 +13,7 @@
 
 import spack.config
 import spack.error
+import spack.install_test
 import spack.multimethod
 import spack.package_base
 import spack.phase_callbacks
diff --git a/lib/spack/spack/install_test.py b/lib/spack/spack/install_test.py
index 3c91f12f200..75fa39983a8 100644
--- a/lib/spack/spack/install_test.py
+++ b/lib/spack/spack/install_test.py
@@ -23,6 +23,7 @@
 from llnl.util.tty.color import colorize
 
 import spack.build_environment
+import spack.compilers
 import spack.config
 import spack.error
 import spack.package_base
@@ -352,15 +353,45 @@ def handle_failures(self):
         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."""
@@ -810,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
@@ -834,10 +865,6 @@ def content_hash(self):
             self._hash = b32_hash
         return self._hash
 
-    # TODO/TLD: Finish this
-    def _run_test(self, pkg, dirty, externals):
-        pkg.do_test(dirty=dirty, externals=externals)
-
     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
@@ -851,16 +878,15 @@ 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
+                pkg.test_suite = self
                 self.set_current_specs(spec, spec)
 
                 # setup per-test directory in the stage dir
@@ -870,8 +896,7 @@ def __call__(self, *args, **kwargs):
                 fs.mkdirp(test_dir)
 
                 # run the package tests
-                # TLD spec.package.do_test(dirty=dirty, externals=externals)
-                self._run_test(spec.package, dirty=dirty, externals=externals)
+                pkg.tester.stand_alone_tests(dirty=dirty, externals=externals)
 
                 # Clean up on success
                 if remove_directory:
diff --git a/lib/spack/spack/package_base.py b/lib/spack/spack/package_base.py
index be82e4758d0..030773dd261 100644
--- a/lib/spack/spack/package_base.py
+++ b/lib/spack/spack/package_base.py
@@ -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.
 
diff --git a/lib/spack/spack/report.py b/lib/spack/spack/report.py
index 409810f58a9..6c089bbb6bf 100644
--- a/lib/spack/spack/report.py
+++ b/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
diff --git a/lib/spack/spack/test/package_class.py b/lib/spack/spack/test/package_class.py
index a8c541930bd..ec6dc373c0a 100644
--- a/lib/spack/spack/test/package_class.py
+++ b/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
diff --git a/lib/spack/spack/test/test_suite.py b/lib/spack/spack/test/test_suite.py
index 3ed4e30d42c..2495ec56204 100644
--- a/lib/spack/spack/test/test_suite.py
+++ b/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