testdriver: fix flakely parsing of version from vim --version output (#…

…624)

With the release of Vim 8.2 we uncovered a bug in the parsing of version
details from the output of vim --version. Fix that bug and add a comment
explaining why we're not doing version stuff in a better way to start
with.

govim.go

@@ -321,13 +321,7 @@ func (g *govimImpl) load() error {
 				return err
 			}
 			g.decodeJSON(v, &details)
-
-			l := details.VersionLong
-			maj := l / 1000000
-			min := (l / 10000) % 10
-			pat := l % 10000
-			g.version = fmt.Sprintf("v%v.%v.%v", maj, min, pat)
-
+			g.version = ParseVersionLong(details.VersionLong)
 			if details.GuiRunning == 1 {
 				g.flavor = FlavorGvim
 			} else {
@@ -975,3 +969,10 @@ type errProto struct {
 func (e errProto) Error() string {
 	return fmt.Sprintf("protocol error: %v", e.underlying)
 }
+
+func ParseVersionLong(l int) string {
+	maj := l / 1000000
+	min := (l / 10000) % 10
+	pat := l % 10000
+	return fmt.Sprintf("v%v.%v.%v", maj, min, pat)
+}

testdriver/testdriver.go

@@ -743,47 +743,29 @@ func Condition(cond string) (bool, error) {
 	}
 	switch f {
 	case govim.FlavorVim, govim.FlavorGvim:
-		cmd := cmd.BuildCommand("-v", "--version")
+		var allArgs []string
+		allArgs = append(allArgs, cmd...)
+		allArgs = append(allArgs, "-v", "--cmd", "echo v:versionlong | qall", "--not-a-term")
+		cmd := exec.Command(allArgs[0], allArgs[1:]...)
 		out, err := cmd.CombinedOutput()
 		if err != nil {
-			return false, fmt.Errorf("failed to run %v: %v\n%s", strings.Join(cmd.Args, " "), err, out)
+			return false, fmt.Errorf("failed to get v:versionlong value from Vim via %v: %v\n%s", strings.Join(cmd.Args, " "), err, out)
 		}
-		version, err := parseVimVersion(out)
+		versionStr := strings.TrimSpace(stripansi.Strip(string(out)))
+		versionInt, err := strconv.Atoi(versionStr)
 		if err != nil {
-			return false, err
+			return false, fmt.Errorf("failed to convert Vim v:versionlong value %v to an integer: %v", versionStr, err)
+		}
+		version := govim.ParseVersionLong(versionInt)
+		if err != nil {
+			return false, fmt.Errorf("failed to parse version from %v: %v", versionStr, err)
 		}
 		return semver.Compare(version, v) >= 0, nil
 	}
 
 	panic("should not reach here")
 }
 
-func parseVimVersion(b []byte) (string, error) {
-	lines := strings.Split(strings.TrimSpace(string(b)), "\n")
-	av := "v"
-	av += strings.Fields(lines[0])[4] // 5th element is the major.minor
-
-	// Depending on OS/build, the patch versions are printed on different lines
-	var patch string
-	for _, line := range lines {
-		if strings.Contains(line, ": ") {
-			patch = strings.Fields(line)[2]
-			patchI := strings.Index(patch, "-")
-			if patchI == -1 {
-				return "", fmt.Errorf("failed to parse patch version from %v", patch)
-			}
-			patch = patch[patchI+1:]
-			break
-		}
-	}
-	av += "." + patch
-	if !semver.IsValid(av) {
-		return "", fmt.Errorf("failed to calculate valid Vim version from %q; got %v", b, av)
-	}
-
-	return av, nil
-}
-
 type LockingBuffer struct {
 	lock          sync.Mutex
 	und           bytes.Buffer

version_test.go

@@ -0,0 +1,19 @@
+package govim
+
+import "testing"
+
+func TestParseVersionLong(t *testing.T) {
+	testVals := []struct {
+		in   int
+		want string
+	}{
+		{in: 8020000, want: "v8.2.0"},
+		{in: 8011711, want: "v8.1.1711"},
+	}
+	for _, v := range testVals {
+		got := ParseVersionLong(v.in)
+		if got != v.want {
+			t.Errorf("ParseVersionLong(%v) gave %q; want %q", v.in, got, v.want)
+		}
+	}
+}