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

js/k6/exec: test.options object #2493

Merged
merged 4 commits into from
Apr 20, 2022
Merged

js/k6/exec: test.options object #2493

merged 4 commits into from
Apr 20, 2022

Conversation

codebien
Copy link
Contributor

@codebien codebien commented Apr 13, 2022

This PR adds the capability for fetching the consolidated and derived options for tests using the k6/execution API. It adds a test.options object exporting all the potential set properties.

A simple example of the expected API:

import exec from 'k6/execution';

export const options = {
  vus: 10,
  duration : '30s'
};

export default function() {
  console.log(exec.test.options.scenarios.default.vus) // 10
}

Open questions

  1. We are defining the default values for some objects/arrays as null. An example is Thresholds.
    Which value do we expect to get if we access properties like Thresholds? undefined or null or {} (empty object)?
    We will use the same value returned from the JSON marshal, at the moment, the k6 options' values are Nullable so the default value will be null.
import exec from 'k6/execution';

export const options = {
  vus: 10,
  duration : '30s'
};

export default function() {
  console.log(exec.test.options.thresholds) // what do you expect to get?
}
  1. This point from the original issue is missing:

A deprecation warning should be added when a set operation on the options object is executed. (Maybe also on getting?)

In case of const, it seems not possible by spec to set back an eventual wrap/proxy for warning the access. Do we have an alternative? We used a classic JS Object but with the Object.freeze function applied.

I would define these cases before polishing and addressing edge cases.

Closes #2450

@codebien codebien self-assigned this Apr 13, 2022
Copy link
Contributor

@mstoykov mstoykov left a comment

Choose a reason for hiding this comment

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

Have you thought about freezing the object that is returned so that users will get an error if they try to modify something?

js/modules/k6/execution/execution.go Outdated Show resolved Hide resolved
@codebien
Copy link
Contributor Author

Have you thought about freezing the object that is returned so that users will get an error if they try to modify something?

@mstoykov it would add a breaking change, if we have to do it I think could be good to give to the users at least one release before doing it, so they would have the time to migrate. WDYT?

@mstoykov
Copy link
Contributor

I meant the exec.test.options ... which a completely new API - so no breaking changes ... or am I missing something?

@codebien
Copy link
Contributor Author

Oh, you're right. 🤦‍♂️ I thought you meant the exported options object.

Copy link
Contributor

@olegbespalov olegbespalov left a comment

Choose a reason for hiding this comment

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

Overall looks good, left a small comment 👍

js/modules/k6/execution/execution.go Outdated Show resolved Hide resolved
@codebien codebien force-pushed the exec-options branch 2 times, most recently from fd05c98 to 280ca72 Compare April 15, 2022 13:20
@codebien
Copy link
Contributor Author

Have you thought about freezing the object that is returned so that users will get an error if they try to modify something?

The goja.Object doesn't provide a freezing method from Go or at least I didn't find it, so I added the properties to the JS object as read-only.

I also added an integration test for monitoring the default values. Unfortunately, they will be null even in the case where we have set a value. This happens because we are coding nullable types as valid: false when we set them for default values.

Here, an example:

func NewRampingVUsConfig(name string) RampingVUsConfig {
return RampingVUsConfig{
BaseConfig: NewBaseConfig(name, rampingVUsType),
StartVUs: null.NewInt(1, false),
GracefulRampDown: types.NewNullDuration(30*time.Second, false),
}
}

Of course, they will be set when the user explicitly set them or the config is derived.

@mstoykov
Copy link
Contributor

doesn't provide a freezing method from Go

It doesn't, but I managed to make it work for the original SharedArray implementation https://github.com/grafana/k6/pull/1739/files

@codebien
Copy link
Contributor Author

Added the object freeze:

import exec from 'k6/execution'

export default function() {
	console.log(Object.isFrozen(exec.test.options))
	console.log(Object.isFrozen(exec.test.options.paused))
	console.log(Object.isFrozen(exec.test.options.httpDebug))
	console.log(Object.isFrozen(exec.test.options.dns))
	console.log(Object.isFrozen(exec.test.options.scenarios))
	console.log(Object.isFrozen(exec.test.options.scenarios.default))
	console.log(Object.isFrozen(exec.test.options.scenarios.default.iterations))
}
INFO[0000] true                                          source=console
INFO[0000] true                                          source=console
INFO[0000] true                                          source=console
INFO[0000] true                                          source=console
INFO[0000] true                                          source=console
INFO[0000] true                                          source=console
INFO[0000] true                                          source=console

js/common/frozen_object.go Outdated Show resolved Hide resolved
js/common/frozen_object.go Outdated Show resolved Hide resolved
js/common/frozen_object.go Outdated Show resolved Hide resolved
olegbespalov
olegbespalov previously approved these changes Apr 20, 2022
Copy link
Contributor

@olegbespalov olegbespalov left a comment

Choose a reason for hiding this comment

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

💪

@sniku sniku requested review from oleiade and removed request for mstoykov April 20, 2022 12:19
@codebien codebien added the documentation-needed A PR which will need a separate PR for documentation label Apr 20, 2022
@codebien codebien added this to the v0.38.0 milestone Apr 20, 2022
@oleiade oleiade self-requested a review April 20, 2022 15:11
oleiade
oleiade previously approved these changes Apr 20, 2022
Copy link
Member

@oleiade oleiade left a comment

Choose a reason for hiding this comment

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

This looks awesome, congrats! Special kudos for adding the FreezeObject construct! 👍🏻 🦖

Regarding the default values of thresholds, I would prefer it to be an empty object {}, but if I console.log(options.thresholds) as of today, it prints undefined which makes sense. I can see both work, so up to you 🦕

@codebien
Copy link
Contributor Author

codebien commented Apr 20, 2022

Regarding the default values of thresholds, I would prefer it to be an empty object {}, but if I console.log(options.thresholds) as of today, it prints undefined which makes sense. I can see both work, so up to you 🦕

@oleiade I updated the description with the decision applied for that part, sorry for not doing it before. Reporting here also ⬇️ :

We will use the same value returned from the JSON marshal, at the moment, the k6 options' values are Nullable so the default value will be null.

At the moment, it is also the same default value reported in the docs so it shouldn't change the current UX.

mstoykov
mstoykov previously approved these changes Apr 20, 2022
Copy link
Contributor

@mstoykov mstoykov left a comment

Choose a reason for hiding this comment

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

LGTM

@codebien
Copy link
Contributor Author

I rebased the fixups and applied the @mstoykov's suggestion

@codebien
Copy link
Contributor Author

This PR will implement the doc: grafana/k6-docs#629

@codebien codebien merged commit 2af84a0 into master Apr 20, 2022
@codebien codebien deleted the exec-options branch April 20, 2022 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation-needed A PR which will need a separate PR for documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

k6/execution: Export consolidated options
4 participants