refactor: `spack.package.possible_dependencies()` handles virtuals

- [x] move some logic for handling virtual packages from the `spack
  dependencies` command into `spack.package.possible_dependencies()`

- [x] rework possible dependencies tests so that expected and actual
  output are on the left/right respectively

4 files changed, 75 insertions, 44 deletions

diff --git a/lib/spack/spack/cmd/dependencies.py b/lib/spack/spack/cmd/dependencies.py
index e65e050bfa..7f390341ef 100644
--- a/lib/spack/spack/cmd/dependencies.py
+++ b/lib/spack/spack/cmd/dependencies.py
@@ -9,6 +9,7 @@ from llnl.util.tty.colify import colify
 import spack.cmd
 import spack.cmd.common.arguments as arguments
 import spack.environment as ev
+import spack.package
 import spack.repo
 import spack.store
 
@@ -52,22 +53,15 @@ def dependencies(parser, args):
 
     else:
         spec = specs[0]
-
-        if not spec.virtual:
-            packages = [spec.package]
-        else:
-            packages = [
-                spack.repo.get(s.name)
-                for s in spack.repo.path.providers_for(spec)]
-
-        dependencies = set()
-        for pkg in packages:
-            possible = pkg.possible_dependencies(
-                args.transitive, args.expand_virtuals, deptype=args.deptype)
-            dependencies.update(possible)
+        dependencies = spack.package.possible_dependencies(
+            spec,
+            transitive=args.transitive,
+            expand_virtuals=args.expand_virtuals,
+            deptype=args.deptype
+        )
 
         if spec.name in dependencies:
-            dependencies.remove(spec.name)
+            del dependencies[spec.name]
 
         if dependencies:
             colify(sorted(dependencies))
diff --git a/lib/spack/spack/package.py b/lib/spack/spack/package.py
index de0f9b3568..9156d7d0dd 100644
--- a/lib/spack/spack/package.py
+++ b/lib/spack/spack/package.py
@@ -605,11 +605,10 @@ class PackageBase(with_metaclass(PackageMeta, PackageViewMixin, object)):
         """
         deptype = spack.dependency.canonical_deptype(deptype)
 
-        if visited is None:
-            visited = {cls.name: set()}
+        visited = {} if visited is None else visited
+        missing = {} if missing is None else missing
 
-        if missing is None:
-            missing = {cls.name: set()}
+        visited.setdefault(cls.name, set())
 
         for name, conditions in cls.dependencies.items():
             # check whether this dependency could be of the type asked for
@@ -624,6 +623,7 @@ class PackageBase(with_metaclass(PackageMeta, PackageViewMixin, object)):
                     providers = spack.repo.path.providers_for(name)
                     dep_names = [spec.name for spec in providers]
                 else:
+                    visited.setdefault(cls.name, set()).add(name)
                     visited.setdefault(name, set())
                     continue
             else:
@@ -2157,13 +2157,20 @@ def possible_dependencies(*pkg_or_spec, **kwargs):
     packages = []
     for pos in pkg_or_spec:
         if isinstance(pos, PackageMeta):
-            pkg = pos
-        elif isinstance(pos, spack.spec.Spec):
-            pkg = pos.package
-        else:
-            pkg = spack.spec.Spec(pos).package
+            packages.append(pos)
+            continue
+
+        if not isinstance(pos, spack.spec.Spec):
+            pos = spack.spec.Spec(pos)
 
-        packages.append(pkg)
+        if spack.repo.path.is_virtual(pos.name):
+            packages.extend(
+                p.package_class
+                for p in spack.repo.path.providers_for(pos.name)
+            )
+            continue
+        else:
+            packages.append(pos.package_class)
 
     visited = {}
     for pkg in packages:
diff --git a/lib/spack/spack/test/cmd/dependencies.py b/lib/spack/spack/test/cmd/dependencies.py
index fc47069181..05d0556936 100644
--- a/lib/spack/spack/test/cmd/dependencies.py
+++ b/lib/spack/spack/test/cmd/dependencies.py
@@ -17,7 +17,7 @@ mpis = ['mpich', 'mpich2', 'multi-provider-mpi', 'zmpi']
 mpi_deps = ['fake']
 
 
-def test_immediate_dependencies(mock_packages):
+def test_direct_dependencies(mock_packages):
     out = dependencies('mpileaks')
     actual = set(re.split(r'\s+', out.strip()))
     expected = set(['callpath'] + mpis)
@@ -47,7 +47,7 @@ def test_transitive_dependencies_with_deptypes(mock_packages):
 
 
 @pytest.mark.db
-def test_immediate_installed_dependencies(mock_packages, database):
+def test_direct_installed_dependencies(mock_packages, database):
     with color_when(False):
         out = dependencies('--installed', 'mpileaks^mpich')
 
diff --git a/lib/spack/spack/test/package_class.py b/lib/spack/spack/test/package_class.py
index b3351ffb49..d540ac663e 100644
--- a/lib/spack/spack/test/package_class.py
+++ b/lib/spack/spack/test/package_class.py
@@ -11,12 +11,17 @@ static DSL metadata for packages.
 """
 
 import pytest
