test: add unit tests for channel upgrade event emission #5475

charleenfei · 2023-12-21T12:42:06Z

Description

closes: #3497

Commit Message / Changelog Entry

type: commit message

see the guidelines for commit messages. (view raw markdown for examples)

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.

Targeted PR against the correct branch (see CONTRIBUTING.md).
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards and Go style guide.
Wrote unit and integration tests.
Updated relevant documentation (docs/) or specification (x/<module>/spec/).
Added relevant godoc comments.
Provide a commit message to be used for the changelog entry in the PR description for review.
Re-reviewed Files changed in the Github PR explorer.
Review Codecov Report in the comment section below once CI passes.

…here are no packet commitments left (#3964)

* Add ChanUpgradeOpen core handler. * Tests tests tests. * Update upgrade open handler based on feedback. * Reformat testing approach. * Move counterpartyhops assignment inline. * Check err of SetChannelState. * Address feedback. * Remove uneeded modification of version.

* chore: adding check for in flight packets in WriteUpgradeAckChannel * added test for TestWriteUpgradeAckChannel * linter * add client update to UpgradeAckChannel test * mv test * merge * fix post-merge * fix merge issues * review comment --------- Co-authored-by: Charly <charly@interchain.berlin> Co-authored-by: Carlos Rodriguez <carlos@interchain.io>

…3895) * Add implementation for message server handling of ChanUpgradeOpen. * Add tests for msg_server. * Address review feedback. * Remove setting of flush status. * Apply suggestions from code review Co-authored-by: Damian Nolan <damiannolan@gmail.com> * Address rest of review comments. --------- Co-authored-by: Damian Nolan <damiannolan@gmail.com>

…fupgrade (#4016)

* chore: update abort upgrade function to panic on error * apply review suggestions --------- Co-authored-by: Carlos Rodriguez <carlos@interchain.io>

* Amend Ack packet to keep acknowledging if we're in the process of flushing pre-upgrade packets. * Use handshake to reach correct state before mutating any fields. * Add test to verify post-ack channel state after last in-flight packet. * Remove unecessary modifications of version for non initializing channel end. * Test both cases: final in-flight packet and non-final one. * Apply suggestions from code review Co-authored-by: Carlos Rodriguez <carlos@interchain.io> * Remove manual setting of flush status. * Update test name, pass mock version to both channels. --------- Co-authored-by: Carlos Rodriguez <carlos@interchain.io>

* Amend timeout to handle in-flight packets. * Update timeout handler per spec. * Update tests to test for toggling of flush status. * Fix small typos in docstring. --------- Co-authored-by: Carlos Rodriguez <carlos@interchain.io>

…4051)

…UpgradeOpen. (#4052) * Pass in counterparty portid, channelid. * use direct check on err. * Force distinct channel identifiers when testing UpgradeOpen.

…or. (#4063)

…4019) * WIP: adding fee upgrade cbs and testing * imp: allow failure expectations when using chain.SendMsgs * fixing import errors from cherry-pick * updating tests and rm try code * rm diff onChanUpgradeTry * Update modules/apps/29-fee/ibc_middleware.go * adding MetadataFromVersion func to pkg types * addressing pr feedback, disable fees on err, rename args, adding testcase * Update modules/apps/29-fee/ibc_middleware_test.go Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com> * abstract out expIsFeeEnabled check in tests * adding additional error context to MetadataFromVersion --------- Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>

…4023) * WIP: adding fee upgrade cbs and testing * imp: allow failure expectations when using chain.SendMsgs * fixing import errors from cherry-pick * updating tests and rm try code * rm diff onChanUpgradeTry * Update modules/apps/29-fee/ibc_middleware.go * adding OnChanUpgradeTry implementation for 29-fee, adding tests * rm CR in test expectation * remove goconst linter as discussed async * adding MetadataFromVersion func to pkg types * addressing pr feedback, disable fees on err, rename args, adding testcase * Update modules/apps/29-fee/ibc_middleware_test.go Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com> * abstract out expIsFeeEnabled check in tests * adding additional error context to MetadataFromVersion * propagate changes from onChanUpgradeInit PR * addressing test assertion feedback --------- Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>

…4028) * WIP: adding fee upgrade cbs and testing * imp: allow failure expectations when using chain.SendMsgs * fixing import errors from cherry-pick * updating tests and rm try code * rm diff onChanUpgradeTry * Update modules/apps/29-fee/ibc_middleware.go * adding OnChanUpgradeTry implementation for 29-fee, adding tests * rm CR in test expectation * remove goconst linter as discussed async * adding onChanUpgradeAck implementation to 29-fee, adding tests * adding MetadataFromVersion func to pkg types * addressing pr feedback, disable fees on err, rename args, adding testcase * Update modules/apps/29-fee/ibc_middleware_test.go Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com> * abstract out expIsFeeEnabled check in tests * adding additional error context to MetadataFromVersion * propagate changes from onChanUpgradeInit PR * addressing test assertion feedback * updating to use types.MetadataFromVersion in OnChanUpgradeAck * updating tests to add additional checks --------- Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>

* use counterparty conn hops. * Clean up tests. * Amend inline comment slightly. * Address nits --------- Co-authored-by: Carlos Rodriguez <carlos@interchain.io>

…gradeAck. (#4075) * Move channel to OPEN if all in-flight packets have been flushed in UpgradeAck. * Add comment noting that counterparty flush status has been verified before usage. * Fix failing tests.

#4134)

* adding transfer checks in upgrade cbs * adding tests and reduce diff by moving code out * add additional assertion on upgrade stored in state * lint: single var declaration instead of block * rename version -> upgradeVersion * Update modules/apps/transfer/ibc_module_test.go * rename sequence -> upgradeSequence * updating to use NewTranferPath in test func * address pr comment - reformat error msg

…-upgrade-event-emission

crodriguezvega

Excellent work, @charleenfei!

modules/core/04-channel/keeper/upgrade_test.go

modules/core/keeper/msg_server.go

…-upgrade-event-emission

…mplete

…ent-emission' of github.com:cosmos/ibc-go into charly/issue#3497-add-unit-tests-for-channel-upgrade-event-emission

…-upgrade-event-emission

DimitrisJim

lgtm! ✨ One event I'm noticing isn't checked is the one emitted when writting error receipt. Home for this check could be in msg_server_test and/or in core? Maybe we could move expected events in the malleate for Try (which emits it).

Fine if this is done in follow up or you wanna push it here.

DimitrisJim · 2024-01-08T13:03:44Z

modules/core/keeper/msg_server_test.go

+						sdk.AttributeKeyModule: channeltypes.AttributeValueCategory,
+					},
+				}
+				ibctesting.AssertEventsLegacy(&suite.Suite, expEvents, events)


guess this can be follow up at some point but this and AssertEvents are sorta confusing as options and their docustring aren't as helpful, it seems like we should have one way to ideally check for events in tests.

Any ideas on the difference between the two so I don't have to check the code?

It seems also odd to have to pass the Suite as an argument. An instance of testing.T could be passed, or an error return should be fine too.

In any case, I think these event testing functions has been around for a while. We can give them some attention in a follow up as you suggested! :)

the EventsLegacy function handles sdk.Events, whereas Events handles ABCI events (typed array), we can double check these later agreed

I'll add a follow up PR for error receipt events @DimitrisJim to make reviewing easier! thanks <3

AssertEvents was added when we were upgrading to SDK v0.50 to be able to assert the events in this test, if I remember correctly. Now that we are in SDK v0.50 I think we move to AssertEvents and remove AssertEventsLegacy and also the EventsMap type alias.

thanks for context @crodriguezvega! I'll open an issue to keep track of this.

…-upgrade-event-emission

damiannolan

Thanks @charleenfei!

I can push a commit to remove the upgrade found check.

modules/core/04-channel/keeper/upgrade.go

damiannolan · 2024-01-08T13:34:45Z

modules/core/keeper/msg_server_test.go

+						sdk.AttributeKeyModule: channeltypes.AttributeValueCategory,
+					},
+				}
+				ibctesting.AssertEventsLegacy(&suite.Suite, expEvents, events)


Any ideas on the difference between the two so I don't have to check the code?

It seems also odd to have to pass the Suite as an argument. An instance of testing.T could be passed, or an error return should be fine too.

In any case, I think these event testing functions has been around for a while. We can give them some attention in a follow up as you suggested! :)

modules/core/keeper/msg_server.go

(cherry picked from commit dc99ee4)

(cherry picked from commit dc99ee4) Co-authored-by: Charly <charly@interchain.io>

DimitrisJim and others added 30 commits June 28, 2023 16:05

Add a check for connection state in UpgradeAck. (#3954)

7f3b04d

update WriteUpgradeTry to change flush status to FLUSHCOMPLETE if t…

86bb6bf

…here are no packet commitments left (#3964)

Add flush status check in SendPacket (#3912)

16ed527

remove depguard from golangci-lint (#4000)

e81d2f8

Set channel state to NOTINFLUSH when Closing (#3995)

25fcad4

Set Counterparty Last Sequence Send (#4005)

5767ad7

Add FlushStatus Checks to RecvPacket (#3914)

92f9e56

helper function for validating a counterparty upgrade against the sel…

c249e1e

…fupgrade (#4016)

Make AbortUpgrade panic on failure (#4011)

05c43dc

* chore: update abort upgrade function to panic on error * apply review suggestions --------- Co-authored-by: Carlos Rodriguez <carlos@interchain.io>

Amend timeout to handle in-flight packets. (#3923)

3203f07

* Amend timeout to handle in-flight packets. * Update timeout handler per spec. * Update tests to test for toggling of flush status. * Fix small typos in docstring. --------- Co-authored-by: Carlos Rodriguez <carlos@interchain.io>

change comment

641ad00

chore: adding callback for OnChanUpgradeRestore in abort scenarios (#…

ef239b4

…4051)

Pass in counterparty portID, channelID when verifying channel in Chan…

fcd8ded

…UpgradeOpen. (#4052) * Pass in counterparty portid, channelid. * use direct check on err. * Force distinct channel identifiers when testing UpgradeOpen.

Remove manual setting of flush status in ChanUpgradeOpen. (#4064)

af76ade

Use counterparty portid, channelid when verifying channel upgrade err…

8e53c77

…or. (#4063)

Use counterparty connection hops when verifying channel state (#4074)

82ea953

* use counterparty conn hops. * Clean up tests. * Amend inline comment slightly. * Address nits --------- Co-authored-by: Carlos Rodriguez <carlos@interchain.io>

chore: adding comment to 29-fee OnChanUpgradeOpen callback (#4123)

dcc004f

Add tests for WriteUpgradeOpenChannel. (#4077)

c39b53e

Add tests for WriteUpgradeCancel. (#4076)

98db69b

Move channel to OPEN if all in-flight packets have been flushed in Up…

ac1acbb

…gradeAck. (#4075) * Move channel to OPEN if all in-flight packets have been flushed in UpgradeAck. * Add comment noting that counterparty flush status has been verified before usage. * Fix failing tests.

imp: use HasInflightPackets to check if channel has finished flushing. (

347346b

#4134)

Remove superfluous ignore directive. (#4158)

edd0cc1

charleenfei requested review from AdityaSripal and damiannolan as code owners December 22, 2023 11:22

charleenfei marked this pull request as draft December 22, 2023 11:22

charleenfei added 2 commits January 4, 2024 14:49

Merge branch 'main' into charly/issue#3497-add-unit-tests-for-channel…

1ebf300

…-upgrade-event-emission

fix tests

9825dde

charleenfei changed the base branch from 04-channel-upgrades to main January 4, 2024 21:32

Merge branch 'main' into charly/issue#3497-add-unit-tests-for-channel…

0fb1609

…-upgrade-event-emission

charleenfei marked this pull request as ready for review January 4, 2024 21:56

Merge branch 'main' into charly/issue#3497-add-unit-tests-for-channel…

2fce5a3

…-upgrade-event-emission

crodriguezvega approved these changes Jan 5, 2024

View reviewed changes

modules/core/04-channel/keeper/upgrade_test.go Show resolved Hide resolved

modules/core/keeper/msg_server.go Show resolved Hide resolved

charleenfei and others added 7 commits January 5, 2024 12:42

pr review

35ffb79

Merge branch 'main' into charly/issue#3497-add-unit-tests-for-channel…

6699526

…-upgrade-event-emission

update for case if keeper is authority and channel is not in flush co…

3b693c5

…mplete

Merge branch 'charly/issue#3497-add-unit-tests-for-channel-upgrade-ev…

f00aff0

…ent-emission' of github.com:cosmos/ibc-go into charly/issue#3497-add-unit-tests-for-channel-upgrade-event-emission

Merge branch 'main' into charly/issue#3497-add-unit-tests-for-channel…

7770dca

…-upgrade-event-emission

Merge branch 'main' into charly/issue#3497-add-unit-tests-for-channel…

276ddf8

…-upgrade-event-emission

Merge branch 'main' into charly/issue#3497-add-unit-tests-for-channel…

83bd9cb

…-upgrade-event-emission

DimitrisJim approved these changes Jan 8, 2024

View reviewed changes

damiannolan added 2 commits January 8, 2024 14:10

Merge branch 'main' into charly/issue#3497-add-unit-tests-for-channel…

9a2cd68

…-upgrade-event-emission

fix: compiler errors on merge of main

da79892

damiannolan approved these changes Jan 8, 2024

View reviewed changes

damiannolan and others added 2 commits January 8, 2024 14:40

chore: rm upgrade found check in WriteUpgradeCancelChannel

a920368

rm test checking for panic as upgrade check was rm

3cea383

charleenfei merged commit dc99ee4 into main Jan 8, 2024
61 checks passed

charleenfei deleted the charly/issue#3497-add-unit-tests-for-channel-upgrade-event-emission branch January 8, 2024 15:59

mergify bot pushed a commit that referenced this pull request Jan 8, 2024

test: add unit tests for channel upgrade event emission (#5475)

b233009

(cherry picked from commit dc99ee4)

mergify bot mentioned this pull request Jan 8, 2024

test: add unit tests for channel upgrade event emission (backport #5475) #5547

Merged

colin-axner pushed a commit that referenced this pull request Jan 8, 2024

test: add unit tests for channel upgrade event emission (#5475) (#5547)

6c5f36e

(cherry picked from commit dc99ee4) Co-authored-by: Charly <charly@interchain.io>

charleenfei mentioned this pull request Jan 8, 2024

test: event emission on writing error receipts for channel upgrades #5549

Merged

9 tasks

DimitrisJim mentioned this pull request Jan 9, 2024

Clean up event testing functions #5551

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: add unit tests for channel upgrade event emission #5475

test: add unit tests for channel upgrade event emission #5475

charleenfei commented Dec 21, 2023

crodriguezvega left a comment

DimitrisJim left a comment

DimitrisJim Jan 8, 2024

damiannolan Jan 8, 2024

charleenfei Jan 8, 2024

charleenfei Jan 8, 2024

crodriguezvega Jan 8, 2024

DimitrisJim Jan 9, 2024

damiannolan left a comment

damiannolan Jan 8, 2024

test: add unit tests for channel upgrade event emission #5475

test: add unit tests for channel upgrade event emission #5475

Conversation

charleenfei commented Dec 21, 2023

Description

Commit Message / Changelog Entry

crodriguezvega left a comment

Choose a reason for hiding this comment

DimitrisJim left a comment

Choose a reason for hiding this comment

DimitrisJim Jan 8, 2024

Choose a reason for hiding this comment

damiannolan Jan 8, 2024

Choose a reason for hiding this comment

charleenfei Jan 8, 2024

Choose a reason for hiding this comment

charleenfei Jan 8, 2024

Choose a reason for hiding this comment

crodriguezvega Jan 8, 2024

Choose a reason for hiding this comment

DimitrisJim Jan 9, 2024

Choose a reason for hiding this comment

damiannolan left a comment

Choose a reason for hiding this comment

damiannolan Jan 8, 2024

Choose a reason for hiding this comment