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.

Sign up for GitHub

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

Jump to bottom

feat: adding hermes to e2e tests. #3408

Merged
merged 24 commits into from
Jun 7, 2023
Merged

feat: adding hermes to e2e tests. #3408

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
67d95cf
feat: adding hermes to e2e tests.
DimitrisJim Apr 4, 2023
a526e8c
Hardcode the tag for testing.
DimitrisJim Apr 4, 2023
a4475ed
Point interchaintest to fork.
DimitrisJim Apr 5, 2023
f747125
Add delays after interchain account registration.
DimitrisJim Apr 6, 2023
ea35187
Add additional delays to failing tests.
DimitrisJim Apr 6, 2023
bf9252d
Update fork; add a wait in MsgPayPacketFeeSingleSender.
DimitrisJim Apr 9, 2023
3fb3004
Point back to interchaintest.
DimitrisJim Apr 26, 2023
efb2244
Remove import identifiers for interchaintest when not needed.
DimitrisJim Apr 26, 2023
394e531
Manually clean up go.sum's entries for the fork. The fork struggle is…
DimitrisJim Apr 26, 2023
921ec82
Merge branch 'main' into jim/3315-integrate-hermes-into-our-e2e-tests
DimitrisJim Apr 27, 2023
4e0db76
Add additional waiting for blocks before querying for packets.
DimitrisJim Apr 27, 2023
99d74b0
Merge branch 'main' into jim/3315-integrate-hermes-into-our-e2e-tests
crodriguezvega May 14, 2023
2bb1e3e
Merge branch 'main' into jim/3315-integrate-hermes-into-our-e2e-tests
DimitrisJim May 15, 2023
77bc38d
Add a wait for the packets in additional places.
DimitrisJim May 15, 2023
3dad322
Merge branch 'main' into jim/3315-integrate-hermes-into-our-e2e-tests
DimitrisJim May 23, 2023
f387da5
Merge branch 'main' into jim/3315-integrate-hermes-into-our-e2e-tests
colin-axner May 25, 2023
777fced
Merge branch 'main' into jim/3315-integrate-hermes-into-our-e2e-tests
DimitrisJim May 30, 2023
b56a20e
Move waiting for blocks in the start relayer function.
DimitrisJim Jun 1, 2023
07b76fb
experiment: remove the sleep, keep waiting for 10 blocks.
DimitrisJim Jun 1, 2023
0c9afbb
Merge branch 'main' into jim/3315-integrate-hermes-into-our-e2e-tests
DimitrisJim Jun 1, 2023
7afcd2e
Merge branch 'main' into jim/3315-integrate-hermes-into-our-e2e-tests
DimitrisJim Jun 2, 2023
2ae97a3
Merge branch 'main' into jim/3315-integrate-hermes-into-our-e2e-tests
DimitrisJim Jun 5, 2023
e8ea7d4
Merge branch 'main' into jim/3315-integrate-hermes-into-our-e2e-tests
DimitrisJim Jun 7, 2023
c45be2a
Revert. Set rly as the default in the workflow file.
DimitrisJim Jun 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 13 additions & 6 deletions e2e/relayer/relayer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,10 @@ const (
Rly = "rly"
Hermes = "hermes"

cosmosRelayerRepository = "damiannolan/rly" //"ghcr.io/cosmos/relayer"
cosmosRelayerUser = "100:1000" // docker run -it --rm --entrypoint echo ghcr.io/cosmos/relayer "$(id -u):$(id -g)"
hermesRelayerRepository = "ghcr.io/informalsystems/hermes"
hermesRelayerUser = "1000:1000"
rlyRelayerRepository = "damiannolan/rly" //"ghcr.io/cosmos/relayer"
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be changed back?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope, these are used when creating the relayer in newHermesRelayer:

ibc-go/e2e/relayer/relayer.go

Line 61 in 77bc38d

customImageOption := relayer.CustomDockerImage(hermesRelayerRepository, tag, hermesRelayerUser)

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry I mean't the damiannolan/rly string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, right, that's pending on cosmos/relayer#1102

rlyRelayerUser = "100:1000" // docker run -it --rm --entrypoint echo ghcr.io/cosmos/relayer "$(id -u):$(id -g)"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there's a public function for this now, but can't remember

Copy link
Contributor

Choose a reason for hiding this comment

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

Here we are

Copy link
Contributor Author

@DimitrisJim DimitrisJim May 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

looked into this. Using a custom image (like we currently do) doesn't use the defaults. Can't use the defaults for rly since we're based on Damian's forked image, can't use the default for hermes since interchaintest hasn't updated their registry url (still points to docker rather than ghcr + is on version 1.2.0). I'll probably open a PR for the second case on interchaintest and we can drop both consts for hermes.

Copy link
Contributor

Choose a reason for hiding this comment

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

cool, once we switch off damian's image (probably after v7.1 release) then we can move to the defaults. Is there an issue to track this work? If not, maybe we can open an issue

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 not, maybe we can open an issue

yup! done with #3615

Copy link
Contributor Author

Choose a reason for hiding this comment

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

re: hermes image/tag updates done strangelove-ventures/interchaintest#571

)

// Config holds configuration values for the relayer used in the tests.
Expand All @@ -35,7 +37,7 @@ func New(t *testing.T, cfg Config, logger *zap.Logger, dockerClient *dockerclien
case Rly:
return newCosmosRelayer(t, cfg.Tag, logger, dockerClient, network)
case Hermes:
return newHermesRelayer()
return newHermesRelayer(t, cfg.Tag, logger, dockerClient, network)
default:
panic(fmt.Sprintf("unknown relayer specified: %s", cfg.Type))
}
Expand All @@ -44,7 +46,7 @@ func New(t *testing.T, cfg Config, logger *zap.Logger, dockerClient *dockerclien
// newCosmosRelayer returns an instance of the go relayer.
// Options are used to allow for relayer version selection and specifying the default processing option.
func newCosmosRelayer(t *testing.T, tag string, logger *zap.Logger, dockerClient *dockerclient.Client, network string) ibc.Relayer {
customImageOption := relayer.CustomDockerImage(cosmosRelayerRepository, tag, cosmosRelayerUser)
customImageOption := relayer.CustomDockerImage(rlyRelayerRepository, tag, rlyRelayerUser)
relayerProcessingOption := relayer.StartupFlags("-p", "events") // relayer processes via events

relayerFactory := interchaintest.NewBuiltinRelayerFactory(ibc.CosmosRly, logger, customImageOption, relayerProcessingOption)
Expand All @@ -55,8 +57,13 @@ func newCosmosRelayer(t *testing.T, tag string, logger *zap.Logger, dockerClient
}

// newHermesRelayer returns an instance of the hermes relayer.
func newHermesRelayer() ibc.Relayer {
panic("hermes relayer not yet implemented for interchaintest")
func newHermesRelayer(t *testing.T, tag string, logger *zap.Logger, dockerClient *dockerclient.Client, network string) ibc.Relayer {
customImageOption := relayer.CustomDockerImage(hermesRelayerRepository, tag, hermesRelayerUser)
relayerFactory := interchaintest.NewBuiltinRelayerFactory(ibc.Hermes, logger, customImageOption)

return relayerFactory.Build(
t, dockerClient, network,
)
}

// RelayerMap is a mapping from test names to a relayer set for that test.
Expand Down
4 changes: 3 additions & 1 deletion e2e/testconfig/testconfig.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@ const (
// defaultRlyTag is the tag that will be used if no relayer tag is specified.
// all images are here https://github.com/cosmos/relayer/pkgs/container/relayer/versions
defaultRlyTag = "latest" // "andrew-tendermint_v0.37" // "v2.2.0"
// defaultHermesTag is the tag that will be used if no relayer tag is specified for hermes.
defaultHermesTag = "v1.4.0"
// defaultChainTag is the tag that will be used for the chains if none is specified.
defaultChainTag = "main"
// defaultRelayerType is the default relayer that will be used if none is specified.
Expand Down Expand Up @@ -286,7 +288,7 @@ func getRelayerConfigFromEnv() relayer.Config {
rlyTag = defaultRlyTag
}
if relayerType == relayer.Hermes {
// TODO: set default hermes version
rlyTag = defaultHermesTag
}
}
return relayer.Config{
Expand Down
4 changes: 2 additions & 2 deletions e2e/testsuite/testsuite.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,13 @@ import (
"context"
"fmt"
"strings"
"time"

dockerclient "github.com/docker/docker/client"
interchaintest "github.com/strangelove-ventures/interchaintest/v7"
"github.com/strangelove-ventures/interchaintest/v7/chain/cosmos"
"github.com/strangelove-ventures/interchaintest/v7/ibc"
"github.com/strangelove-ventures/interchaintest/v7/testreporter"
test "github.com/strangelove-ventures/interchaintest/v7/testutil"
"github.com/stretchr/testify/suite"
"go.uber.org/zap"
"go.uber.org/zap/zaptest"
Expand Down Expand Up @@ -128,7 +128,7 @@ func (s *E2ETestSuite) SetupChainsRelayerAndChannel(ctx context.Context, channel
}
})
// wait for relayer to start.
time.Sleep(time.Second * 10)
s.Require().NoError(test.WaitForBlocks(ctx, 10, chainA, chainB), "failed to wait for blocks")
Copy link
Contributor

Choose a reason for hiding this comment

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

this is what we will be able to remove in the future right?

Copy link
Contributor Author

@DimitrisJim DimitrisJim Jun 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Considering this was in place with rly (only in sleep form) maybe we'll always need to give relayers some time to get up and running. Ideally, we should be able to drop this to 5 blocks (bumped it to 10 for hermes since 5 resulted in old familiar errors indicating that it needed more time).

}

s.InitGRPCClients(chainA)
Expand Down