From 68af7d8384e855973503fa2f82a531b763e39b8c Mon Sep 17 00:00:00 2001
From: Andrew Cholakian <andrew@andrewvc.com>
Date: Thu, 10 Jun 2021 16:12:40 -0500
Subject: [PATCH] Add tests, cleanup

---
 .../browser/source/validatepackage.go         |  8 +--
 .../browser/source/validatepackage_test.go    |  5 ++
 .../monitors/browser/synthexec/synthexec.go   | 10 ++--
 .../browser/synthexec/synthexec_test.go       | 53 +++++++++++++++++++
 4 files changed, 69 insertions(+), 7 deletions(-)

diff --git a/x-pack/heartbeat/monitors/browser/source/validatepackage.go b/x-pack/heartbeat/monitors/browser/source/validatepackage.go
index dbc5b88df582..bc606ffe8373 100644
--- a/x-pack/heartbeat/monitors/browser/source/validatepackage.go
+++ b/x-pack/heartbeat/monitors/browser/source/validatepackage.go
@@ -42,6 +42,10 @@ func parseVersion(version string) string {
 }
 
 func validateVersion(expected string, current string) error {
+	if strings.HasPrefix(current, "file://") {
+		return nil
+	}
+
 	expectedRange, err := semver.NewConstraint(expected)
 	if err != nil {
 		return err
@@ -76,10 +80,6 @@ func validatePackageJSON(path string) error {
 		synthVersion = pkgJson.DevDependencies.SynthVersion
 	}
 
-	if strings.HasPrefix(synthVersion, "file://") {
-		return nil
-	}
-
 	err = validateVersion(ExpectedSynthVersion, synthVersion)
 	if err != nil {
 		return fmt.Errorf("could not validate @elastic/synthetics version: '%s' %w", synthVersion, err)
diff --git a/x-pack/heartbeat/monitors/browser/source/validatepackage_test.go b/x-pack/heartbeat/monitors/browser/source/validatepackage_test.go
index c084078424b8..014e1fd2fa98 100644
--- a/x-pack/heartbeat/monitors/browser/source/validatepackage_test.go
+++ b/x-pack/heartbeat/monitors/browser/source/validatepackage_test.go
@@ -70,6 +70,11 @@ func TestValidateVersion(t *testing.T) {
 			current:   "0.0.1-alpha.11",
 			shouldErr: false,
 		},
+		{
+			expected:  "",
+			current:   "file://blahblahblah",
+			shouldErr: false,
+		},
 	}
 
 	for _, tt := range tests {
diff --git a/x-pack/heartbeat/monitors/browser/synthexec/synthexec.go b/x-pack/heartbeat/monitors/browser/synthexec/synthexec.go
index f0a6c51e81e5..3dc274061dbd 100644
--- a/x-pack/heartbeat/monitors/browser/synthexec/synthexec.go
+++ b/x-pack/heartbeat/monitors/browser/synthexec/synthexec.go
@@ -30,12 +30,12 @@ const debugSelector = "synthexec"
 func SuiteJob(ctx context.Context, suitePath string, params common.MapStr, extraArgs ...string) (jobs.Job, error) {
 	// Run the command in the given suitePath, use '.' as the first arg since the command runs
 	// in the correct dir
-	newCmd, err := suiteCommandFactory(suitePath, append([]string{suitePath}, extraArgs...)...)
+	cmdFactory, err := suiteCommandFactory(suitePath, extraArgs...)
 	if err != nil {
 		return nil, err
 	}
 
-	return startCmdJob(ctx, newCmd, nil, params), nil
+	return startCmdJob(ctx, cmdFactory, nil, params), nil
 }
 
 func suiteCommandFactory(suitePath string, args ...string) (func() *exec.Cmd, error) {
@@ -46,7 +46,11 @@ func suiteCommandFactory(suitePath string, args ...string) (func() *exec.Cmd, er
 
 	newCmd := func() *exec.Cmd {
 		bin := filepath.Join(npmRoot, "node_modules/.bin/elastic-synthetics")
-		cmd := exec.Command(bin, args...)
+		// Always put the suite path first to prevent conflation with variadic args!
+		// See https://github.com/tj/commander.js/blob/master/docs/options-taking-varying-arguments.md
+		// Note, we don't use the -- approach because it's cleaner to always know we can add new options
+		// to the end.
+		cmd := exec.Command(bin, append([]string{suitePath}, args...)...)
 		cmd.Dir = npmRoot
 		return cmd
 	}
diff --git a/x-pack/heartbeat/monitors/browser/synthexec/synthexec_test.go b/x-pack/heartbeat/monitors/browser/synthexec/synthexec_test.go
index 01f7f8649070..730df8a8ec24 100644
--- a/x-pack/heartbeat/monitors/browser/synthexec/synthexec_test.go
+++ b/x-pack/heartbeat/monitors/browser/synthexec/synthexec_test.go
@@ -157,3 +157,56 @@ Loop:
 		})
 	}
 }
+
+func TestSuiteCommandFactory(t *testing.T) {
+	_, filename, _, _ := runtime.Caller(0)
+	origPath := path.Join(filepath.Dir(filename), "../source/fixtures/todos")
+	suitePath, err := filepath.Abs(origPath)
+	require.NoError(t, err)
+	binPath := path.Join(suitePath, "node_modules/.bin/elastic-synthetics")
+
+	tests := []struct {
+		name      string
+		suitePath string
+		extraArgs []string
+		want      []string
+		wantErr   bool
+	}{
+		{
+			"no args",
+			suitePath,
+			nil,
+			[]string{binPath, suitePath},
+			false,
+		},
+		{
+			"with args",
+			suitePath,
+			[]string{"--capability", "foo", "bar", "--rich-events"},
+			[]string{binPath, suitePath, "--capability", "foo", "bar", "--rich-events"},
+			false,
+		},
+		{
+			"no npm root",
+			"/not/a/path/for/sure",
+			nil,
+			nil,
+			true,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			factory, err := suiteCommandFactory(tt.suitePath, tt.extraArgs...)
+
+			if tt.wantErr {
+				require.Error(t, err)
+				return
+			}
+			require.NoError(t, err)
+
+			cmd := factory()
+			got := cmd.Args
+			require.Equal(t, tt.want, got)
+		})
+	}
+}