Updates to stand-alone test documentation (#33703)

2 files changed, 154 insertions, 22 deletions

diff --git a/lib/spack/docs/packaging_guide.rst b/lib/spack/docs/packaging_guide.rst
index f7b8cfe35f..b618497497 100644
--- a/lib/spack/docs/packaging_guide.rst
+++ b/lib/spack/docs/packaging_guide.rst
@@ -5260,6 +5260,16 @@ where each argument has the following meaning:
   will run.
 
   The default of ``None`` corresponds to the current directory (``'.'``).
+  Each call starts with the working directory set to the spec's test stage
+  directory (i.e., ``self.test_suite.test_dir_for_spec(self.spec)``).
+
+.. warning::
+
+   Use of the package spec's installation directory for building and running
+   tests is **strongly** discouraged. Doing so has caused permission errors
+   for shared spack instances *and* for facilities that install the software
+   in read-only file systems or directories.
+
 
 """""""""""""""""""""""""""""""""""""""""
 Accessing package- and test-related files
@@ -5267,10 +5277,10 @@ Accessing package- and test-related files
 
 You may need to access files from one or more locations when writing
 stand-alone tests. This can happen if the software's repository does not
-include test source files or includes files but no way to build the
-executables using the installed headers and libraries. In these
-cases, you may need to reference the files relative to one or more
-root directory. The properties containing package- and test-related
+include test source files or includes files but has no way to build the
+executables using the installed headers and libraries. In these cases,
+you may need to reference the files relative to one or more root
+directory. The properties containing package- (or spec-) and test-related
 directory paths are provided in the table below.
 
 .. list-table:: Directory-to-property mapping
@@ -5279,19 +5289,22 @@ directory paths are provided in the table below.
    * - Root Directory
      - Package Property
      - Example(s)
-   * - Package Installation Files
+   * - Package (Spec) Installation
      - ``self.prefix``
      - ``self.prefix.include``, ``self.prefix.lib``
-   * - Package Dependency's Files
+   * - Dependency Installation
      - ``self.spec['<dependency-package>'].prefix``
      - ``self.spec['trilinos'].prefix.include``
-   * - Test Suite Stage Files
+   * - Test Suite Stage
      - ``self.test_suite.stage``
      - ``join_path(self.test_suite.stage, 'results.txt')``
-   * - Staged Cached Build-time Files
+   * - Spec's Test Stage
+     - ``self.test_suite.test_dir_for_spec``
+     - ``self.test_suite.test_dir_for_spec(self.spec)``
+   * - Current Spec's Build-time Files
      - ``self.test_suite.current_test_cache_dir``
      - ``join_path(self.test_suite.current_test_cache_dir, 'examples', 'foo.c')``
-   * - Staged Custom Package Files
+   * - Current Spec's Custom Test Files
      - ``self.test_suite.current_test_data_dir``
      - ``join_path(self.test_suite.current_test_data_dir, 'hello.f90')``
 
@@ -6375,4 +6388,4 @@ To achieve backward compatibility with the single-class format Spack creates in
 
 Overall the role of the adapter is to route access to attributes of methods first through the ``*Package``
 hierarchy, and then back to the base class builder. This is schematically shown in the diagram above, where
-the adapter role is to "emulate" a method resolution order like the one represented by the red arrows.
\ No newline at end of file
+the adapter role is to "emulate" a method resolution order like the one represented by the red arrows.
diff --git a/lib/spack/spack/install_test.py b/lib/spack/spack/install_test.py
index da2b73032e..3c976febb1 100644
--- a/lib/spack/spack/install_test.py
+++ b/lib/spack/spack/install_test.py
@@ -43,12 +43,24 @@ def get_escaped_text_output(filename):
 
 
 def get_test_stage_dir():
+    """Retrieves the ``config:test_stage`` path to the configured test stage
+    root directory
+
+    Returns:
+        str: absolute path to the configured test stage root or, if none,
+            the default test stage path
+    """
     return spack.util.path.canonicalize_path(
         spack.config.get("config:test_stage", spack.paths.default_test_path)
     )
 
 
 def get_all_test_suites():
+    """Retrieves all validly staged TestSuites
+
+    Returns:
+        list: a list of TestSuite objects, which may be empty if there are none
+    """
     stage_root = get_test_stage_dir()
     if not os.path.isdir(stage_root):
         return []
@@ -68,7 +80,14 @@ def get_all_test_suites():
 
 
 def get_named_test_suites(name):
-    """Return a list of the names of any test suites with that name."""
+    """Retrieves test suites with the provided name.
+
+    Returns:
+        list: a list of matching TestSuite instances, which may be empty if none
+
+    Raises:
+        TestSuiteNameError: If no name is provided
+    """
     if not name:
         raise TestSuiteNameError("Test suite name is required.")
 
@@ -77,6 +96,14 @@ def get_named_test_suites(name):
 
 
 def get_test_suite(name):
+    """Ensure there is only one matching test suite with the provided name.
+
+    Returns:
+        str or None: the name if one matching test suite, else None
+
+    Raises:
+        TestSuiteNameError: If there is more than one matching TestSuite
+    """
     names = get_named_test_suites(name)
     if len(names) > 1:
         raise TestSuiteNameError('Too many suites named "{0}".  May shadow hash.'.format(name))
@@ -87,12 +114,14 @@ def get_test_suite(name):
 
 
 def write_test_suite_file(suite):
-    """Write the test suite to its lock file."""
+    """Write the test suite to its (JSON) lock file."""
     with open(suite.stage.join(test_suite_filename), "w") as f:
         sjson.dump(suite.to_dict(), stream=f)
 
 
 def write_test_summary(num_failed, num_skipped, num_untested, num_specs):
+    """Write a well formatted summary of the totals for each relevant status
+    category."""
     failed = "{0} failed, ".format(num_failed) if num_failed else ""
     skipped = "{0} skipped, ".format(num_skipped) if num_skipped else ""
     no_tests = "{0} no-tests, ".format(num_untested) if num_untested else ""
@@ -108,6 +137,8 @@ def write_test_summary(num_failed, num_skipped, num_untested, num_specs):
 
 
 class TestSuite(object):
+    """The class that manages specs for ``spack test run`` execution."""
+
     def __init__(self, specs, alias=None):
         # copy so that different test suites have different package objects
         # even if they contain the same spec
@@ -122,10 +153,12 @@ class TestSuite(object):
 
     @property
     def name(self):
+        """The name (alias or, if none, hash) of the test suite."""
         return self.alias if self.alias else self.content_hash
 
     @property
     def content_hash(self):
+        """The hash used to uniquely identify the test suite."""
         if not self._hash:
             json_text = sjson.dump(self.to_dict())
             sha = hashlib.sha1(json_text.encode("utf-8"))
@@ -212,48 +245,100 @@ class TestSuite(object):
             raise TestSuiteFailure(self.fails)
 
     def ensure_stage(self):
+        """Ensure the test suite stage directory exists."""
         if not os.path.exists(self.stage):
             fs.mkdirp(self.stage)
 
     @property
     def stage(self):
+        """The root test suite stage directory."""
         return spack.util.prefix.Prefix(os.path.join(get_test_stage_dir(), self.content_hash))
 
     @property
     def results_file(self):
+        """The path to the results summary file."""
         return self.stage.join(results_filename)
 
     @classmethod
     def test_pkg_id(cls, spec):
-        """Build the standard install test package identifier
+        """The standard install test package identifier.
 
         Args:
-        spec (Spec): instance of the spec under test
+            spec (spack.spec.Spec): instance of the spec under test
 
         Returns:
-        (str): the install test package identifier
+            str: the install test package identifier
         """
         return spec.format("{name}-{version}-{hash:7}")
 
     @classmethod
     def test_log_name(cls, spec):
+        """The standard log filename for a spec.
+
+        Args:
+            spec (spack.spec.Spec): instance of the spec under test
+
+        Returns:
+            str: the spec's log filename
+        """
         return "%s-test-out.txt" % cls.test_pkg_id(spec)
 
     def log_file_for_spec(self, spec):
+        """The test log file path for the provided spec.
+
+        Args:
+            spec (spack.spec.Spec): instance of the spec under test
+
+        Returns:
+            str: the path to the spec's log file
+        """
         return self.stage.join(self.test_log_name(spec))
 
     def test_dir_for_spec(self, spec):
+        """The path to the test stage directory for the provided spec.
+
+        Args:
+            spec (spack.spec.Spec): instance of the spec under test
+
+        Returns:
+            str: the spec's test stage directory path
+        """
         return self.stage.join(self.test_pkg_id(spec))
 
     @classmethod
     def tested_file_name(cls, spec):
+        """The standard test status filename for the spec.
+
+        Args:
+            spec (spack.spec.Spec): instance of the spec under test
+
+        Returns:
+            str: the spec's test status filename
+        """
         return "%s-tested.txt" % cls.test_pkg_id(spec)
 
     def tested_file_for_spec(self, spec):
+        """The test status file path for the spec.
+
+        Args:
+            spec (spack.spec.Spec): instance of the spec under test
+
+        Returns:
+            str: the spec's test status file path
+        """
         return self.stage.join(self.tested_file_name(spec))
 
     @property
     def current_test_cache_dir(self):
+        """Path to the test stage directory where the current spec's cached
+        build-time files were automatically copied.
+
+        Returns:
+            str: path to the current spec's staged, cached build-time files.
+
+        Raises:
+            TestSuiteSpecError: If there is no spec being tested
+        """
         if not (self.current_test_spec and self.current_base_spec):
             raise TestSuiteSpecError("Unknown test cache directory: no specs being tested")
 
@@ -263,6 +348,15 @@ class TestSuite(object):
 
     @property
     def current_test_data_dir(self):
+        """Path to the test stage directory where the current spec's custom
+        package (data) files were automatically copied.
+
+        Returns:
+            str: path to the current spec's staged, custom package (data) files
+
+        Raises:
+            TestSuiteSpecError: If there is no spec being tested
+        """
         if not (self.current_test_spec and self.current_base_spec):
             raise TestSuiteSpecError("Unknown test data directory: no specs being tested")
 
@@ -270,13 +364,13 @@ class TestSuite(object):
         base_spec = self.current_base_spec
         return self.test_dir_for_spec(base_spec).data.join(test_spec.name)
 
-    def add_failure(self, exc, msg):
-        current_hash = self.current_base_spec.dag_hash()
-        current_failures = self.failures.get(current_hash, [])
-        current_failures.append((exc, msg))
-        self.failures[current_hash] = current_failures
-
     def write_test_result(self, spec, result):
+        """Write the spec's test result to the test suite results file.
+
+        Args:
+            spec (spack.spec.Spec): instance of the spec under test
+            result (str): result from the spec's test execution (e.g, PASSED)
+        """
         msg = "{0} {1}".format(self.test_pkg_id(spec), result)
         _add_msg_to_file(self.results_file, msg)
 
@@ -295,6 +389,14 @@ class TestSuite(object):
         write_test_suite_file(self)
 
     def to_dict(self):
+        """Build a dictionary for the test suite.
+
+        Returns:
+            dict: The dictionary contains entries for up to two keys:
+
+                specs: list of the test suite's specs in dictionary form
+                alias: the alias, or name, given to the test suite if provided
+        """
         specs = [s.to_dict() for s in self.specs]
         d = {"specs": specs}
         if self.alias:
@@ -303,12 +405,29 @@ class TestSuite(object):
 
     @staticmethod
     def from_dict(d):
+        """Instantiates a TestSuite based on a dictionary specs and an
+        optional alias:
+
+            specs: list of the test suite's specs in dictionary form
+            alias: the test suite alias
+
+
+        Returns:
+            TestSuite: Instance of TestSuite created from the specs
+        """
         specs = [Spec.from_dict(spec_dict) for spec_dict in d["specs"]]
         alias = d.get("alias", None)
         return TestSuite(specs, alias)
 
     @staticmethod
     def from_file(filename):
+        """Instantiate a TestSuite using the specs and optional alias
+        provided in the given file.
+
+        Args:
+            filename (str): The path to the JSON file containing the test
+                suite specs and optional alias.
+        """
         try:
             with open(filename, "r") as f:
                 data = sjson.load(f)
@@ -324,7 +443,7 @@ class TestSuite(object):
 
 
 def _add_msg_to_file(filename, msg):
-    """Add the message to the specified file
+    """Append the message to the specified file.
 
     Args:
         filename (str): path to the file