Skip to content
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

Multi-consumer capable e2e tests #475

Merged
merged 59 commits into from
Nov 23, 2022
Merged

Multi-consumer capable e2e tests #475

merged 59 commits into from
Nov 23, 2022

Conversation

shaspitz
Copy link
Contributor

@shaspitz shaspitz commented Nov 12, 2022

This PR replicates the e2e test setup that we've previously used for a single consumer, and allows that setup to be done on multiple consumers.

In order to prove out functionality, I've improved TestRelayAndApplySlashPacket in this PR to incorporate multiple consumers. Note that future work can change all/some of the remaining e2e tests to consider multiple consumers, see #506

@shaspitz shaspitz changed the base branch from main to testutil-refactor November 12, 2022 05:10
Base automatically changed from testutil-refactor to main November 15, 2022 20:05
addr1 := utils.GetChangePubKeyAddress(providerValUpdates[i])
addr2 := utils.GetChangePubKeyAddress(consumerValUpdates[i])
suite.Require().True(bytes.Equal(addr1, addr2), "validator mismatch")
suite.providerApp, suite.consumerBundles = suite.setupCallback(suite.T())
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If done correctly, none of the following code should change how setup is done. It should merely add for-loops to setup all consumers in the exact same way we previously setup a single consumer

suite.transferPath.EndpointB.ChannelConfig.PortID = transfertypes.PortID
suite.transferPath.EndpointA.ChannelConfig.Version = transfertypes.Version
suite.transferPath.EndpointB.ChannelConfig.Version = transfertypes.Version
// Support tests that were written before multiple consumers were supported.
Copy link
Contributor Author

@shaspitz shaspitz Nov 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is important to make existing single-consumer tests act in the same way as before. As stated above, we could eventually remove these fields in favor of accessing consumer state only through their "bundle"

@@ -304,8 +304,10 @@ func (s *CCVTestSuite) TestUnbondingNoConsumer() {
providerKeeper := s.providerApp.GetProviderKeeper()
providerStakingKeeper := s.providerApp.GetE2eStakingKeeper()

// remove the consumer chain, which was already registered during setup
providerKeeper.DeleteConsumerClientId(s.providerCtx(), s.consumerChain.ChainID)
// remove all consumer chains, which were already registered during setup
Copy link
Contributor Author

@shaspitz shaspitz Nov 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order for onHold to be false below, we need to delete all client IDs established with the provider

)

// ConsumerBundle serves as a way to store useful in-mem consumer app chain state
// and relevant IBC paths in the context of CCV e2e testing.
type ConsumerBundle struct {
Copy link
Contributor Author

@shaspitz shaspitz Nov 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lmk if there's anyway to make this abstraction more generic, or named in a better way. It's pretty much "all the state associated with each consumer that you need for e2e tests"

@shaspitz shaspitz changed the base branch from main to mapped-sent-packets-fix November 21, 2022 18:52
s.SetupTest()
s.SetupCCVChannel()
s.SetupTransferChannel()
Copy link
Contributor Author

@shaspitz shaspitz Nov 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This previous call to s.SetupTransferChannel() was unnecessary and is not included in the new diff


err = s.path.EndpointB.UpdateClient()
Copy link
Contributor Author

@shaspitz shaspitz Nov 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This call to UpdateClient was unnecessary and is not included in new diff

skippedTests map[string]bool

// The first consumer chain among multiple.
consumerChain *ibctesting.TestChain
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can eventually remove these fields in favor of accessing all this state via consumerBundles below. However, this will require large diffs, and is not feasible to do in a single PR

Base automatically changed from mapped-sent-packets-fix to main November 22, 2022 12:16
@shaspitz shaspitz marked this pull request as ready for review November 22, 2022 16:46
@shaspitz shaspitz requested a review from MSalopek November 22, 2022 16:47
Copy link
Contributor

@danwt danwt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work!

@jtremback jtremback merged commit 095a076 into main Nov 23, 2022
@jtremback jtremback deleted the e2e-multiple-consumers branch November 23, 2022 19:12
danwt pushed a commit that referenced this pull request Nov 25, 2022
commit 7fbbc76
Merge: e772dfb 095a076
Author: Marius Poke <[email protected]>
Date:   Fri Nov 25 12:04:15 2022 +0100

    Merge branch 'main' into marius/key-assignment

commit e772dfb
Author: mpoke <[email protected]>
Date:   Fri Nov 25 11:47:25 2022 +0100

    fix address conversion

commit 095a076
Author: Shawn Marshall-Spitzbart <[email protected]>
Date:   Wed Nov 23 11:12:48 2022 -0800

    Multi-consumer capable e2e tests (#475)

    * changes

    * mas

    * Update README.md

    * Update README.md

    * sorry for the friday night emails

    * Update instance_test.go

    * wip

    * wip

    * wip

    * wip

    * wip

    * wip

    * wip

    * wip

    * works

    * Update generic_setup.go

    * Update setup.go

    * smol

    * small

    * path to ccv chan setup

    * todos

    * Update setup.go

    * Create debug_test.go

    * democ

    * Update debug_test.go

    * setup all ccv channels

    * bump to main

    * another bump, missed one

    * fix after merge main

    * fixes

    * Update slashing.go

    * expired client tests

    * checks

    * fixed the stuff

    * smol

    * changes

    * updates

    * cleans

    * clean

    * todo

    * fixes

    * cleans

    * Update slashing.go

    * Update slashing.go

    Co-authored-by: Jehan <[email protected]>
danwt pushed a commit that referenced this pull request Nov 25, 2022
commit aca9578
Author: mpoke <[email protected]>
Date:   Fri Nov 25 14:50:45 2022 +0100

    fix linter

commit 6d8cd10
Author: mpoke <[email protected]>
Date:   Fri Nov 25 14:46:11 2022 +0100

    adding e2e tests

commit 7fbbc76
Merge: e772dfb 095a076
Author: Marius Poke <[email protected]>
Date:   Fri Nov 25 12:04:15 2022 +0100

    Merge branch 'main' into marius/key-assignment

commit e772dfb
Author: mpoke <[email protected]>
Date:   Fri Nov 25 11:47:25 2022 +0100

    fix address conversion

commit 095a076
Author: Shawn Marshall-Spitzbart <[email protected]>
Date:   Wed Nov 23 11:12:48 2022 -0800

    Multi-consumer capable e2e tests (#475)

    * changes

    * mas

    * Update README.md

    * Update README.md

    * sorry for the friday night emails

    * Update instance_test.go

    * wip

    * wip

    * wip

    * wip

    * wip

    * wip

    * wip

    * wip

    * works

    * Update generic_setup.go

    * Update setup.go

    * smol

    * small

    * path to ccv chan setup

    * todos

    * Update setup.go

    * Create debug_test.go

    * democ

    * Update debug_test.go

    * setup all ccv channels

    * bump to main

    * another bump, missed one

    * fix after merge main

    * fixes

    * Update slashing.go

    * expired client tests

    * checks

    * fixed the stuff

    * smol

    * changes

    * updates

    * cleans

    * clean

    * todo

    * fixes

    * cleans

    * Update slashing.go

    * Update slashing.go

    Co-authored-by: Jehan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants