Clean up tests and add Python3 to Travis.

- Clean up spec_syntax tests: don't dependend on DB order.
- spec_syntax hash parsing tests were strongly dependent on the order the
  DB was traversed.
- Tests now specifically grab the specs they want from the mock DB.
- Tests are more readable as a result.

- Add Python3 versions to Travis tests.

3 files changed, 172 insertions, 90 deletions

diff --git a/.travis.yml b/.travis.yml
index 4cd3b14b9c..d7bdf9b2ca 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -22,6 +22,22 @@ matrix:
       os: linux
       language: python
       env: TEST_SUITE=unit
+    - python: '3.3'
+      os: linux
+      language: python
+      env: TEST_SUITE=unit
+    - python: '3.4'
+      os: linux
+      language: python
+      env: TEST_SUITE=unit
+    - python: '3.5'
+      os: linux
+      language: python
+      env: TEST_SUITE=unit
+    - python: '3.6'
+      os: linux
+      language: python
+      env: TEST_SUITE=unit
     - python: '2.7'
       os: linux
       language: python
@@ -45,6 +61,7 @@ addons:
   apt:
     packages:
       - gfortran
+      - mercurial
       - graphviz
 
 # Work around Travis's lack of support for Python on OSX
@@ -60,7 +77,6 @@ install:
   - pip install --upgrade codecov
   - pip install --upgrade flake8
   - pip install --upgrade sphinx
-  - pip install --upgrade mercurial
 
 before_script:
   # Need this for the git tests to succeed.
diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py
index fa88698ea9..095b04f837 100644
--- a/lib/spack/spack/spec.py
+++ b/lib/spack/spack/spec.py
@@ -106,7 +106,6 @@ from six import StringIO
 from six import string_types
 from six import iteritems
 
-import llnl.util.tty as tty
 import spack
 import spack.architecture
 import spack.compilers as compilers
@@ -159,6 +158,7 @@ __all__ = [
     'UnsatisfiableDependencySpecError',
     'AmbiguousHashError',
     'InvalidHashError',
+    'NoSuchHashError',
     'RedundantSpecError']
 
 # Valid pattern for an identifier in Spack
@@ -2952,8 +2952,7 @@ class SpecParser(spack.parse.Parser):
                    spec.dag_hash()[:len(self.token.value)] == self.token.value]
 
         if not matches:
-            tty.die("%s does not match any installed packages." %
-                    self.token.value)
+            raise NoSuchHashError(self.token.value)
 
         if len(matches) != 1:
             raise AmbiguousHashError(
@@ -3325,6 +3324,12 @@ class InvalidHashError(SpecError):
             % (hash, spec))
 
 
