-
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
Add summary to journeys which don't emit journey:end (early node subprocess exits) #29606
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,7 +18,7 @@ func TestExecMultiplexer(t *testing.T) { | |
var testJourneys []*Journey | ||
var testEvents []*SynthEvent | ||
time := float64(0) | ||
for jIdx := 0; jIdx < 3; jIdx++ { | ||
for jIdx := 0; jIdx < 4; jIdx++ { | ||
time++ // fake time to make events seem spaced out | ||
journey := &Journey{ | ||
Name: fmt.Sprintf("J%d", jIdx), | ||
|
@@ -45,11 +45,20 @@ func TestExecMultiplexer(t *testing.T) { | |
}) | ||
} | ||
|
||
testEvents = append(testEvents, &SynthEvent{ | ||
Journey: journey, | ||
Type: "journey/end", | ||
TimestampEpochMicros: time, | ||
}) | ||
// We want one of the test journeys to end with a cmd/status indicating it failed | ||
if jIdx != 4 { | ||
testEvents = append(testEvents, &SynthEvent{ | ||
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. As per previous comment, lets add both and see what happens with summary creation. |
||
Journey: journey, | ||
Type: "journey/end", | ||
TimestampEpochMicros: time, | ||
}) | ||
} else { | ||
testEvents = append(testEvents, &SynthEvent{ | ||
Journey: journey, | ||
Type: "cmd/status", | ||
TimestampEpochMicros: time, | ||
}) | ||
} | ||
} | ||
|
||
// Write the test events in another go routine since writes block | ||
|
@@ -77,7 +86,7 @@ Loop: | |
i := 0 // counter for index, resets on journey change | ||
for _, se := range results { | ||
require.Equal(t, i, se.index) | ||
if se.Type == "journey/end" { | ||
if se.Type == "journey/end" || se.Type == "cmd/status" { | ||
i = 0 | ||
} else { | ||
i++ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -196,8 +196,9 @@ func runCmd( | |
if err != nil { | ||
str := fmt.Sprintf("command exited with status %d: %s", cmd.ProcessState.ExitCode(), err) | ||
mpx.writeSynthEvent(&SynthEvent{ | ||
Type: "cmd/status", | ||
Error: &SynthError{Name: "cmdexit", Message: str}, | ||
Type: "cmd/status", | ||
Error: &SynthError{Name: "cmdexit", Message: str}, | ||
TimestampEpochMicros: float64(time.Now().UnixMicro()), | ||
}) | ||
logp.Warn("Error executing command '%s' (%d): %s", loggableCmd.String(), cmd.ProcessState.ExitCode(), err) | ||
} | ||
|
@@ -243,7 +244,7 @@ func lineToSynthEventFactory(typ string) func(bytes []byte, text string) (res *S | |
logp.Info("%s: %s", typ, text) | ||
return &SynthEvent{ | ||
Type: typ, | ||
TimestampEpochMicros: float64(time.Now().UnixNano() / int64(time.Millisecond)), | ||
TimestampEpochMicros: float64(time.Now().UnixMicro()), | ||
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 am not sure about the intended consequence of this change, I will leave it to @andrewvc who knows it better than me. 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. This change is because timestamps calculated in that way would yield negative values, and thus not appear in my queries (I was filtering for events in the last X years).
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 had no idea the |
||
Payload: map[string]interface{}{ | ||
"message": text, | ||
}, | ||
|
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.
We should guard this summary creation event. If the user has afterAll hook that throws error, we would have both
journey/end
followed bycmd/status
.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.
This is a sort of 'task failed successfully' situation. I guess there's a question here, does a failing after hook invalidate the test result at all? It feels weird to, say, trigger an alert based on an
afterAll
hook.I'm wondering if we really would need to introduce a new concept of warnings into the schema / UI. Thoughts?
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 don't think so, Lots of test runners don't consider failures in the afterAll hooks to be failures. They mainly exist to keep some cleanup mechanism. But having warnings in the UI sounds like a cool idea.
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.
A comment on failures on
afterX
hooksThe
afterAll
hook triggering a failure is a tricky situation. I previously did some work on Jest's hooks here (jestjs/jest#8654) where this same discussion has come up.Given how complex the discussion on hooks semantics is, we didn't get to a conclusion in that PR (which is why it's still open and not much has happened since then).
Anyway, I do feel like failures in
afterHooks
should be test failures. If you think about how hooks could be implemented using a procedural API under the hood, they end up being part of the upper test scope, and thus would trigger failures. For inspiration, see Deno's RFC on their testing API: denoland/deno#10771.Proposed way forward
I'd consider that failing a journey would be out of the scope of this PR if you agree.
For now I'll follow @vigneshshanmugam suggestion of having both events given there's not necessarily success/failure tied to that (as
journey/end
indicates the journey for the hook finished anyway). However, **thecmd/status
event will not include a summary as it refers to a journey itself.IMO, moving forward, it would be great to have warnings based on
cmd/status
events as @andrewvc mentioned too.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.
++, Lets do this separately and open a issue in Synthetics runner.
Mainly, in the runner i didn't go with this idea as we didn't have a good way of reporting the error.
Warnings
would make a lot of sense here.