test: more e2e upgrade testing #1458

czarcas7ic · 2022-05-10T03:17:05Z

Completes all required tasks for: #1414

What is the purpose of the change

Adds further functionality to the e2e and upgrade test framework.

Brief Changelog

Created IBC send and pool creation functions
- These function are called pre upgrade to create state and post upgrade to test if it still works as intended
Adds four pool json files required for pool creation command
Submit, deposit, vote, chain height, and query utility functions created
Now required code 0 (success) from chain. Before only checked that the commands were sent successfully to the docker container which is only half the picture.
Modified the ibc send test so that it will pass regardless of what order it is called in.
Bug fixes to allow for a variety of validator combinations

Testing and Verifying

This change is already covered by the existing e2e test framework and expands upon it. It was also tested locally.

Documentation and Release Note

Does this pull request introduce a new feature or user-facing behavior changes? no
Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? not yet

Co-authored-by: Roman <[email protected]>

codecov-commenter · 2022-05-10T03:31:54Z

Codecov Report

Merging #1458 (ddc5137) into main (f149135) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1458   +/-   ##
=======================================
  Coverage   19.50%   19.50%           
=======================================
  Files         231      231           
  Lines       31527    31527           
=======================================
  Hits         6150     6150           
  Misses      24251    24251           
  Partials     1126     1126

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f149135...ddc5137. Read the comment docs.

tests/e2e/chain/config.go

tests/e2e/e2e_setup_test.go

Co-authored-by: Aleksandr Bezobchuk <[email protected]>

p0mvn

Awesome job. Left some comments, please take a look.

The biggest question I have right now is why do we have to do certain things concurrently between chains? Why does it matter?

tests/e2e/scripts/pool1A.json

tests/e2e/e2e_setup_test.go

tests/e2e/e2e_util_test.go

tests/e2e/e2e_setup_test.go

p0mvn · 2022-05-10T19:34:24Z

tests/e2e/e2e_setup_test.go

 	// submit, deposit, and vote for upgrade proposal
-	s.submitProposal(s.chains[0])
-	s.submitProposal(s.chains[1])
+	currentHeightA := s.chainHeight(s.valResources[s.chains[0].ChainMeta.Id][0].Container.ID)


Can we instead move all these calls to a for loop? We can iterate over all chains and do these actions instead of duplicating calls for every chain

Can you point out which lines you are referring to?

https://github.com/osmosis-labs/osmosis/pull/1458/files/dace939d3b2461b62fbd5c407393b6905339a21d#r869717359

p0mvn · 2022-05-10T19:35:47Z

tests/e2e/e2e_setup_test.go

 		}
 	}
+	// upgrade to locally compiled binary concurently
+	wg.Add(2)


Why do we have to do this concurrently?

Also, we should avoid hardcoding and duplicating calls

2 == len(s.chains)

and go s.upgradeContainers(s.chains[0], &wg) can be done in a loop

Same as before, if we have to upgrade 200 containers I would rather them be done at the same time instead of waiting for each to be finished since they are not dependent on each other.

Similar idea to: https://github.com/osmosis-labs/osmosis/pull/1458/files/dace939d3b2461b62fbd5c407393b6905339a21d#r869717359

tests/e2e/e2e_test.go

czarcas7ic

Thank you so much for the thorough review @p0mvn ! Just had a few questions, the others I will address now!

tests/e2e/e2e_setup_test.go

czarcas7ic · 2022-05-10T21:10:52Z

tests/e2e/e2e_setup_test.go

 	// submit, deposit, and vote for upgrade proposal
-	s.submitProposal(s.chains[0])
-	s.submitProposal(s.chains[1])
+	currentHeightA := s.chainHeight(s.valResources[s.chains[0].ChainMeta.Id][0].Container.ID)


Can you point out which lines you are referring to?

tests/e2e/e2e_setup_test.go

czarcas7ic · 2022-05-10T21:17:04Z

tests/e2e/e2e_setup_test.go

+					if currentHeight == propHeight {
+						counter++
+					}
+					return counter == 3


The purpose of this was to return True 3 times before accepting it as fact. For instance if upgrade height is 72 its possible to return 72 twice and still hit 73 but 3 seconds is too long to hit the same block 3 times in a row without it having been stopped for upgrade. But this is now moot since we will use a hard chain halt height as suggested by you in your previous comment

czarcas7ic · 2022-05-10T21:18:05Z

tests/e2e/e2e_setup_test.go

 		}
 	}
+	// upgrade to locally compiled binary concurently
+	wg.Add(2)


Same as before, if we have to upgrade 200 containers I would rather them be done at the same time instead of waiting for each to be finished since they are not dependent on each other.

tests/e2e/e2e_test.go

tests/e2e/e2e_util_test.go

Co-authored-by: Roman <[email protected]>

p0mvn · 2022-05-10T22:10:28Z

tests/e2e/e2e_setup_test.go

+	currentHeightA := s.chainHeight(s.valResources[s.chains[0].ChainMeta.Id][0].Container.ID)
+	currentHeightB := s.chainHeight(s.valResources[s.chains[1].ChainMeta.Id][0].Container.ID)
+	propHeightA = currentHeightA + 40 + len(s.chains[0].Validators)
+	propHeightB = currentHeightB + 40 + len(s.chains[1].Validators)
+
+	s.submitProposal(s.chains[0], propHeightA)
+	s.submitProposal(s.chains[1], propHeightB)
 	s.depositProposal(s.chains[0])
 	s.depositProposal(s.chains[1])


Suggested change

currentHeightA := s.chainHeight(s.valResources[s.chains[0].ChainMeta.Id][0].Container.ID)

currentHeightB := s.chainHeight(s.valResources[s.chains[1].ChainMeta.Id][0].Container.ID)

propHeightA = currentHeightA + 40 + len(s.chains[0].Validators)

propHeightB = currentHeightB + 40 + len(s.chains[1].Validators)

s.submitProposal(s.chains[0], propHeightA)

s.submitProposal(s.chains[1], propHeightB)

s.depositProposal(s.chains[0])

s.depositProposal(s.chains[1])

for _, chain := range chains {

currentHeight := s.chainHeight(s.valResources[chain.ChainMeta.Id][0].Container.ID)

propHeight := currentHeight + 40 + len(chain.Validators)

s.submitProposal(chain, propHeight)

s.depositProposal(chain)

}

czarcas7ic · 2022-05-12T19:11:20Z

@alexanderbez @p0mvn I addressed a majority of your comments, however one or two might not be addressed since I re-arranged how some of works now.

The most drastic of changes is that IBC sends now continually send until a success code is received. Additionally, there is a check to ensure that IBC send actually arrives on the destination chain before continuing. This has mitigated all issues I was having with account sequence mismatching. Please let me know what you think when you have the time.

alexanderbez · 2022-05-13T15:43:06Z

tests/e2e/chain/config.go

@@ -130,7 +133,7 @@ func addAccount(path, moniker, amountStr string, accAddr sdk.AccAddress) error {
 	return genutil.ExportGenesisFile(genDoc, genFile)
 }

-func initGenesis(c *internalChain) error {
+func initGenesis(c *internalChain, votingPeriod time.Duration) error {


In the future, this can get messy. We should revise this such that we can instead pass the entire module genesis state. But for now, this is OK.

czarcas7ic and others added 11 commits May 5, 2022 21:37

chain init changes

1dacf98

introduce val init options

ce4415d

Apply suggestions from code review

b66a0a1

Co-authored-by: Roman <[email protected]>

chain init change

383f2a4

using new struc

d857d9d

Merge branch 'main' into adam/init-changes-2

1b3488e

Update tests/e2e/e2e_setup_test.go

0d90bab

Co-authored-by: Roman <[email protected]>

lint change

afb2189

address roman comments

eaceee2

move comments to chain package

ff312db

new e2e upgrade features

78b2687

czarcas7ic requested a review from a team May 10, 2022 03:17

Merge branch 'main' into adam/e2e-changes-2

9cbea1a

pool file permissions fix

5b50edb

czarcas7ic requested review from alexanderbez, p0mvn and ValarDragon May 10, 2022 03:58

alexanderbez reviewed May 10, 2022

View reviewed changes

tests/e2e/chain/config.go Outdated Show resolved Hide resolved

tests/e2e/chain/config.go Outdated Show resolved Hide resolved

tests/e2e/e2e_setup_test.go Outdated Show resolved Hide resolved

tests/e2e/e2e_setup_test.go Outdated Show resolved Hide resolved

czarcas7ic and others added 3 commits May 10, 2022 14:07

Apply suggestions from code review

f17c560

Co-authored-by: Aleksandr Bezobchuk <[email protected]>

edits from code review

9f29989

Merge branch 'main' into adam/e2e-changes-2

dace939

p0mvn requested changes May 10, 2022

View reviewed changes

czarcas7ic commented May 10, 2022

View reviewed changes

Apply suggestions from code review

d8ca1ae

Co-authored-by: Roman <[email protected]>

p0mvn reviewed May 10, 2022

View reviewed changes

alexanderbez self-assigned this May 11, 2022

czarcas7ic added 3 commits May 12, 2022 18:34

e2e test rework

08177e0

increase destination time check

b020d98

Merge branch 'main' into adam/e2e-changes-2

81ff7ef

czarcas7ic requested review from p0mvn and alexanderbez May 12, 2022 19:08

alexanderbez reviewed May 13, 2022

View reviewed changes

alexanderbez approved these changes May 13, 2022

View reviewed changes

Merge branch 'main' into adam/e2e-changes-2

97858da

ValarDragon added the A:automerge label May 16, 2022

p0mvn removed the A:automerge label May 16, 2022

czarcas7ic added 2 commits May 16, 2022 16:18

change upgrade from v8 to v9

c4f55df

Merge branch 'main' into adam/e2e-changes-2

176344d

p0mvn approved these changes May 16, 2022

View reviewed changes

Merge branch 'main' into adam/e2e-changes-2

ddc5137

czarcas7ic merged commit 9463346 into main May 16, 2022

czarcas7ic deleted the adam/e2e-changes-2 branch May 16, 2022 16:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: more e2e upgrade testing #1458

test: more e2e upgrade testing #1458

czarcas7ic commented May 10, 2022 •

edited

Loading

codecov-commenter commented May 10, 2022 •

edited

Loading

p0mvn left a comment

p0mvn May 10, 2022

czarcas7ic May 10, 2022

p0mvn May 10, 2022

p0mvn May 10, 2022

czarcas7ic May 10, 2022

p0mvn May 10, 2022

czarcas7ic left a comment •

edited

Loading

czarcas7ic May 10, 2022

czarcas7ic May 10, 2022

czarcas7ic May 10, 2022

p0mvn May 10, 2022

czarcas7ic commented May 12, 2022 •

edited

Loading

alexanderbez May 13, 2022

test: more e2e upgrade testing #1458

test: more e2e upgrade testing #1458

Conversation

czarcas7ic commented May 10, 2022 • edited Loading

What is the purpose of the change

Brief Changelog

Testing and Verifying

Documentation and Release Note

codecov-commenter commented May 10, 2022 • edited Loading

Codecov Report

p0mvn 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

Choose a reason for hiding this comment

czarcas7ic left a comment • edited Loading

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

czarcas7ic commented May 12, 2022 • edited Loading

Choose a reason for hiding this comment

czarcas7ic commented May 10, 2022 •

edited

Loading

codecov-commenter commented May 10, 2022 •

edited

Loading

czarcas7ic left a comment •

edited

Loading

czarcas7ic commented May 12, 2022 •

edited

Loading