feat(tests): add helper functions, keeper test suite. #7052

DimitrisJim · 2024-08-05T14:21:44Z

Description

should probably do trick for keeper funcs for time being, testing package should need additional tweaks in future

closes: #XXXX

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.

DimitrisJim · 2024-08-05T14:22:39Z

testing/path.go

@@ -162,6 +162,20 @@ func (path *Path) SetupClients() {
 	}
 }

+// SetupEurekaClients is a helper function to create clients supporting ibc-eureka on both
+// chains. It assumes the caller does not anticipate any errors.
+func (path *Path) SetupEurekaClients() {


to reduce overall diffs, just named this with Eureka, think we just rename old function to Classic and change occurrences afterwards

I like naming suggestion below SetupCounterparties, nice to avoid introducing Eureka when possible

yup, Aditya will push that suggestion, ideally wanted a single setup function to get things ready but fair counterpoint re Eureka getting disseminated through-out code

DimitrisJim · 2024-08-05T14:22:50Z

modules/core/packet-server/keeper/keeper_test.go

+}
+
+// TODO: Remove, just testing the testing setup.
+func (suite *KeeperTestSuite) TestCreateEurekaClients() {


will rm before merge

DimitrisJim · 2024-08-05T14:23:30Z

modules/core/packet-server/keeper/keeper_test.go

+	suite.chainA = suite.coordinator.GetChain(ibctesting.GetChainID(1))
+	suite.chainB = suite.coordinator.GetChain(ibctesting.GetChainID(2))


should probably be fine with two chains for handler testing. can be changed if necessary

AdityaSripal

Restructuring suggestion.

Also let's remove reference to Eureka in function names and use V2 instead

AdityaSripal · 2024-08-05T15:41:40Z

testing/path.go

@@ -162,6 +162,20 @@ func (path *Path) SetupClients() {
 	}
 }

+// SetupEurekaClients is a helper function to create clients supporting ibc-eureka on both
+// chains. It assumes the caller does not anticipate any errors.
+func (path *Path) SetupEurekaClients() {


Rather than doing this all in one step, let's keep SetupClients unchanged, and add a new function called SetupCounterparties that does the provide counterparty for each side.

Then in our tests, we can setup like so:

path.SetupClients() path.SetupCounterparties()

eventually path.Setup() will just do both

created a SetupV2 that does both

AdityaSripal · 2024-08-05T15:45:14Z

Also we need the Packet-server keeper to be wired up in the app and made a public field so we can call it from suite.chainA for example.

We can modify IBCKeeper later, but let's keep it a top level field of the testing app for now

colin-axner · 2024-08-05T15:48:57Z

testing/endpoint.go

@@ -165,6 +165,18 @@ func (endpoint *Endpoint) UpdateClient() (err error) {
 	return endpoint.Chain.sendMsgs(msg)
 }

+// ProvideCounterparty will construct and execute a MsgProvideCounterparty on the associated endpoint.
+func (endpoint *Endpoint) ProvideCounterparty() (err error) {
+	merklePath := commitmenttypes.NewMerklePath([]byte("ibc"), []byte(""))


Endpoint should probably be modified to have the merkle prefix now

Agreed, let's create an issue and do this later.

Think it should be added as a new field but this should be the default value that can then be malleated later by test writer

colin-axner · 2024-08-05T15:49:40Z

modules/core/packet-server/keeper/keeper_test.go

+	suite.chainB = suite.coordinator.GetChain(ibctesting.GetChainID(2))
+
+	// commit some blocks so that QueryProof returns valid proof (cannot return valid query if height <= 1)
+	suite.coordinator.CommitNBlocks(suite.chainA, 2)


Is this necessary? I think it might be legacy code

hm, good question, I see this through-out, might make sense to trim off through-out later in single PR.

AdityaSripal · 2024-08-05T23:02:54Z

modules/core/packet-server/keeper/keeper.go

@@ -8,14 +8,14 @@ import (

 type Keeper struct {
 	cdc           codec.BinaryCodec
-	channelKeeper types.ChannelKeeper
-	clientKeeper  types.ClientKeeper
+	ChannelKeeper types.ChannelKeeper


made this public so they were retrievable in testchains through the packetserver.

We already do this with IBCKeeper

AdityaSripal · 2024-08-05T23:03:30Z

modules/core/packet-server/types/expected_keepers.go

@@ -16,7 +16,7 @@ type ChannelKeeper interface {
 	GetPacketCommitment(ctx sdk.Context, portID string, channelID string, sequence uint64) []byte

 	// DeletePacketCommitment deletes the packet commitment hash under the commitment path
-	DeletePacketCommitment(ctx sdk.Context, portID string, channelID string, sequence uint64)
+	// DeletePacketCommitment(ctx sdk.Context, portID string, channelID string, sequence uint64)


removed this for now. Can be readded in future PR that needs it

AdityaSripal · 2024-08-05T23:04:14Z

testing/simapp/app.go

@@ -370,6 +372,9 @@ func NewSimApp(
 		appCodec, keys[ibcexported.StoreKey], app.GetSubspace(ibcexported.ModuleName), ibctm.NewConsensusHost(app.StakingKeeper), app.UpgradeKeeper, scopedIBCKeeper, authtypes.NewModuleAddress(govtypes.ModuleName).String(),
 	)

+	// Setup packet server to call on Eureka tests
+	app.PacketServer = packetserverkeeper.NewKeeper(appCodec, app.IBCKeeper.ChannelKeeper, app.IBCKeeper.ClientKeeper)


Needed this so that we can call into these functions from the TestChain struct

I suppose we could also always create the PacketServer.Keeper struct on the fly using the IBCKeeper fields...

yea but think this is perfectly fine for now, lets just add the testing boilerplate and refactor it later when needed

sonarqubecloud · 2024-08-05T23:07:38Z

Quality Gate passed for 'ibc-go'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

DimitrisJim · 2024-08-06T03:34:22Z

testing/path.go

@@ -162,6 +169,18 @@ func (path *Path) SetupClients() {
 	}
 }

+// SetupCounterparties is a helper function to set the counterparties supporting ibc-eureka on both
+// chains. It assumes the caller does not anticipate any errors.
+func (path *Path) SetupCounterparties() {


does this make sense as standalone func? I.e can't see where we'd invoke it separately without it being preceded by SetupClients. We could possibly inline in SetupV2, leaving as potential follow up.

Hmm... yea I did it in case you wanted to SetupClients, test X, Y, Z, then call SetCounterparties after and do some more testing

DimitrisJim · 2024-08-06T03:36:46Z

thanks for follow up tweaks @AdityaSripal! Gonna merge this in a bit so we have it in handler branches. Think any follow ups can be addressed later to not block.

feat(tests): add helper functions, keeper test suite.

97d8a51

DimitrisJim requested review from colin-axner, AdityaSripal, damiannolan, chatton, crodriguezvega, gjermundgaraba and bznein as code owners August 5, 2024 14:21

DimitrisJim commented Aug 5, 2024

View reviewed changes

AdityaSripal reviewed Aug 5, 2024

View reviewed changes

colin-axner reviewed Aug 5, 2024

View reviewed changes

colin-axner approved these changes Aug 5, 2024

View reviewed changes

wire up packet server in app

ad01565

AdityaSripal requested a review from srdtrk as a code owner August 5, 2024 23:02

AdityaSripal reviewed Aug 5, 2024

View reviewed changes

DimitrisJim commented Aug 6, 2024

View reviewed changes

DimitrisJim merged commit dcff30b into feat/ibc-eureka Aug 6, 2024
63 of 65 checks passed

DimitrisJim deleted the jim/eureka-testing-setup branch August 6, 2024 04:42

DimitrisJim mentioned this pull request Aug 7, 2024

Modify Endpoint to have the merkle prefix #7062

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(tests): add helper functions, keeper test suite. #7052

feat(tests): add helper functions, keeper test suite. #7052

DimitrisJim commented Aug 5, 2024

DimitrisJim Aug 5, 2024

colin-axner Aug 5, 2024

DimitrisJim Aug 5, 2024

DimitrisJim Aug 5, 2024

DimitrisJim Aug 5, 2024

AdityaSripal left a comment

AdityaSripal Aug 5, 2024

colin-axner Aug 5, 2024

AdityaSripal Aug 5, 2024

AdityaSripal commented Aug 5, 2024

colin-axner Aug 5, 2024

AdityaSripal Aug 5, 2024

colin-axner Aug 5, 2024

DimitrisJim Aug 6, 2024

AdityaSripal Aug 5, 2024

AdityaSripal Aug 5, 2024

AdityaSripal Aug 5, 2024

AdityaSripal Aug 5, 2024

DimitrisJim Aug 6, 2024

sonarqubecloud bot commented Aug 5, 2024

DimitrisJim Aug 6, 2024

AdityaSripal Aug 6, 2024

DimitrisJim commented Aug 6, 2024

		suite.chainA = suite.coordinator.GetChain(ibctesting.GetChainID(1))
		suite.chainB = suite.coordinator.GetChain(ibctesting.GetChainID(2))

feat(tests): add helper functions, keeper test suite. #7052

feat(tests): add helper functions, keeper test suite. #7052

Conversation

DimitrisJim commented Aug 5, 2024

Description

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

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

AdityaSripal commented Aug 5, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Aug 5, 2024

Quality Gate passed for 'ibc-go'

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DimitrisJim commented Aug 6, 2024