init: remove package_testing global

- refactor the way test dependencies are passed to the concretizer
- remove global state
- update tests

9 files changed, 82 insertions, 75 deletions

diff --git a/lib/spack/spack/__init__.py b/lib/spack/spack/__init__.py
index 002e9185de..05cfa4af58 100644
--- a/lib/spack/spack/__init__.py
+++ b/lib/spack/spack/__init__.py
@@ -43,11 +43,6 @@ except spack.error.SpackError as e:
     tty.die('while initializing Spack RepoPath:', e.message)
 
 
-#: Needed for test dependencies
-from spack.package_prefs import PackageTesting
-package_testing = PackageTesting()
-
-
 #-----------------------------------------------------------------------------
 # When packages call 'from spack import *', we import a set of things that
 # should be useful for builds.
diff --git a/lib/spack/spack/cmd/__init__.py b/lib/spack/spack/cmd/__init__.py
index 0f645af5f0..f57e86ee27 100644
--- a/lib/spack/spack/cmd/__init__.py
+++ b/lib/spack/spack/cmd/__init__.py
@@ -139,14 +139,15 @@ def parse_specs(args, **kwargs):
     """
     concretize = kwargs.get('concretize', False)
     normalize = kwargs.get('normalize', False)
+    tests = kwargs.get('tests', False)
 
     try:
         specs = spack.spec.parse(args)
         for spec in specs:
             if concretize:
-                spec.concretize()  # implies normalize
+                spec.concretize(tests=tests)  # implies normalize
             elif normalize:
-                spec.normalize()
+                spec.normalize(tests=tests)
 
         return specs
 
diff --git a/lib/spack/spack/cmd/bootstrap.py b/lib/spack/spack/cmd/bootstrap.py
index 6361d02132..41653ca1b6 100644
--- a/lib/spack/spack/cmd/bootstrap.py
+++ b/lib/spack/spack/cmd/bootstrap.py
@@ -50,11 +50,6 @@ def setup_parser(subparser):
     cd_group = subparser.add_mutually_exclusive_group()
     arguments.add_common_arguments(cd_group, ['clean', 'dirty'])
 
-    subparser.add_argument(
-        '--run-tests', action='store_true', dest='run_tests',
-        help="run package level tests during installation"
-    )
-
 
 def bootstrap(parser, args, **kwargs):
     kwargs.update({
@@ -62,7 +57,6 @@ def bootstrap(parser, args, **kwargs):
         'keep_stage': args.keep_stage,
         'install_deps': 'dependencies',
         'make_jobs': args.jobs,
-        'run_tests': args.run_tests,
         'verbose': args.verbose,
         'dirty': args.dirty
     })
diff --git a/lib/spack/spack/cmd/install.py b/lib/spack/spack/cmd/install.py
index aed5fddbda..93aeb1009a 100644
--- a/lib/spack/spack/cmd/install.py
+++ b/lib/spack/spack/cmd/install.py
@@ -202,14 +202,16 @@ def install(parser, args, **kwargs):
         reporter.filename = args.log_file
 
     specs = spack.cmd.parse_specs(args.package)
+    tests = False
     if args.test == 'all' or args.run_tests:
-        spack.package_testing.test_all()
+        tests = True
     elif args.test == 'root':
-        for spec in specs:
-            spack.package_testing.test(spec.name)
+        tests = [spec.name for spec in specs]
+    kwargs['tests'] = tests
 
     try:
-        specs = spack.cmd.parse_specs(args.package, concretize=True)
+        specs = spack.cmd.parse_specs(
+            args.package, concretize=True, tests=tests)
     except SpackError as e:
         reporter.concretization_report(e.message)
         raise
diff --git a/lib/spack/spack/package.py b/lib/spack/spack/package.py
index a2e588079e..8df46c9963 100644
--- a/lib/spack/spack/package.py
+++ b/lib/spack/spack/package.py
@@ -1380,6 +1380,7 @@ class PackageBase(with_metaclass(PackageMeta, object)):
                    make_jobs=None,
                    fake=False,
                    explicit=False,
+                   tests=False,
                    dirty=None,
                    **kwargs):
         """Called by commands to install a package and its dependencies.
@@ -1405,6 +1406,8 @@ class PackageBase(with_metaclass(PackageMeta, object)):
             fake (bool): Don't really build; install fake stub files instead.
             explicit (bool): True if package was explicitly installed, False
                 if package was implicitly installed (as a dependency).
