chore(perfomance): Fixing github action to run performance tests in k8s #1739
Conversation
📝 WalkthroughWalkthroughThis pull request introduces modifications to the K6 performance testing infrastructure, focusing on updating workflow configurations and refactoring performance test scripts. Key changes include altering the workflow reference for performance tests, adding new input parameters and secrets for Azure integration, and implementing a new workflow for executing K6 tests. Additionally, the test scripts have been streamlined to utilize a centralized data validation approach, enhancing the overall structure and logic of the performance testing process. Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
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: 0
🧹 Nitpick comments (7)
tests/k6/tests/serviceowner/performance/createDialogWithThresholds.js (1)
2-2: Simplify the setup export statement.The current export statement uses an unnecessary rename since both the imported and exported names are 'setup'.
-export { setup as setup } from '../../performancetest_common/readTestdata.js'; +export { setup } from '../../performancetest_common/readTestdata.js';🧰 Tools
🪛 Biome (1.9.4)
[error] 2-2: Useless rename.
Safe fix: Remove the renaming.
(lint/complexity/noUselessRename)
tests/k6/tests/serviceowner/performance/create-dialog.js (1)
7-9: Consider consolidating imports from readTestdata.js.Multiple imports from the same module can be combined for better readability.
-import { serviceOwners } from '../../performancetest_common/readTestdata.js'; -import { validateTestData } from '../../performancetest_common/readTestdata.js'; -export { setup as setup } from '../../performancetest_common/readTestdata.js'; +import { serviceOwners, validateTestData } from '../../performancetest_common/readTestdata.js'; +export { setup } from '../../performancetest_common/readTestdata.js';🧰 Tools
🪛 Biome (1.9.4)
[error] 9-9: Useless rename.
Safe fix: Remove the renaming.
(lint/complexity/noUselessRename)
tests/k6/tests/performancetest_common/readTestdata.js (3)
55-71: Consider using more descriptive variable names.While the logic is sound, the variable names could be more descriptive:
ixStart→startIndexextras→remainingUsersfunction endUsersPart(totalVus, vuId) { const endUsersLength = endUsers.length; if (totalVus == 1) { return endUsers.slice(0, endUsersLength); } let usersPerVU = Math.floor(endUsersLength / totalVus); - let extras = endUsersLength % totalVus; - let ixStart = (vuId-1) * usersPerVU; + let remainingUsers = endUsersLength % totalVus; + let startIndex = (vuId-1) * usersPerVU; if (vuId <= extras) { usersPerVU++; - ixStart += vuId - 1; + startIndex += vuId - 1; } else { - ixStart += extras; + startIndex += remainingUsers; } - return endUsers.slice(ixStart, ixStart + usersPerVU); + return endUsers.slice(startIndex, startIndex + usersPerVU); }
73-80: Add error handling for edge cases.The setup function should handle cases where VU count is invalid or unavailable.
export function setup() { const totalVus = exec.test.options.scenarios.default.vus; + if (!totalVus || totalVus <= 0) { + throw new Error('Invalid or missing VU count in test options'); + } let parts = []; for (let i = 1; i <= totalVus; i++) { parts.push(endUsersPart(totalVus, i)); } return parts; }
82-99: Add JSDoc documentation for better code clarity.The function would benefit from proper parameter and return type documentation.
+/** + * Validates the test data structure and returns validated end users with their index. + * @param {Array} data - Array of end user arrays, one per VU + * @param {Array|null} serviceOwners - Optional array of service owners + * @returns {{endUsers: Array, index: number}} Object containing validated end users and current index + * @throws {Error} If data structure is invalid + */ export function validateTestData(data, serviceOwners=null) {tests/k6/tests/serviceowner/performance/create-remove-dialog.js (1)
9-9: Simplify the export syntax.The current export syntax is unnecessarily verbose.
-export { setup as setup } from '../../performancetest_common/readTestdata.js'; +export { setup } from '../../performancetest_common/readTestdata.js';🧰 Tools
🪛 Biome (1.9.4)
[error] 9-9: Useless rename.
Safe fix: Remove the renaming.
(lint/complexity/noUselessRename)
tests/k6/tests/serviceowner/performance/create-transmissions.js (1)
22-23: Consider documenting environment variables.The function uses several environment variables (numberOfTransmissions, maxTransmissionsInThread, testid) that should be documented for better maintainability.
+/** + * @param {Array} data - Test data array from setup + * @env {string} numberOfTransmissions - Number of transmissions to create (default: 10) + * @env {string} maxTransmissionsInThread - Max transmissions per thread (default: 100) + * @env {string} testid - Test identifier (default: 'createTransmissions') + */ export default function(data) { const { endUsers, index } = validateTestData(data, serviceOwners); createTransmissions(serviceOwners[0], endUsers[index], traceCalls, numberOfTransmissions, maxTransmissionsInThread, testid); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
tests/k6/common/token.js(1 hunks)tests/k6/tests/enduser/performance/enduser-search.js(2 hunks)tests/k6/tests/graphql/performance/graphql-search.js(2 hunks)tests/k6/tests/performancetest_common/readTestdata.js(2 hunks)tests/k6/tests/serviceowner/performance/create-dialog.js(1 hunks)tests/k6/tests/serviceowner/performance/create-remove-dialog.js(2 hunks)tests/k6/tests/serviceowner/performance/create-transmissions.js(1 hunks)tests/k6/tests/serviceowner/performance/createDialogWithThresholds.js(1 hunks)tests/k6/tests/serviceowner/performance/serviceOwnerSearchWithThresholds.js(1 hunks)tests/k6/tests/serviceowner/performance/serviceowner-search.js(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/k6/common/token.js
🧰 Additional context used
🪛 Biome (1.9.4)
tests/k6/tests/serviceowner/performance/serviceOwnerSearchWithThresholds.js
[error] 2-2: Useless rename.
Safe fix: Remove the renaming.
(lint/complexity/noUselessRename)
tests/k6/tests/serviceowner/performance/createDialogWithThresholds.js
[error] 2-2: Useless rename.
Safe fix: Remove the renaming.
(lint/complexity/noUselessRename)
tests/k6/tests/graphql/performance/graphql-search.js
[error] 8-8: Useless rename.
Safe fix: Remove the renaming.
(lint/complexity/noUselessRename)
tests/k6/tests/serviceowner/performance/create-dialog.js
[error] 9-9: Useless rename.
Safe fix: Remove the renaming.
(lint/complexity/noUselessRename)
tests/k6/tests/serviceowner/performance/create-transmissions.js
[error] 9-9: Useless rename.
Safe fix: Remove the renaming.
(lint/complexity/noUselessRename)
tests/k6/tests/serviceowner/performance/serviceowner-search.js
[error] 5-5: Useless rename.
Safe fix: Remove the renaming.
(lint/complexity/noUselessRename)
tests/k6/tests/enduser/performance/enduser-search.js
[error] 4-4: Useless rename.
Safe fix: Remove the renaming.
(lint/complexity/noUselessRename)
tests/k6/tests/serviceowner/performance/create-remove-dialog.js
[error] 9-9: Useless rename.
Safe fix: Remove the renaming.
(lint/complexity/noUselessRename)
🔇 Additional comments (7)
tests/k6/tests/serviceowner/performance/serviceOwnerSearchWithThresholds.js (1)
2-2: Simplify the setup export statement.The current export statement uses an unnecessary rename since both the imported and exported names are 'setup'.
-export { setup as setup } from '../../performancetest_common/readTestdata.js'; +export { setup } from '../../performancetest_common/readTestdata.js';🧰 Tools
🪛 Biome (1.9.4)
[error] 2-2: Useless rename.
Safe fix: Remove the renaming.
(lint/complexity/noUselessRename)
tests/k6/tests/enduser/performance/enduser-search.js (2)
3-4: LGTM! Good improvement in test data handling.The introduction of validateTestData centralizes validation logic and improves code maintainability.
🧰 Tools
🪛 Biome (1.9.4)
[error] 4-4: Useless rename.
Safe fix: Remove the renaming.
(lint/complexity/noUselessRename)
22-24: Verify validateTestData implementation.The function now relies on validateTestData for input validation. Please ensure that validateTestData includes comprehensive checks for:
- Array structure validation
- Non-empty array verification
- Index bounds checking
✅ Verification successful
validateTestData implementation is comprehensive and correct
The implementation in
tests/k6/tests/performancetest_common/readTestdata.jsincludes all required validations:
- Validates array structure for both outer and inner arrays
- Ensures arrays are non-empty
- Performs index bounds checking using
exec.vu.idInTest🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check validateTestData implementation ast-grep --pattern 'function validateTestData($_) { $$$ }'Length of output: 62
Script:
#!/bin/bash # Search for validateTestData definition rg "validateTestData" -l rg "validateTestData.*=.*" -A 10Length of output: 1667
tests/k6/tests/serviceowner/performance/create-dialog.js (1)
18-20: Verify serviceOwners validation in validateTestData.The function now uses validateTestData with serviceOwners parameter. Please ensure proper validation of serviceOwners array before accessing index 0.
✅ Verification successful
Proper serviceOwners validation confirmed in validateTestData
The validateTestData function in readTestdata.js includes robust validation that ensures serviceOwners is a non-empty array before it's used, making the access to serviceOwners[0] safe.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check validateTestData's serviceOwners validation ast-grep --pattern 'function validateTestData($_, $serviceOwners) { $$$ }'Length of output: 78
Script:
#!/bin/bash # First, find where validateTestData is defined rg "function validateTestData" -A 10Length of output: 1238
tests/k6/tests/serviceowner/performance/create-remove-dialog.js (1)
21-24: LGTM! Clean and efficient implementation.The refactored code is more concise and leverages the new validation utility effectively.
tests/k6/tests/graphql/performance/graphql-search.js (1)
25-25: Add serviceOwners parameter to validateTestData call.The validateTestData function is called without the optional serviceOwners parameter, which is inconsistent with other test files.
- const { endUsers, index } = validateTestData(data); + const { endUsers, index } = validateTestData(data, null);Let's verify the usage pattern across files:
tests/k6/tests/serviceowner/performance/serviceowner-search.js (1)
21-24: LGTM! Clean implementation following the new pattern.The changes align well with the established pattern across other test files.
…runner' into performance/github-runner
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/dispatch-k6-performance.yml (1)
38-38: Remove trailing spaces.There are trailing spaces at the end of line 38.
- type: number + type: number🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 38-38: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/dispatch-k6-performance.yml(2 hunks).github/workflows/workflow-run-k6-performance.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/workflow-run-k6-performance.yml
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/dispatch-k6-performance.yml
[error] 38-38: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Run K6 performance test / k6-test
- GitHub Check: Dry run deploy infrastructure / Deploy to test
🔇 Additional comments (4)
.github/workflows/dispatch-k6-performance.yml (4)
34-38: LGTM! Well-structured parallelism parameter.The new parameter is well-defined with appropriate type constraints and a sensible default value that maintains backward compatibility.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 38-38: trailing spaces
(trailing-spaces)
52-52: LGTM! Clear run name format.The run name clearly displays all key test parameters including the new parallelism value.
68-68: LGTM! Consistent parameter handling.The parallelism parameter is correctly passed using
fromJsonto ensure proper number type conversion, consistent with how thevusparameter is handled.
60-61: Verify Azure authentication configuration.The Azure Client ID and Tenant ID are added, but please verify if additional Azure credentials (like Client Secret or Certificate) are required for the authentication to work properly in the workflow being called.
✅ Verification successful
Azure authentication configuration is properly set up ✅
The workflow is using OIDC-based authentication which only requires the Client ID and Tenant ID. No additional credentials are needed as the configuration is complete with the proper permissions set.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the called workflow requires additional Azure credentials rg -A 10 'AZURE_(CLIENT_ID|TENANT_ID)' .github/workflows/workflow-run-k6-performance.ymlLength of output: 733
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
tests/k6/tests/scripts/run-test-in-k8s.sh (1)
Line range hint
89-98: Add validation for input parameter values.While the script checks for missing parameters, it doesn't validate their values. Consider adding validation for:
- Duration format and minimum value
- VUs being a positive integer
- Parallelism being within reasonable bounds
if [ ${#missing_args[@]} -ne 0 ]; then echo "Error: Missing required arguments: ${missing_args[*]}" help exit 1 fi + +# Validate parameter values +if ! [[ "$vus" =~ ^[1-9][0-9]*$ ]]; then + echo "Error: vus must be a positive integer" + exit 1 +fi + +if ! [[ "$parallelism" =~ ^[1-9][0-9]*$ ]] || [ "$parallelism" -gt 10 ]; then + echo "Error: parallelism must be a positive integer not exceeding 10" + exit 1 +fi + +if ! [[ "$duration" =~ ^[1-9][0-9]*(s|m|h)$ ]]; then + echo "Error: duration must be in format: <number>(s|m|h)" + exit 1 +fi
🧹 Nitpick comments (3)
tests/k6/tests/scripts/run-test-in-k8s.sh (3)
5-7: Add error handling for kubectl context command.While setting the namespace context is good practice, the command should include error handling to ensure the operation succeeded.
failed=0 -kubectl config set-context --current --namespace=dialogporten +if ! kubectl config set-context --current --namespace=dialogporten; then + echo "Error: Failed to set kubernetes context to dialogporten namespace" + exit 1 +fi
44-48: Add timeout handling for kubectl commands.The kubectl commands for logs and status could hang indefinitely. Consider adding timeouts.
- kubectl logs --tail=-1 $pod - status=`kubectl get $pod -o jsonpath='{.status.phase}'` + if ! timeout 30s kubectl logs --tail=-1 $pod; then + echo "Error: Timed out getting logs for $pod" + failed=1 + fi + status=$(timeout 30s kubectl get $pod -o jsonpath='{.status.phase}' || echo "Failed") + if [ "$status" = "Failed" ]; then + echo "Error: Failed to get status for $pod" + failed=1 + fi
158-160: Improve cleanup robustness and documentation.The cleanup function includes a hardcoded sleep duration without explanation and lacks timeout handling for cleanup operations.
- Document why the sleep is necessary
- Make the sleep duration configurable
- Add timeout handling for cleanup operations
- local exit_code=$failed - echo "Sleeping for 15s and then cleaning up resources..." - sleep 15 + local exit_code=$failed + # Wait for Kubernetes to finish any pending operations before cleanup + local cleanup_wait="${CLEANUP_WAIT_DURATION:-15}" + echo "Sleeping for ${cleanup_wait}s and then cleaning up resources..." + sleep "$cleanup_wait" + + echo "Starting cleanup..." + local cleanup_timeout=30 + if ! timeout ${cleanup_timeout}s kubectl delete -f config.yml --ignore-not-found; then + echo "Warning: Cleanup of config.yml timed out after ${cleanup_timeout}s" + fi
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/k6/tests/scripts/run-test-in-k8s.sh(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Dry run deploy infrastructure / Deploy to test
- GitHub Check: build / build-and-test
🤖 I have created a release *beep* *boop* --- ## [1.47.5](v1.47.4...v1.47.5) (2025-01-30) ### Bug Fixes * **auth:** Allow .noconsent scope in EndUser auth policy ([#1760](#1760)) ([d770779](d770779)) * **auth:** Split values when checking EndUser scopes ([#1764](#1764)) ([5957e7d](5957e7d)) ### Miscellaneous Chores * Add FormSubmitted and FormSaved to ActivityType ([#1742](#1742)) ([4b9bad0](4b9bad0)) * **deps:** update dependency ziggycreatures.fusioncache to v2 ([#1752](#1752)) ([dd24928](dd24928)) * **perfomance:** Fixing github action to run performance tests in k8s ([#1739](#1739)) ([166d53d](166d53d)) * Remove old OccuredAt property on DomainEvent ([#1758](#1758)) ([67ee75d](67ee75d)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: Are Almaas <arealmaas@gmail.com>
Description
Related Issue(s)
Verification
Documentation
docs-directory, Altinnpedia or a separate linked PR in altinn-studio-docs., if applicable)