-
Notifications
You must be signed in to change notification settings - Fork 586
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
refactor(api!): replace legacy event asserts with current one #6070
refactor(api!): replace legacy event asserts with current one #6070
Conversation
WalkthroughThis set of changes primarily focuses on refactoring and cleaning up event handling in test suites across various modules. It involves updating event creation and assertion methods to streamline testing and improve code clarity. Significant changes include the replacement of legacy event assertion functions with a unified approach and the adoption of a more consistent event handling mechanism across different testing scenarios. Changes
Assessment against linked issues
Possibly related issues
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
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.
Review Status
Actionable comments generated: 1
Configuration used: .coderabbit.yml
Files selected for processing (6)
- modules/apps/callbacks/types/events_test.go (8 hunks)
- modules/apps/transfer/keeper/msg_server_test.go (1 hunks)
- modules/core/04-channel/keeper/packet_test.go (4 hunks)
- modules/core/04-channel/keeper/timeout_test.go (4 hunks)
- modules/core/keeper/msg_server_test.go (19 hunks)
- testing/events.go (2 hunks)
Files not summarized due to errors (1)
- modules/core/keeper/msg_server_test.go: Error: Message exceeds token limit
Path instructions used (6)
modules/apps/transfer/keeper/msg_server_test.go (2)
**/*.go
Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
**/*_test.go
Assess the unit test code assessing sufficient code coverage for the changes associated in the pull requesttesting/events.go (1)
**/*.go
Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.modules/apps/callbacks/types/events_test.go (2)
**/*.go
Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
**/*_test.go
Assess the unit test code assessing sufficient code coverage for the changes associated in the pull requestmodules/core/04-channel/keeper/timeout_test.go (2)
**/*.go
Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
**/*_test.go
Assess the unit test code assessing sufficient code coverage for the changes associated in the pull requestmodules/core/04-channel/keeper/packet_test.go (2)
**/*.go
Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
**/*_test.go
Assess the unit test code assessing sufficient code coverage for the changes associated in the pull requestmodules/core/keeper/msg_server_test.go (2)
**/*.go
Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
**/*_test.go
Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request
Additional comments (15)
modules/apps/transfer/keeper/msg_server_test.go (1)
- 109-125: The updated event verification logic using
sdk.Events
andToABCIEvents
method for expected and actual events comparison is a good practice. It aligns with the PR's objective to standardize event assertion methods. However, ensure that theAssertEvents
function is robust enough to handle the comparison effectively, especially considering the transition fromsdk.Event
toabci.Event
.testing/events.go (1)
- 14-19: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
Removing the
EventsMap
type and theAssertEventsLegacy
function, as mentioned in the AI-generated summary, is a positive step towards simplifying the event handling logic. This change should make the event assertion more straightforward and maintainable. Ensure that all references to these removed entities are also updated across the test suite to avoid any broken tests.modules/apps/callbacks/types/events_test.go (2)
- 4-4: Adding the import of the
abci
package to useabci.Event
in tests is necessary for the refactor. This aligns with the PR's objective to standardize event assertion methods by usingabci.Event
for event creation. Good job on keeping the imports organized.- 35-50: Refactoring the test cases to use
abci.Event
instead ofsdk.Event
for event creation and usingToABCIEvents
for expected events is a significant improvement. This change ensures consistency with the new event assertion methodology. However, ensure that all tests are updated accordingly and that theAssertEvents
function is capable of accurately comparingabci.Event
instances.modules/core/04-channel/keeper/timeout_test.go (3)
- 6-6: The import path for
abci
has been changed to"github.com/cometbft/cometbft/abci/types"
. Ensure that this new import path is correct and that the CometBFT dependency is intended to replace the previous ABCI import. This change should be verified for compatibility with the rest of the IBC-go project.- 251-251: The signature of the
expEvents
function has been modified to return[]abci.Event
instead of a map. This change aligns with the PR's objective to standardize event assertion methods. However, ensure that all usages ofexpEvents
throughout the test suite have been updated accordingly to handle the new return type.- 337-351: The implementation of
expEvents
now utilizessdk.Events
andsdk.NewEvent
for event creation, converting them to[]abci.Event
usingToABCIEvents()
. This change is part of the refactor to standardize event assertion methods. Ensure that the events created here accurately reflect the expected events for the test cases, particularly in the context of channel flushing completion.modules/core/04-channel/keeper/packet_test.go (2)
- 5-5: The addition of the
abci
package import aligns with the changes made to the event assertion methodology, as theabci.Event
type is now used for event creation and comparison. This change is consistent with the PR's objective to standardize event assertion methods.- 1326-1326: The call to
ibctesting.AssertEvents
within theTestAcknowledgePacket
test case is a good practice for verifying that the expected events are emitted during the test execution. This approach enhances the test's robustness by ensuring that not only the main functionality is correct but also the side effects (events) are as expected. It's a crucial part of testing in event-driven systems like blockchain applications.modules/core/keeper/msg_server_test.go (6)
- 939-955: The event assertions in the test case "success" for
TestChannelUpgradeInit
are well-structured and check for the correct attributes. However, it's important to ensure that the event types and attributes are consistent with the expected behavior of the channel upgrade initialization process.- 1085-1101: In
TestChannelUpgradeTry
, the event assertions are correctly checking for the expected attributes following a successful channel upgrade try. This is crucial for ensuring that the channel upgrade process is correctly emitting events that reflect the state changes.- 1274-1289: The event assertions in
TestChannelUpgradeAck
for the case "success, no pending in-flight packets" accurately reflect the expected state changes and event emissions. It's important that these checks are in place to validate the correctness of the channel upgrade acknowledgment process.- 1878-1894: In
TestChannelUpgradeOpen
, the event assertions for the "success" case correctly validate the expected events and attributes following a successful channel upgrade open. This ensures that the channel state transitions and event emissions are as expected.- 2073-2097: The event assertions in
TestChannelUpgradeCancel
for the case "success: keeper is not authority, valid error receipt so channel changed to match error receipt seq" are comprehensive and ensure that the channel upgrade cancellation process emits the correct events. This is crucial for verifying that the channel state is correctly reverted in response to the cancellation.- 2459-2487: In
TestChannelUpgradeTimeout
, the event assertions for the "success" case are thorough and validate the expected events following a successful channel upgrade timeout. This is important for ensuring that the channel upgrade timeout process correctly handles the timeout condition and emits the appropriate events.
suite.Require().True(found) | ||
suite.Require().Equal(uint64(1), nextSequenceAck, "sequence incremented for UNORDERED channel") | ||
}, | ||
expEvents: func(path *ibctesting.Path) map[string]map[string]string { | ||
return ibctesting.EventsMap{ | ||
types.EventTypeChannelFlushComplete: { | ||
types.AttributeKeyPortID: path.EndpointA.ChannelConfig.PortID, | ||
types.AttributeKeyChannelID: path.EndpointA.ChannelID, | ||
types.AttributeCounterpartyPortID: path.EndpointB.ChannelConfig.PortID, | ||
types.AttributeCounterpartyChannelID: path.EndpointB.ChannelID, | ||
types.AttributeKeyChannelState: path.EndpointA.GetChannel().State.String(), | ||
}, | ||
sdk.EventTypeMessage: { | ||
sdk.AttributeKeyModule: types.AttributeValueCategory, | ||
}, | ||
} | ||
expEvents: func(path *ibctesting.Path) []abci.Event { | ||
return sdk.Events{ | ||
sdk.NewEvent( | ||
types.EventTypeChannelFlushComplete, | ||
sdk.NewAttribute(types.AttributeKeyPortID, path.EndpointA.ChannelConfig.PortID), | ||
sdk.NewAttribute(types.AttributeKeyChannelID, path.EndpointA.ChannelID), | ||
sdk.NewAttribute(types.AttributeCounterpartyPortID, path.EndpointB.ChannelConfig.PortID), | ||
sdk.NewAttribute(types.AttributeCounterpartyChannelID, path.EndpointB.ChannelID), | ||
sdk.NewAttribute(types.AttributeKeyChannelState, path.EndpointA.GetChannel().State.String()), | ||
), | ||
sdk.NewEvent( | ||
sdk.EventTypeMessage, | ||
sdk.NewAttribute(sdk.AttributeKeyModule, types.AttributeValueCategory), | ||
), | ||
}.ToABCIEvents() | ||
}, | ||
}, | ||
{ |
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.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [851-972]
The expEvents
function has been modified to return []abci.Event
instead of the previous map[string]map[string]string
type. This change is part of the refactor to use abci.Event
for event creation and comparison, which is a more direct and type-safe approach to handling events in tests. The conversion of sdk.Events
to abci.Events
using .ToABCIEvents()
at the end of the function is a necessary step to match the expected return type.
However, there's a minor issue with the usage of sdk.Events
within the expEvents
function. The sdk
package is not explicitly imported in the provided code snippet, which could lead to a compilation error if it's not already imported elsewhere in the file. It's important to ensure that all necessary packages are imported to avoid compilation issues.
+ import sdk "github.com/cosmos/cosmos-sdk/types"
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.
Actionable comments posted: 0
Review Status
Configuration used: .coderabbit.yml
Files selected for processing (5)
- modules/apps/callbacks/types/events_test.go (7 hunks)
- modules/apps/transfer/keeper/msg_server_test.go (1 hunks)
- modules/core/02-client/keeper/events_test.go (3 hunks)
- modules/core/04-channel/keeper/packet_test.go (4 hunks)
- modules/core/04-channel/keeper/timeout_test.go (4 hunks)
Files skipped from review as they are similar to previous changes (4)
- modules/apps/callbacks/types/events_test.go
- modules/apps/transfer/keeper/msg_server_test.go
- modules/core/04-channel/keeper/packet_test.go
- modules/core/04-channel/keeper/timeout_test.go
Additional Context Used
Path-based Instructions (1)
modules/core/02-client/keeper/events_test.go (2)
**/*.go
:
Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
**/*_test.go
:
Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request
Additional comments not posted (3)
modules/core/02-client/keeper/events_test.go (3)
4-4
: The addition of thegithub.com/stretchr/testify/require
import is appropriate for the introduced error checking in type assertions within the test functions.
23-24
: The addition of error checking for the type assertion inTestMsgCreateClientEvents
usingrequire.True
is a good practice, ensuring the test fails early if the type assertion is incorrect.
63-64
: The addition of error checking for the type assertion inTestMsgUpdateClientEvents
usingrequire.True
follows best practices, enhancing the test's reliability by ensuring it fails early if the type assertion fails.
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.
Actionable comments posted: 0
Review Status
Configuration used: .coderabbit.yml
Files selected for processing (2)
- modules/core/04-channel/keeper/packet_test.go (4 hunks)
- modules/core/04-channel/keeper/timeout_test.go (4 hunks)
Files skipped from review as they are similar to previous changes (2)
- modules/core/04-channel/keeper/packet_test.go
- modules/core/04-channel/keeper/timeout_test.go
While running |
hm, could you maybe post some of the errors? I can't say I've noticed it. edit: ah, can repro. I'll give it a peek and see what linter is on about again. Appears to me the local linters don't match the ones used in the workflows. New rules might have been added. |
Can use
Need to bump golangci-lint and get the errors fixed there. That's for another PR though. |
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.
Thank you, @akaladarshi. The changes look good to me!
@@ -167,36 +165,6 @@ func ParseProposalIDFromEvents(events []abci.Event) (uint64, error) { | |||
return 0, fmt.Errorf("proposalID event attribute not found") | |||
} | |||
|
|||
// AssertEventsLegacy asserts that expected events are present in the actual events. | |||
// Expected map needs to be a subset of actual events to pass. | |||
func AssertEventsLegacy( |
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.
I added migration docs, since this removal is API breaking.
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.
Actionable comments posted: 0
Actionable comments outside the diff hunks (3)
docs/docs/05-migrations/13-v8-to-v9.md (3)
Line range hint
10-10
: Consider rephrasing to improve clarity and flow: "This guide provides instructions for migrating to a new version of ibc-go."- This guide provides instructions for migrating to a new version of ibc-go. + This guide outlines the steps required to migrate to the latest version of ibc-go.
Line range hint
19-19
: The note about semantic versioning is crucial. Consider highlighting it or making it bold to ensure it catches the reader's attention.- **Note:** ibc-go supports golang semantic versioning and therefore all imports must be updated on major version releases. + **Note:** ibc-go adheres to semantic versioning. Ensure all imports are updated when migrating to a new major version.
[!TIP]
Codebase VerificationThe verification script has identified that the deprecated function
coordinator.Setup
is still being used in the filemodules/core/04-channel/keeper/upgrade_test.go
. This confirms that despite the deprecation notice, some parts of the codebase continue to use the deprecated functions, which could lead to maintenance issues in the future as these functions are planned to be removed in version 10.Given this finding, the review comment is accurate in highlighting the need to ensure that deprecated functions are not used elsewhere in the codebase to prevent future maintenance issues.
Analysis chain
Line range hint
29-42
: The documentation of API removals and deprecations is clear. However, ensure that the deprecated functions are not used elsewhere in the codebase to prevent future maintenance issues.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for usage of deprecated functions in the codebase. rg --type go "coordinator.Setup|coordinator.SetupClients|coordinator.SetupConnections|coordinator.CreateConnections|coordinator.CreateChannels"Length of output: 224
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.
Actionable comments posted: 0
Actionable comments outside the diff hunks (2)
docs/docs/05-migrations/13-v8-to-v9.md (2)
Line range hint
10-10
: Possible spelling mistake in "ibc-go". It should be "IBC-Go" to maintain consistency with official naming conventions.- ibc-go + IBC-Go
Line range hint
29-30
: Ensure there is a space after each period in the listed removals under the "API removals" section to improve readability.- The `exported.ChannelI` and `exported.CounterpartyChannelI` interfaces have been removed. Please use the concrete types.The `exported.ConnectionI` and `exported.CounterpartyConnectionI` interfaces have been removed. Please use the concrete types. + The `exported.ChannelI` and `exported.CounterpartyChannelI` interfaces have been removed. Please use the concrete types. The `exported.ConnectionI` and `exported.CounterpartyConnectionI` interfaces have been removed. Please use the concrete types.
Hey @crodriguezvega. |
Nope! Just reviews, I'll be reviewing it today, sorry for the delays! 🙏 |
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.
This lgtm, thanks! I do wonder why we're sticking with the abci event but that's for another discussion.
suite.Require().True(hasEvent, "event: %s was not found in events", eventName) | ||
} | ||
} | ||
|
||
// AssertEvents asserts that expected events are present in the actual events. | ||
func AssertEvents( |
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.
I wonder why we use the abci.Event
here. I think this could be refactored to use sdk.Event
(which is simply an alias for the abci.Event
) and we can drop all the ToABCIEvent
calls? Might be a thing to consider for a follow-up.
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.
yeah, we can look into this in a followup!
Description
AssertEventsLegacy
withAssertEvents
.closes: #5551
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/
).godoc
comments.Files changed
in the GitHub PR explorer.SonarCloud Report
in the comment section below once CI passes.Summary by CodeRabbit