+            tests (bool or list or set): False to run no tests, True to test
+                all packages, or a list of package names to run tests for some
             dirty (bool): Don't clean the build environment before installing.
             force (bool): Install again, even if already installed.
         """
@@ -1435,7 +1438,6 @@ class PackageBase(with_metaclass(PackageMeta, object)):
                 # is installed
                 if keep_stage is False:
                     self.stage.destroy()
-
                 return self._update_explicit_entry_in_db(rec, explicit)
 
         self._do_install_pop_kwargs(kwargs)
@@ -1454,6 +1456,7 @@ class PackageBase(with_metaclass(PackageMeta, object)):
                     skip_patch=skip_patch,
                     verbose=verbose,
                     make_jobs=make_jobs,
+                    tests=tests,
                     dirty=dirty,
                     **kwargs)
 
@@ -1470,8 +1473,9 @@ class PackageBase(with_metaclass(PackageMeta, object)):
             tty.msg('No binary for %s found: installing from source'
                     % self.name)
 
-        # Set run_tests flag before starting build.
-        self.run_tests = spack.package_testing.check(self.name)
+        # Set run_tests flag before starting build
+        self.run_tests = (tests is True or
+                          tests and self.name in tests)
 
         # Set parallelism before starting build.
         self.make_jobs = make_jobs
@@ -1555,6 +1559,11 @@ class PackageBase(with_metaclass(PackageMeta, object)):
             # preserve verbosity across runs
             return echo
 
+        # hook that allow tests to inspect this Package before installation
+        # see unit_test_check() docs.
+        if not self.unit_test_check():
+            return
+
         try:
             # Create the install prefix and fork the build process.
             if not os.path.exists(self.prefix):
@@ -1594,6 +1603,23 @@ class PackageBase(with_metaclass(PackageMeta, object)):
             # check the filesystem for it.
             self.stage.created = False
 
+    def unit_test_check(self):
+        """Hook for unit tests to assert things about package internals.
+
+        Unit tests can override this function to perform checks after
+        ``Package.install`` and all post-install hooks run, but before
+        the database is updated.
+
+        The overridden function may indicate that the install procedure
+        should terminate early (before updating the database) by
+        returning ``False`` (or any value such that ``bool(result)`` is
+        ``False``).
+
+        Return:
+            (bool): ``True`` to continue, ``False`` to skip ``install()``
+        """
+        return True
+
     def check_for_unfinished_installation(
             self, keep_prefix=False, restage=False):
         """Check for leftover files from partially-completed prior install to
diff --git a/lib/spack/spack/package_prefs.py b/lib/spack/spack/package_prefs.py
index 60b501acee..ca27210678 100644
--- a/lib/spack/spack/package_prefs.py
+++ b/lib/spack/spack/package_prefs.py
@@ -204,25 +204,6 @@ class PackagePrefs(object):
                     if name in pkg.variants)
 
 
-class PackageTesting(object):
-    def __init__(self):
-        self.packages_to_test = set()
-        self._test_all = False
-
-    def test(self, package_name):
-        self.packages_to_test.add(package_name)
-
-    def test_all(self):
-        self._test_all = True
-
-    def clear(self):
-        self._test_all = False
-        self.packages_to_test.clear()
-
-    def check(self, package_name):
-        return self._test_all or (package_name in self.packages_to_test)
-
-
 def spec_externals(spec):
     """Return a list of external specs (w/external directory path filled in),
        one for each known external installation."""
diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py
index cb21e551f0..87800504f7 100644
--- a/lib/spack/spack/spec.py
+++ b/lib/spack/spack/spec.py
@@ -1808,10 +1808,14 @@ class Spec(object):
 
         return changed
 
-    def concretize(self):
+    def concretize(self, tests=False):
         """A spec is concrete if it describes one build of a package uniquely.
         This will ensure that this spec is concrete.
 
+        Args:
+            tests (list or bool): list of packages that will need test
+                dependencies, or True/False for test all/none
+
         If this spec could describe more than one version, variant, or build
         of a package, this will add constraints to make it concrete.
 
@@ -1830,7 +1834,7 @@ class Spec(object):
         force = False
 
         while changed:
-            changes = (self.normalize(force),
+            changes = (self.normalize(force, tests),
                        self._expand_virtual_packages(),
                        self._concretize_helper())
             changed = any(changes)
@@ -2050,7 +2054,7 @@ class Spec(object):
                 raise UnsatisfiableProviderSpecError(required[0], vdep)
 
     def _merge_dependency(
-            self, dependency, visited, spec_deps, provider_index):
+            self, dependency, visited, spec_deps, provider_index, tests):
         """Merge dependency information from a Package into this Spec.
 
         Args:
@@ -2146,10 +2150,10 @@ class Spec(object):
             self._add_dependency(spec_dependency, dependency.type)
 
         changed |= spec_dependency._normalize_helper(
-            visited, spec_deps, provider_index)
+            visited, spec_deps, provider_index, tests)
         return changed
 
-    def _normalize_helper(self, visited, spec_deps, provider_index):
+    def _normalize_helper(self, visited, spec_deps, provider_index, tests):
         """Recursive helper function for _normalize."""
         if self.name in visited:
             return False
@@ -2170,16 +2174,23 @@ class Spec(object):
             for dep_name in self.package_class.dependencies:
                 # Do we depend on dep_name?  If so pkg_dep is not None.
                 dep = self._evaluate_dependency_conditions(dep_name)
