test: exclude threshold tests from all tests #1685
Conversation
📝 WalkthroughWalkthroughThe pull request introduces changes to the CI/CD workflow configuration and the organization of performance test scripts. It updates file paths in the 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)
⏰ Context from checks skipped due to timeout of 90000ms (2)
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: 0
🧹 Nitpick comments (1)
tests/k6/scripts/generate_all_tests.sh (1)
18-18: Consider using more precise path matching.The current pattern matching could unintentionally exclude files if "performance" appears anywhere in the path. Consider using a more precise check that specifically matches the
/performance/directory structure.- if [[ $DIRECTORY_PATH != *"performance"* ]]; then + if [[ $DIRECTORY_PATH != */performance/* ]]; then
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/k6/scripts/generate_all_tests.sh(2 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
🔇 Additional comments (3)
tests/k6/scripts/generate_all_tests.sh (3)
19-27: LGTM!The file processing logic is well-structured and properly indented within the new condition block.
38-38: LGTM!The formatting change follows common shell scripting conventions.
Line range hint
18-38: Verify the script with the new directory structure.The changes align well with the PR objectives. Let's verify that:
- The script correctly identifies and excludes performance tests
- All expected test files are still being processed
✅ Verification successful
Script correctly handles the directory structure and test segregation
The verification confirms that:
- Performance tests are properly identified and excluded across all directories
- Regular test files and utilities are correctly included in the processing
- No false positives or negatives in the test file selection
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the script's behavior with the new directory structure # Test 1: Check if we have any remaining test files in performance directories echo "Test files in performance directories:" fd -e js . tests/k6 | grep -i "performance" # Test 2: List all test files that will be processed (excluding performance) echo -e "\nTest files that will be processed:" fd -e js . tests/k6 | grep -v -i "performance" | grep -v "all-tests.js"Length of output: 2845
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/k6/scripts/generate_all_tests.ps1 (1)
Line range hint
1-24: Add error handling and improve logging.The script would benefit from additional robustness improvements:
- Add validation for empty file list
- Add logging to show which files were included/excluded
- Add documentation about the performance directory requirement
Consider adding these improvements:
param( [Parameter(Mandatory=$true)] [string]$DirectoryPath ) + +# Document the performance directory requirement +<# +.SYNOPSIS + Generates all-tests.js by combining test files from the performance directory. +.PARAMETER DirectoryPath + The root directory to search for test files. Only files under a 'performance' subdirectory will be included. +#> # Verify that the directory exists if (-not (Test-Path $DirectoryPath -PathType Container)) { Write-Error "The supplied directory does not exist!" exit 1 } # Get all *.js files in the directory except for "all-tests.js" $jsFiles = Get-ChildItem -Path $DirectoryPath -Filter "*.js" | Where-Object { $_.Name -ne "all-tests.js" -and $_.Directory.FullName -like "*$(Join-Path 'performance' '*')" } + +# Validate that files were found +if (-not $jsFiles) { + Write-Error "No .js files found in performance directory!" + exit 1 +} + +# Log included files +Write-Output "Including the following test files:" +$jsFiles | ForEach-Object { Write-Output " - $($_.FullName)" }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/k6/scripts/generate_all_tests.ps1(1 hunks)tests/k6/scripts/generate_all_tests.sh(2 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
🔇 Additional comments (4)
tests/k6/scripts/generate_all_tests.sh (3)
17-20: LGTM! The skip condition aligns with PR objectives.The implementation correctly excludes both performance tests and the generated file itself. The pattern matching will work for any nested performance directories, which provides flexibility in the directory structure.
39-39: LGTM! Minor style improvement.The removal of space before the redirection operator is a good practice for shell scripts.
Line range hint
1-39: Verify the impact on existing test files.While the changes look good, let's verify that no critical test files are accidentally excluded by the new skip condition.
✅ Verification successful
Skip condition correctly identifies performance tests and includes regular tests
The verification confirms that:
- Performance tests are properly isolated in dedicated "performance" directories
- Regular test files like enduser-tests.js and serviceowner-tests.js are correctly included
- No critical test files are accidentally excluded by the skip condition
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any test files in performance directories that might be critical # List all JS files in any performance directory echo "JS files in performance directories:" fd -e js . | grep -i "performance" # List all test files that will be included in all-tests.js echo -e "\nJS files that will be included in all-tests.js:" for dir in $(fd -t d . -E "performance"); do for js in "$dir"/*.js; do if [[ -f "$js" && ! "$js" =~ /performance/ && ! "$js" =~ all-tests.js$ ]]; then echo "$js" fi done doneLength of output: 64775
tests/k6/scripts/generate_all_tests.ps1 (1)
Line range hint
25-39: LGTM! Output handling is well implemented.The script properly handles:
- UTF-8 encoding
- Consistent line endings
- No trailing newline
…-tests' into test/exclude-perf-tests-from-all-tests
Description
Removed the threshold tests which are used only in performance tests, and moved the files under
/performance. Also removed some unneded data files that were only required for testing towards at21Related Issue(s)
Verification
Documentation
docs-directory, Altinnpedia or a separate linked PR in altinn-studio-docs., if applicable)