e2e: Restructure to reduce number of runners used

[anhductn2001]

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.

[codecov-commenter]

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%)	⬇️

[chatton]

Things are looking great, but the one big thing that needs to be addressed before we can merge this into main is the one relayer per test.

I think what we can do here, is I'll create a feature branch for this, we can merge your PR into that, then we can work on making the changes required for the relayers in a follow up.

Does that work for you?

There is no rush to immediately start working on the relayer issue, it might be a bit of work. Either way, we'll make sure that you are a co-author on the commit when the feature branch gets merged into main!

Could you change the base branch to be feat/run-full-test-suite-on-each-github-runner?

[chatton]

I think it may be possible to not need to create a separate script for this, but for now I'm happy to leave this is as is and maybe we can create a follow up issue to look into modifying the existing script without some of this duplication.

[chatton]

I'm a little wary of these, if the tests are running in a parallel, stopping the relayer in one test could cause other tests to fail.

Because of this I think we will have to ensure that each test gets its own relayer, and the relayers are only looking at a single channel.

We can do this in a follow up I think, but there may be some flakiness introduced with this.

[chatton]

I see now that we are not running this test with t.Parallel which means that we lose the main benefit of running them in a single suite, I think until we can reliably run the tests in parallel (i.e. figure out how to get the relayer watching a single path) we need to be careful with this.

[anhductn2001]

The problem I'm having is creating a new relayer without creating a new chain.

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

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

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

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.

[anhductn2001]

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

[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

[anhductn2001]

command: make e2e-test-suite entrypoint=TestInterchainAccountsTestSuite

[anhductn2001]

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

[chatton]

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 ?

[DimitrisJim]

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 🥇, 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.

[anhductn2001]

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

[DimitrisJim]

@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! ❤️