e2e: add client recovery proposal test #2130
Conversation
e2e/testsuite/testsuite.go
Outdated
| func (s *E2ETestSuite) SetupClients(ctx context.Context, relayer ibc.Relayer, opts ibc.CreateClientOptions) { | ||
| err := relayer.CreateClients(ctx, s.GetRelayerExecReporter(), s.getPathName(), opts) | ||
| s.Require().NoError(err) | ||
| } |
There was a problem hiding this comment.
I think this is currently not working. My guess is it is no-oping because a client already exists for the given path name. I think we need to generate/add a path for every client/connection/channel (aka endpoint) stack. My vote would be to add the endpoint type and when you call NewEndpoint, it adds the ibctest path name
There was a problem hiding this comment.
Soo looks like you're right, that new client is not being created.
The easy solution here is to wire up the --override flag in ibctest for the relayers rly tx clients --override <path> command.
The client is not begin created b/c in the relayer config, the client already exists so it does not create a new client.
This is what the --override flag is used for; to create a new client even though the config is showing a client-id already exists. But, it is currently not working. Issue here: cosmos/relayer#952
In the meantime, one dirty solution that I think will work is to create another relayer instances with a different home folder path. Then create the new "substituteClient" with the bad trusting period that way.
There was a problem hiding this comment.
Generating a new path (with a new path name) worked
| func (s *E2ETestSuite) generatePathName() string { | ||
| pathName := fmt.Sprintf("%s-path-%d", s.T().Name(), s.pathNameIndex) | ||
| s.pathNameIndex++ | ||
| return strings.ReplaceAll(pathName, "/", "-") |
There was a problem hiding this comment.
for every endpoint (client/connection/channel) stack we need a new path name, so I added an index kept by the E2ETestSuite
| // TODO: update when client identifier created is accessible | ||
| // currently assumes first client is 07-tendermint-0 | ||
| substituteClientID = clienttypes.FormatClientIdentifier(ibcexported.Tendermint, 0) | ||
|
|
||
| // TODO: replace with better handling of path names | ||
| pathName = fmt.Sprintf("%s-path-%d", s.T().Name(), 0) | ||
| pathName = strings.ReplaceAll(pathName, "/", "-") |
There was a problem hiding this comment.
These are a bit hacky, but I think it is better to just do this for now and figure out a better solution later. I think adding the endpoint type should solve the issue in both cases
colin-axner
requested review from
AdityaSripal,
crodriguezvega,
seantking,
charleenfei,
damiannolan and
chatton
as code owners
|
😅 Maybe we should only build some of these tests on commits to main and get notifications of failure? I don't think client recovery proposal e2e tests need to be run on every pr |
|
@colin-axner maybe just manually maintaining the list of tests and where they run will be the best option. We're already starting to have tests we don't want to run all the time. These, icad (and soon upgrade) We can maybe just keep a simple json file in the repo and pass that to different github actions. What do you think? |
Sounds good, maybe we could run them nightly or something? |
Yep we can do that. Once we have all the versions we want to run, we can use a cron schedule in the github actions. |
Description
closes: #1965
closes: #2042
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.
docs/) or specification (x/<module>/spec/)godoccomments.Unreleasedsection inCHANGELOG.mdFiles changedin the Github PR explorerCodecov Reportin the comment section below once CI passes