+import spack.package
 import spack.repo
 
 
-@pytest.fixture
-def mpileaks_possible_deps(mock_packages):
-    mpi_names = [spec.name for spec in spack.repo.path.providers_for('mpi')]
+@pytest.fixture(scope="module")
+def mpi_names(mock_repo_path):
+    return [spec.name for spec in mock_repo_path.providers_for('mpi')]
+
+
+@pytest.fixture()
+def mpileaks_possible_deps(mock_packages, mpi_names):
     possible = {
         'callpath': set(['dyninst'] + mpi_names),
         'dyninst': set(['libdwarf', 'libelf']),
@@ -34,47 +39,72 @@ def mpileaks_possible_deps(mock_packages):
 
 def test_possible_dependencies(mock_packages, mpileaks_possible_deps):
     mpileaks = spack.repo.get('mpileaks')
-    assert (mpileaks.possible_dependencies(expand_virtuals=True) ==
-            mpileaks_possible_deps)
+    assert mpileaks_possible_deps == (
+        mpileaks.possible_dependencies(expand_virtuals=True))
 
-    assert mpileaks.possible_dependencies(expand_virtuals=False) == {
-        'callpath': set(['dyninst']),
+    assert {
+        'callpath': set(['dyninst', 'mpi']),
         'dyninst': set(['libdwarf', 'libelf']),
         'libdwarf': set(['libelf']),
         'libelf': set(),
         'mpi': set(),
-        'mpileaks': set(['callpath']),
-    }
+        'mpileaks': set(['callpath', 'mpi']),
+    } == mpileaks.possible_dependencies(expand_virtuals=False)
+
+
+def test_possible_direct_dependencies(mock_packages, mpileaks_possible_deps):
+    mpileaks = spack.repo.get('mpileaks')
+    deps = mpileaks.possible_dependencies(transitive=False,
+                                          expand_virtuals=False)
+
+    assert {
+        'callpath': set(),
+        'mpi': set(),
+        'mpileaks': set(['callpath', 'mpi']),
+    } == deps
+
+
+def test_possible_dependencies_virtual(mock_packages, mpi_names):
+    expected = dict(
+        (name, set(spack.repo.get(name).dependencies))
+        for name in mpi_names
+    )
+
+    # only one mock MPI has a dependency
+    expected['fake'] = set()
+
+    assert expected == spack.package.possible_dependencies(
+        "mpi", transitive=False)
 
 
 def test_possible_dependencies_missing(mock_packages):
     md = spack.repo.get("missing-dependency")
     missing = {}
     md.possible_dependencies(transitive=True, missing=missing)
-    assert missing["missing-dependency"] == set([
+    assert set([
         "this-is-a-missing-dependency"
-    ])
+    ]) == missing["missing-dependency"]
 
 
 def test_possible_dependencies_with_deptypes(mock_packages):
     dtbuild1 = spack.repo.get('dtbuild1')
 
-    assert dtbuild1.possible_dependencies(deptype=('link', 'run')) == {
+    assert {
         'dtbuild1': set(['dtrun2', 'dtlink2']),
         'dtlink2': set(),
         'dtrun2': set(),
-    }
+    } == dtbuild1.possible_dependencies(deptype=('link', 'run'))
 
-    assert dtbuild1.possible_dependencies(deptype=('build')) == {
+    assert {
         'dtbuild1': set(['dtbuild2', 'dtlink2']),
         'dtbuild2': set(),
         'dtlink2': set(),
-    }
+    } == dtbuild1.possible_dependencies(deptype=('build'))
 
-    assert dtbuild1.possible_dependencies(deptype=('link')) == {
+    assert {
         'dtbuild1': set(['dtlink2']),
         'dtlink2': set(),
-    }
+    } == dtbuild1.possible_dependencies(deptype=('link'))
 
 
 def test_possible_dependencies_with_multiple_classes(
@@ -88,4 +118,4 @@ def test_possible_dependencies_with_multiple_classes(
         'dt-diamond-bottom': set(),
     })
 
-    assert spack.package.possible_dependencies(*pkgs) == expected
+    assert expected == spack.package.possible_dependencies(*pkgs)