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

Restore options.scenarios #2492

Closed
wants to merge 1 commit into from
Closed

Restore options.scenarios #2492

wants to merge 1 commit into from

Conversation

codebien
Copy link
Contributor

@codebien codebien commented Apr 12, 2022

Using as an example the following script, it prints an unexpected {} object using >v0.37.0. This change has been introduced by #2392.

import exec from 'k6/execution';

export const options = {
	scenarios: {
		contacts: {
			executor: 'shared-iterations',
			iterations: 1,
		},
	},
};

export default function() {
	console.log(JSON.stringify(options.scenarios))
}

In v0.36.0 and previous versions, the scenarios key wasn't set during the js.Bundle.Instantiate because the key used for it was scenarios,omitempty.

The current change is an hack for restoring the previous behaviour where the expected output is:

INFO[0000] {"contacts":{"executor":"shared-iterations","iterations":1}}  source=console

Questions

  • Could we rid the entire b.Options.ForEachSpecified("json", logic when k6/execution: Export consolidated options #2450 will be implemented. If yes, probably we should directly implement 2450 and not merge this.
  • Do we have an alternative to make this change less const-string dependent?

@codebien codebien requested a review from mstoykov April 12, 2022 11:34
@codebien codebien self-assigned this Apr 12, 2022
@mstoykov
Copy link
Contributor

This unfortunately means that we won't set the scenarios in the script at all, so if they are not entirely configured from within they will have wrong values ...

It seems to me like the Set doesn't work correctly and it seems to be some goja issue - probably due to dop251/goja#377 (the type definition).

//
// TODO: rid this part when #883 will be implemented
if key == "scenarios" {
return
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return
val = (map[string]lib.ExecutorConfig)(val.(lib.ScenarioConfigs))

This seems like a better fix.
We basically remove the type "around" map and give goja a map which then is set correctly.

This definitely needs a bunch of tests though

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably a better variant will be for this to have some kind of interface ToValue or something like that. Then we can check that val implements it and call the method to get the actual value - the body in this will be this same line.

But I feel that this might be too much work especially as this will likely be the only "exception" so maybe leave that as a comment and hope that we will never have to implement it 🤞

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, this doesn't work really well because it includes the BaseConfig that the executor's config contains. The ideal approach would be to use a mapper like ToValue to return the correct object.

@codebien
Copy link
Contributor Author

Considering the effort required for this, we would directly introduce the final and better solution. I opened an issue for tracking and future updates. #2495

@codebien codebien closed this Apr 14, 2022
@codebien codebien deleted the fix-options-scenarios branch July 14, 2022 18:25
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.

2 participants