Improve tests and logic for solve failure handling

bestiary_test.go

@@ -114,6 +114,8 @@ type fixture struct {
 	downgrade bool
 	// lock file simulator, if one's to be used at all
 	l Lock
+	// projects expected to have errors, if any
+	errp []string
 }
 
 // mklock makes a fixLock, suitable to act as a lock file
@@ -354,6 +356,15 @@ var fixtures = []fixture{
 			"foo 1.0.0",
 		),
 	},
+	{
+		n: "no version that matches requirement",
+		ds: []depspec{
+			dsv("root 0.0.0", "foo >=1.0.0, <2.0.0"),
+			dsv("foo 2.0.0"),
+			dsv("foo 2.1.3"),
+		},
+		errp: []string{"foo", "root"},
+	},
 }
 
 type depspecSourceManager struct {
@@ -552,14 +563,6 @@ func rootDependency() {
 }
 
 func unsolvable() {
-  testResolve("no version that matches requirement", {
-    "myapp 0.0.0": {
-      "foo": ">=1.0.0 <2.0.0"
-    },
-    "foo 2.0.0": {},
-    "foo 2.1.3": {}
-  }, error: noVersion(["myapp", "foo"]));
-
   testResolve("no version that matches combined constraint", {
     "myapp 0.0.0": {
       "foo": "1.0.0",

errors.go

@@ -1,5 +1,10 @@
 package vsolver
 
+import (
+	"bytes"
+	"fmt"
+)
+
 type errorLevel uint8
 
 // TODO consistent, sensible way of handling 'type' and 'severity' - or figure
@@ -30,23 +35,92 @@ func (e *solveError) Error() string {
 }
 
 type noVersionError struct {
-	pn   ProjectName
+	pn    ProjectName
-	v    string
+	fails []failedVersion
-	c    Constraint
-	deps []Dependency
 }
 
 func (e *noVersionError) Error() string {
-	// TODO compose a message out of the data we have
+	if len(e.fails) == 0 {
-	return ""
+		return fmt.Sprintf("No versions could be found for project %q.", e.pn)
+	}
+
+	var buf bytes.Buffer
+	fmt.Fprintf(&buf, "Could not find any versions of %s that met constraints:\n", e.pn)
+	for _, f := range e.fails {
+		fmt.Fprintf(&buf, "\t%s: %s", f.v.Info, f.f.Error())
+	}
+
+	return buf.String()
 }
 
 type disjointConstraintFailure struct {
-	pn   ProjectName
+	goal      Dependency
-	deps []Dependency
+	failsib   []Dependency
+	nofailsib []Dependency
+	c         Constraint
 }
 
 func (e *disjointConstraintFailure) Error() string {
-	// TODO compose a message out of the data we have
+	if len(e.failsib) == 1 {
-	return ""
+		str := "Could not introduce %s at %s, as it has a dependency on %s with constraint %s, which has no overlap with existing constraint %s from %s at %s"
+		return fmt.Sprintf(str, e.goal.Depender.Name, e.goal.Depender.Version.Info, e.goal.Dep.Name, e.goal.Dep.Constraint.Body(), e.failsib[0].Dep.Constraint.Body(), e.failsib[0].Depender.Name, e.failsib[0].Depender.Version.Info)
+	}
+
+	var buf bytes.Buffer
+
+	var sibs []Dependency
+	if len(e.failsib) > 1 {
+		sibs = e.failsib
+
+		str := "Could not introduce %s at %s, as it has a dependency on %s with constraint %s, which has no overlap with the following existing constraints:\n"
+		fmt.Fprintf(&buf, str, e.goal.Depender.Name, e.goal.Depender.Version.Info, e.goal.Dep.Name, e.goal.Dep.Constraint.Body())
+	} else {
+		sibs = e.nofailsib
+
+		str := "Could not introduce %s at %s, as it has a dependency on %s with constraint %s, which does not overlap with the intersection of existing constraints from other currently selected packages:\n"
+		fmt.Fprintf(&buf, str, e.goal.Depender.Name, e.goal.Depender.Version.Info, e.goal.Dep.Name, e.goal.Dep.Constraint.Body())
+	}
+
+	for _, c := range sibs {
+		fmt.Fprintf(&buf, "\t%s at %s with constraint %s\n", c.Depender.Name, c.Depender.Version.Info, c.Dep.Constraint.Body())
+	}
+
+	return buf.String()
+}
+
+// Indicates that an atom could not be introduced because one of its dep
+// constraints does not admit the currently-selected version of the target
+// project.
+type constraintNotAllowedFailure struct {
+	goal Dependency
+	v    Version
+}
+
+func (e *constraintNotAllowedFailure) Error() string {
+	str := "Could not introduce %s at %s, as it has a dependency on %s with constraint %s, which does not allow the currently selected version of %s"
+	return fmt.Sprintf(str, e.goal.Depender.Name, e.goal.Depender.Version.Info, e.goal.Dep.Name, e.goal.Dep.Constraint, e.v.Info)
+}
+
+type versionNotAllowedFailure struct {
+	goal       ProjectAtom
+	failparent []Dependency
+	c          Constraint
+}
+
+func (e *versionNotAllowedFailure) Error() string {
+	if len(e.failparent) == 1 {
+		str := "Could not introduce %s at %s, as it is not allowed by constraint %s from project %s."
+		return fmt.Sprintf(str, e.goal.Name, e.goal.Version.Info, e.failparent[0].Dep.Constraint.Body(), e.failparent[0].Depender.Name)
+	}
+
+	var buf bytes.Buffer
+
+	str := "Could not introduce %s at %s, as it is not allowed by constraints from the following projects:\n"
+	fmt.Fprintf(&buf, str, e.goal.Name, e.goal.Version.Info)
+
+	for _, f := range e.failparent {
+		fmt.Fprintf(&buf, "\t%s at %s with constraint %s\n", f.Depender.Name, f.Depender.Version.Info, f.Dep.Constraint.Body())
+	}
+
+	return buf.String()
 }

solve_test.go

@@ -1,6 +1,7 @@
 package vsolver
 
 import (
+	"strings"
 	"testing"
 
 	"github.com/Sirupsen/logrus"
@@ -36,49 +37,121 @@ func solveAndBasicChecks(fix fixture, t *testing.T) Result {
 	}
 	result := s.Solve(p, nil)
 
-	if result.SolveFailure != nil {
+	if len(fix.errp) > 0 {
-		t.Errorf("(fixture: %q) Solver failed; error was type %T, text: %q", fix.n, result.SolveFailure, result.SolveFailure)
+		if result.SolveFailure == nil {
-		return result
+			t.Errorf("(fixture: %q) Solver succeeded, but expected failure")
-	}
+		}
 
-	if fix.maxAttempts > 0 && result.Attempts > fix.maxAttempts {
+		switch fail := result.SolveFailure.(type) {
-		t.Errorf("(fixture: %q) Solver completed in %v attempts, but expected %v or fewer", result.Attempts, fix.maxAttempts)
+		case *noVersionError:
-	}
+			if fix.errp[0] != string(fail.pn) {
+				t.Errorf("Expected failure on project %s, but was on project %s", fail.pn, fix.errp[0])
+			}
 
-	// Dump result projects into a map for easier interrogation
+			ep := make(map[string]struct{})
-	rp := make(map[string]string)
+			for _, p := range fix.errp[1:] {
-	for _, p := range result.Projects {
+				ep[p] = struct{}{}
-		rp[string(p.Name)] = p.Version.Info
+			}
-	}
 
-	fixlen, rlen := len(fix.r), len(rp)
+			found := make(map[string]struct{})
-	if fixlen != rlen {
+			for _, vf := range fail.fails {
-		// Different length, so they definitely disagree
+				for _, f := range getFailureCausingProjects(vf.f) {
-		t.Errorf("(fixture: %q) Solver reported %v package results, result expected %v", fix.n, rlen, fixlen)
+					found[f] = struct{}{}
-	}
+				}
+			}
 
-	// Whether or not len is same, still have to verify that results agree
+			var missing []string
-	// Walk through fixture/expected results first
+			var extra []string
-	for p, v := range fix.r {
+			for p, _ := range found {
-		if av, exists := rp[p]; !exists {
+				if _, has := ep[p]; !has {
-			t.Errorf("(fixture: %q) Project %q expected but missing from results", fix.n, p)
+					extra = append(extra, p)
-		} else {
+				}
-			// delete result from map so we skip it on the reverse pass
+			}
-			delete(rp, p)
+			if len(extra) > 0 {
-			if v != av {
+				t.Errorf("Expected solve failures due to projects %s, but solve failures also arose from %s", strings.Join(fix.errp[1:], ", "), strings.Join(extra, ", "))
-				t.Errorf("(fixture: %q) Expected version %q of project %q, but actual version was %q", fix.n, v, p, av)
+			}
+
+			for p, _ := range ep {
+				if _, has := found[p]; !has {
+					missing = append(missing, p)
+				}
+			}
+			if len(missing) > 0 {
+				t.Errorf("Expected solve failures due to projects %s, but %s had no failures", strings.Join(fix.errp[1:], ", "), strings.Join(missing, ", "))
+			}
+
+		default:
+			// TODO round these out
+			panic("unhandled solve failure type")
+		}
+	} else {
+		if result.SolveFailure != nil {
+			t.Errorf("(fixture: %q) Solver failed; error was type %T, text: %q", fix.n, result.SolveFailure, result.SolveFailure)
+			return result
+		}
+
+		if fix.maxAttempts > 0 && result.Attempts > fix.maxAttempts {
+			t.Errorf("(fixture: %q) Solver completed in %v attempts, but expected %v or fewer", result.Attempts, fix.maxAttempts)
+		}
+
+		// Dump result projects into a map for easier interrogation
+		rp := make(map[string]string)
+		for _, p := range result.Projects {
+			rp[string(p.Name)] = p.Version.Info
+		}
+
+		fixlen, rlen := len(fix.r), len(rp)
+		if fixlen != rlen {
+			// Different length, so they definitely disagree
+			t.Errorf("(fixture: %q) Solver reported %v package results, result expected %v", fix.n, rlen, fixlen)
+		}
+
+		// Whether or not len is same, still have to verify that results agree
+		// Walk through fixture/expected results first
+		for p, v := range fix.r {
+			if av, exists := rp[p]; !exists {
+				t.Errorf("(fixture: %q) Project %q expected but missing from results", fix.n, p)
+			} else {
+				// delete result from map so we skip it on the reverse pass
+				delete(rp, p)
+				if v != av {
+					t.Errorf("(fixture: %q) Expected version %q of project %q, but actual version was %q", fix.n, v, p, av)
+				}
+			}
+		}
+
+		// Now walk through remaining actual results
+		for p, v := range rp {
+			if fv, exists := fix.r[p]; !exists {
+				t.Errorf("(fixture: %q) Unexpected project %q present in results", fix.n, p)
+			} else if v != fv {
+				t.Errorf("(fixture: %q) Got version %q of project %q, but expected version was %q", fix.n, v, p, fv)
 			}
 		}
 	}
 
-	// Now walk through remaining actual results
+	return result
-	for p, v := range rp {
+
-		if fv, exists := fix.r[p]; !exists {
+}
-			t.Errorf("(fixture: %q) Unexpected project %q present in results", fix.n, p)
+
-		} else if v != fv {
+func getFailureCausingProjects(err error) (projs []string) {
-			t.Errorf("(fixture: %q) Got version %q of project %q, but expected version was %q", fix.n, v, p, fv)
+	switch e := err.(type) {
+	case *noVersionError:
+		projs = append(projs, string(e.pn))
+	case *disjointConstraintFailure:
+		for _, f := range e.failsib {
+			projs = append(projs, string(f.Depender.Name))
 		}
+	case *versionNotAllowedFailure:
+		for _, f := range e.failparent {
+			projs = append(projs, string(f.Depender.Name))
+		}
+	case *constraintNotAllowedFailure:
+		// No sane way of knowing why the currently selected version is
+		// selected, so do nothing
+	default:
+		panic("unknown failtype")
 	}
 
-	return result
+	return
 }

solver.go

@@ -7,14 +7,6 @@ import (
 	"github.com/Sirupsen/logrus"
 )
 
-//type SolveFailure uint
-
-//const (
-// Indicates that no version solution could be found
-//NoVersionSolution SolveFailure = 1 << iota
-//IncompatibleVersionType
-//)
-
 func NewSolver(sm SourceManager, l *logrus.Logger) Solver {
 	if l == nil {
 		l = logrus.New()
@@ -169,15 +161,16 @@ func (s *solver) createVersionQueue(ref ProjectName) (*versionQueue, error) {
 	return q, s.findValidVersion(q)
 }
 
-// findValidVersion walks through a versionQueue until it finds a version that's
+// findValidVersion walks through a versionQueue until it finds a version that
-// valid, as adjudged by the current constraints.
+// satisfies the constraints held in the current state of the solver.
 func (s *solver) findValidVersion(q *versionQueue) error {
-	var err error
 	if emptyVersion == q.current() {
 		// TODO this case shouldn't be reachable, but panic here as a canary
 		panic("version queue is empty, should not happen")
 	}
 
+	faillen := len(q.fails)
+
 	if s.l.Level >= logrus.DebugLevel {
 		s.l.WithFields(logrus.Fields{
 			"name":      q.ref,
@@ -185,25 +178,24 @@ func (s *solver) findValidVersion(q *versionQueue) error {
 			"allLoaded": q.allLoaded,
 		}).Debug("Beginning search through versionQueue for a valid version")
 	}
-
 	for {
-		err = s.satisfiable(ProjectAtom{
+		cur := q.current()
+		err := s.satisfiable(ProjectAtom{
 			Name:    q.ref,
-			Version: q.current(),
+			Version: cur,
 		})
 		if err == nil {
 			// we have a good version, can return safely
 			if s.l.Level >= logrus.DebugLevel {
 				s.l.WithFields(logrus.Fields{
 					"name":    q.ref,
-					"version": q.current().Info,
+					"version": cur.Info,
 				}).Debug("Found acceptable version, returning out")
 			}
 			return nil
 		}
 
-		err = q.advance()
+		if q.advance(err) != nil {
-		if err != nil {
 			// Error on advance, have to bail out
 			if s.l.Level >= logrus.WarnLevel {
 				s.l.WithFields(logrus.Fields{
@@ -215,19 +207,26 @@ func (s *solver) findValidVersion(q *versionQueue) error {
 		}
 		if q.isExhausted() {
 			// Queue is empty, bail with error
-			err = newSolveError(fmt.Sprintf("Exhausted queue for %q without finding a satisfactory version.", q.ref), mustResolve)
 			if s.l.Level >= logrus.InfoLevel {
-				s.l.WithFields(logrus.Fields{
+				s.l.WithField("name", q.ref).Info("Version queue was completely exhausted, marking project as failed")
-					"name": q.ref,
-					"err":  err,
-				}).Info("Version queue was completely exhausted, marking project as failed")
 			}
 			break
 		}
 	}
 
 	s.fail(s.sel.getDependenciesOn(q.ref)[0].Depender.Name)
-	return err
+
+	// Return a compound error of all the new errors encountered during this
+	// attempt to find a new, valid version
+	var fails []failedVersion
+	if len(q.fails) > faillen {
+		fails = q.fails[faillen+1:]
+	}
+
+	return &noVersionError{
+		pn:    q.ref,
+		fails: fails,
+	}
 }
 
 func (s *solver) getLockVersionIfValid(ref ProjectName) *ProjectAtom {
@@ -291,6 +290,7 @@ func (s *solver) satisfiable(pi ProjectAtom) error {
 		}
 
 		deps := s.sel.getDependenciesOn(pi.Name)
+		var failparent []Dependency
 		for _, dep := range deps {
 			if !dep.Dep.Constraint.Admits(pi.Version) {
 				if s.l.Level >= logrus.DebugLevel {
@@ -301,14 +301,14 @@ func (s *solver) satisfiable(pi ProjectAtom) error {
 					}).Debug("Marking other, selected project with conflicting constraint as failed")
 				}
 				s.fail(dep.Depender.Name)
+				failparent = append(failparent, dep)
 			}
 		}
 
-		// TODO msg
+		return &versionNotAllowedFailure{
-		return &noVersionError{
+			goal:       pi,
-			pn:   pi.Name,
+			failparent: failparent,
-			c:    constraint,
+			c:          constraint,
-			deps: deps,
 		}
 	}
 
@@ -345,6 +345,8 @@ func (s *solver) satisfiable(pi ProjectAtom) error {
 			}
 
 			// No admissible versions - visit all siblings and identify the disagreement(s)
+			var failsib []Dependency
+			var nofailsib []Dependency
 			for _, sibling := range siblings {
 				if !sibling.Dep.Constraint.AdmitsAny(dep.Constraint) {
 					if s.l.Level >= logrus.DebugLevel {
@@ -354,16 +356,20 @@ func (s *solver) satisfiable(pi ProjectAtom) error {
 							"depname":       sibling.Depender.Name,
 							"sibconstraint": sibling.Dep.Constraint.Body(),
 							"newconstraint": dep.Constraint.Body(),
-						}).Debug("Marking other, selected project as failed because its constraint is disjoint with our input")
+						}).Debug("Marking other, selected project as failed because its constraint is disjoint with our testee")
 					}
 					s.fail(sibling.Depender.Name)
+					failsib = append(failsib, sibling)
+				} else {
+					nofailsib = append(nofailsib, sibling)
 				}
 			}
 
-			// TODO msg
 			return &disjointConstraintFailure{
-				pn:   dep.Name,
+				goal:      Dependency{Depender: pi, Dep: dep},
-				deps: append(siblings, Dependency{Depender: pi, Dep: dep}),
+				failsib:   failsib,
+				nofailsib: nofailsib,
+				c:         constraint,
 			}
 		}
 
@@ -380,11 +386,9 @@ func (s *solver) satisfiable(pi ProjectAtom) error {
 			}
 			s.fail(dep.Name)
 
-			// TODO msg
+			return &constraintNotAllowedFailure{
-			return &noVersionError{
+				goal: Dependency{Depender: pi, Dep: dep},
-				pn:   dep.Name,
+				v:    selected.Version,
-				c:    dep.Constraint,
-				deps: append(siblings, Dependency{Depender: pi, Dep: dep}),
 			}
 		}
 
@@ -485,7 +489,8 @@ func (s *solver) backtrack() bool {
 		s.unselectLast()
 
 		// Advance the queue past the current version, which we know is bad
-		if q.advance() == nil && !q.isExhausted() {
+		// TODO is it feasible to make available the failure reason here?
+		if q.advance(nil) == nil && !q.isExhausted() {
 			// Search for another acceptable version of this failed dep in its queue
 			if s.findValidVersion(q) == nil {
 				if s.l.Level >= logrus.InfoLevel {

version_queue.go

@@ -5,9 +5,15 @@ import (
 	"strings"
 )
 
+type failedVersion struct {
+	v Version
+	f error
+}
+
 type versionQueue struct {
 	ref                ProjectName
 	pi                 []Version
+	fails              []failedVersion
 	sm                 SourceManager
 	failed             bool
 	hasLock, allLoaded bool
@@ -45,14 +51,20 @@ func (vq *versionQueue) current() Version {
 	return Version{}
 }
 
-func (vq *versionQueue) advance() (err error) {
+func (vq *versionQueue) advance(fail error) (err error) {
 	// The current version may have failed, but the next one hasn't
 	vq.failed = false
 
+	if len(vq.pi) == 0 {
+		return
+	}
+
+	vq.fails = append(vq.fails, failedVersion{
+		v: vq.pi[0],
+		f: fail,
+	})
 	if vq.allLoaded {
-		if len(vq.pi) > 0 {
+		vq.pi = vq.pi[1:]
-			vq.pi = vq.pi[1:]
-		}
 		return
 	}