adding TestSingleSourceCache #756

jmank88 · 2017-06-15T00:53:02Z

What does this do / why do we need it?

This change adds tests for singleSourceCache against the only current implementation, singleSourceCacheMemory. They are more general than they need to be (e.g. equality funcs, cachedLock/Manifest types), but this is in anticipation of being run against a persistent singleSourceCache as well for #431 (which can't rely on == or reflect.DeepEquals as liberally).

jmank88 · 2017-06-15T00:57:37Z

Happy to make this a WIP. Could certainly use some more comments at least, if not more cases.

sdboyer

i think this is generally heading in a very useful direction! some issues with the equality checks in particular, though.

sdboyer · 2017-06-15T01:04:54Z

internal/gps/source_cache_test.go

+			NewBranch(br).Pair(rev1),
+			NewVersion(ver).Pair(rev2),
+		}
+		sort.Slice(versions, func(i, j int) bool {


1.7 still :(

sdboyer · 2017-06-15T01:20:44Z

internal/gps/source_cache_test.go

+}
+
+// compareManifests compares two manifests and reports differences as test errors.
+func compareManifests(t *testing.T, exp, got Manifest) {


nit: s/exp/want/

sdboyer · 2017-06-15T01:21:52Z

internal/gps/source_cache_test.go

+	{
+		exp, got := exp.DependencyConstraints(), got.DependencyConstraints()
+		if !projectConstraintsEqual(exp, got) {
+			t.Errorf("expected constraints equal to %v but got %v", exp, got)


there's a (WNT):, (GOT): pattern used elsewhere in gps' tests. For consistency's sake, let's continue the pattern.

sdboyer · 2017-06-15T01:23:54Z

internal/gps/source_cache_test.go

+	}
+	sort.Strings(a.P.Imports)
+	sort.Strings(b.P.Imports)
+	if len(a.P.Imports) != len(b.P.Imports) {


nit, but let's sort after the length comparison so that we don't do an unnecessary sort.

sdboyer · 2017-06-15T01:25:21Z

internal/gps/source_cache_test.go

+
+// pairedVersionsEqual returns true if the PairedVersions have equal VersionTypes and Strings.
+func pairedVersionsEqual(a, b PairedVersion) bool {
+	return a.Type() == b.Type() && a.String() == b.String()


constraint and version equality are something i've struggled with implementing a few times, as the rabbit hole goes pretty deep. i suppose that doing it with string-based comparisons is a good start, but if we're serious about this then it may entail some more digging in.

I have some ideas to make this one more robust (at least we control all the implementing types, so worst case it is just cumbersome). I'll try to come up with some specific cases that break this approach first though.

👍

it's been a while since i've attempted it, but IIRC, at least part of the problem was that the use cases i was trying to service had differing notions of equality. unfortunately, i can't remember what those use cases were - but, something to keep an eye out for when you look at this.

sdboyer · 2017-06-15T01:27:20Z

internal/gps/source_cache_test.go

+	}
+
+	switch b.Intersect(a) {
+	case a, b:


interesting approach! sadly, i think this will fail if given range constraints from the semver lib, as those contain a slice and thus aren't comparable.

panic: runtime error: comparing uncomparable type semver.rangeConstraint 👎

I'll take another stab at this.

sdboyer · 2017-06-15T01:27:52Z

internal/gps/source_cache_test.go

+	}
+}
+
+// compareLocks compares two manifests and reports differences as test errors.


s/manifests/locks/

sdboyer · 2017-06-15T01:28:33Z

internal/gps/source_cache_test.go

+}
+
+// compareLocks compares two manifests and reports differences as test errors.
+func compareLocks(t *testing.T, exp, got Lock) {


why not reuse lock diffing for this?

sdboyer · 2017-06-15T01:30:55Z

for something this large, please break up later commits so it's easier for me to review the interdiff.

ibrasho · 2017-06-26T10:38:52Z

Something went the wrong after the merge. 😢

jmank88 · 2017-06-26T14:20:26Z

Something went the wrong after the merge. 😢

Yeah, I was trying to preserve the history. Perhaps a proper rebase would have been less jarring. The individual commits are more readable this way, but I can squash it all into a single tidy commit or a new branch with a few separate commits - if either would be easier to review.

Edit: Rebased

jmank88 · 2017-06-26T14:30:38Z

Added a Constraint.Equals method.

Notice that semverConstraint.Equals relies on semver.Constraint's Stringer behavior:

// Constraints compose the fmt.Stringer interface. This method is the
// bijective inverse of NewConstraint(): if a string yielded from this
// method is passed to NewConstraint(), a byte-identical instance of the
// original Constraint will be returend.

jmank88 · 2017-07-12T17:05:45Z

Rebased

jmank88 · 2017-07-13T12:47:09Z

codeclimate appears to have lost track of the branch

sdboyer · 2017-07-18T03:35:02Z

internal/gps/source_cache_test.go

+
+// compareManifests compares two manifests and reports differences as test errors.
+func compareManifests(t *testing.T, want, got Manifest) {
+	{


huh. i didn't know you could do this. this is just to provide a block scope for want and got to be localized within, right?

Yeah, it seemed cleaner than a bunch of 'want- Type' vars.

In general, I've found it handy to isolate small portions of longer functions that aren't candidates for entirely separate functions, if only to help readability regarding variable scope or to give some better scope to a comment than line breaks.

sdboyer

equality semantics are the worst

sdboyer · 2017-07-19T01:35:27Z

internal/gps/version.go

+	if !ok {
+		return false
+	}
+	return v == v2


this is one of those cases where we the definition of "equality" is really contextualized by how we use it. the default branch property on branchVersion is only set correctly by sourceMgr, not by anything that parses manifests. if we rely on this equals operator in the wrong place, then we have problems.

but, perhaps you intend that to be the difference between equals() and Matches()?

The idea was to have a method for verifying the cache serialization strategy, since == and reflect.DeepEquals are too strict. I have renamed the method to identical, and expanded the doc.

the default branch property on branchVersion is only set correctly by sourceMgr, not by anything that parses manifests.

The persistent cache handles this case (and hopefully all the other edge cases!).

sdboyer · 2017-07-19T01:37:44Z

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.


Per the other notes in this review, this should be clearer about the semantics of the equality check.

jmank88 · 2017-07-19T13:46:55Z

internal/gps/version_unifier.go

@@ -269,3 +269,12 @@ func (vtu versionTypeUnion) Intersect(c Constraint) Constraint {

 	return none
 }
+
+func (vtu versionTypeUnion) identical(c Constraint) bool {


I'm having second thoughts about this implementation....

hahaha, yeah, the vtu is a nightmare

Expanded to treat them as sets for which each element must have an identical member.

ibrasho · 2017-07-19T18:12:13Z

internal/gps/constraint_test.go

+		a, b Constraint
+		eq   bool
+	}{
+		{a: NewVersion("test"), b: NewVersion("test"), eq: true},


You can drop the keys here.

ibrasho · 2017-07-19T18:17:29Z

internal/gps/source_cache_test.go

+	const root = "example.com/test"
+
+	t.Run("info", func(t *testing.T) {
+		newSemverConstraint := func(s string) Constraint {


Didn't you already define the same function in internal/gps/constraint_test.go?

sdboyer · 2017-07-19T19:01:12Z

internal/gps/constraints.go

+	// 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.


If the serialization process is stringifying, then this isn't true for branches, because of that default branch prop 😢

It's more complicated than that. The encoding scheme is queued up for a future PR after this one (maybe the next, have to dig it out and rebase).

…ests; more tests

sdboyer · 2017-07-20T02:49:15Z

i think this is ready to merge, but i need those test greens first :/

sdboyer · 2017-07-21T02:47:22Z

good enough. in we go! 🎉

googlebot added the cla: yes label Jun 15, 2017

sdboyer suggested changes Jun 15, 2017

View reviewed changes

sdboyer added the area: gps label Jun 15, 2017

adding TestSingleSourceCache; better constraint equality

04f95df

jmank88 closed this Jul 12, 2017

jmank88 reopened this Jul 12, 2017

jmank88 closed this Jul 13, 2017

jmank88 reopened this Jul 13, 2017

sdboyer reviewed Jul 18, 2017

View reviewed changes

sdboyer closed this Jul 18, 2017

sdboyer reopened this Jul 18, 2017

sdboyer suggested changes Jul 19, 2017

View reviewed changes

rename 'equals' method to 'identical'; better comment

854158e

jmank88 commented Jul 19, 2017

View reviewed changes

jmank88 closed this Jul 19, 2017

jmank88 reopened this Jul 19, 2017

ibrasho reviewed Jul 19, 2017

View reviewed changes

sdboyer reviewed Jul 19, 2017

View reviewed changes

jmank88 added 2 commits July 19, 2017 16:12

improved versionTypeUnion identical(); dry up test helpers; cleaner t…

c59d77b

…ests; more tests

fix extra pointers

35c6387

sdboyer closed this Jul 20, 2017

sdboyer reopened this Jul 20, 2017

jmank88 closed this Jul 20, 2017

jmank88 reopened this Jul 20, 2017

sdboyer merged commit 20e2462 into golang:master Jul 21, 2017

jmank88 deleted the source_cache_tests branch February 2, 2018 21:07

adding TestSingleSourceCache #756

adding TestSingleSourceCache #756

Conversation

jmank88 commented Jun 15, 2017 • edited Loading

What does this do / why do we need it?

jmank88 commented Jun 15, 2017

sdboyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdboyer commented Jun 15, 2017

ibrasho commented Jun 26, 2017

jmank88 commented Jun 26, 2017 • edited Loading

jmank88 commented Jun 26, 2017 • edited Loading

jmank88 commented Jul 12, 2017

jmank88 commented Jul 13, 2017

Choose a reason for hiding this comment

jmank88 Jul 18, 2017 • edited Loading

Choose a reason for hiding this comment

sdboyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmank88 Jul 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmank88 Jul 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdboyer commented Jul 20, 2017

sdboyer commented Jul 21, 2017

jmank88 commented Jun 15, 2017 •

edited

Loading

jmank88 commented Jun 26, 2017 •

edited

Loading

jmank88 commented Jun 26, 2017 •

edited

Loading

jmank88 Jul 18, 2017 •

edited

Loading

jmank88 Jul 19, 2017 •

edited

Loading

jmank88 Jul 19, 2017 •

edited

Loading