Refactor 04-channel/keeper tests to ibc testing pkg #6400

colin-axner · 2020-06-11T01:33:00Z

Description

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.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer

…-remove-dup-channel

codecov · 2020-06-11T01:37:14Z

Codecov Report

Merging #6400 into master will increase coverage by 0.06%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #6400      +/-   ##
==========================================
+ Coverage   56.68%   56.74%   +0.06%     
==========================================
  Files         478      478              
  Lines       28777    28820      +43     
==========================================
+ Hits        16313    16355      +42     
- Misses      11306    11316      +10     
+ Partials     1158     1149       -9

colin-axner · 2020-06-12T23:04:08Z

current issue here is simapp.Setup doesn't allow for a validator set to be set upon genesis. So the validator set being put into the tm header results in a self consensus state that doesnt equal the counterparty's client consensus state. The self consensus state has a nil validator set as tracked by historical info.

somehow a helper func needs to be added to simapp to allow setting a valset at genesis

colin-axner · 2020-06-12T23:29:14Z

I think the solution here is to add a SetupWithValidators func in simapp which takes in a validator set and swaps out the default genesis state in staking for a genesis state with appropriate validators/delegations. I'm not 100% sure if that will be enough or if changes need to be made elsewhere as well

…-remove-dup-channel

colin-axner · 2020-06-18T06:11:22Z

It seems like the current issue here is handling querying of proofs and submission of the correct proof height. When querying for a proof, we are returned the IAVL height of the proof, which actually corresponds to proofHeight + 1 of the tenderminet consensus. So if I want a proof at block height 6, in theory I should be able to query at IAVL height 5 and submit the proof along with the returned proofHeight + 1.

This is quite confusing and it should be explored ways to remedy this as well as document this in the IBC specs.

…-remove-dup-channel

…mos-sdk into colin/5558-remove-dup-channel

fedekunze

Awesome! thanks @colin-axner. Minor comments only

simapp/test_helpers.go

x/ibc/testing/types.go

x/ibc/testing/coordinator.go

x/ibc/testing/chain.go

x/ibc/04-channel/keeper/handshake_test.go

AdityaSripal

Great work!! Have some suggested changes

x/ibc/04-channel/keeper/handshake_test.go

AdityaSripal · 2020-06-25T22:53:48Z

x/ibc/04-channel/keeper/handshake_test.go

