Treat go panics as an interrupt error #2453
Conversation
There was a problem hiding this comment.
LGTM as well, but also agree with @mstoykov's desire for an integration test. With the recent cmd/ changes, it should be relatively easy to create an integration test that injects a mock JS extension that just calls panic() and then start a test that imports it and calls it.
Actually, it will probably be even better to have 2 integration tests, one that panics in the init context and one that panics in the VU context.
olegbespalov
force-pushed
the
feat/2354-panics
branch
from
163f011 to
3aeed9a
Compare
| @@ -186,7 +186,7 @@ func TestVUTags(t *testing.T) { | |||
| }) | |||
| } | |||
|
|
|||
| func TestAbortTest(t *testing.T) { //nolint: tparallel | |||
| func TestAbortTest(t *testing.T) { //nolint:tparallel | |||
There was a problem hiding this comment.
nit: I kind of prefer to not have unrelated changes in unrelated files like that ;)
There was a problem hiding this comment.
It sounds like a personal preference 🤔 Let me try to provide my two cents on why I'm doing this and continue doing it.
It's a common/best practice to leave the place cleaner than before. For example, this is a comment for a machine (linter) comment, and it doesn't require space in between. The same things I usually do when spotting any grammar issues. Such changes itself is super-minor and don't deserve a separated PR, so that's why I prefer not postpone fixing.
Does that make sense? Will it help if I'll do this in a separate commit?
olegbespalov
force-pushed
the
feat/2354-panics
branch
from
72e14c2 to
24d5940
Compare
olegbespalov
added
the
breaking change
What?
We change the behavior, and the go
panic's will be treated as the run interruption.Why?
Otherwise, it is hard to detect panics in the code
Changelog
We changed the behavior of how the k6 treats the panic that can happen during the execution of the JavaScript code. Previously the behavior was to catch the panic and log it as an error. Starting the
v0.38.0whenever k6 sees a panic, it logs an error like before, but more importantly, it will abort the script execution. This change should help identify and fix the issues earlier.Closes #2354