PackageBase should not define builder legacy attributes (#33942)

* Add a regression test for 33928

* PackageBase should not set `(build|install)_time_test_callbacks`

* Fix audits by preserving the current semantic

Co-authored-by: Massimiliano Culpo <massimiliano.culpo@gmail.com>

lib/spack/spack/audit.py

@@ -287,7 +287,7 @@ def _check_build_test_callbacks(pkgs, error_cls):
     errors = []
     for pkg_name in pkgs:
         pkg_cls = spack.repo.path.get_pkg_class(pkg_name)
-        test_callbacks = pkg_cls.build_time_test_callbacks
+        test_callbacks = getattr(pkg_cls, "build_time_test_callbacks", None)
 
         if test_callbacks and "test" in test_callbacks:
             msg = '{0} package contains "test" method in ' "build_time_test_callbacks"

lib/spack/spack/cmd/info.py

@@ -241,8 +241,8 @@ def print_tests(pkg):
     # So the presence of a callback in Spack does not necessarily correspond
     # to the actual presence of built-time tests for a package.
     for callbacks, phase in [
-        (pkg.build_time_test_callbacks, "Build"),
+        (getattr(pkg, "build_time_test_callbacks", None), "Build"),
-        (pkg.install_time_test_callbacks, "Install"),
+        (getattr(pkg, "install_time_test_callbacks", None), "Install"),
     ]:
         color.cprint("")
         color.cprint(section_title("Available {0} Phase Test Methods:".format(phase)))

lib/spack/spack/package_base.py

@@ -528,10 +528,6 @@ class PackageBase(WindowsRPath, PackageViewMixin, metaclass=PackageMeta):
     # These are default values for instance variables.
     #
 
-    #: A list or set of build time test functions to be called when tests
-    #: are executed or 'None' if there are no such test functions.
-    build_time_test_callbacks = None  # type: Optional[List[str]]
-
     #: By default, packages are not virtual
     #: Virtual packages override this attribute
     virtual = False
@@ -540,10 +536,6 @@ class PackageBase(WindowsRPath, PackageViewMixin, metaclass=PackageMeta):
     #: those that do not can be used to install a set of other Spack packages.
     has_code = True
 
-    #: A list or set of install time test functions to be called when tests
-    #: are executed or 'None' if there are no such test functions.
-    install_time_test_callbacks = None  # type: Optional[List[str]]
-
     #: By default we build in parallel.  Subclasses can override this.
     parallel = True
 

lib/spack/spack/test/builder.py

@@ -121,3 +121,17 @@ def test_old_style_compatibility_with_super(spec_str, method_name, expected):
     builder = spack.builder.create(s.package)
     value = getattr(builder, method_name)()
     assert value == expected
+
+
+@pytest.mark.regression("33928")
+@pytest.mark.usefixtures("builder_test_repository", "config", "working_env")
+@pytest.mark.disable_clean_stage_check
+def test_build_time_tests_are_executed_from_default_builder():
+    s = spack.spec.Spec("old-style-autotools").concretized()
+    builder = spack.builder.create(s.package)
+    builder.pkg.run_tests = True
+    for phase_fn in builder:
+        phase_fn.execute()
+
+    assert os.environ.get("CHECK_CALLED") == "1", "Build time tests not executed"
+    assert os.environ.get("INSTALLCHECK_CALLED") == "1", "Install time tests not executed"

var/spack/repos/builder.test/packages/old-style-autotools/package.py

@@ -48,3 +48,9 @@ def after_autoreconf_1(self):
     @run_after("autoreconf", when="@2.0")
     def after_autoreconf_2(self):
         os.environ["AFTER_AUTORECONF_2_CALLED"] = "1"
+
+    def check(self):
+        os.environ["CHECK_CALLED"] = "1"
+
+    def installcheck(self):
+        os.environ["INSTALLCHECK_CALLED"] = "1"