[Heartbeat] Fix broken invocation of synth package (#26228)

andrewvc · web-flow · commit ad7c19fdcf1c · 2021-06-10T17:35:43.000-05:00
Fixes invocation of synthetics broken in #26188 Additionally, for dev purposes, this lets accepts a synthetics version of file:/// as valid to help with debugging development versions of the synthetics agent. Never released, so no changelog needed Still just sticking with unit tests here since testing more deeply with synthetics (esp. master where this is a problem) is a larger problem than we're equipped to handle ATM.
diff --git 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
@@ -75,6 +79,7 @@ func validatePackageJSON(path string) error {
 	if synthVersion == "" {
 		synthVersion = pkgJson.DevDependencies.SynthVersion
 	}
+
 	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
@@ -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
@@ -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(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
@@ -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)
+		})
+	}
+}
