ibc-go
ibc-go copied to clipboard
e2e: Restructure to reduce number of runners used
Description
Restructure to run 1 test suite per runner. And we can still run individual tests the way we do now locally.
Implementation the following:
- Setup of test suite would create docker resources
- Setup of test would create a new channel/connection
- Perform test as usual with the addition of t.Parallel()
closes: #5062
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.
- [ ] Targeted PR against correct branch (see CONTRIBUTING.md).
- [ ] Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
- [ ] Code follows the module structure standards and Go style guide.
- [ ] Wrote unit and integration tests.
- [ ] Updated relevant documentation (
docs/
) or specification (x/<module>/spec/
). - [ ] Added relevant
godoc
comments. - [ ] Provide a commit message to be used for the changelog entry in the PR description for review.
- [ ] Re-reviewed
Files changed
in the Github PR explorer. - [ ] Review
Codecov Report
in the comment section below once CI passes.
Codecov Report
Attention: 30 lines
in your changes are missing coverage. Please review.
Comparison is base (
5322823
) 81.16% compared to head (df30c8a
) 81.02%.
Additional details and impacted files
@@ Coverage Diff @@
## feat/run-full-test-suite-on-each-github-runner #5317 +/- ##
==================================================================================
- Coverage 81.16% 81.02% -0.14%
==================================================================================
Files 199 199
Lines 15278 15313 +35
==================================================================================
+ Hits 12401 12408 +7
- Misses 2408 2435 +27
- Partials 469 470 +1
Files | Coverage Δ | |
---|---|---|
cmd/build_test_matrix/main.go | 51.38% <53.12%> (-10.08%) |
:arrow_down: |
It seems like the Summary progress in the previous commit is running beyond the expected time causing the CI to be pending. Can you help me cancel it? @chatton
We are in big trouble adding t.Parallel(). I'm thinking of ways and trying to solve it. Thanks @chatton , I find your comments absolutely correct.
Currently, with each initialization of a relayer instance, we must come with a new pair of chains. Do you have any ideas to fix this? @chatton
@anhductn2001 once possible solution I can think of is to maintain a map of chainPair -> relayerInstance
, this could require a refactor to the logic of the startRelyerFn
, perhaps maybe a map[pathName]Relayer
?
One thing that might be a problem is if the relayer is configured to watch all channels, we need to make sure that the relayer is configured to only relay packets we care about for the specific test.
For hermes this looks like a setting we will want to configure. The default behaviour will not filter any packets and so everything will be relayed. (interfering with other tests).
Currently, the relayer has a lot of default configuration options that are specified in interchaintest here
It looks like chains.packet_filter
isn't even an option here, so the Config
type may need an update to allow for it.
I think the following things need to happen before we can make this work.
- Update the
Config
struct in interchaintest to allow for enabling these fields - Enable support to make arbitrary changes to the
Config
struct when creating a hermes relayer instance in interchaintest. This may not be as simple as it seems, this code may need to be updated to accept new types or functions which make modifications. - A slightly hackier alternative would be to provide some way to write arbitrary bytes to where the hermes config file will live, this way the logic could live inibc-go
, I think the arbitrary configuration of the config file would be really great feature to exist in interchaintest though, and if possible it would be great to add it there. - Update the
E2ETestSuite
in ibc-go to have a mapping ofpathName -> Relayer
- Update the
E2ETestSuite.StartRelayer
function to start the relevant relayer based on the pathName in the tests.
The interchaintest modifications I suggested will be the tricker thing to implement as it might end up requiring some non-trivial refactors.
Could you help me update branch feat/run-full-test....
. I'm trying to run the tests but seem like they require ics27, ics29, etc @chatton
Hi @anhductn2001, I tried running tests from decentrio:restructure_e2e
and they seem to be running fine, which commands are you using to run tests that aren't working for you so I can try them
command: make e2e-test-suite entrypoint=TestInterchainAccountsTestSuite
raw_log: 'failed to execute message; message index: 0: ORDER_NONE_UNSPECIFIED: invalid
ah! @anhductn2001 that test specifically has an issue on main and is being worked on, can you try running a different test, maybe the transfer successful case TestMsgTransfer_Succeeds_Nonincentivized
?
Hey y'all wanted to give an update on this.
Discussed with @chatton last week and we came up with a couple of steps to actually get this through the finish line. I'll try and add a rough outline of these steps in the parent issue.
Wanted to thank y'all for the hard work you've put into this :1st_place_medal:, it is greatly appreciated. We'll probably re-target this branch to main and base some new work around it. We will make sure each of y'all gets correct attribution when the final commit is merged.
Thank you @DimitrisJim , sounds good, I'm looking forward to it
@chatton has pushed a number of awesome improvements building from this.I think I'll close this PR for now. Thanks so much for initial work on this @anhductn2001! :heart: