Conversation
…only-channels-associated-with-an-individual-test
chatton
requested review from
AdityaSripal,
DimitrisJim,
bznein,
colin-axner,
crodriguezvega,
damiannolan and
gjermundgaraba
as code owners
…only-channels-associated-with-an-individual-test
chatton
changed the title
| // TODO(chatton): explicitly enable watching of ICA channels | ||
| // this will ensure the ICA tests pass, but this will need to be modified to make sure | ||
| // ICA tests will succeed in parallel. |
There was a problem hiding this comment.
I won't create another issue for this since it will be implicitly required to fix before our tests will pass in parallel
…only-channels-associated-with-an-individual-test
| } | ||
|
|
||
| // modifyHermesConfigFile reads the hermes config file, applies a modification function and returns an error if any. | ||
| func modifyHermesConfigFile(ctx context.Context, h *hermes.Relayer, modificationFn func(map[string]interface{}) error) error { |
There was a problem hiding this comment.
Not sure if it would be any better or not, but interchaintest has some utility functions for modifying Toml files:
https://github.com/strangelove-ventures/interchaintest/blob/main/testutil/toml.go
There was a problem hiding this comment.
good call, I think I reinvented the wheel here!
There was a problem hiding this comment.
gjermundgaraba
left a comment
gjermundgaraba
left a comment
There was a problem hiding this comment.
This is a great improvement for the e2e tests!
e2e/testsuite/testsuite.go
Outdated
| channels, err := r.GetChannels(ctx, s.GetRelayerExecReporter(), c.Config().ChainID) | ||
| s.Require().NoError(err) | ||
|
|
||
| if s.channels[s.T().Name()] == nil { | ||
| s.channels[s.T().Name()] = make(map[ibc.Chain][]ibc.ChannelOutput) | ||
| } | ||
| s.channels[s.T().Name()][c] = channels |
There was a problem hiding this comment.
just realised this is wrong! We are currently fetching all channels associated with the chain, not just the one that was created. It means every relayer is still watching all channels, we need to identify the exact channel for this test. I think it is channels[len(channels)-1]
…only-channels-associated-with-an-individual-test
damiannolan
left a comment
damiannolan
left a comment
There was a problem hiding this comment.
Awesome work! Thanks for diving into the depths again! 🫡
Happy to approve and move forward with this if it will improve things for us wrt to runners 🙏🏻
e2e/relayer/relayer.go
Outdated
| return fmt.Errorf("failed to find chain with id %s", chainID) | ||
| } | ||
|
|
||
| var channelIDs [][]string |
There was a problem hiding this comment.
caught me off guard a bit with the 2d slice. Each []string contained within is a portID/channelID tuple?
little nit, but could name this channelEndpoints or something similar
There was a problem hiding this comment.
yeah good call, it's not actually the ids :D
| // relayerPool is a pool of relayers that can be used in tests. | ||
| relayerPool []ibc.Relayer | ||
| // testRelayerMap is a map of test suite names to relayers that are used in the test suite. | ||
| // this is used as a cache after a relayer has been assigned to a test suite. | ||
| testRelayerMap map[string]ibc.Relayer |
There was a problem hiding this comment.
So another way of saying this would be that the relayerPool is essentially cold - available relayers. And the testRelayerMap is hot - a relayer is in use under a particular test name key!
Seems like we can go with this until we can add new relayers on the fly, hopefully! 👍🏻
| ChainSpecs: []*interchaintest.ChainSpec{chainASpec, chainBSpec}, | ||
| // arbitrary number that will not be required if https://github.com/strangelove-ventures/interchaintest/issues/1153 is resolved. | ||
| // It can be overridden in individual test suites in SetupSuite if required. | ||
| RelayerCount: 10, |
There was a problem hiding this comment.
Suppose we could always do the AST way as a somewhat improvement if we need to, and initialise the count based on test funcs in the suite in some other setup func
There was a problem hiding this comment.
yeah, we definitely can improve this arbitrary number.
…only-channels-associated-with-an-individual-test
|
chatton
deleted the
cian/issue#6634-configure-relayers-to-watch-only-channels-associated-with-an-individual-test
branch
3 participants
Description
This PR replaces the single relayer associated with a test suite, and replaces it with a pool of relayers so each test can have its own.
At the moment, relayers need to be specified before the chain is created. For this reason it does not seem possible to lazily create a relayer when a test needs one. I created this issue which when resolved, we should be able to create relayers as needed, rather than pre-loading them.
Each of these relayers has been configured to only watch the channel(s) for a single test with the caveat that it is only configured for hermes (no-op for other relayers currently)
The tests will still be running 1 test per host, however the relayer will be unique to that test, as well as configured to watch only the channels created for that test.
There is an exception for ICA channels at the moment, a wild card is used which means some additional work will need to be done to update the packet filter while the relayer is running. This might be tricky as I believe we only get the ica channelID after the relayer has performed the channel handshake 🤔
closes: #6634
ref: strangelove-ventures/interchaintest#1153
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/).godoccomments.Files changedin the GitHub PR explorer.SonarCloud Reportin the comment section below once CI passes.