+class NoSuchHashError(SpecError):
+    def __init__(self, hash):
+        super(NoSuchHashError, self).__init__(
+            "No installed spec matches the hash: '%s'")
+
+
 class RedundantSpecError(SpecError):
     def __init__(self, spec, addition):
         super(RedundantSpecError, self).__init__(
diff --git a/lib/spack/spack/test/spec_syntax.py b/lib/spack/spack/test/spec_syntax.py
index fcb6cfa907..dfad4a019f 100644
--- a/lib/spack/spack/test/spec_syntax.py
+++ b/lib/spack/spack/test/spec_syntax.py
@@ -122,7 +122,7 @@ class TestSpecSyntax(object):
     def _check_raises(self, exc_type, items):
         for item in items:
             with pytest.raises(exc_type):
-                self.check_parse(item)
+                Spec(item)
 
     # ========================================================================
     # Parse checks
@@ -225,113 +225,174 @@ class TestSpecSyntax(object):
         errors = ['x@@1.2', 'x ^y@@1.2', 'x@1.2::', 'x::']
         self._check_raises(SpecParseError, errors)
 
+    def _check_hash_parse(self, spec):
+        """Check several ways to specify a spec by hash."""
+        # full hash
+        self.check_parse(str(spec), '/' + spec.dag_hash())
+
+        # partial hash
+        self.check_parse(str(spec), '/ ' + spec.dag_hash()[:5])
+
+        # name + hash
+        self.check_parse(str(spec), spec.name + '/' + spec.dag_hash())
+
+        # name + version + space + partial hash
+        self.check_parse(
+            str(spec), spec.name + '@' + str(spec.version) +
+            ' /' + spec.dag_hash()[:6])
+
     def test_spec_by_hash(self, database):
         specs = database.mock.db.query()
-        hashes = [s._hash for s in specs]  # Preserves order of elements
-
-        # Make sure the database is still the shape we expect
-        assert len(specs) > 3
+        assert len(specs)  # make sure something's in the DB
 
-        self.check_parse(str(specs[0]), '/' + hashes[0])
-        self.check_parse(str(specs[1]), '/ ' + hashes[1][:5])
-        self.check_parse(str(specs[2]), specs[2].name + '/' + hashes[2])
-        self.check_parse(str(specs[3]),
-                         specs[3].name + '@' + str(specs[3].version) +
-                         ' /' + hashes[3][:6])
+        for spec in specs:
+            self._check_hash_parse(spec)
 
     def test_dep_spec_by_hash(self, database):
-        specs = database.mock.db.query()
-        hashes = [s._hash for s in specs]  # Preserves order of elements
-
-        # Make sure the database is still the shape we expect
-        assert len(specs) > 10
-        assert specs[4].name in specs[10]
-        assert specs[-1].name in specs[10]
-
-        spec1 = sp.Spec(specs[10].name + '^/' + hashes[4])
-        assert specs[4].name in spec1 and spec1[specs[4].name] == specs[4]
-        spec2 = sp.Spec(specs[10].name + '%' + str(specs[10].compiler) +
-                        ' ^ / ' + hashes[-1])
-        assert (specs[-1].name in spec2 and
-                spec2[specs[-1].name] == specs[-1] and
-                spec2.compiler == specs[10].compiler)
-        spec3 = sp.Spec(specs[10].name + '^/' + hashes[4][:4] +
-                        '^ / ' + hashes[-1][:5])
-        assert (specs[-1].name in spec3 and
-                spec3[specs[-1].name] == specs[-1] and
-                specs[4].name in spec3 and spec3[specs[4].name] == specs[4])
+        mpileaks_zmpi = database.mock.db.query_one('mpileaks ^zmpi')
+        zmpi = database.mock.db.query_one('zmpi')
+        fake = database.mock.db.query_one('fake')
 
-    def test_multiple_specs_with_hash(self, database):
-        specs = database.mock.db.query()
-        hashes = [s._hash for s in specs]  # Preserves order of elements
-
-        assert len(specs) > 3
-
-        output = sp.parse(specs[0].name + '/' + hashes[0] + '/' + hashes[1])
-        assert len(output) == 2
-        output = sp.parse('/' + hashes[0] + '/' + hashes[1])
-        assert len(output) == 2
-        output = sp.parse('/' + hashes[0] + '/' + hashes[1] +
-                          ' ' + specs[2].name)
-        assert len(output) == 3
-        output = sp.parse('/' + hashes[0] +
-                          ' ' + specs[1].name + ' ' + specs[2].name)
-        assert len(output) == 3
-        output = sp.parse('/' + hashes[0] + ' ' +
-                          specs[1].name + ' / ' + hashes[1])
-        assert len(output) == 2
+        assert 'fake' in mpileaks_zmpi
+        assert 'zmpi' in mpileaks_zmpi
 
-    def test_ambiguous_hash(self, database):
-        specs = database.mock.db.query()
-        hashes = [s._hash for s in specs]  # Preserves order of elements
+        mpileaks_hash_fake = sp.Spec('mpileaks ^/' + fake.dag_hash())
+        assert 'fake' in mpileaks_hash_fake
+        assert mpileaks_hash_fake['fake'] == fake
+
+        mpileaks_hash_zmpi = sp.Spec(
+            'mpileaks %' + str(mpileaks_zmpi.compiler) +
+            ' ^ / ' + zmpi.dag_hash())
+        assert 'zmpi' in mpileaks_hash_zmpi
+        assert mpileaks_hash_zmpi['zmpi'] == zmpi
+        assert mpileaks_hash_zmpi.compiler == mpileaks_zmpi.compiler
+
+        mpileaks_hash_fake_and_zmpi = sp.Spec(
+            'mpileaks ^/' + fake.dag_hash()[:4] + '^ / ' + zmpi.dag_hash()[:5])
+        assert 'zmpi' in mpileaks_hash_fake_and_zmpi
+        assert mpileaks_hash_fake_and_zmpi['zmpi'] == zmpi
 
-        # Make sure the database is as expected
-        assert hashes[1][:1] == hashes[2][:1] == 'b'
+        assert 'fake' in mpileaks_hash_fake_and_zmpi
+        assert mpileaks_hash_fake_and_zmpi['fake'] == fake
 
-        ambiguous_hashes = ['/b',
-                            specs[1].name + '/b',
-                            specs[0].name + '^/b',
-                            specs[0].name + '^' + specs[1].name + '/b']
-        self._check_raises(AmbiguousHashError, ambiguous_hashes)
+    def test_multiple_specs_with_hash(self, database):
+        mpileaks_zmpi = database.mock.db.query_one('mpileaks ^zmpi')
+        callpath_mpich2 = database.mock.db.query_one('callpath ^mpich2')
+
+        # name + hash + separate hash
+        specs = sp.parse('mpileaks /' + mpileaks_zmpi.dag_hash() +
+                         '/' + callpath_mpich2.dag_hash())
+        assert len(specs) == 2
+
+        # 2 separate hashes
+        specs = sp.parse('/' + mpileaks_zmpi.dag_hash() +
+                         '/' + callpath_mpich2.dag_hash())
+        assert len(specs) == 2
+
+        # 2 separate hashes + name
+        specs = sp.parse('/' + mpileaks_zmpi.dag_hash() +
+                         '/' + callpath_mpich2.dag_hash() +
+                         ' callpath')
+        assert len(specs) == 3
+
+        # hash + 2 names
+        specs = sp.parse('/' + mpileaks_zmpi.dag_hash() +
+                         ' callpath' +
+                         ' callpath')
+        assert len(specs) == 3
+
+        # hash + name + hash
+        specs = sp.parse('/' + mpileaks_zmpi.dag_hash() +
+                         ' callpath' +
+                         ' / ' + callpath_mpich2.dag_hash())
+        assert len(specs) == 2
+
+    def test_ambiguous_hash(self, database):
+        dbspecs = database.mock.db.query()
+
+        def find_ambiguous(specs, keyfun):
+            """Return the first set of specs that's ambiguous under a
+               particular key function."""
+            key_to_spec = {}
+            for spec in specs:
+                key = keyfun(spec)
+                speclist = key_to_spec.setdefault(key, [])
+                speclist.append(spec)
+                if len(speclist) > 1:
+                    return (key, speclist)
+
+            # If we fail here, we may need to guarantee that there are
+            # some ambiguos specs by adding more specs to the test DB
+            # until this succeeds.
+            raise RuntimeError("no ambiguous specs found for keyfun!")
+
+        # ambiguity in first hash character
+        char, specs = find_ambiguous(dbspecs, lambda s: s.dag_hash()[0])
+        self._check_raises(AmbiguousHashError, ['/' + char])
+
+        # ambiguity in first hash character AND spec name
+        t, specs = find_ambiguous(dbspecs,
+                                  lambda s: (s.name, s.dag_hash()[0]))
+        name, char = t
+        self._check_raises(AmbiguousHashError, [name + '/' + char])
 
     def test_invalid_hash(self, database):
-        specs = database.mock.db.query()
-        hashes = [s._hash for s in specs]  # Preserves order of elements
+        mpileaks_zmpi = database.mock.db.query_one('mpileaks ^zmpi')
+        zmpi = database.mock.db.query_one('zmpi')
 
-        # Make sure the database is as expected
-        assert (hashes[0] != hashes[3] and
-                hashes[1] != hashes[4] and len(specs) > 4)
+        mpileaks_mpich = database.mock.db.query_one('mpileaks ^mpich')
+        mpich = database.mock.db.query_one('mpich')
 
-        inputs = [specs[0].name + '/' + hashes[3],
-                  specs[1].name + '^' + specs[4].name + '/' + hashes[0],
-                  specs[1].name + '^' + specs[4].name + '/' + hashes[1]]
-        self._check_raises(InvalidHashError, inputs)
+        # name + incompatible hash
+        self._check_raises(InvalidHashError, [
+            'zmpi /' + mpich.dag_hash(),
+            'mpich /' + zmpi.dag_hash()])
+
+        # name + dep + incompatible hash
+        self._check_raises(InvalidHashError, [
+            'mpileaks ^mpich /' + mpileaks_zmpi.dag_hash(),
+            'mpileaks ^zmpi /' + mpileaks_mpich.dag_hash()])
 
     def test_nonexistent_hash(self, database):
-        # This test uses database to make sure we don't accidentally access
-        # real installs, however unlikely
+        """Ensure we get errors for nonexistant hashes."""
         specs = database.mock.db.query()
-        hashes = [s._hash for s in specs]  # Preserves order of elements
 
-        # Make sure the database is as expected
-        assert 'abc123' not in [h[:6] for h in hashes]
+        # This hash shouldn't be in the test DB.  What are the odds :)
+        no_such_hash = 'aaaaaaaaaaaaaaa'
+        hashes = [s._hash for s in specs]
+        assert no_such_hash not in [h[:len(no_such_hash)] for h in hashes]
 
-        nonexistant_hashes = ['/abc123',
-                              specs[0].name + '/abc123']
-        self._check_raises(SystemExit, nonexistant_hashes)
+        self._check_raises(NoSuchHashError, [
+            '/' + no_such_hash,
+            'mpileaks /' + no_such_hash])
 
     def test_redundant_spec(self, database):
-        specs = database.mock.db.query()
-        hashes = [s._hash for s in specs]  # Preserves order of elements
+        """Check that redundant spec constraints raise errors.
+
+        TODO (TG): does this need to be an error? Or should concrete
+        specs only raise errors if constraints cause a contradiction?
+
+        """
+        mpileaks_zmpi = database.mock.db.query_one('mpileaks ^zmpi')
+        callpath_zmpi = database.mock.db.query_one('callpath ^zmpi')
+        dyninst = database.mock.db.query_one('dyninst')
+
+        mpileaks_mpich2 = database.mock.db.query_one('mpileaks ^mpich2')
+
+        redundant_specs = [
+            # redudant compiler
+            '/' + mpileaks_zmpi.dag_hash() + '%' + str(mpileaks_zmpi.compiler),
+
+            # redudant version
+            'mpileaks/' + mpileaks_mpich2.dag_hash() +
+            '@' + str(mpileaks_mpich2.version),
+
+            # redundant dependency
+            'callpath /' + callpath_zmpi.dag_hash() + '^ libelf',
 
-        # Make sure the database is as expected
-        assert len(specs) > 3
+            # redundant flags
+            '/' + dyninst.dag_hash() + ' cflags="-O3 -fPIC"']
 
-        redundant_specs = ['/' + hashes[0] + '%' + str(specs[0].compiler),
-                           specs[1].name + '/' + hashes[1] +
-                           '@' + str(specs[1].version),
-                           specs[2].name + '/' + hashes[2] + '^ libelf',
-                           '/' + hashes[3] + ' cflags="-O3 -fPIC"']
         self._check_raises(RedundantSpecError, redundant_specs)
 
     def test_duplicate_variant(self):