fix bug that treated all zeroes as if they were leading zeroes; add test coverage (#3)

Author: eli-darkly

.circleci/config.yml

@@ -95,7 +95,7 @@ jobs:
       - checkout
       - run: go version
       - run: go build ./...
-      - run: go test -race -v ./...
+      - run: go test ./...
 
   benchmarks:
     docker:

Makefile

@@ -20,7 +20,7 @@ clean:
 	go clean
 
 test: build
-	go test -race -v ./...
+	go test ./...
 
 benchmarks: build
 	mkdir -p ./build

parse_test.go

@@ -1,12 +1,19 @@
 package semver
 
 import (
+	"fmt"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
+// In order to catch parsing bugs that depend on the number or order of digits, we use ValuesGenerator
+// (in values_generator.go) to create permutations of many integer values. However, as a practical
+// measure to avoid very long test times, we don't use an equally large range for every value: when
+// generating major, minor, and patch version numbers, we cover a 3-digit range for the major and only
+// one digit for the others, on the assumption that the same numeric parsing logic is used for each.
+
 func assertVersionComponents(t *testing.T, v Version, major int, minor int, patch int, pre string, b string) {
 	assert.Equal(t, major, v.GetMajor())
 	assert.Equal(t, minor, v.GetMinor())
@@ -36,8 +43,12 @@ func TestParseStrict(t *testing.T) {
 	strictParsingTests := func(t *testing.T, parseFn func(string) (Version, error)) {
 		parsingTestsForAnyMode(t, parseFn)
 
-		t.Run("patch version cannot be omitted", parsingShouldFail(parseFn, "2.3"))
-		t.Run("micro and patch versions cannot be omitted", parsingShouldFail(parseFn, "2"))
+		NewValuesGenerator().AddValue(0, 199).AddValue(0, 9).TestAll2(t, func(t *testing.T, a, b int) {
+			t.Run("patch version cannot be omitted", parsingShouldFail(parseFn, fmt.Sprintf("%d.%d", a, b)))
+		})
+		NewValuesGenerator().AddValue(0, 199).TestAll1(t, func(t *testing.T, a int) {
+			t.Run("micro and patch versions cannot be omitted", parsingShouldFail(parseFn, fmt.Sprintf("%d", a)))
+		})
 	}
 
 	t.Run("Parse(s)", func(t *testing.T) {
@@ -54,86 +65,94 @@ func TestParseAllowMissingMinorAndPatch(t *testing.T) {
 
 	parsingTestsForAnyMode(t, parseFn)
 
-	t.Run("patch version can be omitted", parsingShouldSucceed(parseFn,
-		"2.3", 2, 3, 0, "", ""))
+	NewValuesGenerator().AddValue(0, 199).AddValue(0, 9).TestAll2(t, func(t *testing.T, a, b int) {
+		s := fmt.Sprintf("%d.%d", a, b)
+
+		t.Run("patch version can be omitted", parsingShouldSucceed(parseFn,
+			s, a, b, 0, "", ""))
 
-	t.Run("patch version can be omitted with prerelease", parsingShouldSucceed(parseFn,
-		"2.3-beta1", 2, 3, 0, "beta1", ""))
+		t.Run("patch version can be omitted with prerelease", parsingShouldSucceed(parseFn,
+			s+"-beta1", a, b, 0, "beta1", ""))
+
+		t.Run("patch version can be omitted with build", parsingShouldSucceed(parseFn,
+			s+"+build1", a, b, 0, "", "build1"))
+	})
 
-	t.Run("patch version can be omitted with build", parsingShouldSucceed(parseFn,
-		"2.3+build1", 2, 3, 0, "", "build1"))
+	NewValuesGenerator().AddValue(0, 199).TestAll1(t, func(t *testing.T, a int) {
+		s := fmt.Sprintf("%d", a)
 
-	t.Run("micro and patch versions can be omitted", parsingShouldSucceed(parseFn,
-		"2", 2, 0, 0, "", ""))
+		t.Run("micro and patch versions can be omitted", parsingShouldSucceed(parseFn,
+			s, a, 0, 0, "", ""))
 
-	t.Run("micro and patch versions can be omitted with prerelease", parsingShouldSucceed(parseFn,
-		"2-beta1", 2, 0, 0, "beta1", ""))
+		t.Run("micro and patch versions can be omitted with prerelease", parsingShouldSucceed(parseFn,
+			s+"-beta1", a, 0, 0, "beta1", ""))
 
-	t.Run("micro and patch versions can be omitted with build", parsingShouldSucceed(parseFn,
-		"2+build1", 2, 0, 0, "", "build1"))
+		t.Run("micro and patch versions can be omitted with build", parsingShouldSucceed(parseFn,
+			s+"+build1", a, 0, 0, "", "build1"))
+	})
 }
 
 func parsingTestsForAnyMode(t *testing.T, parseFn func(string) (Version, error)) {
 	t.Run("valid", func(t *testing.T) {
-		t.Run("simple complete version", parsingShouldSucceed(parseFn,
-			"2.3.4", 2, 3, 4, "", ""))
-
-		t.Run("version with prerelease (single identifier)", parsingShouldSucceed(parseFn,
-			"2.3.4-beta1", 2, 3, 4, "beta1", ""))
-
-		t.Run("version with prerelease (multi-identifier)", parsingShouldSucceed(parseFn,
-			"2.3.4-beta1.2", 2, 3, 4, "beta1.2", ""))
-
-		t.Run("version with prerelease (multi-identifier with hyphens)", parsingShouldSucceed(parseFn,
-			"2.3.4-beta1-final.2", 2, 3, 4, "beta1-final.2", ""))
+		NewValuesGenerator().AddValue(0, 199).AddValue(0, 9).AddValue(0, 9).
+			TestAll3(t, func(t *testing.T, a, b, c int) {
+				s := fmt.Sprintf("%d.%d.%d", a, b, c)
+				t.Run("simple complete version", parsingShouldSucceed(parseFn,
+					s, a, b, c, "", ""))
 
-		t.Run("version with build (single identifier)", parsingShouldSucceed(parseFn,
-			"2.3.4+build2", 2, 3, 4, "", "build2"))
+				t.Run("version with prerelease (single identifier)", parsingShouldSucceed(parseFn,
+					s+"-beta1", a, b, c, "beta1", ""))
 
-		t.Run("version with build (multi-identifier)", parsingShouldSucceed(parseFn,
-			"2.3.4+build2.4", 2, 3, 4, "", "build2.4"))
+				t.Run("version with build (single identifier)", parsingShouldSucceed(parseFn,
+					s+"+build2", a, b, c, "", "build2"))
 
-		t.Run("version with build (multi-identifier with hyphens)", parsingShouldSucceed(parseFn,
-			"2.3.4+build2-other.4", 2, 3, 4, "", "build2-other.4"))
+				t.Run("prerelease identifier of only letters", parsingShouldSucceed(parseFn,
+					s+"-abc", a, b, c, "abc", ""))
 
-		t.Run("version with prerelease and build", parsingShouldSucceed(parseFn,
-			"2.3.4-beta1.rc2+build2.4", 2, 3, 4, "beta1.rc2", "build2.4"))
+				t.Run("prerelease identifier of only digits", parsingShouldSucceed(parseFn,
+					s+"-123", a, b, c, "123", ""))
 
-		t.Run("major version zero", parsingShouldSucceed(parseFn,
-			"0.3.4", 0, 3, 4, "", ""))
+				t.Run("prerelease identifier of only hyphens", parsingShouldSucceed(parseFn,
+					s+"----", a, b, c, "---", ""))
 
-		t.Run("minor version zero", parsingShouldSucceed(parseFn,
-			"2.0.4", 2, 0, 4, "", ""))
+				t.Run("alphanumeric prerelease identifier with leading zero", parsingShouldSucceed(parseFn,
+					s+"-beta1.0yes", a, b, c, "beta1.0yes", ""))
 
-		t.Run("patch version zero", parsingShouldSucceed(parseFn,
-			"2.3.0", 2, 3, 0, "", ""))
+				t.Run("build identifier of only letters", parsingShouldSucceed(parseFn,
+					s+"+abc", a, b, c, "", "abc"))
 
-		t.Run("prerelease identifier of only letters", parsingShouldSucceed(parseFn,
-			"2.3.4-abc", 2, 3, 4, "abc", ""))
+				t.Run("build identifier of only digits", parsingShouldSucceed(parseFn,
+					s+"+123", a, b, c, "", "123"))
 
-		t.Run("prerelease identifier of only digits", parsingShouldSucceed(parseFn,
-			"2.3.4-123", 2, 3, 4, "123", ""))
+				t.Run("build identifier of only hyphens", parsingShouldSucceed(parseFn,
+					s+"+---", a, b, c, "", "---"))
 
-		t.Run("prerelease identifier of only hyphens", parsingShouldSucceed(parseFn,
-			"2.3.4----", 2, 3, 4, "---", ""))
+				t.Run("alphanumeric build identifier with leading zero", parsingShouldSucceed(parseFn,
+					s+"+build1.0yes", a, b, c, "", "build1.0yes"))
 
-		t.Run("alphanumeric prerelease identifier with leading zero", parsingShouldSucceed(parseFn,
-			"2.3.4-beta1.0yes", 2, 3, 4, "beta1.0yes", ""))
+				t.Run("leading zero in numeric build identifier", parsingShouldSucceed(parseFn,
+					s+"+build1.02", a, b, c, "", "build1.02"))
+			})
 
-		t.Run("build identifier of only letters", parsingShouldSucceed(parseFn,
-			"2.3.4+abc", 2, 3, 4, "", "abc"))
+		NewValuesGenerator().AddValue(0, 199).AddValue(0, 9).AddValue(0, 9).AddValue(0, 2).
+			TestAll4(t, func(t *testing.T, a, b, c, d int) {
+				s := fmt.Sprintf("%d.%d.%d", a, b, c)
+				dd := fmt.Sprintf("%d", d)
+				t.Run("version with prerelease (multi-identifier)", parsingShouldSucceed(parseFn,
+					s+"-beta1."+dd, a, b, c, "beta1."+dd, ""))
 
-		t.Run("build identifier of only digits", parsingShouldSucceed(parseFn,
-			"2.3.4+123", 2, 3, 4, "", "123"))
+				t.Run("version with prerelease (multi-identifier with hyphens)", parsingShouldSucceed(parseFn,
+					s+"-beta1-final."+dd, a, b, c, "beta1-final."+dd, ""))
 
-		t.Run("build identifier of only hyphens", parsingShouldSucceed(parseFn,
-			"2.3.4+---", 2, 3, 4, "", "---"))
+				t.Run("version with build (multi-identifier)", parsingShouldSucceed(parseFn,
+					s+"+build2."+dd, a, b, c, "", "build2."+dd))
 
-		t.Run("alphanumeric build identifier with leading zero", parsingShouldSucceed(parseFn,
-			"2.3.4+build1.0yes", 2, 3, 4, "", "build1.0yes"))
+				t.Run("version with build (multi-identifier with hyphens)", parsingShouldSucceed(parseFn,
+					s+"+build2-other."+dd, a, b, c, "", "build2-other."+dd))
 
-		t.Run("leading zero in numeric build identifier", parsingShouldSucceed(parseFn,
-			"2.3.4+build1.02", 2, 3, 4, "", "build1.02"))
+				t.Run("version with prerelease and build", parsingShouldSucceed(parseFn,
+					s+"-beta1.rc2+build2."+dd, a, b, c, "beta1.rc2", "build2."+dd))
+			})
 	})
 
 	t.Run("invalid", func(t *testing.T) {

syntax.go

@@ -37,7 +37,7 @@ func parsePositiveNumericString(s string) (int, bool) {
 		if ch < '0' || ch > '9' {
 			return 0, false
 		}
-		if ch == '0' && max > 1 {
+		if ch == '0' && i == 0 && max > 1 {
 			return 0, false // leading zeroes aren't allowed
 		}
 		n = n*10 + (int(ch) - int('0'))

values_generator.go

@@ -0,0 +1,88 @@
+package semver
+
+import (
+	"fmt"
+	"testing"
+)
+
+type ValuesGenerator struct {
+	valueDefs []valueConstraint
+}
+
+type valueConstraint struct {
+	min int
+	max int
+}
+
+func NewValuesGenerator() *ValuesGenerator {
+	return &ValuesGenerator{}
+}
+
+func (g *ValuesGenerator) AddValue(min, max int) *ValuesGenerator {
+	g.valueDefs = append(g.valueDefs, valueConstraint{min, max})
+	return g
+}
+
+func (g ValuesGenerator) MakeAllPermutations() [][]int {
+	var ret [][]int
+	numValues := len(g.valueDefs)
+	values := make([]int, numValues)
+	for i := 0; i < numValues; i++ {
+		values[i] = g.valueDefs[i].min
+	}
+	for {
+		copied := make([]int, numValues)
+		copy(copied, values)
+		ret = append(ret, copied)
+
+		// increment the values starting at the left, rolling over and incrementing the next to the right
+		for pos := 0; pos < numValues; pos++ {
+			if values[pos] < g.valueDefs[pos].max {
+				values[pos]++
+				break
+			}
+			if pos == numValues-1 {
+				return ret // we've covered all permutations
+			}
+			values[pos] = g.valueDefs[pos].min
+		}
+	}
+}
+
+func (g ValuesGenerator) TestAll(t *testing.T, action func(*testing.T, []int)) {
+	var permutations = g.MakeAllPermutations()
+	for _, perm := range permutations {
+		values := perm
+		desc := "values"
+		for _, v := range values {
+			desc = desc + fmt.Sprintf(" %d", v)
+		}
+		t.Run(desc, func(t *testing.T) {
+			action(t, values)
+		})
+	}
+}
+
+func (g ValuesGenerator) TestAll1(t *testing.T, action func(*testing.T, int)) {
+	g.TestAll(t, func(t *testing.T, values []int) {
+		action(t, values[0])
+	})
+}
+
+func (g ValuesGenerator) TestAll2(t *testing.T, action func(*testing.T, int, int)) {
+	g.TestAll(t, func(t *testing.T, values []int) {
+		action(t, values[0], values[1])
+	})
+}
+
+func (g ValuesGenerator) TestAll3(t *testing.T, action func(*testing.T, int, int, int)) {
+	g.TestAll(t, func(t *testing.T, values []int) {
+		action(t, values[0], values[1], values[2])
+	})
+}
+
+func (g ValuesGenerator) TestAll4(t *testing.T, action func(*testing.T, int, int, int, int)) {
+	g.TestAll(t, func(t *testing.T, values []int) {
+		action(t, values[0], values[1], values[2], values[3])
+	})
+}