test(e2e): run solana tests when testing upgrades #3601
Conversation
📝 WalkthroughWalkthroughThis pull request updates multiple components to refine the testing process for Solana upgrades. The changes include adding an environment variable in the Makefile and updating the changelog, modifying the test execution flow in the local end-to-end test file, adding an upgrade profile in the Docker Compose configuration, and adjusting both a variable declaration and chain parameter setup in the E2E runner. These modifications conditionally control which tests are run and how chain parameters are configured during upgrades. Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as E2ERunner
participant Local as localE2ETest
participant Deployer as deployerRunner
Runner->>Local: Call localE2ETest()
Local->>Deployer: Check upgrade state (IsUpgrade)
alt Not in upgrade
Local->>Local: Append Solana & SPL tests
else In upgrade
Local->>Local: Skip appending Solana & SPL tests
end
Local->>Runner: Return test list
sequenceDiagram
participant Runner as E2ERunner
participant Setup as ensureSolanaChainParams()
participant UpgradeChk as IsRunningUpgrade
Runner->>Setup: Invoke ensureSolanaChainParams()
Setup->>UpgradeChk: Evaluate upgrade state
alt Running upgrade
Setup->>Setup: Create chainParams without ConfirmationParams
else Normal operation
Setup->>Setup: Use chainParams with ConfirmationParams
end
Setup->>Runner: Return updated chainParams
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
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 (2)
e2e/runner/setup_solana.go (1)
183-202: Added compatibility for v28 during upgrade tests.The conditional logic handles the difference in chain parameter structure for version 28, which does not support ConfirmationParams. However, there's significant code duplication here.
Consider refactoring to reduce duplication by using a common structure and only modifying the necessary fields:
chainParams := &observertypes.ChainParams{ ChainId: chainID, ConfirmationCount: 32, ZetaTokenContractAddress: constant.EVMZeroAddress, ConnectorContractAddress: constant.EVMZeroAddress, Erc20CustodyContractAddress: constant.EVMZeroAddress, GasPriceTicker: 5, WatchUtxoTicker: 0, InboundTicker: 2, OutboundTicker: 2, OutboundScheduleInterval: 2, OutboundScheduleLookahead: 5, BallotThreshold: observertypes.DefaultBallotThreshold, MinObserverDelegation: observertypes.DefaultMinObserverDelegation, IsSupported: true, GatewayAddress: r.GatewayProgram.String(), } if !r.IsRunningUpgrade() { chainParams.ConfirmationParams = &observertypes.ConfirmationParams{ SafeInboundCount: 32, SafeOutboundCount: 32, } }cmd/zetae2e/local/local.go (1)
419-428: Well-organized conditional test inclusion.The implementation conditionally adds Solana tests based on the upgrade state, ensuring newer functionality is only tested in non-upgrade scenarios. This approach maintains backwards compatibility during upgrades while enabling comprehensive testing in standard environments.
Consider enhancing the comment to explicitly explain why these specific tests are excluded during upgrades. For example:
- // Add newly added tests(v29) if not running an upgrade + // These tests verify functionality introduced in v29 that isn't supported + // when upgrading from previous versions, so they're only run in non-upgrade scenarios
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
Makefile(1 hunks)changelog.md(1 hunks)cmd/zetae2e/local/local.go(1 hunks)contrib/localnet/docker-compose.yml(1 hunks)e2e/runner/contract_upgrade_solana.go(1 hunks)e2e/runner/setup_solana.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- e2e/runner/contract_upgrade_solana.go
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.go`: Review the Go code, point out issues relative to ...
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/runner/setup_solana.gocmd/zetae2e/local/local.go
🪛 GitHub Actions: e2e
Makefile
[error] 357-357: make: *** [Makefile:357: start-upgrade-test] Error 18
🔇 Additional comments (3)
changelog.md (1)
44-44: Accurate documentation of new testing capability.This change correctly documents the new functionality of running Solana tests during upgrade testing, which aligns with the implementation changes in other files.
Makefile (1)
359-359:❓ Verification inconclusive
Environment variable configuration for Solana upgrade tests.
This addition enables Solana tests during the upgrade process by setting the appropriate E2E_ARGS environment variable. Note that the pipeline failures show an error on this line, which may need investigation.
🏁 Script executed:
#!/bin/bash # Check if the pipeline failure is due to a legitimate issue with this change grep -B 5 -A 5 "Error 18" ../contrib/localnet/scripts/upgrade-test.sh || echo "Unable to find error context"Length of output: 215
Action Required: Verify Pipeline Configuration for Solana Upgrade Tests
The
export E2E_ARGS="--test-solana"line in the Makefile correctly sets the environment variable needed to enable Solana-specific upgrade tests. However, our investigation did not locate the expected test script at../contrib/localnet/scripts/upgrade-test.sh—the grep command resulted in a "No such file or directory" message. This implies that the pipeline error referencing "Error 18" may stem from an outdated or misconfigured file path rather than a problem with the Makefile change itself.
- Action Items:
- Confirm whether the test script has been moved, renamed, or removed from the repository.
- Review and update the pipeline configuration if necessary to ensure it is referencing the correct file path.
- If the file should exist, investigate why it’s missing from the codebase.
The code change for enabling Solana tests appears valid, but please verify the pipeline configuration separately.
contrib/localnet/docker-compose.yml (1)
225-226: Added Solana service to upgrade profile.The addition of the 'upgrade' profile to the Solana service is necessary to ensure the container is started during upgrade tests. This change correctly enables the infrastructure required for Solana testing in upgrade scenarios.
Description
Closes #3598
The pr also adds some checks to successfully run runner version of solana tests on v28
How Has This Been Tested?
Summary by CodeRabbit
New Features
Tests