test(tests/systemtests): remove v2 checks #23098
Conversation
📝 WalkthroughWalkthroughThis pull request involves simplifying system tests across multiple test files by removing version-specific conditional checks. The changes primarily focus on eliminating Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yml ⛔ Files ignored due to path filters (1)
📒 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 (11)
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
🧹 Nitpick comments (4)
tests/systemtests/unordered_tx_test.go (1)
17-18: Consider creating a tracking issue for the skipped testThe TODO comment indicates that unordered transaction handling is not yet implemented in v2. To ensure this doesn't get overlooked, we should track this technical debt.
Would you like me to create a GitHub issue to track the implementation of unordered transaction handling in v2? This will help ensure the test doesn't remain permanently skipped.
tests/systemtests/snapshots_test.go (1)
30-31: Consider making store commands configurableHardcoding the command and restorable directories might make the test less maintainable. Consider making these configurable through test parameters or environment variables.
-command := "store" -restoreableDirs := []string{fmt.Sprintf("%s/data/application.db", node0Dir), fmt.Sprintf("%s/data/ss", node0Dir)} +const ( + defaultStoreCommand = "store" + defaultAppDBDir = "data/application.db" + defaultSSDir = "data/ss" +) +command := os.Getenv("STORE_COMMAND", defaultStoreCommand) +restoreableDirs := []string{ + filepath.Join(node0Dir, defaultAppDBDir), + filepath.Join(node0Dir, defaultSSDir), +}tests/systemtests/group_test.go (2)
66-68: Extract API endpoints to constantsThe API endpoint URLs are hardcoded in multiple places. Consider extracting these to constants to improve maintainability and reduce duplication.
+const ( + groupPoliciesEndpoint = "/cosmos/group/v1/group_policies_by_group/%s" + voteByProposalEndpoint = "/cosmos/group/v1/vote_by_proposal_voter/%s/%s" +) -groupPoliciesResp := string(systest.GetRequest(t, fmt.Sprintf("%s/cosmos/group/v1/group_policies_by_group/%s", baseurl, groupId))) +groupPoliciesResp := string(systest.GetRequest(t, fmt.Sprintf("%s%s", baseurl, fmt.Sprintf(groupPoliciesEndpoint, groupId))))
119-120: Use the extracted API endpoint constant here as wellThis endpoint construction duplicates the URL pattern. Use the constant defined above to maintain consistency.
-voteResp := string(systest.GetRequest(t, fmt.Sprintf("%s/cosmos/group/v1/vote_by_proposal_voter/%s/%s", baseurl, proposalId, valAddr))) +voteResp := string(systest.GetRequest(t, fmt.Sprintf("%s%s", baseurl, fmt.Sprintf(voteByProposalEndpoint, proposalId, valAddr))))
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tests/systemtests/go.sumis excluded by!**/*.sum
📒 Files selected for processing (12)
systemtests/system.go(1 hunks)systemtests/test_runner.go(1 hunks)systemtests/testnet_init.go(2 hunks)tests/systemtests/README.md(1 hunks)tests/systemtests/bankv2_test.go(0 hunks)tests/systemtests/cometbft_client_test.go(1 hunks)tests/systemtests/go.mod(2 hunks)tests/systemtests/group_test.go(2 hunks)tests/systemtests/mint_test.go(0 hunks)tests/systemtests/snapshots_test.go(3 hunks)tests/systemtests/staking_test.go(1 hunks)tests/systemtests/unordered_tx_test.go(1 hunks)
💤 Files with no reviewable changes (2)
- tests/systemtests/bankv2_test.go
- tests/systemtests/mint_test.go
🧰 Additional context used
📓 Path-based instructions (10)
systemtests/test_runner.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
tests/systemtests/README.md (2)
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
systemtests/system.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
tests/systemtests/staking_test.go (3)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
systemtests/testnet_init.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
tests/systemtests/unordered_tx_test.go (3)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
tests/systemtests/snapshots_test.go (3)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
tests/systemtests/cometbft_client_test.go (3)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
tests/systemtests/group_test.go (3)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
tests/systemtests/go.mod (1)
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: tests (00)
- GitHub Check: test-system-v2
- GitHub Check: Analyze
- GitHub Check: Summary
🔇 Additional comments (8)
systemtests/test_runner.go (1)
26-26: Verify binary availability in CI/CD pipelineThe default binary has been changed from "simd" to "simdv2". Ensure that this binary is consistently available in all environments where tests are run.
✅ Verification successful
Binary change is properly supported ✅
The change from "simd" to "simdv2" as the default binary is well-supported by the codebase:
- Binary is built via tests/systemtests/Makefile and scripts/build/build.mk
- Complete implementation exists in simapp/v2/simdv2/
- Multiple test files already reference this binary
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if simdv2 binary exists in common build locations echo "Checking for simdv2 binary..." for dir in "./build" "./" "../build" "../../build"; do if [ -f "${dir}/simdv2" ]; then echo "Found simdv2 in ${dir}" exit 0 fi done echo "simdv2 binary not found in common locations" exit 1Length of output: 498
Script:
#!/bin/bash # Check for Makefile and build scripts echo "=== Checking build configurations ===" fd Makefile fd "build.*\.sh" echo -e "\n=== Checking for simdv2 references in build files ===" rg "simdv2" -l --type make --type sh echo -e "\n=== Checking for simdv2 references in test files ===" rg "simdv2" --type go -lLength of output: 1309
tests/systemtests/cometbft_client_test.go (1)
30-30: Clean and consistent query implementation!The direct query without version checks simplifies the code while maintaining functionality.
tests/systemtests/README.md (1)
28-28: Documentation accurately reflects binary name change!The update to use
simdv2keeps the documentation in sync with the codebase changes.systemtests/testnet_init.go (2)
74-74: LGTM! Simplified command-line argument handling.The removal of version-specific branching logic and consistent application of the minimum gas prices flag aligns with the PR objective.
120-120: LGTM! Consistent with previous changes.The modification maintains consistency with SingleHostTestnetCmdInitializer and successfully removes version-specific logic.
tests/systemtests/go.mod (2)
175-175: LGTM! Appropriate module replacement.The replace directive correctly points to the local systemtests directory, which is essential for local development and testing.
6-6: Verify compatibility with math v1.5.0.Please ensure that the upgrade from v1.4.0 to v1.5.0 doesn't introduce any breaking changes.
✅ Verification successful
Upgrade to math v1.5.0 appears safe to proceed
The changelog shows the upgrade primarily adds a new GDA-based decimal type while maintaining backward compatibility with existing functionality. No breaking changes are documented between v1.4.0 and v1.5.0.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for breaking changes in math package gh api repos/cosmos/cosmos-sdk/releases/tags/math/v1.5.0 --jq '.body' | grep -i "breaking changes"Length of output: 123
Script:
#!/bin/bash # Try to find the correct repository and check for release information echo "Checking repository structure:" fd -t f "CHANGELOG|BREAKING_CHANGES|RELEASES" -i echo -e "\nChecking go.mod for repository information:" rg "cosmossdk.io/math" -A 2 -B 2 echo -e "\nTrying to find the actual repository:" go mod download -json cosmossdk.io/math@v1.5.0 2>/dev/null | jq .Length of output: 66101
Script:
#!/bin/bash # Check the math package's changelog echo "=== Checking math package changelog ===" if [ -f "math/CHANGELOG.md" ]; then cat math/CHANGELOG.md fi echo -e "\n=== Checking for version tags ===" git tag | grep "math/v1." | sort -VLength of output: 8457
systemtests/system.go (1)
Line range hint
736-744: LGTM! Removal of version check aligns with PR objective.The unconditional configuration of app.toml for all nodes simplifies the code and aligns with the PR objective of removing version-specific checks. The configuration correctly sets up the necessary service addresses for each node.
Let's verify that there are no remaining version-specific configurations in the test files:
✅ Verification successful
Version check removal verified, no remaining conditional version checks found
The removal of the version check in app.toml configuration is safe. The remaining version-related code in the codebase serves different purposes (API imports, CLI commands, binary naming) and doesn't contain conditional logic that would affect the system's behavior based on versions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining version-specific configurations or checks # Test: Search for version-related conditionals. Expect: No remaining version checks. # Look for any remaining version checks in test files rg -g '*.go' -i '(isv2|v1|v2)\s*\(\s*\)' tests/systemtests/Length of output: 61
Script:
#!/bin/bash # Search for version-related patterns in the correct directory echo "=== Searching for version checks ===" rg -g '*.go' -i '(v1|v2|version)' systemtests/ echo -e "\n=== Searching for version-related constants and types ===" ast-grep --pattern 'const $_Version$_ = $_' ast-grep --pattern 'var $_Version$_ = $_' echo -e "\n=== Checking config-related version patterns ===" rg -g '*.go' -i '(isv[0-9]|version.*check|check.*version)' .Length of output: 5089
| rsp = cli.CustomQuery("q", "comet", "block-results", gjson.Get(rsp, "height").String()) | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
Remove unnecessary "q" prefix from query command
The "q" prefix in cli.CustomQuery("q", "comet", "block-results", ...) is inconsistent with both the official command pattern and other test files. Remove the prefix to match the standard format seen in cometbft_client_test.go.
🔗 Analysis chain
Verify query path consistency.
While removing version checks simplifies the code, the query path comet block-results differs from the pattern seen in other files (e.g., comet status in cometbft_client_test.go). Please verify if this should be standardized.
Run this script to check query path consistency across the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistency in comet query paths
# Test: Search for all comet query patterns to ensure consistency
echo "Searching for comet query patterns:"
rg -A 1 'CustomQuery\("q",\s*"comet"'
Length of output: 278
Script:
#!/bin/bash
# Search for all query patterns related to comet/cometbft
echo "Searching for all comet/cometbft related queries:"
rg -i "custom.*query.*comet|comet.*status|block.*results" -A 2
# Also search for specific file mentioned in the comment
echo -e "\nChecking cometbft_client_test.go specifically:"
fd "cometbft_client_test.go" --exec cat {}
Length of output: 29481
|
|
julienrbrt
changed the title
julienrbrt
changed the title
Description
This pr removes v2 checks in system tests as there is no more v1 in the repo
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
Documentation
Tests
Dependencies
go.modwith new dependency versions.cosmossdk.io/mathto version 1.5.0.The changes primarily focus on streamlining test infrastructure and dependency management.