test: add ParsePacketsFromEvents #4975

tbruyelle · 2023-10-27T14:45:04Z

Description

Returns all the packets found in the events. Comes in addition with ParsePacketFromEvents (singular version), which only returns the first packet found in the events.

closes: #4287

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 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.

charleenfei

thanks @tbruyelle 🙏

testing/events.go

srdtrk · 2023-10-27T16:12:49Z

testing/events.go

@@ -120,6 +120,22 @@ func ParsePacketFromEvents(events []abci.Event) (channeltypes.Packet, error) {
 	return channeltypes.Packet{}, fmt.Errorf("acknowledgement event attribute not found")
 }

+// ParsePacketsFromEvents parses events emitted from a MsgRecvPacket and returns
+// all the packets found.
+func ParsePacketsFromEvents(events []abci.Event) ([]channeltypes.Packet, error) {


I'd actually prefer if the main logic was happening in this function and have ParsePacketFromEvents would call into this and pick the first element from the result. I'm ok either way though.

Yeah, the implementation might become easier to reason like that.

Good idea, so I added a test and then moved the logic from ParsePacketFromEvents to ParsePacketsFromEvents.

srdtrk · 2023-10-27T16:13:40Z

Thanks for this :)

crodriguezvega

Thank you, @tbruyelle.

crodriguezvega · 2023-10-29T20:36:30Z

testing/events.go

