-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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] Add browser monitor timeout #32434
Changes from all commits
ec49d8f
7eff66f
f1f6676
814bf0a
61e90e4
5557c2d
f4f7e0a
699a13b
7dcf441
a76d378
bc30562
3c1a77c
39eba7c
581656d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -99,7 +99,7 @@ func TestRunCmd(t *testing.T) { | |
cmd := exec.Command("go", "run", "./main.go") | ||
|
||
stdinStr := "MY_STDIN" | ||
synthEvents := runAndCollect(t, cmd, stdinStr) | ||
synthEvents := runAndCollect(t, cmd, stdinStr, 15*time.Minute) | ||
|
||
t.Run("has echo'd stdin to stdout", func(t *testing.T) { | ||
stdoutEvents := eventsWithType(Stdout, synthEvents) | ||
|
@@ -133,7 +133,7 @@ func TestRunCmd(t *testing.T) { | |
|
||
func TestRunBadExitCodeCmd(t *testing.T) { | ||
cmd := exec.Command("go", "run", "./main.go", "exit") | ||
synthEvents := runAndCollect(t, cmd, "") | ||
synthEvents := runAndCollect(t, cmd, "", 15*time.Minute) | ||
|
||
// go run outputs "exit status 123" to stderr so we have two messages | ||
require.Len(t, synthEvents, 2) | ||
|
@@ -149,11 +149,26 @@ func TestRunBadExitCodeCmd(t *testing.T) { | |
}) | ||
} | ||
|
||
func runAndCollect(t *testing.T, cmd *exec.Cmd, stdinStr string) []*SynthEvent { | ||
func TestRunTimeoutExitCodeCmd(t *testing.T) { | ||
cmd := exec.Command("go", "run", "./main.go") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like this test, are we sure that there isn't a race here? I wonder if on fast systems that might execute fast enough to be flaky. It might be safer to add a sleep into that go program. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a small timeout to the executable just in case |
||
synthEvents := runAndCollect(t, cmd, "", 0*time.Second) | ||
|
||
// go run should not produce any additional stderr output in this case | ||
require.Len(t, synthEvents, 1) | ||
|
||
t.Run("has a cmd status event", func(t *testing.T) { | ||
stdoutEvents := eventsWithType(CmdStatus, synthEvents) | ||
require.Len(t, stdoutEvents, 1) | ||
require.Equal(t, synthEvents[0].Error.Code, "CMD_TIMEOUT") | ||
}) | ||
} | ||
|
||
func runAndCollect(t *testing.T, cmd *exec.Cmd, stdinStr string, cmdTimeout time.Duration) []*SynthEvent { | ||
_, filename, _, _ := runtime.Caller(0) | ||
cmd.Dir = path.Join(filepath.Dir(filename), "testcmd") | ||
ctx := context.WithValue(context.TODO(), SynthexecTimeout, cmdTimeout) | ||
|
||
mpx, err := runCmd(context.TODO(), cmd, &stdinStr, nil, FilterJourneyConfig{}) | ||
mpx, err := runCmd(ctx, cmd, &stdinStr, nil, FilterJourneyConfig{}) | ||
require.NoError(t, err) | ||
|
||
var synthEvents []*SynthEvent | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we discussed the kill/warning is a problem since this will happen even during runs that are not broken. I think the simplest way to do that would be to define an atomic bool that flips when
cmd.Wait
returns and check that here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than use a switch, which introduces a (small) state dependency between the two routines, I'd rather check the exit status with
if !cmd.ProcessState.Exited() { // kill and log error}
. wdyt?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't realize that existed, that's perfect