-
Notifications
You must be signed in to change notification settings - Fork 29
chore: make cosmos e2e run in parallel #606
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR enables parallel execution of Cosmos e2e tests to reduce the overall test execution time. The changes address race conditions caused by global address prefix configuration in the Cosmos SDK and implement dynamic port allocation to prevent conflicts between parallel test instances.
- Introduces thread-safe address prefix management using a mutex
- Replaces hardcoded ports with dynamic port allocation
- Adds
t.Parallel()calls to enable parallel test execution
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| bsn/cosmos/e2e/bcd_test_manager.go | Adds mutex for thread-safe address prefix configuration and helper functions |
| bsn/cosmos/e2e/bcd_handler.go | Implements dynamic port allocation for RPC, P2P, and gRPC connections |
| bsn/cosmos/e2e/bcd_consumer_e2e_test.go | Enables parallel test execution and uses thread-safe prefix functions |
| "encoding/base64" | ||
| "encoding/json" | ||
| "fmt" | ||
| bbnappparams "github.com/babylonlabs-io/babylon-sdk/demo/app/params" |
Copilot
AI
Aug 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The import for bbnappparams is moved to the top but is only used in one function (setBbncAppPrefixesSafely). Consider moving this import closer to where it's used or adding a comment explaining why it needs to be at the top level.
| w.contractAddress = contractAddr | ||
| } | ||
|
|
||
| w.relayerAPIPort = testutil.AllocateUniquePort(t) |
Copilot
AI
Aug 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The relayerAPIPort is allocated in StartRelayer but the other ports (rpcPort, p2pPort, grpcPort) are allocated in NewBcdNodeHandler. For consistency, consider allocating all ports in the same location or adding a comment explaining why this one is different.
| t.Parallel() | ||
| setBbnAddressPrefixesSafely() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
Cosmos e2e tests take way too long to run sequentially, now makes them parallel closes #599
Cosmos e2e tests take way too long to run sequentially, now makes them parallel
closes #599