Skip to content

Commit

Permalink
[Heartbeat] Fix broken invocation of synth package (#26228)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
andrewvc authored Jun 10, 2021
1 parent 30b9c8f commit ad7c19f
Show file tree
Hide file tree
Showing 4 changed files with 70 additions and 3 deletions.
5 changes: 5 additions & 0 deletions x-pack/heartbeat/monitors/browser/source/validatepackage.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
10 changes: 7 additions & 3 deletions x-pack/heartbeat/monitors/browser/synthexec/synthexec.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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
}
Expand Down
53 changes: 53 additions & 0 deletions x-pack/heartbeat/monitors/browser/synthexec/synthexec_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
})
}
}

0 comments on commit ad7c19f

Please sign in to comment.