fix setting js options on the wrong object #1430

mstoykov · 2020-05-01T11:32:50Z

previos to this we were getting a possibly unexported variable "options"
and setting values to it. But the exported options could not have a
local variable with the same name ...

This also fixes a test to specifically need this to work.

codecov-io · 2020-05-01T11:54:44Z

Codecov Report

Merging #1430 into jsPackageOptimizaitons will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@                    Coverage Diff                     @@
##           jsPackageOptimizaitons    #1430      +/-   ##
==========================================================
+ Coverage                   76.33%   76.36%   +0.03%     
==========================================================
  Files                         162      162              
  Lines                       12938    12938              
==========================================================
+ Hits                         9876     9880       +4     
+ Misses                       2546     2542       -4     
  Partials                      516      516

Impacted Files	Coverage Δ
js/bundle.go	`88.65% <100.00%> (ø)`
stats/cloud/collector.go	`78.70% <0.00%> (+0.61%)`	⬆️
lib/executor/vu_handle.go	`97.18% <0.00%> (+2.81%)`	⬆️

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 3d55b1d...d9a307b. Read the comment docs.

imiric

LGTM, though I'm not sure if this could cause any issues, considering it hasn't changed since 2018.

imiric · 2020-05-04T15:14:05Z

js/bundle.go

@@ -223,7 +223,7 @@ func (b *Bundle) Instantiate() (bi *BundleInstance, instErr error) {
 		panic("exported default is not a function")
 	}

-	jsOptions := rt.Get("options")
+	jsOptions := exports.Get("options")


I'm not very familiar with Goja's API, but shouldn't we also conversely do s/rt/exports/ on line 231 below?

🤦 , yes ... and I looked for other instances 🤦

Of course, this will now fail 😭 ... because #722 made the test to expect that if you didn't define options it will be overwritten ... which might not be the worst idea ... I dunno, but we will probably need to discuss this. Also, I remember that there were some problems around this in the cloud which even more leads me to believe @na-- will need to say something on the matter ;)

hmm I first started rebasing this without reading the comments, but looking at the test that's failing, how sure are we that this fix isn't a breaking change?

as long as people use

export let options = {...};

and babel, which is ... I would go with the majority - they won't notice a thing ...
the same code with ES5.1 is

var options = {...}; exports.options=options;

If someone decided that if you just don't make a variable options at all or that variable is then not set to exports.options and then they expect that options (not exports.options) will exist - then yes they will find out that this won't work.
for the record I don't think this has ever happened apart from this test you wrote.

Hm, when it failed for me it was only the first three "variants" which are :
https://github.com/loadimpact/k6/blob/35784c2498a14a06565b5d1e87ef05f721820675/js/runner_test.go#L138-L140

so.. no options or one that is defined as null or undefined ...

I have no idea why you decided that this should work in the first place, hence the mention above for you to look at it. IMO this shouldn't have worked like that and I would be surprised if someone depended on this behavior... and if so I would argue they need to the corresponding changes :D

I have no idea why you decided that this should work in the first place

I think it should work, since I think a valid use case would be to run a k6 script with absolutely no exported script options, but to instead specify the options via environment variables or CLI flags or a JSON config or a mix of those. In that scenario, you should still be able to know with what actual options your script is being executed, from inside of the script...

That said, if I had considered the issue carefully when I implemented it, I might have required an exported empty options to exist for that feature to work... But I didn't, so now this is a breaking change that will probably break some people's scripts, especially those running k6 unsupervised 😞 So, I'd have to give it a 👎, unless there is some bigger issue with the current implementation that I'm missing? What are the problems caused by just setting options in the global script scope, regardless of if it's exported or not? This is sort of how we do __ENV, __VU and __ITER, right?

I would mostly argue that given that we read from (and only) exports.options, when we set options we should be setting them on exports.options.

I highly doubt that this makes much of a difference for anyone writing a script, but if you believe so, I can agree to have options be special variable just like __ENV, __VU and __ITER but I would argue this will need to be also documented. And then just have it copy it from exports.options ... although I have to say I don't like it ...

Yeah, I don't like it very much either... I'd be fine to make the jsOptions := exports.Get("options") change, but the setting of the final consolidated options back seems like it should be either like it currently is, or to both exports and a global variable.

In any case, unless I'm missing some fundamental issue here, this can keep. We've had this issue for a while and nobody has complained AFAIK, so I'm tagging this for 0.28.0

This definitely can wait ... I just found it doing the js optimizations PR :D

codecov-commenter · 2020-05-21T07:46:07Z

Codecov Report

Merging #1430 into new-schedulers will decrease coverage by 0.04%.
The diff coverage is 84.64%.

@@                Coverage Diff                 @@
##           new-schedulers    #1430      +/-   ##
==================================================
- Coverage           76.41%   76.36%   -0.05%     
==================================================
  Files                 162      162              
  Lines               12838    12938     +100     
==================================================
+ Hits                 9810     9880      +70     
- Misses               2511     2542      +31     
+ Partials              517      516       -1

Impacted Files	Coverage Δ
api/v1/status_routes.go	`10.20% <0.00%> (ø)`
cmd/run.go	`9.28% <0.00%> (-0.16%)`	⬇️
lib/consts/consts.go	`0.00% <ø> (ø)`
lib/executors.go	`90.19% <ø> (ø)`
core/local/local.go	`70.92% <68.75%> (+0.74%)`	⬆️
lib/executor/externally_controlled.go	`59.23% <80.00%> (+0.40%)`	⬆️
core/engine.go	`88.09% <85.86%> (-3.37%)`	⬇️
lib/testutils/minirunner/minirunner.go	`80.76% <90.90%> (+1.82%)`	⬆️
js/runner.go	`83.33% <93.10%> (ø)`
lib/executor/variable_arrival_rate.go	`95.81% <97.80%> (-0.40%)`	⬇️
... and 21 more

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 923f886...bda6461. Read the comment docs.

