Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Heartbeat] Fix broken invocation of synth package #26228

Merged
merged 2 commits into from
Jun 10, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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://") {
Copy link
Member

@vigneshshanmugam vigneshshanmugam Jun 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we want to move this behind a flag? Just a thought though, what if users runs a custom locally modified synth version?

But I am also thinking, if they are using the flag, then we are in the same boat. your call.

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)
})
}
}