fix: use NX for test parallelism #17652
Conversation
|
Warning Rate limit exceeded@busla has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 19 minutes and 27 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe pull request introduces changes to the CI/CD pipeline, focusing on test execution and project configuration. A new input parameter Changes
Possibly related PRs
Suggested labels
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: 0
🧹 Nitpick comments (1)
.github/actions/unit-test/action.yml (1)
4-6: Improve the input parameter description.The description "Generated projects" is too vague. Consider providing more context about what these projects represent and how they are generated.
affected-projects: - description: 'Generated projects' + description: 'Comma-separated list of projects affected by changes, generated by NX for parallel test execution'
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/actions/unit-test/action.yml(3 hunks).github/workflows/push.yml(1 hunks)scripts/ci/30_test.sh(1 hunks)scripts/ci/run-affected-in-parallel.sh(0 hunks)
💤 Files with no reviewable changes (1)
- scripts/ci/run-affected-in-parallel.sh
🧰 Additional context used
📓 Path-based instructions (1)
scripts/ci/30_test.sh (1)
Pattern scripts/**/*: "Confirm that the code adheres to the following:
- Script efficiency, readability, and maintainability.
- Usage of environmental variables and configuration files for flexibility.
- Integration with NX workspace utilities and commands."
🔇 Additional comments (3)
scripts/ci/30_test.sh (1)
52-52: LGTM! But verify the variable usage consistency.The change to use NX's
run-manycommand for test execution is good and aligns with the PR objective. However, the script still uses${APP}for flaky test detection and code coverage collection while using${AFFECTED_PROJECTS}for test execution.Please verify that:
- Flaky test detection works correctly with multiple projects
- Code coverage collection works as expected with the new parallel execution
- The environment variable
DD_SERVICEis set correctly for each projectRun this script to check the variable usage:
✅ Verification successful
All variable usages are consistent and working correctly with parallel execution
The script handles all three verification points correctly:
- Flaky test detection works per-project using the APP variable
- Code coverage collection and upload are properly configured per-project
- DD_SERVICE environment variable is correctly set with proper fallbacks
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential issues with variable usage in the test script # Test 1: Check if APP is still used in other scripts echo "Checking APP variable usage in other scripts..." rg -l '\$APP|\$\{APP\}' scripts/ # Test 2: Check if AFFECTED_PROJECTS is consistently used echo "Checking AFFECTED_PROJECTS variable usage..." rg -l '\$AFFECTED_PROJECTS|\$\{AFFECTED_PROJECTS\}' scripts/ # Test 3: Check for potential variable conflicts echo "Checking for potential variable conflicts in CI scripts..." rg '(APP|AFFECTED_PROJECTS)=' scripts/Length of output: 1209
Script:
#!/bin/bash # Check specific usage of APP in test and coverage scripts echo "Checking APP usage in test script..." rg -A 5 -B 5 '\$APP|\$\{APP\}' scripts/ci/30_test.sh echo -e "\nChecking APP usage in coverage script..." rg -A 5 -B 5 '\$APP|\$\{APP\}' scripts/ci/50_upload-coverage.sh echo -e "\nChecking DD_SERVICE configuration..." rg -A 5 -B 5 'DD_SERVICE' scripts/ci/Length of output: 3191
.github/actions/unit-test/action.yml (1)
33-33: LGTM! Clean migration to NX-based test execution.The changes correctly wire up the affected projects through environment variables and simplify test execution by leveraging NX's built-in parallelism capabilities.
Also applies to: 48-49
.github/workflows/push.yml (1)
486-486: LGTM! Consistent integration of NX parallel test execution.The change correctly wires up the affected projects parameter to the unit-test action, completing the integration of NX-based parallel test execution.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
scripts/ci/30_test.sh (5)
37-46: Consider a more specific function name.The function correctly implements flaky test detection, but consider renaming it to
has_flaky_projectsorcontains_flaky_projectsto better reflect its purpose.-is_any_project_flaky() { +has_flaky_projects() {
48-57: Maintain consistency in array comparison logic.The array comparison could be simplified to match the pattern used in
is_any_project_flaky().- if [[ ! " ${projects_uncollectible_coverage[*]} " =~ \ ${project}\ ]]; then + if [[ " ${projects_uncollectible_coverage[*]} " != *" ${project} "* ]]; then
59-62: Consider a more specific variable name.Since EXTRA_OPTS is only used for code coverage, consider renaming it to COVERAGE_OPTS for clarity.
-EXTRA_OPTS="" +COVERAGE_OPTS="" if requires_code_coverage; then - EXTRA_OPTS="--codeCoverage" + COVERAGE_OPTS="--codeCoverage" fi
71-78: Enhance error handling in the retry loop.While the retry logic is solid, consider improving error handling:
- Add a descriptive error message when all retries are exhausted
- Distinguish between different types of failures (e.g., test failures vs infrastructure issues)
for ((i = 1; i <= MAX_RETRIES; i++)); do echo "Running tests for projects: ${AFFECTED_PROJECTS} (attempt: ${i}/${MAX_RETRIES})" - if yarn nx run-many --projects "${AFFECTED_PROJECTS}" --target test ${EXTRA_OPTS} --verbose --no-watchman "$@"; then + if ! yarn nx run-many --projects "${AFFECTED_PROJECTS}" --target test ${EXTRA_OPTS} --verbose --no-watchman "$@"; then + exit_code=$? + if [ $exit_code -eq 1 ]; then + echo "Tests failed with exit code ${exit_code}" + [ $i -eq $MAX_RETRIES ] && echo "All retry attempts exhausted. Tests failed." && exit 1 + continue + else + echo "Infrastructure/setup error (exit code: ${exit_code}). Stopping execution." + exit $exit_code + fi + else exit 0 fi done - -exit 1
Line range hint
1-78: Well-structured implementation of NX-based parallel testing.The script successfully implements parallel test execution using NX while maintaining proper handling of flaky tests and code coverage. The implementation is clean, follows bash best practices, and aligns well with the PR objectives.
Consider documenting the following in the repository's documentation:
- The purpose and configuration of flaky tests
- The criteria for excluding projects from code coverage
- The retry strategy and its rationale
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/ci/30_test.sh(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
scripts/ci/30_test.sh (1)
Pattern scripts/**/*: "Confirm that the code adheres to the following:
- Script efficiency, readability, and maintainability.
- Usage of environmental variables and configuration files for flexibility.
- Integration with NX workspace utilities and commands."
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: prepare
🔇 Additional comments (2)
scripts/ci/30_test.sh (2)
7-7: LGTM! Simplified environment variable initialization.The simplified initialization of DD_SERVICE improves clarity while maintaining the correct DataDog service naming.
64-69: LGTM! Clear retry configuration logic.The retry configuration is well-structured and correctly handles both flaky and non-flaky test scenarios.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #17652 +/- ##
=======================================
Coverage 35.56% 35.56%
=======================================
Files 7033 7033
Lines 150621 150621
Branches 43022 43031 +9
=======================================
+ Hits 53561 53562 +1
+ Misses 97060 97059 -1
Flags with carried forward coverage won't be shown. Click here to find out more. see 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
scripts/ci/30_test.sh (4)
37-46: Add documentation for the expected input format.The function works correctly, but it would be helpful to add a comment explaining that
AFFECTED_PROJECTSshould be a comma-separated list of project names.+# Expects AFFECTED_PROJECTS as a comma-separated list of project names +# Example: "project1,project2,project3" is_any_project_flaky() {
48-57: Optimize the function with early return.The function can be more efficient by returning early when finding the first project that requires code coverage.
requires_code_coverage() { IFS=',' read -ra PROJECTS <<<"$AFFECTED_PROJECTS" for project in "${PROJECTS[@]}"; do if [[ ! " ${projects_uncollectible_coverage[*]} " =~ \ ${project}\ ]]; then return 0 - fi - done - return 1 + fi + done + return 1 }
64-69: Consider moving configuration to a separate file.To improve maintainability and follow the guideline for using configuration files, consider moving
FLAKY_TEST_RETRIESand other configuration values to a separate config file.This would allow easier management of environment-specific settings.
71-78: Enhance error handling and logging.While the NX integration is well-implemented, consider improving error handling by capturing and logging the error output for better debugging in CI.
for ((i = 1; i <= MAX_RETRIES; i++)); do echo "Running tests for projects: ${AFFECTED_PROJECTS} (attempt: ${i}/${MAX_RETRIES})" - if yarn nx run-many --projects "${AFFECTED_PROJECTS}" --target test --parallel="${NX_PARALLEL}" ${EXTRA_OPTS} --verbose --no-watchman "$@"; then + if OUTPUT=$(yarn nx run-many --projects "${AFFECTED_PROJECTS}" --target test --parallel="${NX_PARALLEL}" ${EXTRA_OPTS} --verbose --no-watchman "$@" 2>&1); then exit 0 + else + echo "Test execution failed with output:" + echo "$OUTPUT" fi done
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/ci/30_test.sh(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
scripts/ci/30_test.sh (1)
Pattern scripts/**/*: "Confirm that the code adheres to the following:
- Script efficiency, readability, and maintainability.
- Usage of environmental variables and configuration files for flexibility.
- Integration with NX workspace utilities and commands."
🔇 Additional comments (2)
scripts/ci/30_test.sh (2)
7-7: LGTM! Simplified environment variable initialization.The simplified initialization of
DD_SERVICEimproves readability and removes unnecessary complexity.
59-62: LGTM! Clear and efficient code coverage configuration.The conditional setting of code coverage options is well-implemented.
Move linux parallelism in unit-tests to NX parallelism
Summary by CodeRabbit
Release Notes
New Features
Improvements
Changes
Chores