Don't print empty goja stacktraces on captured panics #3749
Conversation
This has been broken for years after some goja refactoring. There doesn't seem to be a way for this to be fixed without a lot of boilerplate all over the code. And this functionality has not been used in the last few years in my experience. closes #1906
mstoykov
requested review from
oleiade and
joanlopez
and removed request for
a team
| @@ -65,21 +65,15 @@ func ToString(data interface{}) (string, error) { | |||
| } | |||
|
|
|||
| // RunWithPanicCatching catches panic and converts into an InterruptError error that should abort a script | |||
| func RunWithPanicCatching(logger logrus.FieldLogger, rt *goja.Runtime, fn func() error) (err error) { | |||
| func RunWithPanicCatching(logger logrus.FieldLogger, _ *goja.Runtime, fn func() error) (err error) { | |||
There was a problem hiding this comment.
I specifically skipped on cleaning this up as it is currently in js/common package and I didn't want to introduce a potential breaking change.
Depending on the decision from #3748 we might delete it.
If we decided against it - I will probably want to move this off common and in hte js package as I don't think this is functionality anyone but k6 should have.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3749 +/- ##
=======================================
Coverage 70.85% 70.85%
=======================================
Files 289 289
Lines 21159 21156 -3
=======================================
- Hits 14992 14991 -1
+ Misses 5210 5208 -2
Partials 957 957
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Do you mean that |
|
yes @joanlopez - have you noticed something different when there is a panic ? |
What?
Stop printing an empty stacktrace of goja when capturing panics.
Why?
This has been broken for years after some goja refactoring.
There doesn't seem to be a way for this to be fixed without a lot of boilerplate all over the code. And this functionality has not been used in the last few years in my experience.
Checklist
make lint) and all checks pass.make tests) and all tests pass.Related PR(s)/Issue(s)