feat(performance): create a k6 purge script to run after creating dialogs #1435
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces a new script, 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
tests/k6/tests/serviceowner/performance/purge-dialogs.js (3)
85-85: Clarify single-user mode detection logicThe expression used to determine
isSingleUserModecould be simplified for readability. Consider refactoring the logic to make it clearer.Apply this diff to enhance readability:
- const isSingleUserMode = (options.vus ?? 1) === 1 && (options.iterations ?? 1) === 1 && (options.duration ?? 0) === 0; + const vus = options.vus ?? 1; + const iterations = options.iterations ?? 1; + const duration = options.duration ?? 0; + const isSingleUserMode = vus === 1 && iterations === 1 && duration === 0;
102-107: Refactor duplicated 'paramsWithToken' objectThe
paramsWithTokenobject is similarly defined in bothgetDialogsandpurgeDialogs. Consider extracting this into a shared helper function or variable to reduce code duplication.Example of creating a helper function:
function createParamsWithToken(token, additionalHeaders = {}) { return { headers: { Authorization: "Bearer " + token, ...additionalHeaders } }; }Then modify the code accordingly:
- var paramsWithToken = { - headers: { - Authorization: "Bearer " + serviceOwner.token - }, - tags: { name: 'purge dialog' } - } + var paramsWithToken = createParamsWithToken(serviceOwner.token, { tags: { name: 'purge dialog' } });
112-112: Use 'let' instead of 'var' for block scopingReplace
varwithletto use block-level scoping and prevent potential issues related to hoisting.Apply this diff:
- for(var i = dialogIdsToPurge.length - 1; i>=0; i--) { + for(let i = dialogIdsToPurge.length - 1; i >= 0; i--) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
tests/k6/tests/serviceowner/performance/purge-dialogs.js(1 hunks)
🔇 Additional comments (1)
tests/k6/tests/serviceowner/performance/purge-dialogs.js (1)
44-49: Review the termination condition logic in the loop
The condition if (dialogIdsToPurge.length < 200) seems arbitrary and might not correctly handle pagination. This could lead to incomplete retrieval of dialogs when there are more than 200 dialogs to purge. Consider revising the logic to ensure all dialogs are fetched.
Would you like to verify if all dialogs are being retrieved when there are more than 200? If so, you can run the following script:
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
tests/k6/tests/serviceowner/performance/purge-dialogs.js (3)
47-47: Move logging outside the pagination loop.The log statement is inside the loop, which will print multiple times during pagination. Consider moving it after the loop for a single summary.
- console.log("Found " + dialogIdsToPurge.length + " unpurged dialogs");And add after line 54:
+ console.log("Found " + dialogIdsToPurge.length + " unpurged dialogs");
58-64: Document the rationale for the 4-minute setup timeout.The setupTimeout value of '4m' seems arbitrary. Consider adding a comment explaining why this specific duration was chosen and what factors might require adjusting it.
89-89: Improve readability of single-user mode check.The condition using nullish coalescing could be more readable by extracting it to a helper function or adding comments explaining the logic.
- const isSingleUserMode = (options.vus ?? 1) === 1 && (options.iterations ?? 1) === 1 && (options.duration ?? 0) === 0; + // Check if running in single-user mode: + // - Virtual Users (vus) = 1 + // - Single iteration + // - No duration specified + const isSingleUserMode = + (options.vus ?? 1) === 1 && + (options.iterations ?? 1) === 1 && + (options.duration ?? 0) === 0;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
tests/k6/tests/serviceowner/performance/purge-dialogs.js(1 hunks)
🔇 Additional comments (3)
tests/k6/tests/serviceowner/performance/purge-dialogs.js (3)
1-16: LGTM! Well-documented with clear purpose and usage.
The documentation and imports are comprehensive and appropriate for a k6 test script.
66-77: LGTM! Well-structured setup with proper validation.
The setup function properly validates inputs and handles errors appropriately.
48-53:
Review the artificial limit of 200 dialogs.
The code artificially stops pagination after 200 dialogs. This could leave dialogs unpurged if there are more than 200 matching the sentinel value.
Let's check if this limit could be problematic:
Consider removing this limit or making it configurable:
- if (dialogIdsToPurge.length < 200) {
- hasNextPage = response.hasNextPage;
- }
- else {
- hasNextPage = false;
- }
+ hasNextPage = response.hasNextPage;
🤖 I have created a release *beep* *boop* --- ## [1.32.0](v1.31.0...v1.32.0) (2024-11-12) ### Features * **graphql:** Set max execution depth to 10 ([#1431](#1431)) ([8845e49](8845e49)), closes [#1430](#1430) * **performance:** create a k6 purge script to run after creating dialogs ([#1435](#1435)) ([9555d78](9555d78)) * **performance:** Expands search for serviceowners, improved tracing and logging ([#1439](#1439)) ([b1d6eaf](b1d6eaf)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
A script that purges dialogs created by performance tests. Much is copied from common/sentinel.js, but that script didn't quite fit in.
Purges max 200 in one run, searching and purging is time-consuming at the moment
Description
Related Issue(s)
Verification
Documentation
docs-directory, Altinnpedia or a separate linked PR in altinn-studio-docs., if applicable)Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes