Restructure E2Es to setup chain in SetupSuite and create channel in SetupTest #1 #6629

chatton · 2024-06-18T11:28:04Z

Description

This PR addresses the first step of the overall restructuring of E2Es to allow for multiple tests running on an individual host.

The main differences introduced in this PR are.

the SetupChainsRelayerAndChannel function has been removed. Instead, the chains get created in SetupSuite and the channels get created in SetupTest.
If any test needs to provide additional configuration options, they can override the SetupTest or SetupSuite functions as required.

Follow up PRs will handle.

refactoring the relayer configuration to watch a single channel for an individual test
deploy a relayer per test (currently we are still using one relayer)
enabling t.Parallel to enable parallel test execution.
updating our CI to execute a full test suite instead of individual tests.

shout out to @anhductn2001 for the work on the first iteration of this.

ref: #5317

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.

…layer and get channel

…mber-of-runners-used

chatton · 2024-06-18T14:02:43Z

e2e/tests/transfer/upgradesv1_test.go

@@ -0,0 +1,356 @@
+//go:build !test_e2e


I created a new file for these tests since the original channel creation options were all a v1 transfer. This was more straightforward than adding conditional logic based on which test was running in the SetupTest

chatton · 2024-06-18T14:02:51Z

e2e/tests/transfer/upgradesv1_test.go

+func (s *TransferChannelUpgradesV1TestSuite) SetupTest() {
+	opts := s.TransferChannelOptions()
+	opts.Version = transfertypes.V1
+	s.SetupPath(ibc.DefaultClientOpts(), opts)
+}


this is the main addition

chatton · 2024-06-18T14:04:00Z

e2e/tests/upgrades/upgrade_test.go

@@ -55,6 +56,15 @@ type UpgradeTestSuite struct {
 	testsuite.E2ETestSuite
 }