+
                 # If dep is a needed dependency, merge it.
-                if dep and (spack.package_testing.check(self.name) or
-                            set(dep.type) - set(['test'])):
-                    changed |= self._merge_dependency(
-                        dep, visited, spec_deps, provider_index)
+                if dep:
+                    merge = (
+                        # caller requested test dependencies
+                        tests is True or (tests and self.name in tests) or
+                        # this is not a test-only dependency
+                        dep.type - set(['test']))
+
+                    if merge:
+                        changed |= self._merge_dependency(
+                            dep, visited, spec_deps, provider_index, tests)
             any_change |= changed
 
         return any_change
 
-    def normalize(self, force=False):
+    def normalize(self, force=False, tests=False):
         """When specs are parsed, any dependencies specified are hanging off
            the root, and ONLY the ones that were explicitly provided are there.
            Normalization turns a partial flat spec into a DAG, where:
@@ -2220,7 +2231,8 @@ class Spec(object):
         # to package files & their 'when' specs
         visited = set()
 
-        any_change = self._normalize_helper(visited, spec_deps, provider_index)
+        any_change = self._normalize_helper(
+            visited, spec_deps, provider_index, tests)
 
         # If there are deps specified but not visited, they're not
         # actually deps of this package.  Raise an error.
diff --git a/lib/spack/spack/test/cmd/install.py b/lib/spack/spack/test/cmd/install.py
index f9d2ad74cc..f5252fe4b1 100644
--- a/lib/spack/spack/test/cmd/install.py
+++ b/lib/spack/spack/test/cmd/install.py
@@ -53,10 +53,8 @@ def parser():
 
 @pytest.fixture()
 def noop_install(monkeypatch):
-
     def noop(*args, **kwargs):
-        return
-
+        pass
     monkeypatch.setattr(spack.package.PackageBase, 'do_install', noop)
 
 
@@ -77,31 +75,31 @@ def test_install_package_and_dependency(
     assert 'errors="0"' in content
 
 
-@pytest.mark.usefixtures('noop_install', 'builtin_mock', 'config')
-def test_install_runtests():
-    assert not spack.package_testing._test_all
-    assert not spack.package_testing.packages_to_test
-
-    install('--test=root', 'dttop')
-    assert not spack.package_testing._test_all
-    assert spack.package_testing.packages_to_test == set(['dttop'])
+@pytest.mark.disable_clean_stage_check
+def test_install_runtests_notests(monkeypatch, builtin_mock, install_mockery):
+    def check(pkg):
+        assert not pkg.run_tests
+    monkeypatch.setattr(spack.package.PackageBase, 'unit_test_check', check)
+    install('-v', 'dttop')
 
-    spack.package_testing.clear()
 
-    install('--test=all', 'a')
-    assert spack.package_testing._test_all
-    assert not spack.package_testing.packages_to_test
+@pytest.mark.disable_clean_stage_check
+def test_install_runtests_root(monkeypatch, builtin_mock, install_mockery):
+    def check(pkg):
+        assert pkg.run_tests == (pkg.name == 'dttop')
 
-    spack.package_testing.clear()
+    monkeypatch.setattr(spack.package.PackageBase, 'unit_test_check', check)
+    install('--test=root', 'dttop')
 
-    install('--run-tests', 'a')
-    assert spack.package_testing._test_all
-    assert not spack.package_testing.packages_to_test
 
-    spack.package_testing.clear()
+@pytest.mark.disable_clean_stage_check
+def test_install_runtests_all(monkeypatch, builtin_mock, install_mockery):
+    def check(pkg):
+        assert pkg.run_tests
 
-    assert not spack.package_testing._test_all
-    assert not spack.package_testing.packages_to_test
+    monkeypatch.setattr(spack.package.PackageBase, 'unit_test_check', check)
+    install('--test=all', 'a')
+    install('--run-tests', 'a')
 
 
 def test_install_package_already_installed(
diff --git a/lib/spack/spack/test/spec_dag.py b/lib/spack/spack/test/spec_dag.py
index f8dd7a7f74..671eb06979 100644
--- a/lib/spack/spack/test/spec_dag.py
+++ b/lib/spack/spack/test/spec_dag.py
@@ -98,16 +98,14 @@ w->y deptypes are (link, build), w->x and y->z deptypes are (test)
 
     mock_repo = MockPackageMultiRepo([w, x, y, z])
     try:
-        spack.package_testing.test(w.name)
         spack.repo = mock_repo
         spec = Spec('w')
-        spec.concretize()
+        spec.concretize(tests=(w.name,))
 
         assert ('x' in spec)
         assert ('z' not in spec)
     finally:
         spack.repo = saved_repo
-        spack.package_testing.clear()
 
 
 @pytest.mark.usefixtures('refresh_builtin_mock')