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

update goja to latest master #1904

Merged
merged 2 commits into from
Mar 22, 2021
Merged

update goja to latest master #1904

merged 2 commits into from
Mar 22, 2021

Conversation

mstoykov
Copy link
Contributor

No description provided.

@mstoykov mstoykov marked this pull request as draft March 15, 2021 09:06
@codecov-io
Copy link

codecov-io commented Mar 15, 2021

Codecov Report

Merging #1904 (3d7bc97) into master (9d973f5) will decrease coverage by 0.03%.
The diff coverage is n/a.

❗ Current head 3d7bc97 differs from pull request most recent head ef479a8. Consider uploading reports for the commit ef479a8 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1904      +/-   ##
==========================================
- Coverage   71.28%   71.25%   -0.04%     
==========================================
  Files         183      184       +1     
  Lines       14336    14338       +2     
==========================================
- Hits        10220    10217       -3     
- Misses       3476     3495      +19     
+ Partials      640      626      -14     
Flag Coverage Δ
ubuntu 71.20% <ø> (-0.04%) ⬇️
windows 70.85% <ø> (-0.05%) ⬇️

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

Impacted Files Coverage Δ
js/compiler/compiler.go 67.24% <ø> (ø)
stats/kafka/config.go 38.46% <0.00%> (-9.69%) ⬇️
stats/influxdb/config.go 35.34% <0.00%> (-2.59%) ⬇️
lib/options_tls_go1_13.go 100.00% <0.00%> (ø)
lib/options.go 84.00% <0.00%> (+0.21%) ⬆️
js/runner.go 81.44% <0.00%> (+0.57%) ⬆️

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 9d973f5...ef479a8. Read the comment docs.

@mstoykov mstoykov marked this pull request as ready for review March 15, 2021 11:58
@mstoykov mstoykov requested review from imiric and na-- March 15, 2021 11:58
@mstoykov
Copy link
Contributor Author

mstoykov commented Mar 15, 2021

edit: fixed in 91728be

From my investigation, the new failures around test/language/arguments-object/mapped/nonconfigurable-descriptors-basic.js and co. are about goja not doing something properly in non-strict mode with arguments.

It does work perfectly fine in strict mode and as previously the let meant that babel was used which adds "use strict" at the beginning of the script it was ... working.

Given that this is highly unlikely to be relevant to any actual test script and that k6 also always runs in strict mode as well ... I don't think this is a problem.

I will try to figure out if I can propose a fix for some more time and if not will just make an issue in the goja repo

@na-- na-- added this to the v0.32.0 milestone Mar 15, 2021
@mstoykov
Copy link
Contributor Author

Made a PR dop251/goja#266, again I don't think we need to wait on this

require.True(t, strings.HasSuffix(entries[0].Message, "Goja stack:\nfile:///script.js:3:4(12)"))
// broken since goja@f3cfc97811c0b4d8337902c3e42fb2371ba1d524 see
// https://github.com/dop251/goja/issues/179#issuecomment-783572020
// require.True(t, strings.HasSuffix(entries[0].Message, "Goja stack:\nfile:///script.js:3:4(12)"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we OK with this? Is this a breaking change then? Should we test with the re-throwing workaround suggested in that comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made an issue about discussing that #1906.

As I mention there I don't think that we have ever actually used this functionality really and the "workaround" is both error prone IMO and will need a lot of work.

Regardless I am of the opinion that we can have it not work for some time and either fix it a later PR or just leave it until we actually need it again, and figure out what to do then.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. Hopefully not a lot of users were relying on this, but like you mentioned, it wasn't very useful to begin with.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At least the current behaviour was not helpful as combined with the lack of anything but the last frame and the fact that even that line number is usually wrong I spent an hour or two debugging the wrong place before deciding to ignore the "goja stacktrace" and just go with logic and the golang stacktrace

imiric
imiric previously approved these changes Mar 15, 2021
imiric
imiric previously approved these changes Mar 17, 2021
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 🤞 😅

@mstoykov mstoykov merged commit 60d5a05 into master Mar 22, 2021
@mstoykov mstoykov deleted the updateGoja branch March 22, 2021 11:53
@na-- na-- mentioned this pull request Apr 7, 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.

4 participants