-			)
-			suite.Require().NoError(err, "could not create capability")
+			// run test for all types of ordering
+			for _, order := range []types.Order{types.UNORDERED, types.ORDERED} {


Nice touch!

x/ibc/04-channel/keeper/handshake_test.go

AdityaSripal · 2020-06-25T23:35:14Z

x/ibc/testing/chain.go

+	// proof height + 1 is returned as the proof created corresponds to the height the proof
+	// was created in the IAVL tree. Tendermint and subsequently the clients that rely on it
+	// have heights 1 above the IAVL tree. Thus we return proof height + 1
+	return proof, uint64(res.Height) + 1


Note for reviewers @cwgoes @fedekunze : This is an issue that me and @jackzampolin repeatedly ran into when working with the relayer. We fixed the problem by incrementing the query height before passing into proof height.

We should consider whether we want to make this a requirement for relayers to handle correctly or if we want to deal with this peculiarity on the SDK side.

I think we should open up an issue to discuss this

yeah let's create an issue 👍

AdityaSripal · 2020-06-25T23:35:44Z

x/ibc/testing/chain.go

-		Time:   chain.CurrentHeader.Time,
+		Height:  chain.App.LastBlockHeight() + 1,
+		AppHash: chain.App.LastCommitID().Hash,
+		Time:    chain.CurrentHeader.Time,


Is time not getting increased at all?

Yeah I’d add 7 seconds

it is, but in the coordinator. Increasing time here would cause updating clients to use headers from the future. I'll add a comment

x/ibc/testing/types.go

AdityaSripal · 2020-06-25T23:40:38Z

x/ibc/testing/types.go

+}
+
+// NextTestChannel returns the next test channel to be created on this connection
+func (conn *TestConnection) NextTestChannel() TestChannel {


Suggested change

func (conn *TestConnection) NextTestChannel() TestChannel {

func (conn *TestConnection) NewTestChannel() TestChannel {

Hmm, so my issue with this naming is calling NewTestChannel would result in the same test channel being returned

AdityaSripal · 2020-06-25T23:40:52Z

x/ibc/testing/types.go

+
+// FirstOrNextTestChannel returns the first test channel if it exists, otherwise it
+// returns the next test channel to be created.
+func (conn *TestConnection) FirstOrNextTestChannel() TestChannel {


Suggested change

func (conn *TestConnection) FirstOrNextTestChannel() TestChannel {

func (conn *TestConnection) GetTestChannel() TestChannel {

Other options, GetOrCreateTestChannel, FetchTestChannel

A test channel isn't created. In fact just using this func or NextTestChannel would result in referencing a channel that doesn't exist on the chain. I'll update the docs to be more clear with the intention of these funcs.

I like including First since this func is not meant to be used with the creation of multiple test channels on the same connection.

…-remove-dup-channel

Co-authored-by: Aditya <[email protected]> Co-authored-by: Federico Kunze <[email protected]>

…mos-sdk into colin/5558-remove-dup-channel

Co-authored-by: Aditya <[email protected]>

fedekunze

ACK 👍

AdityaSripal

Great work @colin-axner !

Still don't like NextTestChannel and FirstOrNextTestChannel. Maybe @fedekunze can be the tiebreaker here, but don't think it needs to block the entire PR. Approved 🎉

AdityaSripal · 2020-06-26T16:23:14Z

x/ibc/04-channel/keeper/packet_test.go

+			err := suite.coordinator.SendPacket(suite.chainA, suite.chainB, packet, clientB)
+			suite.Require().NoError(err)
+			// set sequence to 2
+			packet = types.NewPacket(validPacketData, 2, channelA.PortID, channelA.ID, channelB.PortID, channelB.ID, timeoutHeight, disabledTimeoutTimestamp)


good way to test this!

@AdityaSripal

* update testing pkg and first keeper test * fix version bug * add more helper testing funcs * move create header to testing pkg * fix connection state bug * add staking genesis state * update simapp with setting validator helper func * update simapp with valset helper * fix app hash issue * update to query from correct iavl proof height * first keeper test passing * second test passing 🎉 * fix build * update tests in all keeper_test * fix lint * begin refactoring querier test * update first querier test and update testing helpers * finish updating rest of querier tests * rename ChannelID in TestChannel to ID * remove usage of chain id for calling helper funcs * update openinit and opentry tests * finish opening channel handshake tests * finish handshake tests * general testing pkg cleanup * finish packetsend refactor * update recvpacket * packet executed refactored * finish packet test 🎉 * all tests passing * cleanup and increase code cov * remove todos in favor of opened issue #6509 * bump invalid id to meet validation requirements * bubble up proof height + 1 * Apply suggestions from code review Co-authored-by: Aditya <[email protected]> Co-authored-by: Federico Kunze <[email protected]> * fix uninit conn test * fix compile and address various pr review issues * Update x/ibc/04-channel/keeper/handshake_test.go Co-authored-by: Aditya <[email protected]> * Update x/ibc/04-channel/keeper/handshake_test.go Co-authored-by: Aditya <[email protected]> * address @AdityaSripal comments and increase cov Co-authored-by: Aditya <[email protected]> Co-authored-by: Federico Kunze <[email protected]>

colin-axner added 2 commits June 10, 2020 17:49

update testing pkg and first keeper test

48adee9

Merge branch 'master' of github.com:cosmos/cosmos-sdk into colin/5558…

b855979

…-remove-dup-channel

colin-axner added T: Tests x/ibc labels Jun 11, 2020

colin-axner added 6 commits June 11, 2020 14:48

fix version bug

4493ff7

merge master

8154210

add more helper testing funcs

59cbe4f

merge master

57c8c90

move create header to testing pkg

edcbd00

fix connection state bug

190a885

colin-axner added 7 commits June 12, 2020 17:02

add staking genesis state

5378d54

update simapp with setting validator helper func

f22d3af

merge master

8019f55

update simapp with valset helper

f6fe54f

fix app hash issue

0c49a0c

Merge branch 'master' of github.com:cosmos/cosmos-sdk into colin/5558…

e1bbac7

…-remove-dup-channel

update to query from correct iavl proof height

2f3e1d0

colin-axner added 8 commits June 19, 2020 12:31

first keeper test passing

6663892

second test passing 🎉

bc9125b

Merge branch 'master' of github.com:cosmos/cosmos-sdk into colin/5558…

0cedb42

…-remove-dup-channel

fix build

3d9a869

update tests in all keeper_test

28d3223

Merge branch 'master' of github.com:cosmos/cosmos-sdk into colin/5558…

65074fb

…-remove-dup-channel

fix lint

ee6ce43

begin refactoring querier test

53a8433

colin-axner requested review from alexanderbez, clevinson, fedekunze and jgimeno as code owners June 25, 2020 14:50

colin-axner added R4R and removed WIP labels Jun 25, 2020

colin-axner requested a review from AdityaSripal June 25, 2020 14:52

colin-axner added 3 commits June 25, 2020 17:11

bump invalid id to meet validation requirements

d1de17f

Merge branch 'colin/5558-remove-dup-channel' of github.com:cosmos/cos…

a804ce0

…mos-sdk into colin/5558-remove-dup-channel

bubble up proof height + 1

b55c2eb

fedekunze suggested changes Jun 25, 2020

View reviewed changes

AdityaSripal requested changes Jun 25, 2020

View reviewed changes

colin-axner and others added 7 commits June 26, 2020 11:04

Merge branch 'master' of github.com:cosmos/cosmos-sdk into colin/5558…

2d96bc4

…-remove-dup-channel

Apply suggestions from code review

fb89629

Co-authored-by: Aditya <[email protected]> Co-authored-by: Federico Kunze <[email protected]>

fix uninit conn test

8123337

Merge branch 'colin/5558-remove-dup-channel' of github.com:cosmos/cos…

ce1c268

…mos-sdk into colin/5558-remove-dup-channel

fix compile and address various pr review issues

cbab3ea

Update x/ibc/04-channel/keeper/handshake_test.go

42ee985

Co-authored-by: Aditya <[email protected]>

Update x/ibc/04-channel/keeper/handshake_test.go

5c8a180

Co-authored-by: Aditya <[email protected]>

fedekunze approved these changes Jun 26, 2020

View reviewed changes

colin-axner mentioned this pull request Jun 26, 2020

Duplicate nextRecvSeq check in RecvPacket #6519

Closed

4 tasks

address @AdityaSripal comments and increase cov

9e208ce

colin-axner mentioned this pull request Jun 26, 2020

Common test harness code for IBC tests #5558

Closed

5 tasks

Merge branch 'master' into colin/5558-remove-dup-channel

3327a88

AdityaSripal approved these changes Jun 26, 2020

View reviewed changes

AdityaSripal added the A:automerge Automatically merge PR once all prerequisites pass. label Jun 26, 2020

Merge branch 'master' into colin/5558-remove-dup-channel

5c62ddf

mergify bot merged commit 43837b1 into master Jun 26, 2020

mergify bot deleted the colin/5558-remove-dup-channel branch June 26, 2020 16:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor 04-channel/keeper tests to ibc testing pkg #6400

Refactor 04-channel/keeper tests to ibc testing pkg #6400

colin-axner commented Jun 11, 2020 •

edited

Loading

codecov bot commented Jun 11, 2020 •

edited

Loading

colin-axner commented Jun 12, 2020

colin-axner commented Jun 12, 2020

colin-axner commented Jun 18, 2020

fedekunze left a comment

AdityaSripal left a comment

AdityaSripal Jun 25, 2020

AdityaSripal Jun 25, 2020

colin-axner Jun 26, 2020

fedekunze Jun 26, 2020

AdityaSripal Jun 25, 2020

fedekunze Jun 25, 2020

colin-axner Jun 26, 2020 •

edited

Loading

AdityaSripal Jun 25, 2020

colin-axner Jun 26, 2020

AdityaSripal Jun 25, 2020

colin-axner Jun 26, 2020 •

edited

Loading

fedekunze left a comment

AdityaSripal left a comment

AdityaSripal Jun 26, 2020

	func (conn *TestConnection) NextTestChannel() TestChannel {
	func (conn *TestConnection) NewTestChannel() TestChannel {

	func (conn *TestConnection) FirstOrNextTestChannel() TestChannel {
	func (conn *TestConnection) GetTestChannel() TestChannel {

Refactor 04-channel/keeper tests to ibc testing pkg #6400

Refactor 04-channel/keeper tests to ibc testing pkg #6400

Conversation

colin-axner commented Jun 11, 2020 • edited Loading

Description

codecov bot commented Jun 11, 2020 • edited Loading

Codecov Report

colin-axner commented Jun 12, 2020

colin-axner commented Jun 12, 2020

colin-axner commented Jun 18, 2020

fedekunze left a comment

Choose a reason for hiding this comment

AdityaSripal 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colin-axner Jun 26, 2020 • edited Loading

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

colin-axner Jun 26, 2020 • edited Loading

Choose a reason for hiding this comment

fedekunze left a comment

Choose a reason for hiding this comment

AdityaSripal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colin-axner commented Jun 11, 2020 •

edited

Loading

codecov bot commented Jun 11, 2020 •

edited

Loading

colin-axner Jun 26, 2020 •

edited

Loading

colin-axner Jun 26, 2020 •

edited

Loading