commands: add tests for all subcommands of spack pkg

lib/spack/spack/cmd/pkg.py

@@ -7,15 +7,15 @@
 
 import os
 import argparse
+import re
 
 import llnl.util.tty as tty
 from llnl.util.tty.colify import colify
-from llnl.util.filesystem import working_dir
 
+import spack.cmd
 import spack.paths
 import spack.repo
 from spack.util.executable import which
-from spack.cmd import spack_is_git_repo
 
 description = "query packages associated with particular git revisions"
 section = "developer"
@@ -57,6 +57,10 @@ def setup_parser(subparser):
     add_parser.add_argument(
         'rev2', nargs='?', default='HEAD',
         help="revision to compare to rev1 (default is HEAD)")
+    add_parser.add_argument(
+        '-t', '--type', action='store', default='C',
+        help="Types of changes to show (A: added, R: removed, "
+        "C: changed); default is 'C'")
 
     rm_parser = sp.add_parser('removed', help=pkg_removed.__doc__)
     rm_parser.add_argument(
@@ -84,20 +88,20 @@ def get_git():
 
 
 def list_packages(rev):
-    pkgpath = packages_path()
-    relpath = pkgpath[len(spack.paths.prefix + os.path.sep):] + os.path.sep
-
     git = get_git()
-    with working_dir(spack.paths.prefix):
+
-        output = git('ls-tree', '--full-tree', '--name-only', rev, relpath,
+    # git ls-tree does not support ... merge-base syntax, so do it manually
-                     output=str)
+    if rev.endswith('...'):
-    return sorted(line[len(relpath):] for line in output.split('\n') if line)
+        ref = rev.replace('...', '')
+        rev = git('merge-base', ref, 'HEAD', output=str).strip()
+
+    output = git('ls-tree', '--name-only', rev, output=str)
+    return sorted(line for line in output.split('\n')
+                  if line and not line.startswith('.'))
 
 
 def pkg_add(args):
     """add a package to the git stage with `git add`"""
-    pkgpath = packages_path()
-
     for pkg_name in args.packages:
         filename = spack.repo.path.filename_for_package_name(pkg_name)
         if not os.path.isfile(filename):
@@ -105,8 +109,7 @@ def pkg_add(args):
                     pkg_name, filename)
 
         git = get_git()
-        with working_dir(spack.paths.prefix):
+        git('add', filename)
-            git('-C', pkgpath, 'add', filename)
 
 
 def pkg_list(args):
@@ -151,24 +154,38 @@ def pkg_added(args):
 
 def pkg_changed(args):
     """show packages changed since a commit"""
-    pkgpath = spack.repo.path.get_repo('builtin').packages_path
+    lower_type = args.type.lower()
-    rel_pkg_path = os.path.relpath(pkgpath, spack.paths.prefix)
+    if not re.match('^[arc]*$', lower_type):
+        tty.die("Invald change type: '%s'." % args.type,
+                "Can contain only A (added), R (removed), or C (changed)")
+
+    removed, added = diff_packages(args.rev1, args.rev2)
 
     git = get_git()
-    paths = git('diff', '--name-only', args.rev1, args.rev2, pkgpath,
+    out = git('diff', '--relative', '--name-only', args.rev1, args.rev2,
-                output=str).strip().split('\n')
+              output=str).strip()
 
-    packages = set([])
+    lines = [] if not out else re.split(r'\s+', out)
-    for path in paths:
+    changed = set()
-        path = path.replace(rel_pkg_path + os.sep, '')
+    for path in lines:
         pkg_name, _, _ = path.partition(os.sep)
-        packages.add(pkg_name)
+        if pkg_name not in added and pkg_name not in removed:
+            changed.add(pkg_name)
 
+    packages = set()
+    if 'a' in lower_type:
+        packages |= added
+    if 'r' in lower_type:
+        packages |= removed
+    if 'c' in lower_type:
+        packages |= changed
+
+    if packages:
         colify(sorted(packages))
 
 
 def pkg(parser, args):
-    if not spack_is_git_repo():
+    if not spack.cmd.spack_is_git_repo():
         tty.die("This spack is not a git clone. Can't use 'spack pkg'")
 
     action = {'add': pkg_add,

lib/spack/spack/repo.py

@@ -150,7 +150,8 @@ def _create_new_cache(self):
             pkg_dir = os.path.join(self.packages_path, pkg_name)
 
             # Warn about invalid names that look like packages.
-            if not valid_module_name(pkg_name):
+            if (not valid_module_name(pkg_name)
+                    and not pkg_name.startswith('.')):
                 msg = 'Skipping package at {0}. '
                 msg += '"{1}" is not a valid Spack module name.'
                 tty.warn(msg.format(pkg_dir, pkg_name))

lib/spack/spack/test/cmd/pkg.py

@@ -0,0 +1,229 @@
+# Copyright 2013-2019 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 __future__ import print_function
+
+import pytest
+import re
+import shutil
+
+from llnl.util.filesystem import mkdirp, working_dir
+
+import spack.main
+import spack.cmd.pkg
+from spack.util.executable import which
+
+pytestmark = pytest.mark.skipif(not which('git'),
+                                reason="spack pkg tests require git")
+
+#: new fake package template
+pkg_template = '''\
+from spack import *
+
+class {name}(Package):
+    homepage = "http://www.example.com"
+    url      = "http://www.example.com/test-1.0.tar.gz"
+
+    version('1.0', '0123456789abcdef0123456789abcdef')
+
+    def install(self, spec, prefix):
+        pass
+'''
+
+abc = set(('pkg-a', 'pkg-b', 'pkg-c'))
+abd = set(('pkg-a', 'pkg-b', 'pkg-d'))
+
+
+# Force all tests to use a git repository *in* the mock packages repo.
+@pytest.fixture(scope='module')
+def mock_pkg_git_repo(tmpdir_factory):
+    """Copy the builtin.mock repo and make a mutable git repo inside it."""
+    tmproot = tmpdir_factory.mktemp('mock_pkg_git_repo')
+    repo_path = tmproot.join('builtin.mock')
+
+    shutil.copytree(spack.paths.mock_packages_path, str(repo_path))
+    mock_repo = spack.repo.RepoPath(str(repo_path))
+    mock_repo_packages = mock_repo.repos[0].packages_path
+
+    git = which('git', required=True)
+    with working_dir(mock_repo_packages):
+        git('init')
+
+        # initial commit with mock packages
+        git('add', '.')
+        git('commit', '-m', 'initial mock repo commit')
+
+        # add commit with pkg-a, pkg-b, pkg-c packages
+        mkdirp('pkg-a', 'pkg-b', 'pkg-c')
+        with open('pkg-a/package.py', 'w') as f:
+            f.write(pkg_template.format(name='PkgA'))
+        with open('pkg-c/package.py', 'w') as f:
+            f.write(pkg_template.format(name='PkgB'))
+        with open('pkg-b/package.py', 'w') as f:
+            f.write(pkg_template.format(name='PkgC'))
+        git('add', 'pkg-a', 'pkg-b', 'pkg-c')
+        git('commit', '-m', 'add pkg-a, pkg-b, pkg-c')
+
+        # remove pkg-c, add pkg-d
+        with open('pkg-b/package.py', 'a') as f:
+            f.write('\n# change pkg-b')
+        git('add', 'pkg-b')
+        mkdirp('pkg-d')
+        with open('pkg-d/package.py', 'w') as f:
+            f.write(pkg_template.format(name='PkgD'))
+        git('add', 'pkg-d')
+        git('rm', '-rf', 'pkg-c')
+        git('commit', '-m', 'change pkg-b, remove pkg-c, add pkg-d')
+
+    with spack.repo.swap(mock_repo):
+        yield mock_repo_packages
+
+
+@pytest.fixture(scope='module')
+def mock_pkg_names():
+    repo = spack.repo.path.get_repo('builtin.mock')
+    names = set(name for name in repo.all_package_names()
+                if not name.startswith('pkg-'))
+    return names
+
+
+def split(output):
+    """Split command line output into an array."""
+    output = output.strip()
+    return re.split(r'\s+', output) if output else []
+
+
+pkg = spack.main.SpackCommand('pkg')
+
+
+def test_packages_path():
+    assert (spack.cmd.pkg.packages_path() ==
+            spack.repo.path.get_repo('builtin').packages_path)
+
+
+def test_mock_packages_path(mock_packages):
+    assert (spack.cmd.pkg.packages_path() ==
+            spack.repo.path.get_repo('builtin.mock').packages_path)
+
+
+def test_pkg_add(mock_pkg_git_repo):
+    with working_dir(mock_pkg_git_repo):
+        mkdirp('pkg-e')
+        with open('pkg-e/package.py', 'w') as f:
+            f.write(pkg_template.format(name='PkgE'))
+
+    pkg('add', 'pkg-e')
+
+    git = which('git', required=True)
+    with working_dir(mock_pkg_git_repo):
+        try:
+            assert ('A  pkg-e/package.py' in
+                    git('status', '--short', output=str))
+        finally:
+            shutil.rmtree('pkg-e')
+
+    with pytest.raises(spack.main.SpackCommandError):
+        pkg('add', 'does-not-exist')
+
+
+def test_pkg_list(mock_pkg_git_repo, mock_pkg_names):
+    out = split(pkg('list', 'HEAD^^'))
+    assert sorted(mock_pkg_names) == sorted(out)
+
+    out = split(pkg('list', 'HEAD^'))
+    assert sorted(
+        mock_pkg_names.union(['pkg-a', 'pkg-b', 'pkg-c'])) == sorted(out)
+
+    out = split(pkg('list', 'HEAD'))
+    assert sorted(
+        mock_pkg_names.union(['pkg-a', 'pkg-b', 'pkg-d'])) == sorted(out)
+
+    # test with three dots to make sure pkg calls `git merge-base`
+    out = split(pkg('list', 'HEAD^^...'))
+    assert sorted(mock_pkg_names) == sorted(out)
+
+
+def test_pkg_diff(mock_pkg_git_repo, mock_pkg_names):
+    out = split(pkg('diff', 'HEAD^^', 'HEAD^'))
+    assert out == ['HEAD^:', 'pkg-a', 'pkg-b', 'pkg-c']
+
+    out = split(pkg('diff', 'HEAD^^', 'HEAD'))
+    assert out == ['HEAD:', 'pkg-a', 'pkg-b', 'pkg-d']
+
+    out = split(pkg('diff', 'HEAD^', 'HEAD'))
+    assert out == ['HEAD^:', 'pkg-c', 'HEAD:', 'pkg-d']
+
+
+def test_pkg_added(mock_pkg_git_repo):
+    out = split(pkg('added', 'HEAD^^', 'HEAD^'))
+    assert out == ['pkg-a', 'pkg-b', 'pkg-c']
+
+    out = split(pkg('added', 'HEAD^^', 'HEAD'))
+    assert out == ['pkg-a', 'pkg-b', 'pkg-d']
+
+    out = split(pkg('added', 'HEAD^', 'HEAD'))
+    assert out == ['pkg-d']
+
+    out = split(pkg('added', 'HEAD', 'HEAD'))
+    assert out == []
+
+
+def test_pkg_removed(mock_pkg_git_repo):
+    out = split(pkg('removed', 'HEAD^^', 'HEAD^'))
+    assert out == []
+
+    out = split(pkg('removed', 'HEAD^^', 'HEAD'))
+    assert out == []
+
+    out = split(pkg('removed', 'HEAD^', 'HEAD'))
+    assert out == ['pkg-c']
+
+
+def test_pkg_changed(mock_pkg_git_repo):
+    out = split(pkg('changed', 'HEAD^^', 'HEAD^'))
+    assert out == []
+
+    out = split(pkg('changed', '--type', 'c', 'HEAD^^', 'HEAD^'))
+    assert out == []
+
+    out = split(pkg('changed', '--type', 'a', 'HEAD^^', 'HEAD^'))
+    assert out == ['pkg-a', 'pkg-b', 'pkg-c']
+
+    out = split(pkg('changed', '--type', 'r', 'HEAD^^', 'HEAD^'))
+    assert out == []
+
+    out = split(pkg('changed', '--type', 'ar', 'HEAD^^', 'HEAD^'))
+    assert out == ['pkg-a', 'pkg-b', 'pkg-c']
+
+    out = split(pkg('changed', '--type', 'arc', 'HEAD^^', 'HEAD^'))
+    assert out == ['pkg-a', 'pkg-b', 'pkg-c']
+
+    out = split(pkg('changed', 'HEAD^', 'HEAD'))
+    assert out == ['pkg-b']
+
+    out = split(pkg('changed', '--type', 'c', 'HEAD^', 'HEAD'))
+    assert out == ['pkg-b']
+
+    out = split(pkg('changed', '--type', 'a', 'HEAD^', 'HEAD'))
+    assert out == ['pkg-d']
+
+    out = split(pkg('changed', '--type', 'r', 'HEAD^', 'HEAD'))
+    assert out == ['pkg-c']
+
+    out = split(pkg('changed', '--type', 'ar', 'HEAD^', 'HEAD'))
+    assert out == ['pkg-c', 'pkg-d']
+
+    out = split(pkg('changed', '--type', 'arc', 'HEAD^', 'HEAD'))
+    assert out == ['pkg-b', 'pkg-c', 'pkg-d']
+
+    # invalid type argument
+    with pytest.raises(spack.main.SpackCommandError):
+        pkg('changed', '--type', 'foo')
+
+
+def test_pkg_fails_when_not_git_repo(monkeypatch):
+    monkeypatch.setattr(spack.cmd, 'spack_is_git_repo', lambda: False)
+    with pytest.raises(spack.main.SpackCommandError):
+        pkg('added')