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: make test variables and correlation variables defined under a containing name #112

Merged
merged 6 commits into from
Aug 29, 2024
Merged

fix: make test variables and correlation variables defined under a containing name #112

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3e71590
generate variables into a single name
Llandy3d Aug 28, 2024
2d43781
correlation variables extracted into containing variable
Llandy3d Aug 28, 2024
8cb3584
to lowercase
Llandy3d Aug 28, 2024
eda1f49
replacer uses new correlation_vars name
Llandy3d Aug 28, 2024
69cd986
fix test
Llandy3d Aug 28, 2024
4194e5d
do not show VARS if no variable is present
Llandy3d Aug 28, 2024
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
9 changes: 5 additions & 4 deletions src/codegen/codegen.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ describe('Code generation', () => {
let match
let regex
let url
const correlation_vars = {}
sleep(1)
}
`
Expand Down Expand Up @@ -83,7 +84,7 @@ describe('Code generation', () => {
]

const expectedResult = `
const test = "test"
const VARS = {"test": "test",}
`

expect(generateVariableDeclarations(variables).replace(/\s/g, '')).toBe(
Expand Down Expand Up @@ -176,15 +177,15 @@ describe('Code generation', () => {
params = { headers: {}, cookies: {} }
url = http.url\`http://test.k6.io/api/v1/login\`
resp = http.request('POST', url, null, params)
let correl_0 = resp.json().user_id
correlation_vars[0] = resp.json().user_id

params = { headers: {}, cookies: {} }
url = http.url\`http://test.k6.io/api/v1/users/\${correl_0}\`
url = http.url\`http://test.k6.io/api/v1/users/\${correlation_vars[0]}\`
resp = http.request('GET', url, null, params)

params = { headers: {}, cookies: {} }
url = http.url\`http://test.k6.io/api/v1/users\`
resp = http.request('POST', url, \`${JSON.stringify({ user_id: '${correl_0}' })}\`, params)
resp = http.request('POST', url, \`${JSON.stringify({ user_id: '${correlation_vars[0]}' })}\`, params)

`

Expand Down
11 changes: 9 additions & 2 deletions src/codegen/codegen.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,16 @@ export function generateScript({
}

export function generateVariableDeclarations(variables: Variable[]): string {
return variables
if (variables.length === 0) {
return ''
}

const variables_lines = variables
.filter(({ name }) => name)
.map(({ name, value }) => `const ${name} = "${value}"`)
.map(({ name, value }) => `"${name}": "${value}",`)
.join('\n')

return `const VARS = {\n${variables_lines}\n}\n`
Copy link
Member Author

Choose a reason for hiding this comment

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

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 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd vote to keep outside of default

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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 👍

}

export function generateVUCode(
Expand Down Expand Up @@ -76,6 +82,7 @@ export function generateVUCode(
let match
let regex
let url
const correlation_vars = {}
`,
groupSnippets,
thinkTime.sleepType === 'iterations' ? generateSleep(thinkTime.timing) : '',
Expand Down
27 changes: 10 additions & 17 deletions src/rules/correlation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,9 +208,8 @@ const extractCorrelationRegex = (
}

const getCorrelationVariableSnippet = (uniqueId: number | undefined) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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 :(

Copy link
Collaborator

Choose a reason for hiding this comment

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

Added a PR here: #114

return `let correl_${uniqueId}
if (match) {
correl_${uniqueId} = match[1]
return `if (match) {
correlation_vars[${uniqueId}] = match[1]
}`
}

Expand Down Expand Up @@ -470,7 +469,7 @@ const extractCorrelationJsonBody = (
: `.${selector.path}`

const correlationExtractionSnippet = `
let correl_${uniqueId} = resp.json()${json_path}`
correlation_vars[${uniqueId}] = resp.json()${json_path}`
return {
extractedValue,
correlationExtractionSnippet: correlationExtractionSnippet,
Expand Down Expand Up @@ -528,7 +527,7 @@ if (import.meta.vitest) {
}

const correlationExtractionSnippet = `
let correl_1 = resp.json().user_id`
correlation_vars[1] = resp.json().user_id`
const expectedResult = {
extractedValue: '444',
correlationExtractionSnippet: correlationExtractionSnippet,
Expand All @@ -554,9 +553,8 @@ let correl_1 = resp.json().user_id`
const correlationExtractionSnippet = `
regex = new RegExp('${selector.begin}(.*?)${selector.end}')
match = resp.body.match(regex)
let correl_1
if (match) {
correl_1 = match[1]
correlation_vars[1] = match[1]
}`
const expectedResult = {
extractedValue: 'bob',
Expand Down Expand Up @@ -588,9 +586,8 @@ let correl_1 = resp.json().user_id`
const correlationExtractionSnippet = `
regex = new RegExp('${selector.begin}(.*?)${selector.end}')
match = resp.headers["${canonicalHeaderKey('Content-type')}"].match(regex)
let correl_1
if (match) {
correl_1 = match[1]
correlation_vars[1] = match[1]
}`
const expectedResult = {
extractedValue: '/',
Expand Down Expand Up @@ -622,9 +619,8 @@ let correl_1 = resp.json().user_id`
const correlationExtractionSnippet = `
regex = new RegExp('${selector.begin}(.*?)${selector.end}')
match = resp.url.match(regex)
let correl_1
if (match) {
correl_1 = match[1]
correlation_vars[1] = match[1]
}`
const expectedResult = {
extractedValue: 'v1',
Expand All @@ -650,9 +646,8 @@ let correl_1 = resp.json().user_id`
const correlationExtractionSnippet = `
regex = new RegExp('${selector.regex}')
match = resp.body.match(regex)
let correl_1
if (match) {
correl_1 = match[1]
correlation_vars[1] = match[1]
}`
const expectedResult = {
extractedValue: 'bob',
Expand All @@ -678,9 +673,8 @@ let correl_1 = resp.json().user_id`
const correlationExtractionSnippet = `
regex = new RegExp('${selector.regex}')
match = resp.headers["${canonicalHeaderKey('Content-type')}"].match(regex)
let correl_1
if (match) {
correl_1 = match[1]
correlation_vars[1] = match[1]
}`
const expectedResult = {
extractedValue: '/',
Expand Down Expand Up @@ -711,9 +705,8 @@ let correl_1 = resp.json().user_id`
const correlationExtractionSnippet = `
regex = new RegExp('${selector.regex}')
match = resp.url.match(regex)
let correl_1
if (match) {
correl_1 = match[1]
correlation_vars[1] = match[1]
}`
const expectedResult = {
extractedValue: 'v1',
Expand Down
16 changes: 12 additions & 4 deletions src/rules/correlation.utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,23 +28,31 @@ export function replaceCorrelatedValues({
}) {
// Default behaviour replaces all occurences of the string
if (!rule.replacer) {
return replaceTextMatches(request, extractedValue, `correl_${uniqueId}`)
return replaceTextMatches(
request,
extractedValue,
`correlation_vars[${uniqueId}]`
)
}

switch (rule.replacer.selector.type) {
case 'begin-end':
return replaceBeginEnd(
rule.replacer.selector,
request,
`correl_${uniqueId}`
`correlation_vars[${uniqueId}]`
)
case 'regex':
return replaceRegex(rule.replacer.selector, request, `correl_${uniqueId}`)
return replaceRegex(
rule.replacer.selector,
request,
`correlation_vars[${uniqueId}]`
)
case 'json':
return replaceJsonBody(
rule.replacer.selector,
request,
`correl_${uniqueId}`
`correlation_vars[${uniqueId}]`
)
default:
return exhaustive(rule.replacer.selector)
Expand Down
Loading