js: Print stack trace when exception in handleSummary() #3416
Conversation
js/runner.go
Outdated
| if handleSummaryFn, ok := goja.AssertFunction(fn); ok { | ||
| callbackResult, _, _, err = vu.runFn(summaryCtx, false, handleSummaryFn, nil, vu.Runtime.ToValue(summaryDataForJS)) | ||
| if err != nil { | ||
| errext.Fprint(r.preInitState.Logger, err) |
There was a problem hiding this comment.
Alternatively to what I suggested at https://github.com/grafana/k6/pull/3416/files#r1370043306, I could add another log line here, giving that feedback to the user in a different line (and maybe as debug?).
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3416 +/- ##
=======================================
Coverage 73.34% 73.34%
=======================================
Files 258 259 +1
Lines 19645 19653 +8
=======================================
+ Hits 14408 14415 +7
Misses 4352 4352
- Partials 885 886 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
LGTM in general 👏
Can we maybe try option 2 from https://github.com/grafana/k6/pull/3416/files#r1370141890
That is true, but maybe we can extend Line 669 in 036f8a0 |
Changed, as per what discussed "offline", thanks! |
Co-authored-by: Mihail Stoykov <312246+mstoykov@users.noreply.github.com>
What?
It splits the execution of the
handleSummary()wrapper, so if there's an exception on the script, it prints the stack trace first, then falls back to the default summary.It also moves the logic to print
errext.HasHintanderrext.Exceptiondetails for a given error into a shared function within theerrextpackage, following "the fprint pattern" (that accepts a "writer", in this case a "logger"), so it can be reused where it was used until now (rootCommand) and in the aforementioned exception handling.Why?
Currently, exceptions in
handleSummary()do not print stack, just a message that there was an exception, and that makes debugging it way harder than it should.Checklist
make lint) and all checks pass.make tests) and all tests pass.Related PR(s)/Issue(s)
Closes #3257
Other notes
I haven't added any new test because if I'm not wrong, I think all the logic changed is already covered by tests present in
summary_test.go, likeTestExceptionInHandleSummaryFallsBackToTextSummarythat precisely asserts that the default summary is used as summary when thehandleSummary()throws an exception.In fact, if I'm not wrong, the main behavior should not change, it's just a matter of bringing feedback to the user (logs) in a slightly different way (more clear/explicit).
Thanks!