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

feat: adding hermes to e2e tests. #3408

Merged
merged 24 commits into from
Jun 7, 2023
Merged
Show file tree
Hide file tree
Changes from 22 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
4 changes: 2 additions & 2 deletions .github/workflows/e2e-test-workflow-call.yml
Original file line number Diff line number Diff line change
Expand Up @@ -50,12 +50,12 @@ on:
relayer-type:
description: "The type of relayer to use"
required: false
default: "rly"
default: "hermes"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Temporarily, for checking test results

Copy link
Contributor

Choose a reason for hiding this comment

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

will we be reverting this before merging?

type: string
relayer-tag:
description: "The tag to use for the relayer"
required: false
default: "" # the tests themselves will choose a sensible default when unset.
default: "1.4.0" # the tests themselves will choose a sensible default when unset.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ditto, temporarily.

Copy link
Contributor

Choose a reason for hiding this comment

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

also reverting this

type: string
build-and-push-docker-image:
description: "Flag to specify if the docker image should be built and pushed beforehand"
Expand Down
19 changes: 13 additions & 6 deletions e2e/relayer/relayer.go
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:

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

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

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