refactor(api!): replace legacy event asserts with current one #6070
Conversation
akaladarshi
requested review from
colin-axner,
AdityaSripal,
damiannolan,
srdtrk,
chatton,
crodriguezvega,
charleenfei and
DimitrisJim
as code owners
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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.EventsandToABCIEventsmethod 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 theAssertEventsfunction is robust enough to handle the comparison effectively, especially considering the transition fromsdk.Eventtoabci.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
EventsMaptype and theAssertEventsLegacyfunction, 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
abcipackage to useabci.Eventin tests is necessary for the refactor. This aligns with the PR's objective to standardize event assertion methods by usingabci.Eventfor event creation. Good job on keeping the imports organized.- 35-50: Refactoring the test cases to use
abci.Eventinstead ofsdk.Eventfor event creation and usingToABCIEventsfor 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 theAssertEventsfunction is capable of accurately comparingabci.Eventinstances.modules/core/04-channel/keeper/timeout_test.go (3)
- 6-6: The import path for
abcihas 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
expEventsfunction has been modified to return[]abci.Eventinstead of a map. This change aligns with the PR's objective to standardize event assertion methods. However, ensure that all usages ofexpEventsthroughout the test suite have been updated accordingly to handle the new return type.- 337-351: The implementation of
expEventsnow utilizessdk.Eventsandsdk.NewEventfor event creation, converting them to[]abci.EventusingToABCIEvents(). 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
abcipackage import aligns with the changes made to the event assertion methodology, as theabci.Eventtype 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.AssertEventswithin theTestAcknowledgePackettest 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
TestChannelUpgradeInitare 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
TestChannelUpgradeAckfor 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
TestChannelUpgradeCancelfor 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.
📝 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.
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 thegit.luolix.top/stretchr/testify/requireimport 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 inTestMsgCreateClientEventsusingrequire.Trueis 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 inTestMsgUpdateClientEventsusingrequire.Truefollows best practices, enhancing the test's reliability by ensuring it fails early if the type assertion fails.
There was a problem hiding this comment.
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. |
crodriguezvega
changed the title
| @@ -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.
I added migration docs, since this removal is API breaking.
There was a problem hiding this comment.
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.Setupis 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.
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! 🙏 |
| 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.
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.
yeah, we can look into this in a followup!
Description
AssertEventsLegacywithAssertEvents.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/).godoccomments.Files changedin the GitHub PR explorer.SonarCloud Reportin the comment section below once CI passes.Summary by CodeRabbit