+func (s *UpgradeTestSuite) SetupTest() {
+	channelOpts := s.TransferChannelOptions()
+	// TODO(chatton) hack to handle special case for the v8 to v8.1 upgrade test.


maybe not the end of the world as it's a once off, I can create an issue for this but it would be nice to have a generalized way of specifying per channel options per test name. Kind of chicken and egg situation as it's happening in SetupTest

chatton · 2024-06-18T14:04:44Z

e2e/tests/upgrades/upgrade_test.go

@@ -223,7 +233,9 @@ func (s *UpgradeTestSuite) TestChainUpgrade() {
 	t := s.T()

 	ctx := context.Background()
-	chain := s.SetupSingleChain(ctx)


I opted to remove this fn for now, and more native support for a single chain in a follow up (will create issues soon)

I got you brother! :D #3160

chatton · 2024-06-18T14:04:56Z

e2e/tests/wasm/grandpa_test.go

@@ -64,6 +64,61 @@ type GrandpaTestSuite struct {
 	testsuite.E2ETestSuite
 }

+func (s *GrandpaTestSuite) SetupSuite() {
+	s.SetupChains(context.Background(), nil, func(options *testsuite.ChainOptions) {


configuration remained unchanged

chatton · 2024-06-18T14:05:53Z

e2e/testsuite/testsuite.go

+		channelOpts := defaultChannelOpts(chains)
+		chain1, chain2 := chains[i], chains[i+1]
+
+		if modificationProvider != nil {


provides a mechanism to selectively modify channels when there are more than 2 chains in a test.

provide me modifications, almighty provider! 🙌🏻

chatton · 2024-06-18T14:14:50Z

e2e/testsuite/testsuite.go

 		})
 	}

 	s.startRelayerFn = func(relayer ibc.Relayer) {
+		// depending on the test, the path names will be different.
+		// whenever a relayer is started, it should use the paths associated with the test the relayer is running in.
+		pathNames, ok := s.testPaths[s.T().Name()]


path names only needs to be correct for rly, it is a no-op for hermes

chatton · 2024-06-18T14:15:51Z

e2e/testsuite/testsuite.go

-func (s *E2ETestSuite) GetPathName(idx int64) string {
-	pathName := fmt.Sprintf("%s-path-%d", s.T().Name(), idx)
+func GetPathName(idx int64) string {
+	pathName := fmt.Sprintf("path-%d", idx)


removing the name from the string as s.T().Name() is different depending on the context of the invocation. using an incrementing int will behave as expected everywhere.

chatton · 2024-06-18T14:53:19Z

looks like there are some client tests failing, definitely related will investigate these

gjermundgaraba · 2024-06-18T15:14:54Z

e2e/testsuite/testsuite.go

+
+// SetupSuite will by default create chains with no additional options. If additional options are required,
+// the test suite must define the SetupSuite function and provide the required options.
+func (s *E2ETestSuite) SetupSuite() {


Just to be clear, this means that all tests in one suite will share the same chains, and so we must be careful that accounts are not shared across the tests to avoid account sequence mismatches, resusing balances, etc, etc.

yes that's correct! By default we are always generating addresses that won't collide by default, but we need to keep this in mind in case we end up encountering a new category of error

Yeah, I think that will help a lot with running e2e tests faster, which is a big win!

gjermundgaraba · 2024-06-18T15:20:26Z

e2e/testsuite/testsuite.go

+// GetChannel returns the ibc.ChannelOutput for the current test.
+// this defaults to the first entry in the list, and will be what is needed in the case of
+// a single channel test.
+func (s *E2ETestSuite) GetChannel() ibc.ChannelOutput {


Would it better to rename this to be more precise? Either something like GetChainAChannel or refactor it to take in chain1 and chain2 to get the channel between them? A bit more verbose, but also potentially useful in more contexts?

I guess my lens is a bit skewed with all the forwarding tests, so feel free to ignore :D

Not a bad idea to be explicit here, I think GetChainAChannel would be better.

e2e/testsuite/testsuite.go

damiannolan

LGTM! Really nice improvements made in this PR, love it ❤️

I think we can follow up separately with any of the next steps, would be in favour of merging this sooner than later! 🚢 🚢 🚢

damiannolan · 2024-06-18T15:23:53Z

e2e/tests/upgrades/upgrade_test.go

@@ -223,7 +233,9 @@ func (s *UpgradeTestSuite) TestChainUpgrade() {
 	t := s.T()

 	ctx := context.Background()
-	chain := s.SetupSingleChain(ctx)


I got you brother! :D #3160

damiannolan · 2024-06-18T15:28:45Z

e2e/testsuite/testsuite.go

-		transferVersion = transfertypes.V2
-	}
-
+func (s *E2ETestSuite) FeeMiddlewareChannelOptions() ibc.CreateChannelOptions {


mega nit, I see this was actually named this a long time ago, but would be nice if it was FeeTransferChannelOptions imo!

damiannolan · 2024-06-18T15:43:51Z

e2e/testsuite/testsuite.go

+	// TODO: we need to create a relayer for each test that will run
+	// having a single relayer for all tests will cause issues when running tests in parallel.


Assuming this TODO is directly related to the one onGetRelayer below on this file.

What issues get caused by having a single relayer for tests running in parallel? 🤔 Just curious about the specifics

We can create an issue to follow up on this too ofc! 👍🏻

In lots of tests we don't have a relayer started at the beginning, e.g. when doing a message transfer. So another test who has started a relayer, will cause that relayer to relay packets on a different test causing assertions to fail.

damiannolan · 2024-06-18T15:46:35Z

e2e/testsuite/testsuite.go

+		channelOpts := defaultChannelOpts(chains)
+		chain1, chain2 := chains[i], chains[i+1]
+
+		if modificationProvider != nil {


provide me modifications, almighty provider! 🙌🏻

damiannolan · 2024-06-18T16:00:41Z

I think we should do something about the wasm e2e tests which are failing 🤔

gjermundgaraba

I agree that this would be good to get merged sooner rather than later with follow-up issues - as long as the tests run OK :)

chatton · 2024-06-19T08:02:16Z

sorry if I wasn't clear, the plan was already to do those things in follow ups! Will merge as soon as I have passing tests for all non-wasm E2Es!

…mber-of-runners-used

sonarcloud · 2024-06-19T10:34:54Z

Quality Gate passed for 'ibc-go'

Issues
11 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

…etupTest #1 (#6629) * wip: refactoring tests to create chains in SetupSuite * chore: refactoring memo test to use new setup fns * chore: replace calls to setup chains and relayer with calls to get relayer and get channel * chore: fix wiring in ThreeChainSetup * chore: remove unused fn * chore: add modification function based on chain inputs * chore: update grandpa test with new structure * chore: remove single chain methods * chore: move upgrade tests into different files * chore: fixing client tests * chore: use paths associated with the test * chore: add getter to fetch paths * chore: fix client tests * chore: fix misbehaviour test * chore: fix misbehaviour test * chore: mereg main * chore: pr feedback

chatton added 6 commits June 18, 2024 09:13

wip: refactoring tests to create chains in SetupSuite

fc03da4

chore: refactoring memo test to use new setup fns

4aa71a2

chore: replace calls to setup chains and relayer with calls to get re…

1973a95

…layer and get channel

chore: fix wiring in ThreeChainSetup

465b71e

chore: remove unused fn

d7b7a8c

chore: add modification function based on chain inputs

f567a1b

gjermundgaraba mentioned this pull request Jun 18, 2024

test(transfer): last chain in forwarding packet is ICS20 v1 #6622

Merged

10 tasks

chatton added 4 commits June 18, 2024 12:56

chore: update grandpa test with new structure

b05b7a2

chore: remove single chain methods

cbf4cdc

Merge branch 'main' into cian/issue#5317-e2e-restructure-to-reduce-nu…

37ad10e

…mber-of-runners-used

chore: move upgrade tests into different files

c9ef8db

chatton changed the title ~~Restructure E2Es to setup chain in SetupSuite and create channel in SetupTest~~ [WIP] Restructure E2Es to setup chain in SetupSuite and create channel in SetupTest Jun 18, 2024

chatton marked this pull request as ready for review June 18, 2024 12:33

chatton requested review from colin-axner, AdityaSripal, crodriguezvega, damiannolan, DimitrisJim, gjermundgaraba and bznein as code owners June 18, 2024 12:33

chatton marked this pull request as draft June 18, 2024 12:33

chatton added 2 commits June 18, 2024 14:10

chore: fixing client tests

b552d20

chore: use paths associated with the test

617d059

chatton marked this pull request as ready for review June 18, 2024 14:01

chatton changed the title ~~[WIP] Restructure E2Es to setup chain in SetupSuite and create channel in SetupTest~~ Restructure E2Es to setup chain in SetupSuite and create channel in SetupTest Jun 18, 2024

chatton commented Jun 18, 2024

View reviewed changes

chatton changed the title ~~Restructure E2Es to setup chain in SetupSuite and create channel in SetupTest~~ Restructure E2Es to setup chain in SetupSuite and create channel in SetupTest #1 Jun 18, 2024

chatton commented Jun 18, 2024

View reviewed changes

chore: add getter to fetch paths

03ef304

chatton mentioned this pull request Jun 18, 2024

Configure relayers to watch only channels associated with an individual test #2 #6634

Closed

3 tasks

gjermundgaraba added the priority PRs that need prompt reviews label Jun 18, 2024

chore: fix client tests

421775d

gjermundgaraba reviewed Jun 18, 2024

View reviewed changes

damiannolan approved these changes Jun 18, 2024

View reviewed changes

gjermundgaraba approved these changes Jun 18, 2024

View reviewed changes

chatton added 2 commits June 19, 2024 09:56

chore: fix misbehaviour test

ce31c36

chore: fix misbehaviour test

0af42e9

chatton force-pushed the cian/issue#5317-e2e-restructure-to-reduce-number-of-runners-used branch from 4084011 to 0af42e9 Compare June 19, 2024 08:58

chatton added 4 commits June 19, 2024 09:58

Merge branch 'main' into cian/issue#5317-e2e-restructure-to-reduce-nu…

069a3e8

…mber-of-runners-used

chore: mereg main

db224cd

chore: pr feedback

34942a2

Merge branch 'main' into cian/issue#5317-e2e-restructure-to-reduce-nu…

9aab448

…mber-of-runners-used

chatton enabled auto-merge June 19, 2024 09:49

chatton added this pull request to the merge queue Jun 19, 2024

Merged via the queue into main with commit 10aa0ef Jun 19, 2024
138 of 144 checks passed

chatton deleted the cian/issue#5317-e2e-restructure-to-reduce-number-of-runners-used branch June 19, 2024 10:40

chatton mentioned this pull request Jun 19, 2024

Update Compatibility Matricies with changes from E2E restructuring #6645

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restructure E2Es to setup chain in SetupSuite and create channel in SetupTest #1 #6629

Restructure E2Es to setup chain in SetupSuite and create channel in SetupTest #1 #6629

chatton commented Jun 18, 2024 •

edited

Loading

chatton Jun 18, 2024

chatton Jun 18, 2024

chatton Jun 18, 2024

chatton Jun 18, 2024

damiannolan Jun 18, 2024

chatton Jun 18, 2024

chatton Jun 18, 2024

damiannolan Jun 18, 2024

chatton Jun 18, 2024

chatton Jun 18, 2024

chatton commented Jun 18, 2024

gjermundgaraba Jun 18, 2024

chatton Jun 18, 2024

gjermundgaraba Jun 18, 2024

gjermundgaraba Jun 18, 2024

chatton Jun 18, 2024

damiannolan left a comment

damiannolan Jun 18, 2024

damiannolan Jun 18, 2024

damiannolan Jun 18, 2024

chatton Jun 19, 2024

damiannolan Jun 18, 2024

damiannolan commented Jun 18, 2024

gjermundgaraba left a comment

chatton commented Jun 19, 2024

sonarcloud bot commented Jun 19, 2024

		// TODO: we need to create a relayer for each test that will run
		// having a single relayer for all tests will cause issues when running tests in parallel.

Restructure E2Es to setup chain in SetupSuite and create channel in SetupTest #1 #6629

Restructure E2Es to setup chain in SetupSuite and create channel in SetupTest #1 #6629

Conversation

chatton commented Jun 18, 2024 • edited Loading

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chatton commented Jun 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damiannolan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damiannolan commented Jun 18, 2024

gjermundgaraba left a comment

Choose a reason for hiding this comment

chatton commented Jun 19, 2024

sonarcloud bot commented Jun 19, 2024

Quality Gate passed for 'ibc-go'

chatton commented Jun 18, 2024 •

edited

Loading