ci: split system tests in two job #22080
Conversation
julienrbrt
added
the
backport/v0.52.x
📝 WalkthroughWalkthroughThe changes in this pull request involve modifications to the GitHub Actions workflows for testing within the Cosmos SDK. The 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: 1
🧹 Outside diff range and nitpick comments (3)
.github/workflows/v2-test.yml (3)
127-137: LGTM: Comprehensive diff check, with a minor suggestionThe get-diff-action step is well-configured to detect changes in relevant files across the repository. This is crucial for optimizing the workflow by running subsequent steps only when necessary.
Consider adding
**/*.prototo the PATTERNS if your project uses Protocol Buffers, as changes in these files might also require running the system tests.
138-141: LGTM: Conditional musl installation looks goodThe step to install the musl library is correctly conditioned on detected changes, which is an efficient approach. This is necessary for building the simd binary in a Docker environment.
Consider caching the musl installation to speed up future workflow runs. You can use GitHub Actions' cache action for this purpose.
146-151: LGTM: Artifact upload on failure is a good practiceThe artifact upload step is well-configured to run only on failure, which is excellent for debugging purposes. The 3-day retention period for the testnet setup files is reasonable for temporary debug artifacts.
Consider adding a step to notify the team (e.g., via Slack or email) when this artifact is uploaded, as it indicates a test failure that requires attention.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (2)
- .github/workflows/test.yml (1 hunks)
- .github/workflows/v2-test.yml (1 hunks)
🔇 Additional comments (3)
.github/workflows/v2-test.yml (2)
113-126: LGTM: Job definition and setup steps are well-configuredThe new
test-system-v2job is properly defined with appropriate setup steps. Fetching tags and setting up Go with caching for bothsimapp/v2andsystemtestare good practices that should improve the workflow's efficiency.
113-151: Summary: Changes align well with PR objectivesThe introduction of the
test-system-v2job successfully achieves the PR's objective of splitting system tests into a separate job. This new job is well-structured with appropriate steps for environment setup, test execution, and artifact management. The conditional execution based on detected changes helps optimize the workflow's efficiency.These changes should improve the organization and potentially the speed of the CI pipeline, especially for PRs that don't affect system test-related files.
To ensure this change doesn't negatively impact other parts of the workflow, please verify that:
- The new job is correctly integrated into any workflow dependencies.
- The overall workflow completion time is not significantly increased.
You may want to run this workflow a few times on test branches to gather performance metrics.
.github/workflows/test.yml (1)
Line range hint
147-172: Verify the removal of v2 system tests from this job.The changes to the
test-systemjob appear to remove the v2 system tests from this workflow. A comment has been added indicating that v2 system tests are now in a separate file namedv2-test.yml. This change likely aims to improve the organization and potentially the performance of the CI pipeline.However, to ensure the completeness of the testing process:
- Confirm that the
v2-test.ymlfile exists and contains the removed v2 system tests.- Verify that the
v2-test.ymlworkflow is properly configured to run as part of the CI process.- Ensure that any dependencies or setup steps required for v2 system tests are properly migrated to the new file.
To verify these points, you can run the following script:
This script will help verify the existence and basic configuration of the
v2-test.ymlfile.✅ Verification successful
v2 System Tests Successfully Separated
The
v2-test.ymlfile exists, contains system tests, and is configured to run on similar events as the main workflow. No additional dependencies related to v2 system tests were found intest.yml, indicating a successful separation.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if v2-test.yml exists if [ -f ".github/workflows/v2-test.yml" ]; then echo "v2-test.yml file exists." # Check if v2-test.yml contains system tests if grep -q "system tests" .github/workflows/v2-test.yml; then echo "v2-test.yml contains system tests." else echo "Warning: v2-test.yml does not seem to contain system tests." fi # Check if v2-test.yml is configured to run on similar events if grep -q "pull_request:" .github/workflows/v2-test.yml && \ grep -q "push:" .github/workflows/v2-test.yml && \ grep -q "branches:" .github/workflows/v2-test.yml; then echo "v2-test.yml is configured to run on similar events." else echo "Warning: v2-test.yml may not be configured to run on the same events as the main workflow." fi else echo "Error: v2-test.yml file not found in .github/workflows/ directory." fiLength of output: 498
Script:
#!/bin/bash # Extract setup steps related to v2 system tests from test.yml v2_dependencies=$(ast-grep --lang yaml --pattern $'jobs.test-system.steps { $$$ @step }' .github/workflows/test.yml | grep -i 'v2' | grep -i 'setup') echo "Fetched v2 dependencies from test.yml: $v2_dependencies" # Check if these dependencies exist in v2-test.yml if [ -f ".github/workflows/v2-test.yml" ]; then for dep in $v2_dependencies; do if grep -q "$dep" .github/workflows/v2-test.yml; then echo "Dependency '$dep' is present in v2-test.yml." else echo "Warning: Dependency '$dep' is missing in v2-test.yml." fi done else echo "Error: v2-test.yml file not found." fiLength of output: 281
| - name: system tests v2 | ||
| if: env.GIT_DIFF | ||
| run: | | ||
| COSMOS_BUILD_OPTIONS=v2 make test-system |
There was a problem hiding this comment.
💡 Codebase verification
LGTM: System tests execution step is well-configured
The system tests execution step is correctly conditioned on detected changes and uses a Makefile target, which is a good practice for managing complex test commands.
However, the purpose of the COSMOS_BUILD_OPTIONS=v2 environment variable is not documented. It is used in scripts/init-simapp-v2.sh to ensure SIMD_BIN is set. Please consider adding comments or documentation to explain its significance for future maintainers.
🔗 Analysis chain
LGTM: System tests execution step is well-configured
The system tests execution step is correctly conditioned on detected changes and uses a Makefile target, which is a good practice for managing complex test commands.
Could you please clarify the purpose of the COSMOS_BUILD_OPTIONS=v2 environment variable? It would be helpful to add a comment explaining its significance for future maintainers.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify if there's any documentation or comments about COSMOS_BUILD_OPTIONS
rg -i "COSMOS_BUILD_OPTIONS.*v2" --type md
Length of output: 44
Script:
#!/bin/bash
# Search for occurrences of COSMOS_BUILD_OPTIONS=v2 in code files excluding documentation
rg -i "COSMOS_BUILD_OPTIONS\s*=\s*v2" --type yaml --type yml --type sh --type bash --type make
Length of output: 124
Script:
#!/bin/bash
# Search for occurrences of COSMOS_BUILD_OPTIONS=v2 in code files excluding documentation
rg -i "COSMOS_BUILD_OPTIONS\s*=\s*v2" --type yaml --type sh --type bash --type make
Length of output: 114
Script:
#!/bin/bash
# Search for occurrences of COSMOS_BUILD_OPTIONS=v2 in code files excluding documentation
rg -i "COSMOS_BUILD_OPTIONS\s*=\s*v2" --type yaml --type sh --type make
Length of output: 230
(cherry picked from commit 30003f6)
Description
Closes: #XXXX
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit