ibc-go icon indicating copy to clipboard operation
ibc-go copied to clipboard

e2e: Restructure to reduce number of runners used

Open anhductn2001 opened this issue 1 year ago • 12 comments

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:

  1. Setup of test suite would create docker resources
  2. Setup of test would create a new channel/connection
  3. 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.

anhductn2001 avatar Dec 05 '23 14:12 anhductn2001

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

Impacted file tree graph

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

codecov-commenter avatar Dec 07 '23 07:12 codecov-commenter

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

anhductn2001 avatar Dec 13 '23 07:12 anhductn2001

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.

anhductn2001 avatar Jan 03 '24 14:01 anhductn2001

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 avatar Jan 04 '24 09:01 anhductn2001

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

  1. Update the Config struct in interchaintest to allow for enabling these fields
  2. 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 in ibc-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.
  3. Update the E2ETestSuite in ibc-go to have a mapping of pathName -> Relayer
  4. 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.

chatton avatar Jan 04 '24 10:01 chatton

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

anhductn2001 avatar Jan 19 '24 03:01 anhductn2001

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

chatton avatar Jan 22 '24 12:01 chatton

command: make e2e-test-suite entrypoint=TestInterchainAccountsTestSuite

anhductn2001 avatar Jan 22 '24 14:01 anhductn2001

raw_log: 'failed to execute message; message index: 0: ORDER_NONE_UNSPECIFIED: invalid

anhductn2001 avatar Jan 22 '24 14:01 anhductn2001

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 ?

chatton avatar Jan 23 '24 10:01 chatton

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.

DimitrisJim avatar Apr 19 '24 13:04 DimitrisJim

Thank you @DimitrisJim , sounds good, I'm looking forward to it

anhductn2001 avatar Apr 19 '24 15:04 anhductn2001

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

DimitrisJim avatar Jul 05 '24 08:07 DimitrisJim