chore(k6): Fix parsing error during k6 run in ci #1158
Conversation
📝 WalkthroughWalkthroughThe pull request includes modifications to the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
oskogstad
force-pushed
the
chore/attempt-at-fixing-parsing-error-during-k6-run-in-ci
branch
from
1aee441 to
ab4096f
Compare
oskogstad
changed the title
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range comments (3)
tests/k6/tests/serviceowner/concurrency.js (1)
Refactor String Concatenations to Template Literals
Several instances of string concatenation were found in
dialogSearch.jsfiles:
tests/k6/tests/serviceowner/dialogSearch.js:
- Lines involving
'dialogs/' + dialogId + '/activities?' + i- Multiple other concatenations in various
getSOcalls.
tests/k6/tests/enduser/dialogSearch.js:
- Lines involving
'dialogs/' + defaultFilter + '&Search=' + titleToSearchFor- Multiple other concatenations in various
getEUandconsole.logcalls.Refactoring these concatenations to use template literals will enhance readability and maintainability.
🔗 Analysis chain
Line range hint
1-35: Summary: Effective implementation with minor suggestionsThe changes in this file successfully address the PR objectives by:
- Updating the import to use
postBatchSOfor batch operations.- Implementing a batch approach for concurrent requests, which should resolve the output order issues.
The new implementation looks good, with only minor suggestions for improvement (using template literals and
constdeclarations).To ensure consistency across the codebase:
This will help identify if similar changes are needed in other test files for consistency.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if similar changes are needed in other test files # Search for files that might need similar updates echo "Files potentially needing similar updates:" rg --type js 'postSOAsync' tests/k6/tests # Check for other occurrences of string concatenation in test files echo "\nOther occurrences of string concatenation that could be replaced with template literals:" rg --type js '\+\s*\w+\s*\+' tests/k6/testsLength of output: 4788
🧰 Tools
🪛 Biome
[error] 23-23: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 29-29: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 19-19: This let declares a variable that is only assigned once.
'batch' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 22-22: This let declares a variable that is only assigned once.
'activity' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
tests/k6/tests/serviceowner/dialogSearch.js (2)
Line range hint
179-185: LGTM! Consider adding a more specific error check.The new test case for handling an invalid process parameter is well-structured and correctly tests for a 400 status code. Great job on improving the test coverage!
To make the test even more robust, consider checking for a specific error message or error code in the response. This would ensure that the correct error is being thrown for the invalid process. For example:
expect(r.json().errors[0]).to.have.property('code').that.equals('InvalidProcessIdentifier');Replace 'InvalidProcessIdentifier' with the actual error code you expect from the API.
🧰 Tools
🪛 Biome
[error] 181-181: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
Line range hint
187-192: LGTM! Consider using a template literal for the URL.The update to use the
processToSeachForvariable in the test case is a good improvement for consistency. Well done!To address the static analysis hint and improve code style, consider using a template literal for the URL string:
let r = getSO(`dialogs/?CreatedAfter=${createdAfter}&process=${processToSeachFor}`);This change will make the code more readable and consistent with modern JavaScript practices.
🧰 Tools
🪛 Biome
[error] 181-181: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (9)
- src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Queries/Search/SearchDialogQueryValidator.cs (2 hunks)
- tests/k6/common/request.js (2 hunks)
- tests/k6/common/testimports.js (1 hunks)
- tests/k6/suites/all-single-pass.js (0 hunks)
- tests/k6/tests/all-tests.js (1 hunks)
- tests/k6/tests/enduser/dialogSearch.js (1 hunks)
- tests/k6/tests/serviceowner/concurrency.js (2 hunks)
- tests/k6/tests/serviceowner/dialogCreateInvalidProcess.js (1 hunks)
- tests/k6/tests/serviceowner/dialogSearch.js (1 hunks)
💤 Files with no reviewable changes (1)
- tests/k6/suites/all-single-pass.js
🧰 Additional context used
🪛 Biome
tests/k6/tests/serviceowner/concurrency.js
[error] 23-23: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 19-19: This let declares a variable that is only assigned once.
'batch' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 22-22: This let declares a variable that is only assigned once.
'activity' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
tests/k6/tests/serviceowner/dialogSearch.js
[error] 181-181: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
🔇 Additional comments (9)
tests/k6/tests/all-tests.js (1)
7-7: LGTM: Newline added at end of file.The addition of a newline at the end of the file is a good practice. It ensures compatibility with various text processing tools and can prevent issues in some environments.
However, I noticed that the PR objectives mention fixing parsing errors and output order issues, which are not directly visible in this file. These fixes might be implemented in other files not shown in this review, or they could be related to how this file is processed or executed in the CI environment.
To verify if there are any other relevant changes, let's check for modifications in other k6-related files:
✅ Verification successful
LGTM: PR successfully addresses parsing errors and output order issues across multiple k6 test files.
All required fixes related to parsing and output have been implemented consistently throughout the affected test suites and common modules. The addition of
postBatchSOenhances batch processing, and the corrected process strings resolve the previously identified parsing errors.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other modified k6-related files # Test: Look for modified k6 test files git diff --name-only origin/main | grep -E 'tests/k6/.*\.js$' # Test: Check for changes in CI configuration related to k6 git diff --name-only origin/main | grep -E '\.github/workflows/.*k6.*\.yml$'Length of output: 447
Script:
#!/bin/bash # Description: Retrieve diffs for all modified k6-related JavaScript files to verify fixes # List of modified k6 test files from the previous output modified_files=( "tests/k6/common/request.js" "tests/k6/common/testimports.js" "tests/k6/suites/all-single-pass.js" "tests/k6/tests/all-tests.js" "tests/k6/tests/enduser/dialogSearch.js" "tests/k6/tests/serviceowner/concurrency.js" "tests/k6/tests/serviceowner/dialogCreateInvalidProcess.js" "tests/k6/tests/serviceowner/dialogSearch.js" ) # Iterate over each modified file and display its diff for file in "${modified_files[@]}"; do echo "Diff for ${file}:" git diff origin/main -- "${file}" echo -e "\n" doneLength of output: 8028
tests/k6/common/testimports.js (1)
11-11: LGTM! Verify the implementation ofpostBatchSO.The addition of
postBatchSOto the exports looks good and aligns with the existing pattern of exporting service operation functions. This change may be related to addressing the parsing issues mentioned in the PR objectives.To ensure the correctness of this change, let's verify the implementation of
postBatchSOin the './request.js' file:✅ Verification successful
Verified:
postBatchSOis correctly implemented and exported intests/k6/common/request.js.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of postBatchSO in request.js # Test: Search for the postBatchSO function definition rg --type js -A 10 'function postBatchSO' tests/k6/common/request.js # Test: Check if postBatchSO is properly exported rg --type js 'export.*postBatchSO' tests/k6/common/request.jsLength of output: 644
tests/k6/tests/serviceowner/concurrency.js (2)
1-1: LGTM: Import statement updated to support batch operationsThe import statement has been updated to include
postBatchSOand removepostSOAsync. This change aligns with the PR objective of fixing parsing errors during k6 run in CI and supports the new approach for handling concurrent requests.
Line range hint
28-35: LGTM: Cleanup and assertions are correctThe cleanup process and assertions for the batch results are well-implemented and consistent with the new batch operation approach. This ensures that:
- The test doesn't leave residual data in the system.
- All concurrent operations are verified for success.
No changes are needed in this section.
🧰 Tools
🪛 Biome
[error] 23-23: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 29-29: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 19-19: This let declares a variable that is only assigned once.
'batch' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 22-22: This let declares a variable that is only assigned once.
'activity' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Queries/Search/SearchDialogQueryValidator.cs (1)
5-5: LGTM: New using directive added.The addition of
using Digdir.Domain.Dialogporten.Domain.Common;is appropriate and likely related to the changes in the Process property validation.tests/k6/common/request.js (2)
19-19: LGTM: Minor formatting improvementThe removal of the empty line improves code readability by eliminating unnecessary whitespace.
Line range hint
1-73: Summary: Changes align with PR objectives and improve code qualityThe changes in this file contribute positively to the PR's objectives of fixing parsing errors during k6 runs in CI:
- The new
postBatchSOfunction allows for efficient batch processing of requests, which could help address the issue of output from concurrency tests being printed in the incorrect order.- By using consistent request parameter handling and error checking, this function may also contribute to better handling of invalid URI tests.
The suggested improvements to
postBatchSOwould further enhance its robustness and flexibility, potentially preventing future parsing issues.Overall, these changes appear to be a step in the right direction for improving the reliability of k6 tests in the CI environment.
🧰 Tools
🪛 Biome
[error] 75-75: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
[error] 76-76: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
tests/k6/tests/serviceowner/dialogSearch.js (1)
Line range hint
1-203: Great improvements to the dialog search tests!The changes in this file effectively address the PR objectives:
- The new test case for an invalid process (lines 179-185) directly tackles the parsing issue for invalid URI tests mentioned in the PR description.
- The update to the existing test case (lines 187-192) improves consistency in test data usage.
These changes contribute to enhancing the reliability and functionality of the k6 testing framework within the project's CI pipeline. Good job on improving both the test coverage and the overall quality of the test suite!
🧰 Tools
🪛 Biome
[error] 181-181: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
tests/k6/tests/enduser/dialogSearch.js (1)
200-200: Approve: Improved invalid process filter testThis change addresses the parsing issue mentioned in the PR objectives while maintaining the effectiveness of the test. The new invalid process filter 'inval|d' is a good choice because:
- It doesn't contain spaces or special characters that might cause parsing problems in the CI environment.
- It still represents an invalid process identifier, ensuring the test's purpose is preserved.
- The test expectations remain unchanged, correctly asserting a 400 status code and checking for error messages.
This modification should resolve the parsing error during k6 runs in CI without compromising the test's integrity.
| @@ -6,7 +6,7 @@ export default function (){ | |||
|
|
|||
| describe ('Attempt to create dialog with invalid URI', () => { | |||
| let dialog = dialogToInsert(); | |||
| dialog.process = '?? ?'; | |||
| dialog.process = 'inval|d'; | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM! Consider enhancing test coverage.
The change from '?? ?' to 'inval|d' for dialog.process is appropriate and aligns with the PR objective of addressing parsing issues for invalid URI tests. This should effectively trigger the expected 400 error.
To further improve the test case:
- Consider adding assertions to check for specific error messages or codes related to the invalid URI.
- It might be beneficial to include additional test cases with different types of invalid URIs to ensure comprehensive coverage.
Would you like assistance in implementing these suggestions?
| let batch = []; | ||
|
|
||
| for (let i=0; i<10; i++) { | ||
| let activity = { type: "Information", performedBy: { actorType: "serviceOwner" }, description: [ { value: i.toString(), languageCode: "nb"}]}; | ||
| promises.push(postSOAsync('dialogs/' + dialogId + '/activities?' + i, activity)) | ||
| batch.push(['dialogs/' + dialogId + '/activities?' + i, activity]); | ||
| } | ||
|
|
||
| const results = await Promise.all(promises); | ||
| const results = postBatchSO(batch); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM: Improved concurrency test implementation with minor suggestions
The changes effectively address the PR objective of fixing the output order from concurrency tests by using a batch operation instead of individual promises. This simplifies the concurrency handling and should resolve the parsing issues.
However, there are a few minor improvements we can make:
- On line 23, consider using a template literal instead of string concatenation:
batch.push([`dialogs/${dialogId}/activities?${i}`, activity]);- On lines 19 and 22, you can use
constinstead ofletas these variables are not reassigned:
const batch = [];
// ...
for (let i=0; i<10; i++) {
const activity = { /* ... */ };
// ...
}These changes will improve code readability and follow best practices.
🧰 Tools
🪛 Biome
[error] 23-23: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 19-19: This let declares a variable that is only assigned once.
'batch' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 22-22: This let declares a variable that is only assigned once.
'activity' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
| .IsValidUri() | ||
| .MaximumLength(Constants.DefaultMaxUriLength) | ||
| .When(x => x.Process is not null); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM: Improved Process property validation.
The changes to the Process property validation are good improvements:
- Using
IsValidUri()likely provides a more comprehensive check for well-formed URIs. - Adding a maximum length constraint with
Constants.DefaultMaxUriLengthis a good practice to prevent issues with extremely long URIs.
These changes align well with the PR objective of addressing the parsing issue related to invalid URI tests.
Consider adding a comment explaining the purpose of Constants.DefaultMaxUriLength for better code documentation. For example:
.MaximumLength(Constants.DefaultMaxUriLength) // Limit URI length to prevent potential issues with extremely long URIs| export function postBatchSO(batch) { | ||
| const processedBatch = batch.map(([url, body, params]) => { | ||
| params = extend(true, {}, params, { headers: { 'Content-Type': 'application/json' }}); | ||
| return ['POST', baseUrlServiceOwner + url, JSON.stringify(body), getServiceOwnerRequestParams(params)]; | ||
| }); | ||
|
|
||
| return http.batch(processedBatch); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Good implementation, with suggestions for improvement
The postBatchSO function is a valuable addition for efficient batch processing of requests. It effectively reuses existing utility functions and maintains consistency with the codebase. However, consider the following improvements:
- Add input validation for the
batchparameter to ensure it's an array and each item has the expected structure. - Consider supporting different HTTP methods, not just POST, to increase flexibility.
- For consistency with other functions, add a
tokenOptionsparameter and pass it togetServiceOwnerRequestParams.
Here's a suggested implementation incorporating these improvements:
export function batchRequestsSO(batch, tokenOptions = null) {
if (!Array.isArray(batch)) {
throw new Error('batch must be an array');
}
const processedBatch = batch.map(([method, url, body, params]) => {
if (!['GET', 'POST', 'PUT', 'PATCH', 'DELETE'].includes(method)) {
throw new Error(`Unsupported HTTP method: ${method}`);
}
params = extend(true, {}, params, { headers: { 'Content-Type': 'application/json' }});
return [
method,
baseUrlServiceOwner + url,
body ? JSON.stringify(body) : null,
getServiceOwnerRequestParams(params, tokenOptions)
];
});
return http.batch(processedBatch);
}This implementation adds input validation, supports multiple HTTP methods, and includes the tokenOptions parameter.
oskogstad
deleted the
chore/attempt-at-fixing-parsing-error-during-k6-run-in-ci
branch
Related issue: #1184
Summary by CodeRabbit
New Features
Bug Fixes
Documentation