-
Notifications
You must be signed in to change notification settings - Fork 31
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
[fix] Multi variable support in check values. #82
Conversation
dd35718
to
811c22e
Compare
Isn't it pretty weird that the name of the check would be e.g. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple of questions. Nothing big.
const getVariableName = (expr) => (expressions.variable.exec(expr) || [])[1] | ||
|
||
const isMultiVariableString = (expr) => { | ||
const matches = [...expr.matchAll(expressions.variables)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have a polyfill for the matchAll
-function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For node usage we're good (node 12). For cloud app usage, It looks like core-js covers that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just FYI there is no more core-js in the cloud/k6, but k6 has support for matchAll from goja
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mstoykov The matchAll
is run in node/browser when converting the har file to a script. So it doesn't run in k6.
Yes I agree with that. That behaviour is not introduced by this PR thought.. But I'll change it 👍 |
LGTM. You can merge when you feel like it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🎉
FIX check value referencing multiple variables only resolving first variable.
HAR fragment / desired outcome / before PR result
NOTE: Only multi variable usage generate TemplateLiteral while single variable usage still generates MemberExpression.