testing: increase installer coverage (#15237)

lib/spack/spack/installer.py

@@ -36,6 +36,7 @@
 import sys
 import time
 
+import llnl.util.filesystem as fs
 import llnl.util.lock as lk
 import llnl.util.tty as tty
 import spack.binary_distribution as binary_distribution
@@ -43,14 +44,12 @@
 import spack.error
 import spack.hooks
 import spack.package
+import spack.package_prefs as prefs
 import spack.repo
 import spack.store
 
-from llnl.util.filesystem import \
-    chgrp, install, install_tree, mkdirp, touch, working_dir
 from llnl.util.tty.color import colorize, cwrite
 from llnl.util.tty.log import log_output
-from spack.package_prefs import get_package_dir_permissions, get_package_group
 from spack.util.environment import dump_environment
 from spack.util.executable import which
 
@@ -133,21 +132,21 @@ def _do_fake_install(pkg):
     chmod = which('chmod')
 
     # Install fake command
-    mkdirp(pkg.prefix.bin)
-    touch(os.path.join(pkg.prefix.bin, command))
+    fs.mkdirp(pkg.prefix.bin)
+    fs.touch(os.path.join(pkg.prefix.bin, command))
     chmod('+x', os.path.join(pkg.prefix.bin, command))
 
     # Install fake header file
-    mkdirp(pkg.prefix.include)
-    touch(os.path.join(pkg.prefix.include, header + '.h'))
+    fs.mkdirp(pkg.prefix.include)
+    fs.touch(os.path.join(pkg.prefix.include, header + '.h'))
 
     # Install fake shared and static libraries
-    mkdirp(pkg.prefix.lib)
+    fs.mkdirp(pkg.prefix.lib)
     for suffix in [dso_suffix, '.a']:
-        touch(os.path.join(pkg.prefix.lib, library + suffix))
+        fs.touch(os.path.join(pkg.prefix.lib, library + suffix))
 
     # Install fake man page
-    mkdirp(pkg.prefix.man.man1)
+    fs.mkdirp(pkg.prefix.man.man1)
 
     packages_dir = spack.store.layout.build_packages_path(pkg.spec)
     dump_packages(pkg.spec, packages_dir)
@@ -380,14 +379,14 @@ def dump_packages(spec, path):
     Dump all package information for a spec and its dependencies.
 
     This creates a package repository within path for every namespace in the
-    spec DAG, and fills the repos wtih package files and patch files for every
+    spec DAG, and fills the repos with package files and patch files for every
     node in the DAG.
 
     Args:
         spec (Spec): the Spack spec whose package information is to be dumped
         path (str): the path to the build packages directory
     """
-    mkdirp(path)
+    fs.mkdirp(path)
 
     # Copy in package.py files from any dependencies.
     # Note that we copy them in as they are in the *install* directory
@@ -428,7 +427,7 @@ def dump_packages(spec, path):
         if node is spec:
             spack.repo.path.dump_provenance(node, dest_pkg_dir)
         elif source_pkg_dir:
-            install_tree(source_pkg_dir, dest_pkg_dir)
+            fs.install_tree(source_pkg_dir, dest_pkg_dir)
 
 
 def install_msg(name, pid):
@@ -464,17 +463,17 @@ def log(pkg):
         tty.debug(e)
 
     # Archive the whole stdout + stderr for the package
-    install(pkg.log_path, pkg.install_log_path)
+    fs.install(pkg.log_path, pkg.install_log_path)
 
     # Archive the environment used for the build
-    install(pkg.env_path, pkg.install_env_path)
+    fs.install(pkg.env_path, pkg.install_env_path)
 
     if os.path.exists(pkg.configure_args_path):
         # Archive the args used for the build
-        install(pkg.configure_args_path, pkg.install_configure_args_path)
+        fs.install(pkg.configure_args_path, pkg.install_configure_args_path)
 
     # Finally, archive files that are specific to each package
-    with working_dir(pkg.stage.path):
+    with fs.working_dir(pkg.stage.path):
         errors = six.StringIO()
         target_dir = os.path.join(
             spack.store.layout.metadata_path(pkg.spec), 'archived-files')
@@ -496,8 +495,8 @@ def log(pkg):
                     target = os.path.join(target_dir, f)
                     # We must ensure that the directory exists before
                     # copying a file in
-                    mkdirp(os.path.dirname(target))
-                    install(f, target)
+                    fs.mkdirp(os.path.dirname(target))
+                    fs.install(f, target)
                 except Exception as e:
                     tty.debug(e)
 
@@ -508,7 +507,7 @@ def log(pkg):
 
         if errors.getvalue():
             error_file = os.path.join(target_dir, 'errors.txt')
-            mkdirp(target_dir)
+            fs.mkdirp(target_dir)
             with open(error_file, 'w') as err:
                 err.write(errors.getvalue())
             tty.warn('Errors occurred when archiving files.\n\t'
@@ -1074,10 +1073,10 @@ def build_process():
                                                   pkg.name, 'src')
                         tty.msg('{0} Copying source to {1}'
                                 .format(pre, src_target))
-                        install_tree(pkg.stage.source_path, src_target)
+                        fs.install_tree(pkg.stage.source_path, src_target)
 
                     # Do the real install in the source directory.
-                    with working_dir(pkg.stage.source_path):
+                    with fs.working_dir(pkg.stage.source_path):
                         # Save the build environment in a file before building.
                         dump_environment(pkg.env_path)
 
@@ -1289,20 +1288,20 @@ def _setup_install_dir(self, pkg):
             spack.store.layout.create_install_directory(pkg.spec)
         else:
             # Set the proper group for the prefix
-            group = get_package_group(pkg.spec)
+            group = prefs.get_package_group(pkg.spec)
             if group:
-                chgrp(pkg.spec.prefix, group)
+                fs.chgrp(pkg.spec.prefix, group)
 
             # Set the proper permissions.
             # This has to be done after group because changing groups blows
             # away the sticky group bit on the directory
             mode = os.stat(pkg.spec.prefix).st_mode
-            perms = get_package_dir_permissions(pkg.spec)
+            perms = prefs.get_package_dir_permissions(pkg.spec)
             if mode != perms:
                 os.chmod(pkg.spec.prefix, perms)
 
             # Ensure the metadata path exists as well
-            mkdirp(spack.store.layout.metadata_path(pkg.spec), mode=perms)
+            fs.mkdirp(spack.store.layout.metadata_path(pkg.spec), mode=perms)
 
     def _update_failed(self, task, mark=False, exc=None):
         """

lib/spack/spack/test/install.py

@@ -7,7 +7,7 @@
 import pytest
 import shutil
 
-from llnl.util.filesystem import mkdirp, touch, working_dir
+import llnl.util.filesystem as fs
 
 from spack.package import InstallError, PackageBase, PackageStillNeededError
 import spack.error
@@ -380,11 +380,11 @@ def test_pkg_build_paths(install_mockery):
 
     # Backward compatibility checks
     log_dir = os.path.dirname(log_path)
-    mkdirp(log_dir)
-    with working_dir(log_dir):
+    fs.mkdirp(log_dir)
+    with fs.working_dir(log_dir):
         # Start with the older of the previous log filenames
         older_log = 'spack-build.out'
-        touch(older_log)
+        fs.touch(older_log)
         assert spec.package.log_path.endswith(older_log)
 
         # Now check the newer log filename
@@ -416,11 +416,11 @@ def test_pkg_install_paths(install_mockery):
 
     # Backward compatibility checks
     log_dir = os.path.dirname(log_path)
-    mkdirp(log_dir)
-    with working_dir(log_dir):
+    fs.mkdirp(log_dir)
+    with fs.working_dir(log_dir):
         # Start with the older of the previous install log filenames
         older_log = 'build.out'
-        touch(older_log)
+        fs.touch(older_log)
         assert spec.package.install_log_path.endswith(older_log)
 
         # Now check the newer install log filename
@@ -437,7 +437,8 @@ def test_pkg_install_paths(install_mockery):
     shutil.rmtree(log_dir)
 
 
-def test_pkg_install_log(install_mockery):
+def test_log_install_without_build_files(install_mockery):
+    """Test the installer log function when no build files are present."""
     # Get a basic concrete spec for the trivial install package.
     spec = Spec('trivial-install-test-package').concretized()
 
@@ -445,17 +446,40 @@ def test_pkg_install_log(install_mockery):
     with pytest.raises(IOError, match="No such file or directory"):
         spack.installer.log(spec.package)
 
-    # Set up mock build files and try again
+
+def test_log_install_with_build_files(install_mockery, monkeypatch):
+    """Test the installer's log function when have build files."""
+    config_log = 'config.log'
+
+    # Retain the original function for use in the monkey patch that is used
+    # to raise an exception under the desired condition for test coverage.
+    orig_install_fn = fs.install
+
+    def _install(src, dest):
+        orig_install_fn(src, dest)
+        if src.endswith(config_log):
+            raise Exception('Mock log install error')
+
+    monkeypatch.setattr(fs, 'install', _install)
+
+    spec = Spec('trivial-install-test-package').concretized()
+
+    # Set up mock build files and try again to include archive failure
     log_path = spec.package.log_path
     log_dir = os.path.dirname(log_path)
-    mkdirp(log_dir)
-    with working_dir(log_dir):
-        touch(log_path)
-        touch(spec.package.env_path)
-        touch(spec.package.configure_args_path)
+    fs.mkdirp(log_dir)
+    with fs.working_dir(log_dir):
+        fs.touch(log_path)
+        fs.touch(spec.package.env_path)
+        fs.touch(spec.package.configure_args_path)
 
     install_path = os.path.dirname(spec.package.install_log_path)
-    mkdirp(install_path)
+    fs.mkdirp(install_path)
+
+    source = spec.package.stage.source_path
+    config = os.path.join(source, 'config.log')
+    fs.touchp(config)
+    spec.package.archive_files = ['missing', '..', config]
 
     spack.installer.log(spec.package)
 
@@ -463,6 +487,21 @@ def test_pkg_install_log(install_mockery):
     assert os.path.exists(spec.package.install_env_path)
     assert os.path.exists(spec.package.install_configure_args_path)
 
+    archive_dir = os.path.join(install_path, 'archived-files')
+    source_dir = os.path.dirname(source)
+    rel_config = os.path.relpath(config, source_dir)
+
+    assert os.path.exists(os.path.join(archive_dir, rel_config))
+    assert not os.path.exists(os.path.join(archive_dir, 'missing'))
+
+    expected_errs = [
+        'OUTSIDE SOURCE PATH',   # for '..'
+        'FAILED TO ARCHIVE'      # for rel_config
+    ]
+    with open(os.path.join(archive_dir, 'errors.txt'), 'r') as fd:
+        for ln, expected in zip(fd, expected_errs):
+            assert expected in ln
+
     # Cleanup
     shutil.rmtree(log_dir)
 

lib/spack/spack/test/installer.py

@@ -7,15 +7,19 @@
 import py
 import pytest
 
+import llnl.util.filesystem as fs
 import llnl.util.tty as tty
+import llnl.util.lock as ulk
 
 import spack.binary_distribution
 import spack.compilers
 import spack.directory_layout as dl
 import spack.installer as inst
-import spack.util.lock as lk
+import spack.package_prefs as prefs
 import spack.repo
 import spack.spec
+import spack.store
+import spack.util.lock as lk
 
 
 def _mock_repo(root, namespace):
@@ -152,7 +156,7 @@ def test_process_external_package_module(install_mockery, monkeypatch, capfd):
 
 def test_process_binary_cache_tarball_none(install_mockery, monkeypatch,
                                            capfd):
-    """Tests to cover _process_binary_cache_tarball when no tarball."""
+    """Tests of _process_binary_cache_tarball when no tarball."""
     monkeypatch.setattr(spack.binary_distribution, 'download_tarball', _none)
 
     pkg = spack.repo.get('trivial-install-test-package')
@@ -162,7 +166,7 @@ def test_process_binary_cache_tarball_none(install_mockery, monkeypatch,
 
 
 def test_process_binary_cache_tarball_tar(install_mockery, monkeypatch, capfd):
-    """Tests to cover _process_binary_cache_tarball with a tar file."""
+    """Tests of _process_binary_cache_tarball with a tar file."""
     def _spec(spec):
         return spec
 
@@ -179,6 +183,25 @@ def _spec(spec):
     assert 'Installing a from binary cache' in capfd.readouterr()[0]
 
 
+def test_try_install_from_binary_cache(install_mockery, mock_packages,
+                                       monkeypatch, capsys):
+    """Tests SystemExit path for_try_install_from_binary_cache."""
+    def _spec(spec, force):
+        spec = spack.spec.Spec('mpi').concretized()
+        return {spec: None}
+
+    spec = spack.spec.Spec('mpich')
+    spec.concretize()
+
+    monkeypatch.setattr(spack.binary_distribution, 'get_spec', _spec)
+
+    with pytest.raises(SystemExit):
+        inst._try_install_from_binary_cache(spec.package, False, False)
+
+    captured = capsys.readouterr()
+    assert 'add a spack mirror to allow download' in str(captured)
+
+
 def test_installer_init_errors(install_mockery):
     """Test to ensure cover installer constructor errors."""
     with pytest.raises(ValueError, match='must be a package'):
@@ -189,17 +212,18 @@ def test_installer_init_errors(install_mockery):
         inst.PackageInstaller(pkg)
 
 
-def test_installer_strings(install_mockery):
-    """Tests of installer repr and str for coverage purposes."""
+def test_installer_repr(install_mockery):
     spec, installer = create_installer('trivial-install-test-package')
 
-    # Cover __repr__
     irep = installer.__repr__()
     assert irep.startswith(installer.__class__.__name__)
     assert "installed=" in irep
     assert "failed=" in irep
 
-    # Cover __str__
+
+def test_installer_str(install_mockery):
+    spec, installer = create_installer('trivial-install-test-package')
+
     istr = str(installer)
     assert "#tasks=0" in istr
     assert "installed (0)" in istr
@@ -207,7 +231,6 @@ def test_installer_strings(install_mockery):
 
 
 def test_installer_last_phase_error(install_mockery, capsys):
-    """Test to cover last phase error."""
     spec = spack.spec.Spec('trivial-install-test-package')
     spec.concretize()
     assert spec.concrete
@@ -220,7 +243,6 @@ def test_installer_last_phase_error(install_mockery, capsys):
 
 
 def test_installer_ensure_ready_errors(install_mockery):
-    """Test to cover _ensure_ready errors."""
     spec, installer = create_installer('trivial-install-test-package')
 
     fmt = r'cannot be installed locally.*{0}'
@@ -247,24 +269,102 @@ def test_installer_ensure_ready_errors(install_mockery):
         installer._ensure_install_ready(spec.package)
 
 
-def test_ensure_locked_have(install_mockery, tmpdir):
-    """Test to cover _ensure_locked when already have lock."""
+def test_ensure_locked_err(install_mockery, monkeypatch, tmpdir, capsys):
+    """Test _ensure_locked when a non-lock exception is raised."""
+    mock_err_msg = 'Mock exception error'
+
+    def _raise(lock, timeout):
+        raise RuntimeError(mock_err_msg)
+
+    spec, installer = create_installer('trivial-install-test-package')
+
+    monkeypatch.setattr(ulk.Lock, 'acquire_read', _raise)
+    with tmpdir.as_cwd():
+        with pytest.raises(RuntimeError):
+            installer._ensure_locked('read', spec.package)
+
+        out = str(capsys.readouterr()[1])
+        assert 'Failed to acquire a read lock' in out
+        assert mock_err_msg in out
+
+
+def test_ensure_locked_have(install_mockery, tmpdir, capsys):
+    """Test _ensure_locked when already have lock."""
     spec, installer = create_installer('trivial-install-test-package')
 
     with tmpdir.as_cwd():
+        # Test "downgrade" of a read lock (to a read lock)
         lock = lk.Lock('./test', default_timeout=1e-9, desc='test')
         lock_type = 'read'
         tpl = (lock_type, lock)
         installer.locks[installer.pkg_id] = tpl
         assert installer._ensure_locked(lock_type, spec.package) == tpl
 
+        # Test "upgrade" of a read lock without read count to a write
+        lock_type = 'write'
+        err = 'Cannot upgrade lock'
+        with pytest.raises(ulk.LockUpgradeError, match=err):
+            installer._ensure_locked(lock_type, spec.package)
 
-def test_package_id(install_mockery):
-    """Test to cover package_id functionality."""
+        out = str(capsys.readouterr()[1])
+        assert 'Failed to upgrade to a write lock' in out
+        assert 'exception when releasing read lock' in out
+
+        # Test "upgrade" of the read lock *with* read count to a write
+        lock._reads = 1
+        tpl = (lock_type, lock)
+        assert installer._ensure_locked(lock_type, spec.package) == tpl
+
+        # Test "downgrade" of the write lock to a read lock
+        lock_type = 'read'
+        tpl = (lock_type, lock)
+        assert installer._ensure_locked(lock_type, spec.package) == tpl
+
+
+@pytest.mark.parametrize('lock_type,reads,writes', [
+    ('read', 1, 0),
+    ('write', 0, 1)])
+def test_ensure_locked_new_lock(
+        install_mockery, tmpdir, lock_type, reads, writes):
+    pkg_id = 'a'
+    spec, installer = create_installer(pkg_id)
+    with tmpdir.as_cwd():
+        ltype, lock = installer._ensure_locked(lock_type, spec.package)
+        assert ltype == lock_type
+        assert lock is not None
+        assert lock._reads == reads
+        assert lock._writes == writes
+
+
+def test_ensure_locked_new_warn(install_mockery, monkeypatch, tmpdir, capsys):
+    orig_pl = spack.database.Database.prefix_lock
+
+    def _pl(db, spec, timeout):
+        lock = orig_pl(db, spec, timeout)
+        lock.default_timeout = 1e-9 if timeout is None else None
+        return lock
+
+    pkg_id = 'a'
+    spec, installer = create_installer(pkg_id)
+
+    monkeypatch.setattr(spack.database.Database, 'prefix_lock', _pl)
+
+    lock_type = 'read'
+    ltype, lock = installer._ensure_locked(lock_type, spec.package)
+    assert ltype == lock_type
+    assert lock is not None
+
+    out = str(capsys.readouterr()[1])
+    assert 'Expected prefix lock timeout' in out
+
+
+def test_package_id_err(install_mockery):
     pkg = spack.repo.get('trivial-install-test-package')
     with pytest.raises(ValueError, match='spec is not concretized'):
         inst.package_id(pkg)
 
+
+def test_package_id_ok(install_mockery):
     spec = spack.spec.Spec('trivial-install-test-package')
     spec.concretize()
     assert spec.concrete
@@ -273,36 +373,44 @@ def test_package_id(install_mockery):
 
 
 def test_fake_install(install_mockery):
-    """Test to cover fake install basics."""
     spec = spack.spec.Spec('trivial-install-test-package')
     spec.concretize()
     assert spec.concrete
+
     pkg = spec.package
     inst._do_fake_install(pkg)
     assert os.path.isdir(pkg.prefix.lib)
 
 
-def test_packages_needed_to_bootstrap_compiler(install_mockery, monkeypatch):
-    """Test to cover most of _packages_needed_to_boostrap_compiler."""
-    # TODO: More work is needed to go beyond the dependency check
-    def _no_compilers(pkg, arch_spec):
-        return []
-
-    # Test path where no compiler packages returned
+def test_packages_needed_to_bootstrap_compiler_none(install_mockery):
     spec = spack.spec.Spec('trivial-install-test-package')
     spec.concretize()
     assert spec.concrete
+
     packages = inst._packages_needed_to_bootstrap_compiler(spec.package)
     assert not packages
 
-    # Test up to the dependency check
-    monkeypatch.setattr(spack.compilers, 'compilers_for_spec', _no_compilers)
-    with pytest.raises(spack.repo.UnknownPackageError, match='not found'):
-        inst._packages_needed_to_bootstrap_compiler(spec.package)
+
+def test_packages_needed_to_bootstrap_compiler_packages(install_mockery,
+                                                        monkeypatch):
+    spec = spack.spec.Spec('trivial-install-test-package')
+    spec.concretize()
+
+    def _conc_spec(compiler):
+        return spack.spec.Spec('a').concretized()
+
+    # Ensure we can get past functions that are precluding obtaining
+    # packages.
+    monkeypatch.setattr(spack.compilers, 'compilers_for_spec', _none)
+    monkeypatch.setattr(spack.compilers, 'pkg_spec_for_compiler', _conc_spec)
+    monkeypatch.setattr(spack.spec.Spec, 'concretize', _noop)
+
+    packages = inst._packages_needed_to_bootstrap_compiler(spec.package)
+    assert packages
 
 
 def test_dump_packages_deps_ok(install_mockery, tmpdir, mock_repo_path):
-    """Test to add coverage to dump_packages with dependencies happy path."""
+    """Test happy path for dump_packages with dependencies."""
 
     spec_name = 'simple-inheritance'
     spec = spack.spec.Spec(spec_name).concretized()
@@ -314,7 +422,7 @@ def test_dump_packages_deps_ok(install_mockery, tmpdir, mock_repo_path):
 
 
 def test_dump_packages_deps_errs(install_mockery, tmpdir, monkeypatch, capsys):
-    """Test to add coverage to dump_packages with dependencies."""
+    """Test error paths for dump_packages with dependencies."""
     orig_bpp = spack.store.layout.build_packages_path
     orig_dirname = spack.repo.Repo.dirname_for_package_name
     repo_err_msg = "Mock dirname_for_package_name"
@@ -354,59 +462,45 @@ def _repoerr(repo, name):
     assert "Couldn't copy in provenance for cmake" in out
 
 
-@pytest.mark.tld
-def test_check_deps_status_errs(install_mockery, monkeypatch):
-    """Test to cover _check_deps_status failures."""
+def test_check_deps_status_install_failure(install_mockery, monkeypatch):
     spec, installer = create_installer('a')
 
     # Make sure the package is identified as failed
-    orig_fn = spack.database.Database.prefix_failed
     monkeypatch.setattr(spack.database.Database, 'prefix_failed', _true)
 
     with pytest.raises(inst.InstallError, match='install failure'):
         installer._check_deps_status()
 
-    monkeypatch.setattr(spack.database.Database, 'prefix_failed', orig_fn)
 
-    # Ensure do not acquire the lock
+def test_check_deps_status_write_locked(install_mockery, monkeypatch):
+    spec, installer = create_installer('a')
+
+    # Ensure the lock is not acquired
     monkeypatch.setattr(inst.PackageInstaller, '_ensure_locked', _not_locked)
 
     with pytest.raises(inst.InstallError, match='write locked by another'):
         installer._check_deps_status()
 
 
-@pytest.mark.tld
 def test_check_deps_status_external(install_mockery, monkeypatch):
-    """Test to cover _check_deps_status for external."""
     spec, installer = create_installer('a')
 
-    deps = spec.dependencies()
-    assert len(deps) > 0
-    dep_id = 'b'
-
-    # Ensure the known dependent is installed if flagged as external
+    # Mock the known dependent, b, as external so assumed to be installed
     monkeypatch.setattr(spack.spec.Spec, 'external', True)
     installer._check_deps_status()
-    assert dep_id in installer.installed
+    assert 'b' in installer.installed
 
 
-@pytest.mark.tld
 def test_check_deps_status_upstream(install_mockery, monkeypatch):
-    """Test to cover _check_deps_status for upstream."""
     spec, installer = create_installer('a')
 
-    deps = spec.dependencies()
-    assert len(deps) > 0
-    dep_id = 'b'
-
-    # Ensure the known dependent, b, is installed if flagged as upstream
+    # Mock the known dependent, b, as installed upstream
     monkeypatch.setattr(spack.package.PackageBase, 'installed_upstream', True)
     installer._check_deps_status()
-    assert dep_id in installer.installed
+    assert 'b' in installer.installed
 
 
 def test_add_bootstrap_compilers(install_mockery, monkeypatch):
-    """Test to cover _add_bootstrap_compilers."""
     def _pkgs(pkg):
         spec = spack.spec.Spec('mpi').concretized()
         return [(spec.package, True)]
@@ -445,7 +539,6 @@ def test_installer_init_queue(install_mockery):
 
 
 def test_install_task_use_cache(install_mockery, monkeypatch):
-    """Test _install_task to cover use_cache path."""
     spec, installer = create_installer('trivial-install-test-package')
     task = create_build_task(spec.package)
 
@@ -454,25 +547,27 @@ def test_install_task_use_cache(install_mockery, monkeypatch):
     assert spec.package.name in installer.installed
 
 
-def test_install_task_stop_iter(install_mockery, monkeypatch, capfd):
-    """Test _install_task to cover the StopIteration exception."""
-    mock_err_msg = 'mock stop iteration'
+def test_install_task_add_compiler(install_mockery, monkeypatch, capfd):
+    config_msg = 'mock add_compilers_to_config'
 
-    def _raise(installer, pkg):
-        raise StopIteration(mock_err_msg)
+    def _add(_compilers):
+        tty.msg(config_msg)
 
     spec, installer = create_installer('a')
     task = create_build_task(spec.package)
+    task.compiler = True
 
+    # Preclude any meaningful side-effects
     monkeypatch.setattr(spack.package.PackageBase, 'unit_test_check', _true)
-    monkeypatch.setattr(inst.PackageInstaller, '_setup_install_dir', _raise)
+    monkeypatch.setattr(inst.PackageInstaller, '_setup_install_dir', _noop)
+    monkeypatch.setattr(spack.build_environment, 'fork', _noop)
+    monkeypatch.setattr(spack.database.Database, 'add', _noop)
+    monkeypatch.setattr(spack.compilers, 'add_compilers_to_config', _add)
 
     installer._install_task(task)
-    out = capfd.readouterr()[0]
 
-    assert mock_err_msg in out
-    assert 'Package stage directory' in out
-    assert spec.package.stage.source_path in out
+    out = capfd.readouterr()[0]
+    assert config_msg in out
 
 
 def test_release_lock_write_n_exception(install_mockery, tmpdir, capsys):
@@ -529,8 +624,36 @@ def _rmtask(installer, pkg_id):
     assert len(installer.build_tasks) == 1
 
 
-def test_cleanup_failed(install_mockery, tmpdir, monkeypatch, capsys):
-    """Test to increase coverage of _cleanup_failed."""
+def test_setup_install_dir_grp(install_mockery, monkeypatch, capfd):
+    """Test _setup_install_dir's group change."""
+    mock_group = 'mockgroup'
+    mock_chgrp_msg = 'Changing group for {0} to {1}'
+
+    def _get_group(spec):
+        return mock_group
+
+    def _chgrp(path, group):
+        tty.msg(mock_chgrp_msg.format(path, group))
+
+    monkeypatch.setattr(prefs, 'get_package_group', _get_group)
+    monkeypatch.setattr(fs, 'chgrp', _chgrp)
+
+    spec, installer = create_installer('trivial-install-test-package')
+
+    fs.touchp(spec.prefix)
+    metadatadir = spack.store.layout.metadata_path(spec)
+    # Should fail with a "not a directory" error
+    with pytest.raises(OSError, match=metadatadir):
+        installer._setup_install_dir(spec.package)
+
+    out = str(capfd.readouterr()[0])
+
+    expected_msg = mock_chgrp_msg.format(spec.prefix, mock_group)
+    assert expected_msg in out
+
+
+def test_cleanup_failed_err(install_mockery, tmpdir, monkeypatch, capsys):
+    """Test _cleanup_failed exception path."""
     msg = 'Fake release_write exception'
 
     def _raise_except(lock):
@@ -550,13 +673,14 @@ def _raise_except(lock):
         assert msg in out
 
 
-def test_update_failed_no_mark(install_mockery):
-    """Test of _update_failed sans mark and dependent build tasks."""
+def test_update_failed_no_dependent_task(install_mockery):
+    """Test _update_failed with missing dependent build tasks."""
     spec, installer = create_installer('dependent-install')
-    task = create_build_task(spec.package)
 
-    installer._update_failed(task)
-    assert installer.failed['dependent-install'] is None
+    for dep in spec.traverse(root=False):
+        task = create_build_task(dep.package)
+        installer._update_failed(task, mark=False)
+        assert installer.failed[task.pkg_id] is None
 
 
 def test_install_uninstalled_deps(install_mockery, monkeypatch, capsys):
@@ -710,3 +834,12 @@ def _install(installer, task, **kwargs):
         installer.install()
 
     assert 'b' in installer.installed
+
+
+def test_install_skip_patch(install_mockery, mock_fetch):
+    """Test the path skip_patch install path."""
+    spec, installer = create_installer('b')
+
+    installer.install(fake=False, skip_patch=True)
+
+    assert 'b' in installer.installed