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.

Sign up for GitHub

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

fix setting js options on the wrong object #1430

Closed
wants to merge 2 commits into from
Closed

fix setting js options on the wrong object #1430

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ad80fcc
fix setting js options on the wrong object
mstoykov May 1, 2020
35784c2
Also set "options" on the "exports" not just in the runtime
mstoykov May 4, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions js/bundle.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,11 +249,13 @@ func (b *Bundle) Instantiate() (bi *BundleInstance, instErr error) {
bi.exports[k] = fn
}

jsOptions := rt.Get("options")
jsOptions := exports.Get("options")
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 ;)

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@na-- na-- May 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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?

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 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 ...

Copy link
Member

@na-- na-- May 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

var jsOptionsObj *goja.Object
if jsOptions == nil || goja.IsNull(jsOptions) || goja.IsUndefined(jsOptions) {
jsOptionsObj = rt.NewObject()
rt.Set("options", jsOptionsObj)
if err := exports.Set("options", jsOptionsObj); err != nil {
return nil, err
}
} else {
jsOptionsObj = jsOptions.ToObject(rt)
}
Expand Down
17 changes: 9 additions & 8 deletions js/runner_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,7 @@ func TestOptionsSettingToScript(t *testing.T) {
t.Run(fmt.Sprintf("Variant#%d", i), func(t *testing.T) {
t.Parallel()
data := variant + `
exports.options = options;
exports.default = function() {
if (!options) {
throw new Error("Expected options to be defined!");
Expand Down Expand Up @@ -179,19 +180,19 @@ func TestOptionsSettingToScript(t *testing.T) {
func TestOptionsPropagationToScript(t *testing.T) {
t.Parallel()
data := `
var options = { setupTimeout: "1s", myOption: "test" };
exports.options = options;
exports.options = { setupTimeout: "1s", myOption: "test" };
exports.default = function() {
if (options.external) {
if (exports.options.external) {
throw new Error("Unexpected property external!");
}
if (options.myOption != "test") {
throw new Error("expected myOption to remain unchanged but it was '" + options.myOption + "'");
if (exports.options.myOption != "test") {
throw new Error("expected myOption to remain unchanged but it was '" + exports.options.myOption + "'");
}
if (options.setupTimeout != __ENV.expectedSetupTimeout) {
throw new Error("expected setupTimeout to be " + __ENV.expectedSetupTimeout + " but it was " + options.setupTimeout);
if (exports.options.setupTimeout != __ENV.expectedSetupTimeout) {
throw new Error("expected setupTimeout to be " + __ENV.expectedSetupTimeout + " but it was " + exports.options.setupTimeout);
}
};`
};
`

expScriptOptions := lib.Options{SetupTimeout: types.NullDurationFrom(1 * time.Second)}
r1, err := getSimpleRunner("/script.js", data,
Expand Down