test(integration): port x/evidence tests to server v2 #22709
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request involves significant changes to the testing framework and application configuration within a Cosmos SDK-based application. Key modifications include the removal of 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
|
akhilkumarpilli
changed the title
![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 (2)
tests/integration/v2/evidence/infraction_test.go (2)
85-85: Consider usingtesting.TBinstead of*testing.TChanging the parameter from
tb testing.TBtot *testing.TininitFixturereduces flexibility, astesting.TBallows both*testing.Tand*testing.B. Unless specific methods of*testing.Tare required, consider retainingtesting.TBfor broader compatibility.Apply this diff to revert to
testing.TB:-func initFixture(t *testing.T) *fixture { +func initFixture(tb testing.TB) *fixture { tb.Helper() // ... }
398-401: Ensure consistent use of context variablesIn the
populateValidatorsfunction, bothf.ctxandctxare used for context. This might lead to confusion or unintended behavior. Consider using a single context variable consistently throughout the function.Apply this diff to use
ctxconsistently:-func populateValidators(t assert.TestingT, f *fixture) { +func populateValidators(t assert.TestingT, f *fixture, ctx context.Context) { // ... - assert.NilError(t, f.bankKeeper.MintCoins(f.ctx, minttypes.ModuleName, totalSupply)) + assert.NilError(t, f.bankKeeper.MintCoins(ctx, minttypes.ModuleName, totalSupply)) for _, addr := range valAddresses { - assert.NilError(t, f.bankKeeper.SendCoinsFromModuleToAccount(f.ctx, minttypes.ModuleName, sdk.AccAddress(addr), initCoins)) + assert.NilError(t, f.bankKeeper.SendCoinsFromModuleToAccount(ctx, minttypes.ModuleName, sdk.AccAddress(addr), initCoins)) } }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (6)
tests/integration/evidence/app_config.go(0 hunks)tests/integration/v2/app.go(1 hunks)tests/integration/v2/distribution/module_test.go(1 hunks)tests/integration/v2/evidence/genesis_test.go(1 hunks)tests/integration/v2/evidence/infraction_test.go(15 hunks)tests/integration/v2/services.go(2 hunks)
💤 Files with no reviewable changes (1)
- tests/integration/evidence/app_config.go
✅ Files skipped from review due to trivial changes (2)
- tests/integration/v2/distribution/module_test.go
- tests/integration/v2/app.go
🧰 Additional context used
📓 Path-based instructions (3)
tests/integration/v2/services.go (2)
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"
tests/integration/v2/evidence/genesis_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/integration/v2/evidence/infraction_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"
🔇 Additional comments (20)
tests/integration/v2/evidence/infraction_test.go (11)
1-1: Package name change is appropriate
Changing the package name from keeper_test to evidence accurately reflects the focus of the tests on the evidence module and improves clarity.
68-68: Initialization of cometInfoService is correct
The initialization of cometInfoService with &services.ContextAwareCometInfoService{} is appropriate and aligns with the updates for context management.
74-82: Struct fixture fields are correctly updated
Adding the consensusKeeper field and updating other keeper references in the fixture struct enhances the setup for the tests and ensures that all necessary keepers are accessible.
123-123: Use of ctx from fixture is consistent
Assigning ctx := f.ctx ensures that the test uses the consistent context from the fixture, maintaining proper context for the test execution.
149-151: Correct setup of validator signing info
Initializing the validator signing info with the correct height and time ensures accurate tracking of the validator's signing behavior.
170-171: Properly setting Comet info in context
Using integration.SetCometInfo to set the Comet info in the context before calling BeginBlocker is appropriate and ensures that evidence processing has the necessary information.
192-192: Updating context with new header information
Adjusting the context with integration.SetHeaderInfo to simulate the passage of time aligns with test requirements for unbonding period checks.
221-221: Initializing context with specific height and time
Setting the header info at the beginning of the test provides a consistent starting point for the test scenario.
251-253: Ensuring consensus parameters are set
Setting the consensus parameters ensures that the evidence age calculations are based on the correct configuration, which is vital for the test's validity.
273-273: Setting initial header info in context
Initializing the context with the current time is important for accurately testing validator behavior after rotation.
336-338: Creating a new context with Comet info
Assigning ctxWithCometInfo by setting the Comet info into the context is appropriate for isolating the test scenario involving evidence handling.
tests/integration/v2/evidence/genesis_test.go (5)
1-1: Package name change is appropriate
Changing the package name from evidence_test to evidence conforms to Go testing conventions and enhances the consistency of the test package structure.
22-22: Updating ctx to use context.Context
Changing the ctx field from sdk.Context to context.Context aligns with the updated context usage throughout the codebase and ensures compatibility with functions expecting context.Context.
27-30: Refactored SetupTest improves test initialization
Using initFixture in SetupTest simplifies the test setup process and ensures consistent initialization across tests.
29-30: Assigning context and keeper from fixture
Setting suite.ctx and suite.keeper from the fixture provides the necessary components for the tests, maintaining modularity and reusability.
Line range hint 58-88: Test cases are correctly structured
The test cases in TestInitGenesis and TestExportGenesis are well-defined, and the use of malleate and posttests functions enhances test clarity and reusability.
tests/integration/v2/services.go (4)
85-91: Retrieve header info without assuming mutable context
In HeaderInfoFromContext, accessing the header information without modifying the context is appropriate. Ensure that the function handles cases where the integration context may not be present.
93-95: Properly setting Comet info in context
Using context.WithValue in SetCometInfo to associate Comet info with the context is acceptable and aligns with Go's context usage guidelines.
Line range hint 119-136: Implementation of BranchService is sound
The custom BranchService correctly implements the branch.Service interface, and the methods handle context properly without unintended side effects.
Line range hint 168-175: RouterService correctly manages handlers
The RouterService provides a clear mechanism to register and invoke message handlers based on type URLs, enhancing modularity and extensibility.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
tests/integration/v2/services.go (1)
86-92: Add documentation for HeaderInfoFromContextConsider adding a doc comment explaining the function's behavior, especially the empty return case when context type doesn't match.
Add this documentation:
+// HeaderInfoFromContext retrieves header information from the integration context. +// Returns an empty header.Info if the context doesn't contain integration context. func HeaderInfoFromContext(ctx context.Context) header.Info {
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (1)
tests/integration/v2/services.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
tests/integration/v2/services.go (2)
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"
🔇 Additional comments (3)
tests/integration/v2/services.go (3)
12-12: LGTM!
The import is correctly aliased and follows Go conventions.
94-96: LGTM!
The implementation correctly follows context best practices by using a predefined key from the core package.
77-84:
Critical: Context values should be immutable
The current implementation modifies the context value directly, which violates Go's context immutability principle and could lead to race conditions.
Apply this diff to fix the issue:
func SetHeaderInfo(ctx context.Context, h header.Info) context.Context {
iCtx, ok := ctx.Value(contextKey).(*integrationContext)
if !ok {
return ctx
}
- iCtx.header = h
- return context.WithValue(ctx, contextKey, iCtx)
+ return context.WithValue(ctx, contextKey, &integrationContext{
+ state: iCtx.state,
+ gasMeter: iCtx.gasMeter,
+ header: h,
+ })
}
Description
Closes: #22668
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
Chores
app_config.gofile, simplifying the application configuration.Documentation
app.gofile.Refactor
genesis_test.gofile for improved clarity and maintainability.infraction_test.gotoevidence_test.go, enhancing context management.services.goto improve robustness.