fix: make test variables and correlation variables defined under a containing name #112
Conversation
| .join('\n') | ||
|
|
||
| return `const VARS = {\n${variables_lines}\n}\n` |
There was a problem hiding this comment.
This is currently defined outside of the default function, it makes it more readable but I'm not sure if it's the way we want to do it.
For comparison I left the the CORRELATION_VARS inside the default function to see how it would be when defined inside the default function, do you think it's better to have them defined outside of inside the default function ?
For correlation it doesn't change much as it's a single line, for test variables we are defining them on script generation so the list might be long 🤔
There was a problem hiding this comment.
I'd vote to keep outside of default
There was a problem hiding this comment.
Inside of the default function makes sense because it's the scope they're being used, however, as you pointed out that it might get long, I believe leaving then outside makes it cleaner.
There was a problem hiding this comment.
I don't know whether the placement really makes any difference, but conceptually, it feels like correlation vars should definitely be inside the default function, as it is what k6 docs refer to as VU code (with the outside code being called init code). Correlated values are meant to be scoped to the VU/iteration, so having them inside the default function feels natural.
There was a problem hiding this comment.
yap, if we also consider the logic, having it inside means that it is always reset to the empty state on start of iteration, while having it global means we would be leaving leftover status, and that would be a bug.
So globally defined makes sense only for the Test variables 👍
| .join('\n') | ||
|
|
||
| return `const VARS = {\n${variables_lines}\n}\n` |
There was a problem hiding this comment.
I'd vote to keep outside of default
src/codegen/codegen.test.ts
Outdated
| @@ -30,12 +30,15 @@ describe('Code generation', () => { | |||
|
|
|||
| export const options = {} | |||
|
|
|||
| const VARS = {} | |||
There was a problem hiding this comment.
Should we skip adding this line when there's no variable defined? Just to reduce noise
src/codegen/codegen.test.ts
Outdated
| export default function() { | ||
| let params | ||
| let resp | ||
| let match | ||
| let regex | ||
| let url | ||
| const CORRELATION_VARS = {} |
There was a problem hiding this comment.
JS doesn't restrict you from modifying properties of an object defined as const, but perhaps let correlation_vars would be a better fit here, to indicate this value is expected to be mutated?
There was a problem hiding this comment.
@e-fisher wouldn't using let allow the user to shoot themselves in the foot by reassigning it in one of the custom code snippets?
We can wrap it with - disregard this, it only applies to test varsObject.freeze to prevent reassignment as well 🤔
There was a problem hiding this comment.
I don't have a strong preference but possibly sliding more towards const since we don't want that value to be reassigned, but just the inner structure to be mutated 🤔
| @@ -208,9 +208,8 @@ const extractCorrelationRegex = ( | |||
| } | |||
|
|
|||
| const getCorrelationVariableSnippet = (uniqueId: number | undefined) => { | |||
There was a problem hiding this comment.
Can it really be undefined? In this case, we need to add a check/throw an error in such cases, because correlation_vars[undefined] will throw
There was a problem hiding this comment.
no, I don't think it can be undefined, the type system there will need to be improved because we will always be having the uniqueId at that point, but we are using a variable that it's allowed to be undefined :(
There was a problem hiding this comment.
Added a PR here: #114
That is because |
Closes https://github.com/grafana/k6-cloud/issues/2607
Test variables:
Correlation variable: