From b549548f6917388676fad1baf28b38e40546f6e6 Mon Sep 17 00:00:00 2001
From: Massimiliano Culpo <massimiliano.culpo@gmail.com>
Date: Fri, 30 Dec 2022 19:15:38 +0100
Subject: Simplify creation of test and install reports (#34712)

The code in Spack to generate install and test reports currently suffers from unneeded complexity. For
instance, we have classes in Spack core packages, like `spack.reporters.CDash`, that need an
`argparse.Namespace` to be initialized and have "hard-coded" string literals on which they branch to
change their behavior:

```python
if do_fn.__name__ == "do_test" and skip_externals:
    package["result"] = "skipped"
else:
    package["result"] = "success"
package["stdout"] = fetch_log(pkg, do_fn, self.dir)
package["installed_from_binary_cache"] = pkg.installed_from_binary_cache
if do_fn.__name__ == "_install_task" and installed_already:
    return
```
This PR attempt to polish the major issues encountered in both `spack.report` and `spack.reporters`.

Details:
- [x] `spack.reporters` is now a package that contains both the base class `Reporter` and all
      the derived classes (`JUnit` and `CDash`)
- [x] Classes derived from `spack.reporters.Reporter` don't take an `argparse.Namespace` anymore
       as argument to `__init__`. The rationale is that code for commands should be built upon Spack
       core classes, not vice-versa.
- [x] An `argparse.Action` has been coded to create the correct `Reporter` object based on command
       line arguments
- [x] The context managers to generate reports from either `spack install` or from `spack test` have
       been greatly simplified, and have been made less "dynamic" in nature. In particular, the `collect_info`
       class has been deleted in favor of two more specific context managers. This allows for a simpler
       structure of the code, and less knowledge required to client code (in particular on which method to patch)
- [x] The `InfoCollector` class has been turned into a simple hierarchy, so to avoid conditional statements
       within methods that assume a knowledge of the context in which the method is called.
---
 lib/spack/spack/ci.py                   |  19 +-
 lib/spack/spack/cmd/common/arguments.py |  59 ++++++
 lib/spack/spack/cmd/install.py          |  64 +++----
 lib/spack/spack/cmd/test.py             |  49 +++--
 lib/spack/spack/report.py               | 329 +++++++++++++++-----------------
 lib/spack/spack/reporter.py             |  23 ---
 lib/spack/spack/reporters/__init__.py   |   5 +
 lib/spack/spack/reporters/base.py       |  18 ++
 lib/spack/spack/reporters/cdash.py      |  78 ++++----
 lib/spack/spack/reporters/junit.py      |  24 +--
 lib/spack/spack/test/reporters.py       |  22 +--
 11 files changed, 368 insertions(+), 322 deletions(-)
 delete mode 100644 lib/spack/spack/reporter.py
 create mode 100644 lib/spack/spack/reporters/base.py

(limited to 'lib')

diff --git a/lib/spack/spack/ci.py b/lib/spack/spack/ci.py
index 381deb3c79..1129587b84 100644
--- a/lib/spack/spack/ci.py
+++ b/lib/spack/spack/ci.py
@@ -39,9 +39,8 @@ import spack.util.spack_yaml as syaml
 import spack.util.url as url_util
 import spack.util.web as web_util
 from spack.error import SpackError
-from spack.reporters.cdash import CDash
+from spack.reporters import CDash, CDashConfiguration
 from spack.reporters.cdash import build_stamp as cdash_build_stamp
-from spack.util.pattern import Bunch
 
 JOB_RETRY_CONDITIONS = [
     "always",
@@ -2358,10 +2357,14 @@ class CDashHandler(object):
             tty.warn(msg)
 
     def report_skipped(self, spec, directory_name, reason):
-        cli_args = self.args()
-        cli_args.extend(["package", [spec.name]])
-        it = iter(cli_args)
-        kv = {x.replace("--", "").replace("-", "_"): next(it) for x in it}
-
-        reporter = CDash(Bunch(**kv))
+        configuration = CDashConfiguration(
+            upload_url=self.upload_url,
+            packages=[spec.name],
+            build=self.build_name,
+            site=self.site,
+            buildstamp=self.build_stamp,
+            track=None,
+            ctest_parsing=False,
+        )
+        reporter = CDash(configuration=configuration)
         reporter.test_skipped_report(directory_name, spec, reason)
diff --git a/lib/spack/spack/cmd/common/arguments.py b/lib/spack/spack/cmd/common/arguments.py
index 7e68ac594b..42c5d611b2 100644
--- a/lib/spack/spack/cmd/common/arguments.py
+++ b/lib/spack/spack/cmd/common/arguments.py
@@ -13,6 +13,7 @@ import spack.config
 import spack.dependency as dep
 import spack.environment as ev
 import spack.modules
+import spack.reporters
 import spack.spec
 import spack.store
 from spack.util.pattern import Args
@@ -123,6 +124,64 @@ class DeptypeAction(argparse.Action):
         setattr(namespace, self.dest, deptype)
 
 
+def _cdash_reporter(namespace):
+    """Helper function to create a CDash reporter. This function gets an early reference to the
+    argparse namespace under construction, so it can later use it to create the object.
+    """
+
+    def _factory():
+        def installed_specs(args):
+            if getattr(args, "spec", ""):
+                packages = args.spec
+            elif getattr(args, "specs", ""):
+                packages = args.specs
+            elif getattr(args, "package", ""):
+                # Ensure CI 'spack test run' can output CDash results
+                packages = args.package
+            else:
+                packages = []
+                for file in args.specfiles:
+                    with open(file, "r") as f:
+                        s = spack.spec.Spec.from_yaml(f)
+                        packages.append(s.format())
+            return packages
+
+        configuration = spack.reporters.CDashConfiguration(
+            upload_url=namespace.cdash_upload_url,
+            packages=installed_specs(namespace),
+            build=namespace.cdash_build,
+            site=namespace.cdash_site,
+            buildstamp=namespace.cdash_buildstamp,
+            track=namespace.cdash_track,
+            ctest_parsing=getattr(namespace, "ctest_parsing", False),
+        )
+        return spack.reporters.CDash(configuration=configuration)
+
+    return _factory
+
+
+class CreateReporter(argparse.Action):
+    """Create the correct object to generate reports for installation and testing."""
+
+    def __call__(self, parser, namespace, values, option_string=None):
+        setattr(namespace, self.dest, values)
+        if values == "junit":
+            setattr(namespace, "reporter", spack.reporters.JUnit)
+        elif values == "cdash":
+            setattr(namespace, "reporter", _cdash_reporter(namespace))
+
+
+@arg
+def log_format():
+    return Args(
+        "--log-format",
+        default=None,
+        action=CreateReporter,
+        choices=("junit", "cdash"),
+        help="format to be used for log files",
+    )
+
+
 # TODO: merge constraint and installed_specs
 @arg
 def constraint():
diff --git a/lib/spack/spack/cmd/install.py b/lib/spack/spack/cmd/install.py
index 3f9a948a23..211c5c889d 100644
--- a/lib/spack/spack/cmd/install.py
+++ b/lib/spack/spack/cmd/install.py
@@ -8,9 +8,10 @@ import os
 import shutil
 import sys
 import textwrap
+from typing import List
 
 import llnl.util.filesystem as fs
-import llnl.util.tty as tty
+from llnl.util import lang, tty
 
 import spack.build_environment
 import spack.cmd
@@ -232,12 +233,7 @@ installation for top-level packages (but skip tests for dependencies).
 if 'all' is chosen, run package tests during installation for all
 packages. If neither are chosen, don't run tests for any packages.""",
     )
-    subparser.add_argument(
-        "--log-format",
-        default=None,
-        choices=spack.report.valid_formats,
-        help="format to be used for log files",
-    )
+    arguments.add_common_arguments(subparser, ["log_format"])
     subparser.add_argument(
         "--log-file",
         default=None,
@@ -262,6 +258,12 @@ def default_log_file(spec):
     return fs.os.path.join(dirname, basename)
 
 
+def report_filename(args: argparse.Namespace, specs: List[spack.spec.Spec]) -> str:
+    """Return the filename to be used for reporting to JUnit or CDash format."""
+    result = args.log_file or args.cdash_upload_url or default_log_file(specs[0])
+    return result
+
+
 def install_specs(specs, install_kwargs, cli_args):
     try:
         if ev.active_environment():
@@ -361,19 +363,8 @@ SPACK_CDASH_AUTH_TOKEN
     parser.print_help()
 
 
-def _create_log_reporter(args):
-    # TODO: remove args injection to spack.report.collect_info, since a class in core
-    # TODO: shouldn't know what are the command line arguments a command use.
-    reporter = spack.report.collect_info(
-        spack.package_base.PackageInstaller, "_install_task", args.log_format, args
-    )
-    if args.log_file:
-        reporter.filename = args.log_file
-    return reporter
-
-
 def install_all_specs_from_active_environment(
-    install_kwargs, only_concrete, cli_test_arg, reporter
+    install_kwargs, only_concrete, cli_test_arg, reporter_factory
 ):
     """Install all specs from the active environment
 
@@ -415,12 +406,10 @@ def install_all_specs_from_active_environment(
         tty.msg(msg)
         return
 
-    if not reporter.filename:
-        reporter.filename = default_log_file(specs[0])
-    reporter.specs = specs
+    reporter = reporter_factory(specs) or lang.nullcontext()
 
     tty.msg("Installing environment {0}".format(env.name))
-    with reporter("build"):
+    with reporter:
         env.install_all(**install_kwargs)
 
     tty.debug("Regenerating environment views for {0}".format(env.name))
@@ -439,7 +428,7 @@ def compute_tests_install_kwargs(specs, cli_test_arg):
     return False
 
 
-def specs_from_cli(args, install_kwargs, reporter):
+def specs_from_cli(args, install_kwargs):
     """Return abstract and concrete spec parsed from the command line."""
     abstract_specs = spack.cmd.parse_specs(args.spec)
     install_kwargs["tests"] = compute_tests_install_kwargs(abstract_specs, args.test)
@@ -449,7 +438,9 @@ def specs_from_cli(args, install_kwargs, reporter):
         )
     except SpackError as e:
         tty.debug(e)
-        reporter.concretization_report(e.message)
+        if args.log_format is not None:
+            reporter = args.reporter()
+            reporter.concretization_report(report_filename(args, abstract_specs), e.message)
         raise
     return abstract_specs, concrete_specs
 
@@ -514,7 +505,17 @@ def install(parser, args):
     if args.deprecated:
         spack.config.set("config:deprecated", True, scope="command_line")
 
-    reporter = _create_log_reporter(args)
+    def reporter_factory(specs):
+        if args.log_format is None:
+            return None
+
+        context_manager = spack.report.build_context_manager(
+            reporter=args.reporter(),
+            filename=report_filename(args, specs=specs),
+            specs=specs,
+        )
+        return context_manager
+
     install_kwargs = install_kwargs_from_args(args)
 
     if not args.spec and not args.specfiles:
@@ -523,12 +524,12 @@ def install(parser, args):
             install_kwargs=install_kwargs,
             only_concrete=args.only_concrete,
             cli_test_arg=args.test,
-            reporter=reporter,
+            reporter_factory=reporter_factory,
         )
         return
 
     # Specs from CLI
-    abstract_specs, concrete_specs = specs_from_cli(args, install_kwargs, reporter)
+    abstract_specs, concrete_specs = specs_from_cli(args, install_kwargs)
 
     # Concrete specs from YAML or JSON files
     specs_from_file = concrete_specs_from_file(args)
@@ -538,11 +539,8 @@ def install(parser, args):
     if len(concrete_specs) == 0:
         tty.die("The `spack install` command requires a spec to install.")
 
-    if not reporter.filename:
-        reporter.filename = default_log_file(concrete_specs[0])
-    reporter.specs = concrete_specs
-
-    with reporter("build"):
+    reporter = reporter_factory(concrete_specs) or lang.nullcontext()
+    with reporter:
         if args.overwrite:
             require_user_confirmation_for_overwrite(concrete_specs, args)
             install_kwargs["overwrite"] = [spec.dag_hash() for spec in concrete_specs]
diff --git a/lib/spack/spack/cmd/test.py b/lib/spack/spack/cmd/test.py
index 4da35c8a35..59736ff94c 100644
--- a/lib/spack/spack/cmd/test.py
+++ b/lib/spack/spack/cmd/test.py
@@ -13,8 +13,8 @@ import shutil
 import sys
 import textwrap
 
-import llnl.util.tty as tty
-import llnl.util.tty.colify as colify
+from llnl.util import lang, tty
+from llnl.util.tty import colify
 
 import spack.cmd
 import spack.cmd.common.arguments as arguments
@@ -63,12 +63,7 @@ def setup_parser(subparser):
     run_parser.add_argument(
         "--keep-stage", action="store_true", help="Keep testing directory for debugging"
     )
-    run_parser.add_argument(
-        "--log-format",
-        default=None,
-        choices=spack.report.valid_formats,
-        help="format to be used for log files",
-    )
+    arguments.add_common_arguments(run_parser, ["log_format"])
     run_parser.add_argument(
         "--log-file",
         default=None,
@@ -231,10 +226,23 @@ environment variables:
 
     # Set up reporter
     setattr(args, "package", [s.format() for s in test_suite.specs])
-    reporter = spack.report.collect_info(
-        spack.package_base.PackageBase, "do_test", args.log_format, args
-    )
-    if not reporter.filename:
+    reporter = create_reporter(args, specs_to_test, test_suite) or lang.nullcontext()
+
+    with reporter:
+        test_suite(
+            remove_directory=not args.keep_stage,
+            dirty=args.dirty,
+            fail_first=args.fail_first,
+            externals=args.externals,
+        )
+
+
+def create_reporter(args, specs_to_test, test_suite):
+    if args.log_format is None:
+        return None
+
+    filename = args.cdash_upload_url
+    if not filename:
         if args.log_file:
             if os.path.isabs(args.log_file):
                 log_file = args.log_file
@@ -243,16 +251,15 @@ environment variables:
                 log_file = os.path.join(log_dir, args.log_file)
         else:
             log_file = os.path.join(os.getcwd(), "test-%s" % test_suite.name)
-        reporter.filename = log_file
-    reporter.specs = specs_to_test
+        filename = log_file
 
-    with reporter("test", test_suite.stage):
-        test_suite(
-            remove_directory=not args.keep_stage,
-            dirty=args.dirty,
-            fail_first=args.fail_first,
-            externals=args.externals,
-        )
+    context_manager = spack.report.test_context_manager(
+        reporter=args.reporter(),
+        filename=filename,
+        specs=specs_to_test,
+        raw_logs_dir=test_suite.stage,
+    )
+    return context_manager
 
 
 def test_list(args):
diff --git a/lib/spack/spack/report.py b/lib/spack/spack/report.py
index bc7c4f3ac8..7ff50daa40 100644
--- a/lib/spack/spack/report.py
+++ b/lib/spack/spack/report.py
@@ -3,9 +3,8 @@
 #
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
 """Tools to produce reports of spec installations"""
-import argparse
-import codecs
 import collections
+import contextlib
 import functools
 import os
 import time
@@ -16,74 +15,59 @@ import llnl.util.lang
 
 import spack.build_environment
 import spack.fetch_strategy
+import spack.install_test
+import spack.installer
 import spack.package_base
-from spack.install_test import TestSuite
-from spack.reporter import Reporter
-from spack.reporters.cdash import CDash
-from spack.reporters.junit import JUnit
+import spack.reporters
+import spack.spec
 
-report_writers = {None: Reporter, "junit": JUnit, "cdash": CDash}
 
-#: Allowed report formats
-valid_formats = list(report_writers.keys())
+class InfoCollector:
+    """Base class for context manager objects that collect information during the execution of
+    certain package functions.
 
-__all__ = ["valid_formats", "collect_info"]
-
-
-def fetch_log(pkg, do_fn, dir):
-    log_files = {
-        "_install_task": pkg.build_log_path,
-        "do_test": os.path.join(dir, TestSuite.test_log_name(pkg.spec)),
-    }
-    try:
-        with codecs.open(log_files[do_fn.__name__], "r", "utf-8") as f:
-            return "".join(f.readlines())
-    except Exception:
-        return "Cannot open log for {0}".format(pkg.spec.cshort_spec)
-
-
-class InfoCollector(object):
-    """Decorates PackageInstaller._install_task, which is called via
-    PackageBase.do_install for individual specs, to collect information
-    on the installation of certain specs.
-
-    When exiting the context this change will be rolled-back.
-
-    The data collected is available through the ``specs``
-    attribute once exited, and it's organized as a list where
-    each item represents the installation of one of the spec.
+    The data collected is available through the ``specs`` attribute once exited, and it's
+    organized as a list where each item represents the installation of one spec.
 
     """
 
     wrap_class: Type
     do_fn: str
     _backup_do_fn: Callable
-    input_specs: List["spack.spec.Spec"]
+    input_specs: List[spack.spec.Spec]
     specs: List[Dict[str, Any]]
-    dir: str
 
-    def __init__(self, wrap_class: Type, do_fn: str, specs: List["spack.spec.Spec"], dir: str):
+    def __init__(self, wrap_class: Type, do_fn: str, specs: List[spack.spec.Spec]):
         #: Class for which to wrap a function
         self.wrap_class = wrap_class
         #: Action to be reported on
         self.do_fn = do_fn
-        #: Backup of PackageBase function
+        #: Backup of the wrapped class function
         self._backup_do_fn = getattr(self.wrap_class, do_fn)
         #: Specs that will be acted on
         self.input_specs = specs
-        #: This is where we record the data that will be included
-        #: in our report.
-        self.specs = []
-        #: Record directory for test log paths
-        self.dir = dir
+        #: This is where we record the data that will be included in our report
+        self.specs: List[Dict[str, Any]] = []
+
+    def fetch_log(self, pkg: spack.package_base.PackageBase) -> str:
+        """Return the stdout log associated with the function being monitored
+
+        Args:
+            pkg: package under consideration
+        """
+        raise NotImplementedError("must be implemented by derived classes")
+
+    def extract_package_from_signature(self, instance, *args, **kwargs):
+        """Return the package instance, given the signature of the wrapped function."""
+        raise NotImplementedError("must be implemented by derived classes")
 
     def __enter__(self):
         # Initialize the spec report with the data that is available upfront.
+        Property = collections.namedtuple("Property", ["name", "value"])
         for input_spec in self.input_specs:
             name_fmt = "{0}_{1}"
             name = name_fmt.format(input_spec.name, input_spec.dag_hash(length=7))
-
-            spec = {
+            spec_record = {
                 "name": name,
                 "nerrors": None,
                 "nfailures": None,
@@ -93,45 +77,17 @@ class InfoCollector(object):
                 "properties": [],
                 "packages": [],
             }
+            spec_record["properties"].append(Property("architecture", input_spec.architecture))
+            spec_record["properties"].append(Property("compiler", input_spec.compiler))
+            self.init_spec_record(input_spec, spec_record)
+            self.specs.append(spec_record)
 
-            self.specs.append(spec)
-
-            Property = collections.namedtuple("Property", ["name", "value"])
-            spec["properties"].append(Property("architecture", input_spec.architecture))
-            spec["properties"].append(Property("compiler", input_spec.compiler))
-
-            # Check which specs are already installed and mark them as skipped
-            # only for install_task
-            if self.do_fn == "_install_task":
-                for dep in filter(lambda x: x.installed, input_spec.traverse()):
-                    package = {
-                        "name": dep.name,
-                        "id": dep.dag_hash(),
-                        "elapsed_time": "0.0",
-                        "result": "skipped",
-                        "message": "Spec already installed",
-                    }
-                    spec["packages"].append(package)
-
-        def gather_info(do_fn):
-            """Decorates do_fn to gather useful information for
-            a CI report.
-
-            It's defined here to capture the environment and build
-            this context as the installations proceed.
-            """
-
-            @functools.wraps(do_fn)
-            def wrapper(instance, *args, **kwargs):
-                if isinstance(instance, spack.package_base.PackageBase):
-                    pkg = instance
-                elif hasattr(args[0], "pkg"):
-                    pkg = args[0].pkg
-                else:
-                    raise Exception
+        def gather_info(wrapped_fn):
+            """Decorates a function to gather useful information for a CI report."""
 
-                # We accounted before for what is already installed
-                installed_already = pkg.spec.installed
+            @functools.wraps(wrapped_fn)
+            def wrapper(instance, *args, **kwargs):
+                pkg = self.extract_package_from_signature(instance, *args, **kwargs)
 
                 package = {
                     "name": pkg.name,
@@ -147,8 +103,8 @@ class InfoCollector(object):
                 # installed explicitly will also be installed as a
                 # dependency of another spec. In this case append to both
                 # spec reports.
-                for s in llnl.util.lang.dedupe([pkg.spec.root, pkg.spec]):
-                    name = name_fmt.format(s.name, s.dag_hash(length=7))
+                for current_spec in llnl.util.lang.dedupe([pkg.spec.root, pkg.spec]):
+                    name = name_fmt.format(current_spec.name, current_spec.dag_hash(length=7))
                     try:
                         item = next((x for x in self.specs if x["name"] == name))
                         item["packages"].append(package)
@@ -156,132 +112,165 @@ class InfoCollector(object):
                         pass
 
                 start_time = time.time()
-                value = None
                 try:
-                    value = do_fn(instance, *args, **kwargs)
-
-                    externals = kwargs.get("externals", False)
-                    skip_externals = pkg.spec.external and not externals
-                    if do_fn.__name__ == "do_test" and skip_externals:
-                        package["result"] = "skipped"
-                    else:
-                        package["result"] = "success"
-                    package["stdout"] = fetch_log(pkg, do_fn, self.dir)
+
+                    value = wrapped_fn(instance, *args, **kwargs)
+                    package["stdout"] = self.fetch_log(pkg)
                     package["installed_from_binary_cache"] = pkg.installed_from_binary_cache
-                    if do_fn.__name__ == "_install_task" and installed_already:
-                        return
+                    self.on_success(pkg, kwargs, package)
+                    return value
 
-                except spack.build_environment.InstallError as e:
+                except spack.build_environment.InstallError as exc:
                     # An InstallError is considered a failure (the recipe
                     # didn't work correctly)
                     package["result"] = "failure"
-                    package["message"] = e.message or "Installation failure"
-                    package["stdout"] = fetch_log(pkg, do_fn, self.dir)
+                    package["message"] = exc.message or "Installation failure"
+                    package["stdout"] = self.fetch_log(pkg)
                     package["stdout"] += package["message"]
-                    package["exception"] = e.traceback
+                    package["exception"] = exc.traceback
                     raise
 
-                except (Exception, BaseException) as e:
+                except (Exception, BaseException) as exc:
                     # Everything else is an error (the installation
                     # failed outside of the child process)
                     package["result"] = "error"
-                    package["stdout"] = fetch_log(pkg, do_fn, self.dir)
-                    package["message"] = str(e) or "Unknown error"
+                    package["stdout"] = self.fetch_log(pkg)
+                    package["message"] = str(exc) or "Unknown error"
                     package["exception"] = traceback.format_exc()
                     raise
 
                 finally:
                     package["elapsed_time"] = time.time() - start_time
 
-                return value
-
             return wrapper
 
         setattr(self.wrap_class, self.do_fn, gather_info(getattr(self.wrap_class, self.do_fn)))
 
-    def __exit__(self, exc_type, exc_val, exc_tb):
+    def on_success(self, pkg: spack.package_base.PackageBase, kwargs, package_record):
+        """Add additional properties on function call success."""
+        raise NotImplementedError("must be implemented by derived classes")
+
+    def init_spec_record(self, input_spec: spack.spec.Spec, record):
+        """Add additional entries to a spec record when entering the collection context."""
 
+    def __exit__(self, exc_type, exc_val, exc_tb):
         # Restore the original method in PackageBase
         setattr(self.wrap_class, self.do_fn, self._backup_do_fn)
-
         for spec in self.specs:
             spec["npackages"] = len(spec["packages"])
             spec["nfailures"] = len([x for x in spec["packages"] if x["result"] == "failure"])
             spec["nerrors"] = len([x for x in spec["packages"] if x["result"] == "error"])
-            spec["time"] = sum([float(x["elapsed_time"]) for x in spec["packages"]])
+            spec["time"] = sum(float(x["elapsed_time"]) for x in spec["packages"])
 
 
-class collect_info(object):
-    """Collects information to build a report while installing
-    and dumps it on exit.
+class BuildInfoCollector(InfoCollector):
+    """Collect information for the PackageInstaller._install_task method.
 
-    If the format name is not ``None``, this context manager decorates
-    PackageInstaller._install_task when entering the context for a
-    PackageBase.do_install operation and unrolls the change when exiting.
+    Args:
+        specs: specs whose install information will be recorded
+    """
 
-    Within the context, only the specs that are passed to it
-    on initialization will be recorded for the report. Data from
-    other specs will be discarded.
+    def __init__(self, specs: List[spack.spec.Spec]):
+        super().__init__(spack.installer.PackageInstaller, "_install_task", specs)
+
+    def init_spec_record(self, input_spec, record):
+        # Check which specs are already installed and mark them as skipped
+        for dep in filter(lambda x: x.installed, input_spec.traverse()):
+            package = {
+                "name": dep.name,
+                "id": dep.dag_hash(),
+                "elapsed_time": "0.0",
+                "result": "skipped",
+                "message": "Spec already installed",
+            }
+            record["packages"].append(package)
 
-    Examples:
+    def on_success(self, pkg, kwargs, package_record):
+        package_record["result"] = "success"
 
-        .. code-block:: python
+    def fetch_log(self, pkg):
+        try:
+            with open(pkg.build_log_path, "r", encoding="utf-8") as stream:
+                return "".join(stream.readlines())
+        except Exception:
+            return f"Cannot open log for {pkg.spec.cshort_spec}"
 
-            # The file 'junit.xml' is written when exiting
-            # the context
-            s = [Spec('hdf5').concretized()]
-            with collect_info(PackageBase, do_install, s, 'junit', 'a.xml'):
-                # A report will be generated for these specs...
-                for spec in s:
-                    getattr(class, function)(spec)
-                # ...but not for this one
-                Spec('zlib').concretized().do_install()
+    def extract_package_from_signature(self, instance, *args, **kwargs):
+        return args[0].pkg
+
+
+class TestInfoCollector(InfoCollector):
+    """Collect information for the PackageBase.do_test method.
 
     Args:
-        class: class on which to wrap a function
-        function: function to wrap
-        format_name: one of the supported formats
-        args: args passed to function
+        specs: specs whose install information will be recorded
+        record_directory: record directory for test log paths
+    """
+
+    dir: str
+
+    def __init__(self, specs: List[spack.spec.Spec], record_directory: str):
+        super().__init__(spack.package_base.PackageBase, "do_test", specs)
+        self.dir = record_directory
+
+    def on_success(self, pkg, kwargs, package_record):
+        externals = kwargs.get("externals", False)
+        skip_externals = pkg.spec.external and not externals
+        if skip_externals:
+            package_record["result"] = "skipped"
+        package_record["result"] = "success"
+
+    def fetch_log(self, pkg: spack.package_base.PackageBase):
+        log_file = os.path.join(self.dir, spack.install_test.TestSuite.test_log_name(pkg.spec))
+        try:
+            with open(log_file, "r", encoding="utf-8") as stream:
+                return "".join(stream.readlines())
+        except Exception:
+            return f"Cannot open log for {pkg.spec.cshort_spec}"
+
+    def extract_package_from_signature(self, instance, *args, **kwargs):
+        return instance
+
+
+@contextlib.contextmanager
+def build_context_manager(
+    reporter: spack.reporters.Reporter,
+    filename: str,
+    specs: List[spack.spec.Spec],
+):
+    """Decorate a package to generate a report after the installation function is executed.
 
-    Raises:
-        ValueError: when ``format_name`` is not in ``valid_formats``
+    Args:
+        reporter: object that generates the report
+        filename:  filename for the report
+        specs: specs that need reporting
     """
+    collector = BuildInfoCollector(specs)
+    try:
+        with collector:
+            yield
+    finally:
+        reporter.build_report(filename, specs=collector.specs)
 
-    def __init__(self, cls: Type, function: str, format_name: str, args: argparse.Namespace):
-        self.cls = cls
-        self.function = function
-        self.filename = None
-        self.ctest_parsing = getattr(args, "ctest_parsing", False)
-        if args.cdash_upload_url:
-            self.format_name = "cdash"
-            self.filename = "cdash_report"
-        else:
-            self.format_name = format_name
-        # Check that the format is valid.
-        if self.format_name not in valid_formats:
-            raise ValueError("invalid report type: {0}".format(self.format_name))
-        self.report_writer = report_writers[self.format_name](args)
-
-    def __call__(self, type, dir=None):
-        self.type = type
-        self.dir = dir or os.getcwd()
-        return self
-
-    def concretization_report(self, msg):
-        self.report_writer.concretization_report(self.filename, msg)
 
-    def __enter__(self):
-        if self.format_name:
-            # Start the collector and patch self.function on appropriate class
-            self.collector = InfoCollector(self.cls, self.function, self.specs, self.dir)
-            self.collector.__enter__()
+@contextlib.contextmanager
+def test_context_manager(
+    reporter: spack.reporters.Reporter,
+    filename: str,
+    specs: List[spack.spec.Spec],
+    raw_logs_dir: str,
+):
+    """Decorate a package to generate a report after the test function is executed.
 
-    def __exit__(self, exc_type, exc_val, exc_tb):
-        if self.format_name:
-            # Close the collector and restore the original function
-            self.collector.__exit__(exc_type, exc_val, exc_tb)
-
-            report_data = {"specs": self.collector.specs}
-            report_data["ctest-parsing"] = self.ctest_parsing
-            report_fn = getattr(self.report_writer, "%s_report" % self.type)
-            report_fn(self.filename, report_data)
+    Args:
+        reporter: object that generates the report
+        filename:  filename for the report
+        specs: specs that need reporting
+        raw_logs_dir: record directory for test log paths
+    """
+    collector = TestInfoCollector(specs, raw_logs_dir)
+    try:
+        with collector:
+            yield
+    finally:
+        reporter.test_report(filename, specs=collector.specs)
diff --git a/lib/spack/spack/reporter.py b/lib/spack/spack/reporter.py
deleted file mode 100644
index 6dc8cff2e0..0000000000
--- a/lib/spack/spack/reporter.py
+++ /dev/null
@@ -1,23 +0,0 @@
-# Copyright 2013-2022 Lawrence Livermore National Security, LLC and other
-# Spack Project Developers. See the top-level COPYRIGHT file for details.
-#
-# SPDX-License-Identifier: (Apache-2.0 OR MIT)
-
-
-__all__ = ["Reporter"]
-
-
-class Reporter(object):
-    """Base class for report writers."""
-
-    def __init__(self, args):
-        self.args = args
-
-    def build_report(self, filename, report_data):
-        pass
-
-    def test_report(self, filename, report_data):
-        pass
-
-    def concretization_report(self, filename, msg):
-        pass
diff --git a/lib/spack/spack/reporters/__init__.py b/lib/spack/spack/reporters/__init__.py
index 0fde365d42..30e1de2d80 100644
--- a/lib/spack/spack/reporters/__init__.py
+++ b/lib/spack/spack/reporters/__init__.py
@@ -2,3 +2,8 @@
 # Spack Project Developers. See the top-level COPYRIGHT file for details.
 #
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
+from .base import Reporter
+from .cdash import CDash, CDashConfiguration
+from .junit import JUnit
+
+__all__ = ["JUnit", "CDash", "CDashConfiguration", "Reporter"]
diff --git a/lib/spack/spack/reporters/base.py b/lib/spack/spack/reporters/base.py
new file mode 100644
index 0000000000..bf44376bda
--- /dev/null
+++ b/lib/spack/spack/reporters/base.py
@@ -0,0 +1,18 @@
+# Copyright 2013-2022 Lawrence Livermore National Security, LLC and other
+# Spack Project Developers. See the top-level COPYRIGHT file for details.
+#
+# SPDX-License-Identifier: (Apache-2.0 OR MIT)
+from typing import Any, Dict, List
+
+
+class Reporter:
+    """Base class for report writers."""
+
+    def build_report(self, filename: str, specs: List[Dict[str, Any]]):
+        raise NotImplementedError("must be implemented by derived classes")
+
+    def test_report(self, filename: str, specs: List[Dict[str, Any]]):
+        raise NotImplementedError("must be implemented by derived classes")
+
+    def concretization_report(self, filename: str, msg: str):
+        raise NotImplementedError("must be implemented by derived classes")
diff --git a/lib/spack/spack/reporters/cdash.py b/lib/spack/spack/reporters/cdash.py
index 27beca2e40..46a7aee9f7 100644
--- a/lib/spack/spack/reporters/cdash.py
+++ b/lib/spack/spack/reporters/cdash.py
@@ -12,6 +12,7 @@ import re
 import socket
 import time
 import xml.sax.saxutils
+from typing import Dict
 from urllib.parse import urlencode
 from urllib.request import HTTPHandler, Request, build_opener
 
@@ -24,15 +25,14 @@ import spack.package_base
 import spack.platforms
 import spack.util.git
 from spack.error import SpackError
-from spack.reporter import Reporter
-from spack.reporters.extract import extract_test_parts
 from spack.util.crypto import checksum
 from spack.util.log_parse import parse_log_events
 
-__all__ = ["CDash"]
+from .base import Reporter
+from .extract import extract_test_parts
 
 # Mapping Spack phases to the corresponding CTest/CDash phase.
-map_phases_to_cdash = {
+MAP_PHASES_TO_CDASH = {
     "autoreconf": "configure",
     "cmake": "configure",
     "configure": "configure",
@@ -42,8 +42,14 @@ map_phases_to_cdash = {
 }
 
 # Initialize data structures common to each phase's report.
-cdash_phases = set(map_phases_to_cdash.values())
-cdash_phases.add("update")
+CDASH_PHASES = set(MAP_PHASES_TO_CDASH.values())
+CDASH_PHASES.add("update")
+
+
+CDashConfiguration = collections.namedtuple(
+    "CDashConfiguration",
+    ["upload_url", "packages", "build", "site", "buildstamp", "track", "ctest_parsing"],
+)
 
 
 def build_stamp(track, timestamp):
@@ -64,13 +70,13 @@ class CDash(Reporter):
     CDash instance hosted at https://mydomain.com/cdash.
     """
 
-    def __init__(self, args):
-        Reporter.__init__(self, args)
+    def __init__(self, configuration: CDashConfiguration):
+        #: Set to False if any error occurs when building the CDash report
         self.success = True
-        # Posixpath is used here to support the underlying template enginge
-        # Jinja2, which expects `/` path separators
-        self.template_dir = posixpath.join("reports", "cdash")
-        self.cdash_upload_url = args.cdash_upload_url
+
+        # Jinja2 expects `/` path separators
+        self.template_dir = "reports/cdash"
+        self.cdash_upload_url = configuration.upload_url
 
         if self.cdash_upload_url:
             self.buildid_regexp = re.compile("<buildId>([0-9]+)</buildId>")
@@ -81,38 +87,26 @@ class CDash(Reporter):
             tty.verbose("Using CDash auth token from environment")
             self.authtoken = os.environ.get("SPACK_CDASH_AUTH_TOKEN")
 
-        if getattr(args, "spec", ""):
-            packages = args.spec
-        elif getattr(args, "specs", ""):
-            packages = args.specs
-        elif getattr(args, "package", ""):
-            # Ensure CI 'spack test run' can output CDash results
-            packages = args.package
-        else:
-            packages = []
-            for file in args.specfiles:
-                with open(file, "r") as f:
-                    s = spack.spec.Spec.from_yaml(f)
-                    packages.append(s.format())
-        self.install_command = " ".join(packages)
-        self.base_buildname = args.cdash_build or self.install_command
-        self.site = args.cdash_site or socket.gethostname()
+        self.install_command = " ".join(configuration.packages)
+        self.base_buildname = configuration.build or self.install_command
+        self.site = configuration.site or socket.gethostname()
         self.osname = platform.system()
         self.osrelease = platform.release()
         self.target = spack.platforms.host().target("default_target")
         self.endtime = int(time.time())
         self.buildstamp = (
-            args.cdash_buildstamp
-            if args.cdash_buildstamp
-            else build_stamp(args.cdash_track, self.endtime)
+            configuration.buildstamp
+            if configuration.buildstamp
+            else build_stamp(configuration.track, self.endtime)
         )
-        self.buildIds = collections.OrderedDict()
+        self.buildIds: Dict[str, str] = {}
         self.revision = ""
         git = spack.util.git.git()
         with working_dir(spack.paths.spack_root):
             self.revision = git("rev-parse", "HEAD", output=str).strip()
         self.generator = "spack-{0}".format(spack.main.get_version())
         self.multiple_packages = False
+        self.ctest_parsing = configuration.ctest_parsing
 
     def report_build_name(self, pkg_name):
         return (
@@ -129,7 +123,7 @@ class CDash(Reporter):
         self.current_package_name = package["name"]
         self.buildname = self.report_build_name(self.current_package_name)
         report_data = self.initialize_report(directory_name)
-        for phase in cdash_phases:
+        for phase in CDASH_PHASES:
             report_data[phase] = {}
             report_data[phase]["loglines"] = []
             report_data[phase]["status"] = 0
@@ -149,10 +143,10 @@ class CDash(Reporter):
                 match = self.phase_regexp.search(line)
             if match:
                 current_phase = match.group(1)
-                if current_phase not in map_phases_to_cdash:
+                if current_phase not in MAP_PHASES_TO_CDASH:
                     current_phase = ""
                     continue
-                cdash_phase = map_phases_to_cdash[current_phase]
+                cdash_phase = MAP_PHASES_TO_CDASH[current_phase]
                 if cdash_phase not in phases_encountered:
                     phases_encountered.append(cdash_phase)
                 report_data[cdash_phase]["loglines"].append(
@@ -239,13 +233,13 @@ class CDash(Reporter):
                 f.write(t.render(report_data))
             self.upload(phase_report)
 
-    def build_report(self, directory_name, input_data):
+    def build_report(self, directory_name, specs):
         # Do an initial scan to determine if we are generating reports for more
         # than one package. When we're only reporting on a single package we
         # do not explicitly include the package's name in the CDash build name.
-        self.multipe_packages = False
+        self.multiple_packages = False
         num_packages = 0
-        for spec in input_data["specs"]:
+        for spec in specs:
             # Do not generate reports for packages that were installed
             # from the binary cache.
             spec["packages"] = [
@@ -263,7 +257,7 @@ class CDash(Reporter):
                 break
 
         # Generate reports for each package in each spec.
-        for spec in input_data["specs"]:
+        for spec in specs:
             duration = 0
             if "time" in spec:
                 duration = int(spec["time"])
@@ -392,10 +386,10 @@ class CDash(Reporter):
 
         self.report_test_data(directory_name, package, phases, report_data)
 
-    def test_report(self, directory_name, input_data):
+    def test_report(self, directory_name, specs):
         """Generate reports for each package in each spec."""
         tty.debug("Processing test report")
-        for spec in input_data["specs"]:
+        for spec in specs:
             duration = 0
             if "time" in spec:
                 duration = int(spec["time"])
@@ -404,7 +398,7 @@ class CDash(Reporter):
                     directory_name,
                     package,
                     duration,
-                    input_data["ctest-parsing"],
+                    self.ctest_parsing,
                 )
 
         self.finalize_report()
diff --git a/lib/spack/spack/reporters/junit.py b/lib/spack/spack/reporters/junit.py
index ee080fc6ab..f902bb74c5 100644
--- a/lib/spack/spack/reporters/junit.py
+++ b/lib/spack/spack/reporters/junit.py
@@ -2,36 +2,32 @@
 # Spack Project Developers. See the top-level COPYRIGHT file for details.
 #
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
-
 import os.path
-import posixpath
 
 import spack.tengine
-from spack.reporter import Reporter
 
-__all__ = ["JUnit"]
+from .base import Reporter
 
 
 class JUnit(Reporter):
     """Generate reports of spec installations for JUnit."""
 
-    def __init__(self, args):
-        Reporter.__init__(self, args)
-        # Posixpath is used here to support the underlying template enginge
-        # Jinja2, which expects `/` path separators
-        self.template_file = posixpath.join("reports", "junit.xml")
+    _jinja_template = "reports/junit.xml"
+
+    def concretization_report(self, filename, msg):
+        pass
 
-    def build_report(self, filename, report_data):
+    def build_report(self, filename, specs):
         if not (os.path.splitext(filename))[1]:
             # Ensure the report name will end with the proper extension;
             # otherwise, it currently defaults to the "directory" name.
             filename = filename + ".xml"
 
-        # Write the report
+        report_data = {"specs": specs}
         with open(filename, "w") as f:
             env = spack.tengine.make_environment()
-            t = env.get_template(self.template_file)
+            t = env.get_template(self._jinja_template)
             f.write(t.render(report_data))
 
-    def test_report(self, filename, report_data):
-        self.build_report(filename, report_data)
+    def test_report(self, filename, specs):
+        self.build_report(filename, specs)
diff --git a/lib/spack/spack/test/reporters.py b/lib/spack/spack/test/reporters.py
index 72aff79210..d4eea713b8 100644
--- a/lib/spack/spack/test/reporters.py
+++ b/lib/spack/spack/test/reporters.py
@@ -7,10 +7,9 @@ import pytest
 import llnl.util.filesystem as fs
 import llnl.util.tty as tty
 
-import spack.reporters.cdash
 import spack.reporters.extract
 import spack.spec
-from spack.util.pattern import Bunch
+from spack.reporters import CDash, CDashConfiguration
 
 # Use a path variable to appease Spack style line length checks
 fake_install_prefix = fs.join_path(
@@ -152,22 +151,23 @@ Results for test suite abcdefghijklmn
 
 
 def test_reporters_report_for_package_no_stdout(tmpdir, monkeypatch, capfd):
-    class MockCDash(spack.reporters.cdash.CDash):
+    class MockCDash(CDash):
         def upload(*args, **kwargs):
             # Just return (Do NOT try to upload the report to the fake site)
             return
 
-    args = Bunch(
-        cdash_upload_url="https://fake-upload",
-        package="fake-package",
-        cdash_build="fake-cdash-build",
-        cdash_site="fake-site",
-        cdash_buildstamp=None,
-        cdash_track="fake-track",
+    configuration = CDashConfiguration(
+        upload_url="https://fake-upload",
+        packages="fake-package",
+        build="fake-cdash-build",
+        site="fake-site",
+        buildstamp=None,
+        track="fake-track",
+        ctest_parsing=False,
     )
     monkeypatch.setattr(tty, "_debug", 1)
 
-    reporter = MockCDash(args)
+    reporter = MockCDash(configuration=configuration)
     pkg_data = {"name": "fake-package"}
     reporter.test_report_for_package(tmpdir.strpath, pkg_data, 0, False)
     err = capfd.readouterr()[1]
-- 
cgit v1.2.3-60-g2f50