previos to this we were getting a possibly unexported variable "options" and setting values to it. But the exported options could not have a local variable with the same name ... This also fixes a test to specifically need this to work.

na--

#1430 (comment)

The base branch was changed.

codecov-io · 2020-12-15T17:14:19Z

Codecov Report

Merging #1430 (d9a307b) into master (e0ddf5d) will decrease coverage by 0.33%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1430      +/-   ##
==========================================
- Coverage   76.69%   76.36%   -0.34%     
==========================================
  Files         163      162       -1     
  Lines       12986    12938      -48     
==========================================
- Hits         9960     9880      -80     
- Misses       2515     2542      +27     
- Partials      511      516       +5

Impacted Files	Coverage Δ
js/bundle.go	`88.65% <100.00%> (-0.69%)`	⬇️
js/common/util.go	`100.00% <100.00%> (ø)`
cmd/config.go	`73.01% <0.00%> (-10.57%)`	⬇️
js/modules/k6/k6.go	`91.48% <0.00%> (-8.52%)`	⬇️
lib/executor/base_config.go	`81.39% <0.00%> (-6.11%)`	⬇️
lib/executors.go	`90.19% <0.00%> (-3.93%)`	⬇️
lib/executor/externally_controlled.go	`59.23% <0.00%> (-3.10%)`	⬇️
core/local/local.go	`70.92% <0.00%> (-3.09%)`	⬇️
lib/executor/variable_arrival_rate.go	`95.81% <0.00%> (-1.05%)`	⬇️
lib/executor/helpers.go	`96.34% <0.00%> (-0.29%)`	⬇️
... and 37 more

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 e0ddf5d...35784c2. Read the comment docs.

na-- · 2021-11-23T13:14:01Z

I think we should close this and instead expose the consolidated script options through the k6/execution.test object as I mentioned in #2259. That way we won't have any ambiguity... @mstoykov, what do you think?

mstoykov · 2021-11-25T11:55:52Z

shouldn't we stop updating options as well in order to make people use the k6/execution one?

na-- · 2021-11-25T17:54:32Z

probably 🤷‍♂️ though that would be a breaking change, and we know for a fact that people are relying on accessing the options in the script, so it might be simpler to just keep the current behavior and maybe add a deprecation warning for a while, even after we have execution.test.options...

mstoykov requested review from imiric and na-- May 1, 2020 11:32

mstoykov mentioned this pull request May 1, 2020

Js package optimizaitons #1429

Merged

imiric previously approved these changes May 4, 2020

View reviewed changes

Base automatically changed from jsPackageOptimizaitons to new-schedulers May 21, 2020 07:16

na-- force-pushed the fixJSWrongOptionsSetting branch from 4c391e5 to bda6461 Compare May 21, 2020 07:45

na-- approved these changes May 21, 2020

View reviewed changes

na-- force-pushed the fixJSWrongOptionsSetting branch from eda63de to bda6461 Compare May 21, 2020 09:19

mstoykov added 2 commits May 21, 2020 13:10

fix setting js options on the wrong object

ad80fcc

previos to this we were getting a possibly unexported variable "options" and setting values to it. But the exported options could not have a local variable with the same name ... This also fixes a test to specifically need this to work.

Also set "options" on the "exports" not just in the runtime

35784c2

na-- force-pushed the fixJSWrongOptionsSetting branch from bda6461 to 35784c2 Compare May 21, 2020 10:10

na-- requested changes May 21, 2020

View reviewed changes

na-- added this to the v0.28.0 milestone May 21, 2020

mstoykov changed the base branch from new-schedulers to master July 7, 2020 08:32

na-- removed this from the v0.28.0 milestone Sep 8, 2020

na-- mentioned this pull request Nov 23, 2021

k6/execution: add more information about the current test run #2259

Open

mstoykov mentioned this pull request Dec 2, 2021

Function cannot read tag value from options.tags object directly #2278

Closed

mstoykov closed this Jan 31, 2022

na-- deleted the fixJSWrongOptionsSetting branch February 21, 2022 07:27

mstoykov mentioned this pull request Jun 13, 2022

Add to showcase how currently options is accessible everywhere #2566

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix setting js options on the wrong object #1430

fix setting js options on the wrong object #1430

mstoykov commented May 1, 2020

codecov-io commented May 1, 2020

imiric left a comment

imiric May 4, 2020

mstoykov May 4, 2020

mstoykov May 4, 2020

na-- May 21, 2020

mstoykov May 21, 2020

mstoykov May 21, 2020

na-- May 21, 2020 •

edited

Loading

mstoykov May 21, 2020

na-- May 21, 2020 •

edited

Loading

mstoykov May 21, 2020

codecov-commenter commented May 21, 2020 •

edited

Loading

na-- left a comment

codecov-io commented Dec 15, 2020

na-- commented Nov 23, 2021

mstoykov commented Nov 25, 2021

na-- commented Nov 25, 2021

fix setting js options on the wrong object #1430

fix setting js options on the wrong object #1430

Conversation

mstoykov commented May 1, 2020

codecov-io commented May 1, 2020

Codecov Report

imiric left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

na-- May 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

na-- May 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented May 21, 2020 • edited Loading

Codecov Report

na-- left a comment

Choose a reason for hiding this comment

codecov-io commented Dec 15, 2020

Codecov Report

na-- commented Nov 23, 2021

mstoykov commented Nov 25, 2021

na-- commented Nov 25, 2021

na-- May 21, 2020 •

edited

Loading

na-- May 21, 2020 •

edited

Loading

codecov-commenter commented May 21, 2020 •

edited

Loading