testdriver: fix data race introduced in #1101

With #1101 a data race was accidentally introduced where the global
testsetup.VimCommand was overwritten.
We now make a local copy that is modified instead.

Fixing this revealed that #1101 wasn't enough. Testscript conditions
that matches a vim version also used the testscript subcommand instead
of real vim. It now uses the real vim.

Closes #1104

cmd/govim/main_test.go

@@ -42,6 +42,8 @@ func init() {
 }
 
 func TestMain(m *testing.M) {
+	// Snapshot PATH since it is prepended by testscript.
+	os.Setenv(testsetup.EnvTestPathEnv, os.Getenv("PATH"))
 	os.Exit(testscript.RunMain(m, map[string]func() int{
 		"vim":         testdriver.Vim,
 		"vimexprwait": testdriver.VimExprWait,
@@ -340,7 +342,7 @@ func execvim() int {
 	// testscript prepends PATH with the directory that contain
 	// subcommands. Since we are going to execute vim here, and also
 	// have a subcommand named "vim", we need to find the real vim path.
-	v, err := testsetup.RealVimPath()
+	v, err := testsetup.LookPath("vim")
 	if err != nil {
 		fmt.Fprintf(os.Stderr, "failed to locate vim: %v", err)
 		return 1

govim_test.go

@@ -29,6 +29,8 @@ var (
 )
 
 func TestMain(m *testing.M) {
+	// Snapshot PATH since it is prepended by testscript.
+	os.Setenv(testsetup.EnvTestPathEnv, os.Getenv("PATH"))
 	os.Exit(testscript.RunMain(m, map[string]func() int{
 		"vim": testdriver.Vim,
 	}))

testdriver/testdriver.go

@@ -196,7 +196,8 @@ func NewTestDriver(c *Config) (*TestDriver, error) {
 		"GOVIMTESTDRIVER_SOCKET="+res.driverListener.Addr().String(),
 	)
 
-	vimCmd := cmd
+	vimCmd := make([]string, len(cmd))
+	copy(vimCmd, cmd)
 	if e := os.Getenv("VIM_COMMAND"); e != "" {
 		vimCmd = strings.Fields(e)
 	}
@@ -220,7 +221,7 @@ func NewTestDriver(c *Config) (*TestDriver, error) {
 		if vimCmd[i] != "vim" {
 			continue
 		}
-		abs, err := testsetup.RealVimPath()
+		abs, err := testsetup.LookPath("vim")
 		if err != nil {
 			return nil, fmt.Errorf("failed to find vim binary: %v", err)
 		}
@@ -249,6 +250,7 @@ func NewTestDriver(c *Config) (*TestDriver, error) {
 func (d *TestDriver) Logf(format string, a ...interface{}) {
 	fmt.Fprintf(d.log, format+"\n", a...)
 }
+
 func (d *TestDriver) LogStripANSI(r io.Reader) {
 	scanner := bufio.NewScanner(r)
 	for {
@@ -588,7 +590,6 @@ func (d *TestDriver) listenDriver() error {
 		}
 		return nil
 	})
-
 	if err != nil {
 		return fmt.Errorf("%v", err)
 	}
@@ -933,8 +934,22 @@ func Condition(cond string) (bool, error) {
 }
 
 func getVimFlavourVersion(c testsetup.Command) (string, error) {
-	var allArgs []string
-	allArgs = append(allArgs, c...)
+	allArgs := make([]string, len(c))
+	// testscript prepends PATH with the directory that contain
+	// subcommands. Since we are going to execute vim here, and also
+	// also have a subcommand named "vim", we must specify the entire path if
+	// "vim" is a part of c.
+	for i := range c {
+		if c[i] != "vim" {
+			allArgs[i] = c[i]
+			continue
+		}
+		abs, err := testsetup.LookPath("vim")
+		if err != nil {
+			return "", fmt.Errorf("failed to find vim binary: %v", err)
+		}
+		allArgs[i] = abs
+	}
 	allArgs = append(allArgs, "-v", "--cmd", "echo v:versionlong | qall", "--not-a-term")
 	cmd := exec.Command(allArgs[0], allArgs[1:]...)
 	out, err := cmd.CombinedOutput()

testsetup/testsetup.go

@@ -41,6 +41,11 @@ const (
 	// EnvTestRaceSlowdown is a floating point factor by which to adjust
 	// EnvErrLogMatchWait for race tests
 	EnvTestRaceSlowdown = "GOVIM_TEST_RACE_SLOWDOWN"
+
+	// EnvTestPathEnv is used to override PATH when looking for executables.
+	// Testscript prepends PATH with the subcommand folder, so EnvTestPathEnv
+	// acts as a snapshot of PATH before it was modified.
+	EnvTestPathEnv = "GOVIM_TEST_PATH_ENV"
 )
 
 // vim versions
@@ -157,25 +162,43 @@ func EnvLookupFlavorCommand() (flav govim.Flavor, cmd Command, err error) {
 	return flav, cmd, nil
 }
 
-// RealVimPath iterates over PATH returning the second vim
-// executable found. Reason being that testscript prepends
-// the PATH with the subcommand dir causing a conflict where
-// the first vim found is the subcommand.
-func RealVimPath() (path string, err error) {
-	var firstFound bool
-	for _, dir := range filepath.SplitList(os.Getenv("PATH")) {
-		path := filepath.Join(dir, "vim")
-		d, err := os.Stat(path)
+// LookPath searches for an executable named file, using EnvTestPathEnv as PATH
+// (if set). The ordinary PATH env is used if it isn't set.
+// If file contains a slash, it is tried directly.
+// The result may be an absolute path or a path relative to the current directory.
+func LookPath(file string) (string, error) {
+	findExecutable := func(file string) error {
+		d, err := os.Stat(file)
 		if err != nil {
-			continue
+			return err
+		}
+		if m := d.Mode(); !m.IsDir() && m&0111 != 0 {
+			return nil
+		}
+		return os.ErrPermission
+	}
+
+	if strings.Contains(file, "/") {
+		err := findExecutable(file)
+		if err == nil {
+			return file, nil
 		}
-		if m := d.Mode(); m.IsDir() || m&0111 == 0 {
-			continue
+		return "", err
+	}
+
+	pathEnv := os.Getenv(EnvTestPathEnv)
+	if pathEnv == "" {
+		pathEnv = os.Getenv("PATH")
+	}
+	for _, dir := range filepath.SplitList(pathEnv) {
+		if dir == "" {
+			// Unix shell semantics: path element "" means "."
+			dir = "."
 		}
-		if firstFound {
+		path := filepath.Join(dir, file)
+		if err := findExecutable(path); err == nil {
 			return path, nil
 		}
-		firstFound = true
 	}
-	return "", fmt.Errorf("not found")
+	return "", fmt.Errorf("failed to find executable in provided path")
 }