+func ParsePacketsFromEvents(events []abci.Event) ([]channeltypes.Packet, error) {
+	var packets []channeltypes.Packet
+	for i, ev := range events {
+		if ev.Type == channeltypes.EventTypeSendPacket {


I think we could also remove this if statement, because the same one is in ParsePacketFromEvents.

done with the logic moved from one function to the other.

testing/events.go

crodriguezvega · 2023-10-29T20:41:48Z

testing/events.go

@@ -120,6 +120,22 @@ func ParsePacketFromEvents(events []abci.Event) (channeltypes.Packet, error) {
 	return channeltypes.Packet{}, fmt.Errorf("acknowledgement event attribute not found")
 }

+// ParsePacketsFromEvents parses events emitted from a MsgRecvPacket and returns
+// all the packets found.
+func ParsePacketsFromEvents(events []abci.Event) ([]channeltypes.Packet, error) {


Yeah, the implementation might become easier to reason like that.

crodriguezvega · 2023-11-08T10:58:25Z

@tbruyelle if you agree with the review comments, would you be able to work on them? Otherwise, if you're busy, we are happy to take over the PR and address the comments. If you give me permissions to push to your branch, I am happy to do that.

tbruyelle · 2023-11-08T11:15:30Z

@tbruyelle if you agree with the review comments, would you be able to work on them? Otherwise, if you're busy, we are happy to take over the PR and address the comments. If you give me permissions to push to your branch, I am happy to do that.

@crodriguezvega oups indeed I was super busy the past weeks, but today is quieter, I'll address the comments, thanks for the ping!

tbruyelle · 2023-11-08T14:50:58Z

@crodriguezvega I addressed all the comments. As requested I moved the logic from one function to the other, and also added a test before that to ensure I didn't break anything during the process.

crodriguezvega

Thank you for addressing the comments and adding a test, @tbruyelle. I have left a few more comments now. :)

testing/events.go

crodriguezvega · 2023-11-12T22:08:18Z

testing/events_test.go

+)
+
+func TestParsePacketsFromEvents(t *testing.T) {
+	tests := []struct {


nit: this is the naming we use in the majority of other tests.

Suggested change

tests := []struct {

testCases := []struct {

crodriguezvega · 2023-11-12T22:10:39Z

testing/events_test.go

+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			require := require.New(t)
+			assert := assert.New(t)
+
+			allPackets, err := ibctesting.ParsePacketsFromEvents(tt.events)
+
+			if tt.expectedError != "" {
+				assert.ErrorContains(err, tt.expectedError)
+			} else {
+				require.NoError(err)
+				assert.Equal(tt.expectedPackets, allPackets)
+			}
+
+			firstPacket, err := ibctesting.ParsePacketFromEvents(tt.events)
+
+			if tt.expectedError != "" {
+				assert.ErrorContains(err, tt.expectedError)
+			} else {
+				require.NoError(err)
+				assert.Equal(tt.expectedPackets[0], firstPacket)
+			}
+		})
+	}


I think this could be written like this (assuming that tests is renamed to testCases):

for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { allPackets, err := ibctesting.ParsePacketsFromEvents(tc.events) if tc.expectedError == "" { require.NoError(t, err) require.Equal(t, tc.expectedPackets, allPackets) } else { require.ErrorContains(t, err, tc.expectedError) } firstPacket, err := ibctesting.ParsePacketFromEvents(tc.events) if tc.expectedError == "" { require.NoError(t, err) require.Equal(t, tc.expectedPackets[0], firstPacket) } else { require.ErrorContains(t, err, tc.expectedError) } }) }

So that:

We don't need to import assert.

We execute first the checks for the success case.

crodriguezvega · 2023-11-12T22:11:53Z

testing/events_test.go

+			expectedError: "ibctesting.ParsePacketsFromEvents: strconv.ParseUint: parsing \"x\": invalid syntax",
+		},
+		{
+			name: "ok: events with packets",


We usually have the success case the first on of the slice.

Suggested change

name: "ok: events with packets",

name: "success",

crodriguezvega · 2023-11-12T22:13:35Z

testing/events_test.go

+		{
+			name:          "fail: no events",
+			events:        []abci.Event{},
+			expectedError: "ibctesting.ParsePacketsFromEvents: acknowledgement event attribute not found",


Since we are using ErrorContains it could be enough to just have this (same for the other failure test cases):

Suggested change

expectedError: "ibctesting.ParsePacketsFromEvents: acknowledgement event attribute not found",

expectedError: "acknowledgement event attribute not found",

tbruyelle · 2023-11-14T12:35:59Z

@crodriguezvega comments addressed here 8e06417

rename tests to testCases, tt to tc
remove assert package usage
revert if tc.expectedError condition
rename success case to "success", move it on top
remove expectedError common prefix

codecov-commenter · 2023-11-14T12:48:44Z

Codecov Report

Merging #4975 (4974057) into main (523861a) will decrease coverage by 0.03%.
Report is 17 commits behind head on main.
The diff coverage is 20.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4975      +/-   ##
==========================================
- Coverage   79.59%   79.57%   -0.03%     
==========================================
  Files         189      189              
  Lines       13256    13261       +5     
==========================================
+ Hits        10551    10552       +1     
- Misses       2284     2288       +4     
  Partials      421      421

Files	Coverage Δ
modules/core/02-client/keeper/keeper.go	`82.31% <100.00%> (+0.06%)`	⬆️
modules/core/02-client/types/params.go	`100.00% <ø> (ø)`
modules/light-clients/06-solomachine/module.go	`45.00% <0.00%> (-5.00%)`	⬇️
modules/light-clients/07-tendermint/module.go	`45.00% <0.00%> (-5.00%)`	⬇️

crodriguezvega

Nice work, @tbruyelle. Thank you for addressing all the comments!

If you rebase with main, I will try to merge this before your branch gets out of sync again. 🤞

tbruyelle · 2023-11-16T08:33:28Z

Nice work, @tbruyelle. Thank you for addressing all the comments!

If you rebase with main, I will try to merge this before your branch gets out of sync again. 🤞

Thank you Carlos, I've just rebased on main!

chatton · 2023-11-16T10:24:31Z

ah, those grandpa failures can be safely disregarded. This will be resolved with #5092

Closes: cosmos#4287 Returns all the packets found in the events. Comes in addition with ParsePacketFromEvents (singular version), which only returns the first packet found in the events.

Because errmod used in clienttypes.ParseHeight() returns a local path.

- rename tests to testCases, tt to tc - remove assert package usage - revert `if tc.expectedError` condition - rename success case to "success", move it on top - remove `expectedError` common prefix

tbruyelle requested review from colin-axner, AdityaSripal, crodriguezvega, charleenfei, damiannolan, chatton and DimitrisJim as code owners October 27, 2023 14:45

charleenfei approved these changes Oct 27, 2023

View reviewed changes

srdtrk reviewed Oct 27, 2023

View reviewed changes

crodriguezvega reviewed Oct 29, 2023

View reviewed changes

crodriguezvega reviewed Nov 12, 2023

View reviewed changes

github-actions bot added the testing Testing package and unit/integration tests label Nov 14, 2023

crodriguezvega approved these changes Nov 16, 2023

View reviewed changes

tbruyelle force-pushed the tbruyelle/test/add-ParsePacketsFromEvents branch from 8e06417 to bc9bc8a Compare November 16, 2023 08:32

tbruyelle added 5 commits November 16, 2023 21:58

test: add ParsePacketsFromEvents

287fd67

Closes: cosmos#4287 Returns all the packets found in the events. Comes in addition with ParsePacketFromEvents (singular version), which only returns the first packet found in the events.

add TestParsePacketsFromEvents

84112ea

refactor: move logic to ParsePacketsFromEvents

5a5b64d

replace EqualError with ErrorContains

2dff975

Because errmod used in clienttypes.ParseHeight() returns a local path.

review updates

4974057

- rename tests to testCases, tt to tc - remove assert package usage - revert `if tc.expectedError` condition - rename success case to "success", move it on top - remove `expectedError` common prefix

tbruyelle force-pushed the tbruyelle/test/add-ParsePacketsFromEvents branch from bc9bc8a to 4974057 Compare November 16, 2023 20:58

crodriguezvega merged commit 1f2a9b5 into cosmos:main Nov 17, 2023
56 checks passed

hoangdv2429 pushed a commit to hoangdv2429/ibc-go that referenced this pull request Dec 9, 2023

test: add ParsePacketsFromEvents (cosmos#4975)

2ab5b97

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: add ParsePacketsFromEvents #4975

test: add ParsePacketsFromEvents #4975

tbruyelle commented Oct 27, 2023

charleenfei left a comment

srdtrk Oct 27, 2023

crodriguezvega Oct 29, 2023

tbruyelle Nov 8, 2023

srdtrk commented Oct 27, 2023

crodriguezvega left a comment

crodriguezvega Oct 29, 2023

tbruyelle Nov 8, 2023

crodriguezvega Oct 29, 2023

crodriguezvega commented Nov 8, 2023

tbruyelle commented Nov 8, 2023

tbruyelle commented Nov 8, 2023

crodriguezvega left a comment

crodriguezvega Nov 12, 2023

crodriguezvega Nov 12, 2023 •

edited

Loading

crodriguezvega Nov 12, 2023

crodriguezvega Nov 12, 2023

tbruyelle commented Nov 14, 2023

codecov-commenter commented Nov 14, 2023 •

edited

Loading

crodriguezvega left a comment

tbruyelle commented Nov 16, 2023

chatton commented Nov 16, 2023 •

edited

Loading

	expectedError: "ibctesting.ParsePacketsFromEvents: acknowledgement event attribute not found",
	expectedError: "acknowledgement event attribute not found",

test: add ParsePacketsFromEvents #4975

test: add ParsePacketsFromEvents #4975

Conversation

tbruyelle commented Oct 27, 2023

Description

Commit Message / Changelog Entry

charleenfei left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srdtrk commented Oct 27, 2023

crodriguezvega left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crodriguezvega commented Nov 8, 2023

tbruyelle commented Nov 8, 2023

tbruyelle commented Nov 8, 2023

crodriguezvega left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crodriguezvega Nov 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbruyelle commented Nov 14, 2023

codecov-commenter commented Nov 14, 2023 • edited Loading

Codecov Report

crodriguezvega left a comment

Choose a reason for hiding this comment

tbruyelle commented Nov 16, 2023

chatton commented Nov 16, 2023 • edited Loading

crodriguezvega Nov 12, 2023 •

edited

Loading

codecov-commenter commented Nov 14, 2023 •

edited

Loading

chatton commented Nov 16, 2023 •

edited

Loading