From 04f95df0e452026a32041dd928d4a6750b0e1218 Mon Sep 17 00:00:00 2001 From: Jordan Krage Date: Wed, 14 Jun 2017 18:30:21 -0500 Subject: [PATCH 1/4] adding TestSingleSourceCache; better constraint equality --- internal/gps/constraint_test.go | 32 +++ internal/gps/constraints.go | 20 ++ internal/gps/lock.go | 4 +- internal/gps/source_cache.go | 36 +++ internal/gps/source_cache_test.go | 426 ++++++++++++++++++++++++++++++ internal/gps/version.go | 43 +++ internal/gps/version_unifier.go | 9 + 7 files changed, 568 insertions(+), 2 deletions(-) create mode 100644 internal/gps/source_cache_test.go diff --git a/internal/gps/constraint_test.go b/internal/gps/constraint_test.go index d3619bfde2..000c6ede3b 100644 --- a/internal/gps/constraint_test.go +++ b/internal/gps/constraint_test.go @@ -927,3 +927,35 @@ func TestTypedConstraintString(t *testing.T) { } } } + +func TestConstraintsEqual(t *testing.T) { + for _, test := range []struct { + a, b Constraint + eq bool + }{ + {a: NewVersion("test"), b: NewVersion("test"), eq: true}, + {a: NewVersion("test"), b: NewVersion("test2"), eq: false}, + {a: NewBranch("test"), b: NewBranch("test"), eq: true}, + {a: NewBranch("test"), b: newDefaultBranch("test"), eq: false}, + {a: newDefaultBranch("test"), b: newDefaultBranch("test"), eq: true}, + {a: Revision("test"), b: Revision("test"), eq: true}, + {a: Revision("test"), b: Revision("test2"), eq: false}, + {a: testSemverConstraint(t, "v2.10.7"), b: testSemverConstraint(t, "v2.10.7"), eq: true}, + } { + if test.eq != test.a.equals(test.b) { + want := "equal" + if !test.eq { + want = "not " + want + } + t.Errorf("expected %s:\n\t(a) %#v\n\t(b) %#v", want, test.a, test.b) + } + } +} + +func testSemverConstraint(t *testing.T, body string) Constraint { + c, err := NewSemverConstraint(body) + if err != nil { + t.Fatal(err) + } + return c +} diff --git a/internal/gps/constraints.go b/internal/gps/constraints.go index 6e8c5f6017..a7e452b973 100644 --- a/internal/gps/constraints.go +++ b/internal/gps/constraints.go @@ -52,6 +52,9 @@ type Constraint interface { // It also forces Constraint to be a private/sealed interface, which is a // design goal of the system. typedString() string + + // equals returns true if the constraints are logically equivalent. + equals(Constraint) bool } // NewSemverConstraint attempts to construct a semver Constraint object from the @@ -172,6 +175,14 @@ func (c semverConstraint) Intersect(c2 Constraint) Constraint { return none } +func (c semverConstraint) equals(c2 Constraint) bool { + sc2, ok := c2.(semverConstraint) + if !ok { + return false + } + return c.c.String() == sc2.c.String() +} + // IsAny indicates if the provided constraint is the wildcard "Any" constraint. func IsAny(c Constraint) bool { _, ok := c.(anyConstraint) @@ -211,6 +222,10 @@ func (anyConstraint) Intersect(c Constraint) Constraint { return c } +func (anyConstraint) equals(c Constraint) bool { + return IsAny(c) +} + // noneConstraint is the empty set - it matches no versions. It mirrors the // behavior of the semver package's none type. type noneConstraint struct{} @@ -239,6 +254,11 @@ func (noneConstraint) Intersect(Constraint) Constraint { return none } +func (noneConstraint) equals(c Constraint) bool { + _, ok := c.(noneConstraint) + return ok +} + // A ProjectConstraint combines a ProjectIdentifier with a Constraint. It // indicates that, if packages contained in the ProjectIdentifier enter the // depgraph, they must do so at a version that is allowed by the Constraint. diff --git a/internal/gps/lock.go b/internal/gps/lock.go index db71c694e9..f221ad997b 100644 --- a/internal/gps/lock.go +++ b/internal/gps/lock.go @@ -226,7 +226,7 @@ func prepLock(l Lock) safeLock { } // SortLockedProjects sorts a slice of LockedProject in alphabetical order by -// ProjectRoot. +// ProjectIdentifier. func SortLockedProjects(lps []LockedProject) { sort.Stable(lpsorter(lps)) } @@ -242,5 +242,5 @@ func (lps lpsorter) Len() int { } func (lps lpsorter) Less(i, j int) bool { - return lps[i].pi.ProjectRoot < lps[j].pi.ProjectRoot + return lps[i].Ident().less(lps[j].Ident()) } diff --git a/internal/gps/source_cache.go b/internal/gps/source_cache.go index b0b5e073aa..d10ec0b399 100644 --- a/internal/gps/source_cache.go +++ b/internal/gps/source_cache.go @@ -221,3 +221,39 @@ func (c *singleSourceCacheMemory) toUnpaired(v Version) (UnpairedVersion, bool) panic(fmt.Sprintf("unknown version type %T", v)) } } + +// cachedManifest implements RootManifest and is populated from cached data. +type cachedManifest struct { + constraints, overrides ProjectConstraints + ignored, required map[string]bool +} + +func (m *cachedManifest) DependencyConstraints() ProjectConstraints { + return m.constraints +} + +func (m *cachedManifest) Overrides() ProjectConstraints { + return m.overrides +} + +func (m *cachedManifest) IgnoredPackages() map[string]bool { + return m.ignored +} + +func (m *cachedManifest) RequiredPackages() map[string]bool { + return m.required +} + +// cachedManifest implements Lock and is populated from cached data. +type cachedLock struct { + inputHash []byte + projects []LockedProject +} + +func (l *cachedLock) InputHash() []byte { + return l.inputHash +} + +func (l *cachedLock) Projects() []LockedProject { + return l.projects +} diff --git a/internal/gps/source_cache_test.go b/internal/gps/source_cache_test.go new file mode 100644 index 0000000000..a08ed3a131 --- /dev/null +++ b/internal/gps/source_cache_test.go @@ -0,0 +1,426 @@ +// Copyright 2017 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package gps + +import ( + "sort" + "testing" + + "github.com/golang/dep/internal/gps/pkgtree" + "github.com/pkg/errors" +) + +var testAnalyzerInfo = ProjectAnalyzerInfo{ + Name: "test-analyzer", + Version: 1, +} + +func TestSingleSourceCache(t *testing.T) { + const root = "example.com/test" + + t.Run("info", func(t *testing.T) { + newSemverConstraint := func(s string) Constraint { + c, err := NewSemverConstraint(s) + if err != nil { + t.Fatal(errors.Wrapf(err, "failed to create semver constraint: %s", s)) + } + return c + } + + const rev Revision = "revision" + + c := newMemoryCache() + + var m Manifest = &cachedManifest{ + constraints: ProjectConstraints{ + ProjectRoot("foo"): ProjectProperties{}, + ProjectRoot("bar"): ProjectProperties{ + Source: "whatever", + Constraint: newSemverConstraint("> 1.3"), + }, + }, + overrides: ProjectConstraints{ + ProjectRoot("b"): ProjectProperties{ + Constraint: NewVersion("2.0.0"), + }, + }, + ignored: map[string]bool{ + "a": true, + "b": true, + }, + required: map[string]bool{ + "c": true, + "d": true, + }, + } + var l Lock = &cachedLock{ + inputHash: []byte("test_hash"), + projects: []LockedProject{ + NewLockedProject(mkPI("github.com/sdboyer/gps"), NewVersion("v0.10.0"), []string{"gps"}), + NewLockedProject(mkPI("github.com/sdboyer/gps"), NewVersion("v0.10.0"), nil), + NewLockedProject(mkPI("github.com/sdboyer/gps"), NewVersion("v0.10.0"), []string{"gps", "flugle"}), + NewLockedProject(mkPI("foo"), NewVersion("nada"), []string{"foo"}), + NewLockedProject(mkPI("github.com/sdboyer/gps"), NewVersion("v0.10.0"), []string{"flugle", "gps"}), + }, + } + c.setManifestAndLock(rev, testAnalyzerInfo, m, l) + + gotM, gotL, ok := c.getManifestAndLock(rev, testAnalyzerInfo) + if !ok { + t.Error("no manifest and lock found for revision") + } + compareManifests(t, m, gotM) + if dl := DiffLocks(l, gotL); dl != nil { + t.Errorf("lock differences:\n\t %#v", dl) + } + + m = &cachedManifest{ + constraints: ProjectConstraints{ + ProjectRoot("foo"): ProjectProperties{ + Source: "whatever", + }, + }, + overrides: ProjectConstraints{ + ProjectRoot("bar"): ProjectProperties{ + Constraint: NewVersion("2.0.0"), + }, + }, + ignored: map[string]bool{ + "c": true, + "d": true, + }, + required: map[string]bool{ + "a": true, + "b": true, + }, + } + l = &cachedLock{ + inputHash: []byte("different_test_hash"), + projects: []LockedProject{ + NewLockedProject(mkPI("github.com/sdboyer/gps"), NewVersion("v0.10.0").Pair("278a227dfc3d595a33a77ff3f841fd8ca1bc8cd0"), []string{"gps"}), + NewLockedProject(mkPI("github.com/sdboyer/gps"), NewVersion("v0.11.0"), []string{"gps"}), + NewLockedProject(mkPI("github.com/sdboyer/gps"), Revision("278a227dfc3d595a33a77ff3f841fd8ca1bc8cd0"), []string{"gps"}), + }, + } + c.setManifestAndLock(rev, testAnalyzerInfo, m, l) + + gotM, gotL, ok = c.getManifestAndLock(rev, testAnalyzerInfo) + if !ok { + t.Error("no manifest and lock found for revision") + } + compareManifests(t, m, gotM) + if dl := DiffLocks(l, gotL); dl != nil { + t.Errorf("lock differences:\n\t %#v", dl) + } + }) + + t.Run("pkgTree", func(t *testing.T) { + c := newMemoryCache() + + const rev Revision = "rev_adsfjkl" + + if got, ok := c.getPackageTree(rev); ok { + t.Fatalf("unexpected result before setting package tree: %v", got) + } + + pt := pkgtree.PackageTree{ + ImportRoot: root, + Packages: map[string]pkgtree.PackageOrErr{ + "simple": { + P: pkgtree.Package{ + ImportPath: "simple", + CommentPath: "comment", + Name: "simple", + Imports: []string{ + "github.com/golang/dep/internal/gps", + "sort", + }, + }, + }, + "m1p": { + P: pkgtree.Package{ + ImportPath: "m1p", + CommentPath: "", + Name: "m1p", + Imports: []string{ + "github.com/golang/dep/internal/gps", + "os", + "sort", + }, + }, + }, + }, + } + c.setPackageTree(rev, pt) + + got, ok := c.getPackageTree(rev) + if !ok { + t.Errorf("no package tree found:\n\t(WNT): %#v", pt) + } + comparePackageTree(t, pt, got) + + pt = pkgtree.PackageTree{ + ImportRoot: root, + Packages: map[string]pkgtree.PackageOrErr{ + "test": { + Err: errors.New("error"), + }, + }, + } + c.setPackageTree(rev, pt) + + got, ok = c.getPackageTree(rev) + if !ok { + t.Errorf("no package tree found:\n\t(WNT): %#v", pt) + } + comparePackageTree(t, pt, got) + }) + + t.Run("versions", func(t *testing.T) { + c := newMemoryCache() + + const rev1, rev2 = "rev1", "rev2" + const br, ver = "branch_name", "2.10" + versions := []PairedVersion{ + NewBranch(br).Pair(rev1), + NewVersion(ver).Pair(rev2), + } + SortPairedForDowngrade(versions) + c.storeVersionMap(versions, true) + + t.Run("getAllVersions", func(t *testing.T) { + got := c.getAllVersions() + if len(got) != len(versions) { + t.Errorf("unexpected versions:\n\t(GOT): %#v\n\t(WNT): %#v", got, versions) + } else { + SortPairedForDowngrade(got) + for i := range versions { + if !versions[i].equals(got[i]) { + t.Errorf("unexpected versions:\n\t(GOT): %#v\n\t(WNT): %#v", got, versions) + break + } + } + } + }) + + revToUV := map[Revision]UnpairedVersion{ + rev1: NewBranch(br), + rev2: NewVersion(ver), + } + + t.Run("getVersionsFor", func(t *testing.T) { + for rev, want := range revToUV { + rev, want := rev, want + t.Run(string(rev), func(t *testing.T) { + uvs, ok := c.getVersionsFor(rev) + if !ok { + t.Errorf("no version found:\n\t(WNT) %#v", want) + } else if len(uvs) != 1 { + t.Errorf("expected one result but got %d", len(uvs)) + } else { + uv := uvs[0] + if uv.Type() != want.Type() { + t.Errorf("expected version type %d but got %d", want.Type(), uv.Type()) + } + if uv.String() != want.String() { + t.Errorf("expected version %q but got %q", want.String(), uv.String()) + } + } + }) + } + }) + + t.Run("getRevisionFor", func(t *testing.T) { + for want, uv := range revToUV { + want, uv := want, uv + t.Run(uv.String(), func(t *testing.T) { + rev, ok := c.getRevisionFor(uv) + if !ok { + t.Errorf("expected revision %q but got none", want) + } else if rev != want { + t.Errorf("expected revision %q but got %q", want, rev) + } + }) + } + }) + + t.Run("toRevision", func(t *testing.T) { + for want, uv := range revToUV { + want, uv := want, uv + t.Run(uv.String(), func(t *testing.T) { + rev, ok := c.toRevision(uv) + if !ok { + t.Errorf("expected revision %q but got none", want) + } else if rev != want { + t.Errorf("expected revision %q but got %q", want, rev) + } + }) + } + }) + + t.Run("toUnpaired", func(t *testing.T) { + for rev, want := range revToUV { + rev, want := rev, want + t.Run(want.String(), func(t *testing.T) { + uv, ok := c.toUnpaired(rev) + if !ok { + t.Errorf("no UnpairedVersion found:\n\t(WNT): %#v", uv) + } else if !uv.equals(want) { + t.Errorf("unexpected UnpairedVersion:\n\t(GOT): %#v\n\t(WNT): %#v", uv, want) + } + }) + } + }) + }) +} + +// compareManifests compares two manifests and reports differences as test errors. +func compareManifests(t *testing.T, want, got Manifest) { + { + want, got := want.DependencyConstraints(), got.DependencyConstraints() + if !projectConstraintsEqual(want, got) { + t.Errorf("unexpected constraints:\n\t(GOT): %#v\n\t(WNT): %#v", got, want) + } + } + + wantRM, wantOK := want.(RootManifest) + gotRM, gotOK := got.(RootManifest) + if wantOK && !gotOK { + t.Errorf("expected RootManifest:\n\t(GOT): %#v", got) + return + } + if gotOK && !wantOK { + t.Errorf("didn't expected RootManifest:\n\t(GOT): %#v", got) + return + } + + { + want, got := wantRM.IgnoredPackages(), gotRM.IgnoredPackages() + if !mapStringBoolEqual(want, got) { + t.Errorf("unexpected ignored packages:\n\t(GOT): %#v\n\t(WNT): %#v", got, want) + } + } + + { + want, got := wantRM.Overrides(), gotRM.Overrides() + if !projectConstraintsEqual(want, got) { + t.Errorf("unexpected overrides:\n\t(GOT): %#v\n\t(WNT): %#v", got, want) + } + } + + { + want, got := wantRM.RequiredPackages(), gotRM.RequiredPackages() + if !mapStringBoolEqual(want, got) { + t.Errorf("unexpected required packages:\n\t(GOT): %#v\n\t(WNT): %#v", got, want) + } + } +} + +// comparePackageTree compares two pkgtree.PackageTree and reports differences as test errors. +func comparePackageTree(t *testing.T, want, got pkgtree.PackageTree) { + if got.ImportRoot != want.ImportRoot { + t.Errorf("expected package tree root %q but got %q", want.ImportRoot, got.ImportRoot) + } + { + want, got := want.Packages, got.Packages + if len(want) != len(got) { + t.Errorf("unexpected packages:\n\t(GOT): %#v\n\t(WNT): %#v", got, want) + } else { + for k, v := range want { + if v2, ok := got[k]; !ok { + t.Errorf("key %q: expected %v but got none", k, v) + } else if !packageOrErrEqual(v, v2) { + t.Errorf("key %q: expected %v but got %v", k, v, v2) + } + } + } + } +} + +func projectConstraintsEqual(want, got ProjectConstraints) bool { + loop, check := want, got + if len(got) > len(want) { + loop, check = got, want + } + for pr, pp := range loop { + pp2, ok := check[pr] + if !ok { + return false + } + if pp.Source != pp2.Source { + return false + } + if pp.Constraint == nil || pp2.Constraint == nil { + if pp.Constraint != nil || pp2.Constraint != nil { + return false + } + } else if !pp.Constraint.equals(pp2.Constraint) { + return false + } + } + return true +} + +func mapStringBoolEqual(exp, got map[string]bool) bool { + loop, check := exp, got + if len(got) > len(exp) { + loop, check = got, exp + } + for k, v := range loop { + v2, ok := check[k] + if !ok || v != v2 { + return false + } + } + return true +} + +func safeError(err error) string { + if err == nil { + return "" + } + return err.Error() +} + +// packageOrErrEqual return true if the pkgtree.PackageOrErrs are equal. Error equality is +// string based. Imports and TestImports are treated as sets, and will be sorted. +func packageOrErrEqual(a, b pkgtree.PackageOrErr) bool { + if safeError(a.Err) != safeError(b.Err) { + return false + } + if a.P.Name != b.P.Name { + return false + } + if a.P.ImportPath != b.P.ImportPath { + return false + } + if a.P.CommentPath != b.P.CommentPath { + return false + } + + if len(a.P.Imports) != len(b.P.Imports) { + return false + } + sort.Strings(a.P.Imports) + sort.Strings(b.P.Imports) + for i := range a.P.Imports { + if a.P.Imports[i] != b.P.Imports[i] { + return false + } + } + + if len(a.P.TestImports) != len(b.P.TestImports) { + return false + } + sort.Strings(a.P.TestImports) + sort.Strings(b.P.TestImports) + for i := range a.P.TestImports { + if a.P.TestImports[i] != b.P.TestImports[i] { + return false + } + } + + return true +} diff --git a/internal/gps/version.go b/internal/gps/version.go index 29b71334df..125c1a84be 100644 --- a/internal/gps/version.go +++ b/internal/gps/version.go @@ -190,6 +190,14 @@ func (r Revision) Intersect(c Constraint) Constraint { return none } +func (r Revision) equals(c Constraint) bool { + r2, ok := c.(Revision) + if !ok { + return false + } + return r == r2 +} + type branchVersion struct { name string isDefault bool @@ -274,6 +282,14 @@ func (v branchVersion) Pair(r Revision) PairedVersion { } } +func (v branchVersion) equals(c Constraint) bool { + v2, ok := c.(branchVersion) + if !ok { + return false + } + return v == v2 +} + type plainVersion string func (v plainVersion) String() string { @@ -355,6 +371,14 @@ func (v plainVersion) Pair(r Revision) PairedVersion { } } +func (v plainVersion) equals(c Constraint) bool { + v2, ok := c.(plainVersion) + if !ok { + return false + } + return v == v2 +} + type semVersion struct { sv semver.Version } @@ -446,6 +470,14 @@ func (v semVersion) Pair(r Revision) PairedVersion { } } +func (v semVersion) equals(c Constraint) bool { + v2, ok := c.(semVersion) + if !ok { + return false + } + return v == v2 +} + type versionPair struct { v UnpairedVersion r Revision @@ -547,6 +579,17 @@ func (v versionPair) Intersect(c2 Constraint) Constraint { return none } +func (v versionPair) equals(c Constraint) bool { + v2, ok := c.(versionPair) + if !ok { + return false + } + if v.r != v2.r { + return false + } + return v.v.equals(v2.v) +} + // compareVersionType is a sort func helper that makes a coarse-grained sorting // decision based on version type. // diff --git a/internal/gps/version_unifier.go b/internal/gps/version_unifier.go index 71657559ef..7cde8d9dc9 100644 --- a/internal/gps/version_unifier.go +++ b/internal/gps/version_unifier.go @@ -269,3 +269,12 @@ func (vtu versionTypeUnion) Intersect(c Constraint) Constraint { return none } + +func (vtu versionTypeUnion) equals(c Constraint) bool { + for _, v := range vtu { + if v.equals(c) { + return true + } + } + return false +} From 854158e4624d4e237edaaf120dbc7b88a1bd5135 Mon Sep 17 00:00:00 2001 From: Jordan Krage Date: Wed, 19 Jul 2017 08:30:05 -0500 Subject: [PATCH 2/4] rename 'equals' method to 'identical'; better comment --- internal/gps/constraint_test.go | 2 +- internal/gps/constraints.go | 15 ++++++++++----- internal/gps/source_cache_test.go | 6 +++--- internal/gps/version.go | 12 ++++++------ internal/gps/version_unifier.go | 4 ++-- 5 files changed, 22 insertions(+), 17 deletions(-) diff --git a/internal/gps/constraint_test.go b/internal/gps/constraint_test.go index 000c6ede3b..3b88ab577d 100644 --- a/internal/gps/constraint_test.go +++ b/internal/gps/constraint_test.go @@ -942,7 +942,7 @@ func TestConstraintsEqual(t *testing.T) { {a: Revision("test"), b: Revision("test2"), eq: false}, {a: testSemverConstraint(t, "v2.10.7"), b: testSemverConstraint(t, "v2.10.7"), eq: true}, } { - if test.eq != test.a.equals(test.b) { + if test.eq != test.a.identical(test.b) { want := "equal" if !test.eq { want = "not " + want diff --git a/internal/gps/constraints.go b/internal/gps/constraints.go index a7e452b973..d3d382a595 100644 --- a/internal/gps/constraints.go +++ b/internal/gps/constraints.go @@ -53,8 +53,13 @@ type Constraint interface { // design goal of the system. typedString() string - // equals returns true if the constraints are logically equivalent. - equals(Constraint) bool + // identical returns true if the constraints are identical. + // + // Identical Constraints behave identically for all methods defined by the + // interface. A Constraint is always identical to itself. + // + // Constraints serialized for caching are de-serialized into identical instances. + identical(Constraint) bool } // NewSemverConstraint attempts to construct a semver Constraint object from the @@ -175,7 +180,7 @@ func (c semverConstraint) Intersect(c2 Constraint) Constraint { return none } -func (c semverConstraint) equals(c2 Constraint) bool { +func (c semverConstraint) identical(c2 Constraint) bool { sc2, ok := c2.(semverConstraint) if !ok { return false @@ -222,7 +227,7 @@ func (anyConstraint) Intersect(c Constraint) Constraint { return c } -func (anyConstraint) equals(c Constraint) bool { +func (anyConstraint) identical(c Constraint) bool { return IsAny(c) } @@ -254,7 +259,7 @@ func (noneConstraint) Intersect(Constraint) Constraint { return none } -func (noneConstraint) equals(c Constraint) bool { +func (noneConstraint) identical(c Constraint) bool { _, ok := c.(noneConstraint) return ok } diff --git a/internal/gps/source_cache_test.go b/internal/gps/source_cache_test.go index a08ed3a131..342ae6489a 100644 --- a/internal/gps/source_cache_test.go +++ b/internal/gps/source_cache_test.go @@ -197,7 +197,7 @@ func TestSingleSourceCache(t *testing.T) { } else { SortPairedForDowngrade(got) for i := range versions { - if !versions[i].equals(got[i]) { + if !versions[i].identical(got[i]) { t.Errorf("unexpected versions:\n\t(GOT): %#v\n\t(WNT): %#v", got, versions) break } @@ -267,7 +267,7 @@ func TestSingleSourceCache(t *testing.T) { uv, ok := c.toUnpaired(rev) if !ok { t.Errorf("no UnpairedVersion found:\n\t(WNT): %#v", uv) - } else if !uv.equals(want) { + } else if !uv.identical(want) { t.Errorf("unexpected UnpairedVersion:\n\t(GOT): %#v\n\t(WNT): %#v", uv, want) } }) @@ -356,7 +356,7 @@ func projectConstraintsEqual(want, got ProjectConstraints) bool { if pp.Constraint != nil || pp2.Constraint != nil { return false } - } else if !pp.Constraint.equals(pp2.Constraint) { + } else if !pp.Constraint.identical(pp2.Constraint) { return false } } diff --git a/internal/gps/version.go b/internal/gps/version.go index 125c1a84be..0462892826 100644 --- a/internal/gps/version.go +++ b/internal/gps/version.go @@ -190,7 +190,7 @@ func (r Revision) Intersect(c Constraint) Constraint { return none } -func (r Revision) equals(c Constraint) bool { +func (r Revision) identical(c Constraint) bool { r2, ok := c.(Revision) if !ok { return false @@ -282,7 +282,7 @@ func (v branchVersion) Pair(r Revision) PairedVersion { } } -func (v branchVersion) equals(c Constraint) bool { +func (v branchVersion) identical(c Constraint) bool { v2, ok := c.(branchVersion) if !ok { return false @@ -371,7 +371,7 @@ func (v plainVersion) Pair(r Revision) PairedVersion { } } -func (v plainVersion) equals(c Constraint) bool { +func (v plainVersion) identical(c Constraint) bool { v2, ok := c.(plainVersion) if !ok { return false @@ -470,7 +470,7 @@ func (v semVersion) Pair(r Revision) PairedVersion { } } -func (v semVersion) equals(c Constraint) bool { +func (v semVersion) identical(c Constraint) bool { v2, ok := c.(semVersion) if !ok { return false @@ -579,7 +579,7 @@ func (v versionPair) Intersect(c2 Constraint) Constraint { return none } -func (v versionPair) equals(c Constraint) bool { +func (v versionPair) identical(c Constraint) bool { v2, ok := c.(versionPair) if !ok { return false @@ -587,7 +587,7 @@ func (v versionPair) equals(c Constraint) bool { if v.r != v2.r { return false } - return v.v.equals(v2.v) + return v.v.identical(v2.v) } // compareVersionType is a sort func helper that makes a coarse-grained sorting diff --git a/internal/gps/version_unifier.go b/internal/gps/version_unifier.go index 7cde8d9dc9..d3d5454b88 100644 --- a/internal/gps/version_unifier.go +++ b/internal/gps/version_unifier.go @@ -270,9 +270,9 @@ func (vtu versionTypeUnion) Intersect(c Constraint) Constraint { return none } -func (vtu versionTypeUnion) equals(c Constraint) bool { +func (vtu versionTypeUnion) identical(c Constraint) bool { for _, v := range vtu { - if v.equals(c) { + if v.identical(c) { return true } } From c59d77ba788210e98ce42ceac49748c7a66ae8de Mon Sep 17 00:00:00 2001 From: Jordan Krage Date: Wed, 19 Jul 2017 16:12:32 -0500 Subject: [PATCH 3/4] improved versionTypeUnion identical(); dry up test helpers; cleaner tests; more tests --- internal/gps/constraint_test.go | 28 +++++++++++++++++----------- internal/gps/source_cache_test.go | 10 +--------- internal/gps/version_unifier.go | 22 +++++++++++++++++++--- 3 files changed, 37 insertions(+), 23 deletions(-) diff --git a/internal/gps/constraint_test.go b/internal/gps/constraint_test.go index 3b88ab577d..412afa3385 100644 --- a/internal/gps/constraint_test.go +++ b/internal/gps/constraint_test.go @@ -7,6 +7,8 @@ package gps import ( "fmt" "testing" + + "github.com/pkg/errors" ) // gu - helper func for stringifying what we assume is a VersionPair (otherwise @@ -928,22 +930,26 @@ func TestTypedConstraintString(t *testing.T) { } } -func TestConstraintsEqual(t *testing.T) { +func TestConstraintsIdentical(t *testing.T) { for _, test := range []struct { a, b Constraint eq bool }{ - {a: NewVersion("test"), b: NewVersion("test"), eq: true}, - {a: NewVersion("test"), b: NewVersion("test2"), eq: false}, - {a: NewBranch("test"), b: NewBranch("test"), eq: true}, - {a: NewBranch("test"), b: newDefaultBranch("test"), eq: false}, - {a: newDefaultBranch("test"), b: newDefaultBranch("test"), eq: true}, - {a: Revision("test"), b: Revision("test"), eq: true}, - {a: Revision("test"), b: Revision("test2"), eq: false}, - {a: testSemverConstraint(t, "v2.10.7"), b: testSemverConstraint(t, "v2.10.7"), eq: true}, + {Any(), Any(), true}, + {none, noneConstraint{}, true}, + {NewVersion("test"), NewVersion("test"), true}, + {NewVersion("test"), NewVersion("test2"), false}, + {NewBranch("test"), NewBranch("test"), true}, + {NewBranch("test"), newDefaultBranch("test"), false}, + {newDefaultBranch("test"), newDefaultBranch("test"), true}, + {Revision("test"), Revision("test"), true}, + {Revision("test"), Revision("test2"), false}, + {testSemverConstraint(t, "v2.10.7"), testSemverConstraint(t, "v2.10.7"), true}, + {&versionTypeUnion{NewVersion("test"), NewBranch("branch")}, + &versionTypeUnion{NewBranch("branch"), NewVersion("test")}, true}, } { if test.eq != test.a.identical(test.b) { - want := "equal" + want := "identical" if !test.eq { want = "not " + want } @@ -955,7 +961,7 @@ func TestConstraintsEqual(t *testing.T) { func testSemverConstraint(t *testing.T, body string) Constraint { c, err := NewSemverConstraint(body) if err != nil { - t.Fatal(err) + t.Fatal(errors.Wrapf(err, "failed to create semver constraint: %s", body)) } return c } diff --git a/internal/gps/source_cache_test.go b/internal/gps/source_cache_test.go index 342ae6489a..79e69c339b 100644 --- a/internal/gps/source_cache_test.go +++ b/internal/gps/source_cache_test.go @@ -21,14 +21,6 @@ func TestSingleSourceCache(t *testing.T) { const root = "example.com/test" t.Run("info", func(t *testing.T) { - newSemverConstraint := func(s string) Constraint { - c, err := NewSemverConstraint(s) - if err != nil { - t.Fatal(errors.Wrapf(err, "failed to create semver constraint: %s", s)) - } - return c - } - const rev Revision = "revision" c := newMemoryCache() @@ -38,7 +30,7 @@ func TestSingleSourceCache(t *testing.T) { ProjectRoot("foo"): ProjectProperties{}, ProjectRoot("bar"): ProjectProperties{ Source: "whatever", - Constraint: newSemverConstraint("> 1.3"), + Constraint: testSemverConstraint(t, "> 1.3"), }, }, overrides: ProjectConstraints{ diff --git a/internal/gps/version_unifier.go b/internal/gps/version_unifier.go index d3d5454b88..fc88437f9c 100644 --- a/internal/gps/version_unifier.go +++ b/internal/gps/version_unifier.go @@ -271,10 +271,26 @@ func (vtu versionTypeUnion) Intersect(c Constraint) Constraint { } func (vtu versionTypeUnion) identical(c Constraint) bool { + vtu2, ok := c.(*versionTypeUnion) + if !ok { + return false + } + if len(vtu) != len(*vtu2) { + return false + } + used := make([]bool, len(vtu)) +outter: for _, v := range vtu { - if v.identical(c) { - return true + for i, v2 := range *vtu2 { + if used[i] { + continue + } + if v.identical(v2) { + used[i] = true + continue outter + } } + return false } - return false + return true } From 35c6387eb3fb00fda3a8d33bff853b93bf027fe2 Mon Sep 17 00:00:00 2001 From: Jordan Krage Date: Wed, 19 Jul 2017 16:16:29 -0500 Subject: [PATCH 4/4] fix extra pointers --- internal/gps/constraint_test.go | 4 ++-- internal/gps/version_unifier.go | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/internal/gps/constraint_test.go b/internal/gps/constraint_test.go index 412afa3385..b37ea17071 100644 --- a/internal/gps/constraint_test.go +++ b/internal/gps/constraint_test.go @@ -945,8 +945,8 @@ func TestConstraintsIdentical(t *testing.T) { {Revision("test"), Revision("test"), true}, {Revision("test"), Revision("test2"), false}, {testSemverConstraint(t, "v2.10.7"), testSemverConstraint(t, "v2.10.7"), true}, - {&versionTypeUnion{NewVersion("test"), NewBranch("branch")}, - &versionTypeUnion{NewBranch("branch"), NewVersion("test")}, true}, + {versionTypeUnion{NewVersion("test"), NewBranch("branch")}, + versionTypeUnion{NewBranch("branch"), NewVersion("test")}, true}, } { if test.eq != test.a.identical(test.b) { want := "identical" diff --git a/internal/gps/version_unifier.go b/internal/gps/version_unifier.go index fc88437f9c..6c86480db2 100644 --- a/internal/gps/version_unifier.go +++ b/internal/gps/version_unifier.go @@ -271,17 +271,17 @@ func (vtu versionTypeUnion) Intersect(c Constraint) Constraint { } func (vtu versionTypeUnion) identical(c Constraint) bool { - vtu2, ok := c.(*versionTypeUnion) + vtu2, ok := c.(versionTypeUnion) if !ok { return false } - if len(vtu) != len(*vtu2) { + if len(vtu) != len(vtu2) { return false } used := make([]bool, len(vtu)) outter: for _, v := range vtu { - for i, v2 := range *vtu2 { + for i, v2 := range vtu2 { if used[i] { continue }