Add a unit test for concrete specs that do not satisfy abstract specs

alalazo · alalazo · commit 3466bd29d5df · 2023-03-04T00:09:51.000+01:00
diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py
@@ -3417,7 +3417,7 @@ def constrain(self, other, deps=True):
         if deps:
             changed |= self._constrain_dependencies(other)
 
-        if other.concrete and not self.concrete:
+        if other.concrete and not self.concrete and other.satisfies(self):
             self._finalize_concretization()
 
         return changed
diff --git a/lib/spack/spack/test/spec_semantics.py b/lib/spack/spack/test/spec_semantics.py
@@ -21,26 +21,9 @@
     InvalidVariantValueError,
     MultipleValuesInExclusiveVariantError,
     UnknownVariantError,
-    substitute_abstract_variants,
 )
 
 
-def make_spec(spec_like, concrete):
-    spec = Spec(spec_like)
-    if concrete:
-        spec._mark_concrete()
-        substitute_abstract_variants(spec)
-    return spec
-
-
-def check_cannot_constrain(target_spec, constraint_spec):
-    target = make_spec(target_spec, True)
-    constraint = Spec(constraint_spec)
-
-    with pytest.raises(UnsatisfiableSpecError):
-        constraint.copy().constrain(target)
-
-
 def check_constrain(expected, spec, constraint):
     exp = Spec(expected)
     spec = Spec(spec)
@@ -302,6 +285,41 @@ def test_constraining_abstract_specs_with_empty_intersection(self, lhs, rhs):
         with pytest.raises(UnsatisfiableSpecError):
             rhs.constrain(lhs)
 
+    @pytest.mark.parametrize(
+        "lhs,rhs",
+        [
+            ("mpich", "mpich +foo"),
+            ("mpich", "mpich~foo"),
+            ("mpich", "mpich foo=1"),
+            ("mpich", "mpich++foo"),
+            ("mpich", "mpich~~foo"),
+            ("mpich", "mpich foo==1"),
+            # Flags semantics is currently different from other variant
+            pytest.param("mpich", 'mpich cppflags="-O3"', marks=pytest.mark.xfail),
+            pytest.param(
+                "multivalue-variant foo=bar", "multivalue-variant +foo", marks=pytest.mark.xfail
+            ),
+            pytest.param(
+                "multivalue-variant foo=bar", "multivalue-variant ~foo", marks=pytest.mark.xfail
+            ),
+        ],
+    )
+    def test_concrete_specs_which_do_not_satisfy_abstract(
+        self, lhs, rhs, default_mock_concretization
+    ):
+        lhs, rhs = default_mock_concretization(lhs), Spec(rhs)
+
+        assert lhs.intersects(rhs)
+        assert rhs.intersects(lhs)
+        assert not lhs.satisfies(rhs)
+        assert not rhs.satisfies(lhs)
+
+        with pytest.raises(UnsatisfiableSpecError):
+            assert lhs.constrain(rhs)
+
+        with pytest.raises(UnsatisfiableSpecError):
+            assert rhs.constrain(lhs)
+
     def test_satisfies_single_valued_variant(self):
         """Tests that the case reported in
         https://github.com/spack/spack/pull/2386#issuecomment-282147639
@@ -401,36 +419,6 @@ def test_unsatisfiable_multi_value_variant(self, default_mock_concretization):
         with pytest.raises(MultipleValuesInExclusiveVariantError):
             a.concretize()
 
-    def test_unsatisfiable_variant_types(self):
-        # These should fail due to incompatible types
-
-        # FIXME: these needs to be checked as the new relaxed
-        # FIXME: semantic makes them fail (constrain does not raise)
-        # check_unsatisfiable('multivalue-variant +foo',
-        #                     'multivalue-variant foo="bar"')
-        # check_unsatisfiable('multivalue-variant ~foo',
-        #                     'multivalue-variant foo="bar"')
-
-        check_cannot_constrain(
-            target_spec='multivalue-variant foo="bar"', constraint_spec="multivalue-variant +foo"
-        )
-
-        check_cannot_constrain(
-            target_spec='multivalue-variant foo="bar"', constraint_spec="multivalue-variant ~foo"
-        )
-
-    def test_unsatisfiable_variants(self):
-        # 'mpich' is concrete:
-        check_cannot_constrain("mpich", "mpich+foo")
-        check_cannot_constrain("mpich", "mpich~foo")
-        check_cannot_constrain("mpich", "mpich foo=1")
-        check_cannot_constrain("mpich", "mpich++foo")
-        check_cannot_constrain("mpich", "mpich~~foo")
-        check_cannot_constrain("mpich", "mpich foo==1")
-
-    def test_unsatisfiable_compiler_flag(self):
-        check_cannot_constrain("mpich", 'mpich cppflags="-O3"')
-
     def test_copy_satisfies_transitive(self):
         spec = Spec("dttop")
         spec.concretize()
