Skip to content
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

Treat errors from Throw() as plain goja values #1775

Merged
merged 1 commit into from
Jun 8, 2021
Merged

Conversation

imiric
Copy link
Contributor

@imiric imiric commented Dec 17, 2020

This avoids outputting "GoError" as mentioned in #877 (comment) .

I split it off from #1769 to avoid the clutter in that PR.

@na--
Copy link
Member

na-- commented Jan 4, 2021

Hmm I'm not sure this is the way to fix the issue. Even if we don't use goja's GoError, we probably should use a custom error wrapper type of our own, so we can then type assert by it.

@imiric
Copy link
Contributor Author

imiric commented Jan 11, 2021

@na-- I tried porting lib.Exception added in #1769 and panicking with it in Throw(), but it messes up the logged stack trace when k6.fail() is used, and we'd have to resort to manually building the stack trace as done in https://github.com/loadimpact/k6/blob/ccffbcaaddd7f2d472cdced0da847fde2140ef88/js/runner.go#L627-L637

This approach doesn't produce the same trace as what goja produces internally. Before (as done in #1769):

ERRO[0000] fail: failed
        at github.com/loadimpact/k6/js/common.Bind.func1 (native)
        at another (file:///home/ivan/Documents/Load%20Impact/tests/test-877-fail.js:6:13(4))
        at myfail (file:///home/ivan/Documents/Load%20Impact/tests/test-877-fail.js:14:10(2))
        at file:///home/ivan/Documents/Load%20Impact/tests/test-877-fail.js:28:11(6)  executor=shared-iterations scenario=default

After (with the ported lib.Exception):

ERRO[0000] failed
file:///home/ivan/Documents/Load%20Impact/tests/test-877-fail.js:18:1(26)

Where did you consider adding a type assertion for it? I also think an internal error type would make sense here, but not if it messes up the current logging.

@na-- na-- added this to the v1.0.0 milestone Jun 7, 2021
@na--
Copy link
Member

na-- commented Jun 7, 2021

Sorry for the 6-month late response... 😅 I think we discussed this in Slack, but still should have said something here... 🤦‍♂️

Anyway, prompted by #2049, I just tried your fix (replacing panic(rt.NewGoError(err)) with panic(rt.ToValue(err)) in js.common.Throw() on top of #2046 and it actually seems to work like a charm! Stack traces and exit codes are preserved and GoError disappears, so we should probably release this even in k6 v0.33.0?

@na-- na-- modified the milestones: v1.0.0, v0.33.0 Jun 7, 2021
This avoids outputting "GoError" as mentioned in
#877 (comment)
@codecov-commenter
Copy link

codecov-commenter commented Jun 7, 2021

Codecov Report

Merging #1775 (11c4b8a) into master (da5eebc) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

❗ Current head 11c4b8a differs from pull request most recent head 47597c9. Consider uploading reports for the commit 47597c9 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1775      +/-   ##
==========================================
- Coverage   71.73%   71.70%   -0.03%     
==========================================
  Files         179      179              
  Lines       14100    14100              
==========================================
- Hits        10115    10111       -4     
- Misses       3347     3351       +4     
  Partials      638      638              
Flag Coverage Δ
ubuntu 71.62% <100.00%> (-0.05%) ⬇️
windows 71.34% <100.00%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
js/common/util.go 68.75% <100.00%> (ø)
loader/loader.go 79.28% <0.00%> (-3.58%) ⬇️
lib/executor/vu_handle.go 95.23% <0.00%> (+0.95%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update da5eebc...47597c9. Read the comment docs.

Copy link
Member

@na-- na-- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. We can either merge this first and I will rebase #2046, or the other way around, 🤷‍♂️ will be simpler, probably about the same.

@imiric imiric merged commit 86315c6 into master Jun 8, 2021
@imiric imiric deleted the fix/throw-go-error branch June 8, 2021 12:14
imiric pushed a commit to grafana/k6-docs that referenced this pull request Jun 29, 2021
imiric pushed a commit to grafana/k6-docs that referenced this pull request Jun 29, 2021
imiric pushed a commit to grafana/k6-docs that referenced this pull request Jun 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants