refactor: e2e setup to be more extensible for state sync #1565

p0mvn · 2022-05-23T21:10:33Z

Closes: #XXX

What is the purpose of the change

This PR refactors e2e test setup to have an abstraction chainConfig that has information about which validators should not be run during initialization. This is a first step toward testing state-sync as we want to postpone running some nodes so that we can test nodes "catching up".

Additionally, this PR extracts a separate package docker for managing and storing all information related to docker images.

Brief Changelog

create a chainConfig struct to encapsulate all configurations related to chains in a single abstraction
- remove global variables propHeightA, propHeightB, votingPeriodA, votingPeriodB. Instead, these are part of the chainConfig struct
- skipRunValidatorIndexes - this is needed to skip certain validators from being run during setup so that we can test state-sync post-initialization and upgrade
remove code duplication for every chain. Instead, always loop over chainConfigs so that if we add more chains later, the setup would work out of the box

Testing and Verifying

tested running both with and without upgrade

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? no
How is the feature or change documented? not applicable

codecov-commenter · 2022-05-23T21:17:15Z

Codecov Report

Merging #1565 (3ddd10a) into main (8aaa84b) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1565   +/-   ##
=======================================
  Coverage   19.47%   19.47%           
=======================================
  Files         242      242           
  Lines       32258    32258           
=======================================
  Hits         6282     6282           
  Misses      24822    24822           
  Partials     1154     1154

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 8aaa84b...3ddd10a. Read the comment docs.

czarcas7ic

Great work, and thank you for cleaning up some of my the code mess I made!

czarcas7ic · 2022-05-24T14:32:59Z

tests/e2e/docker/image_config.go

+	// It is used when skipping upgrade by setting OSMOSIS_E2E_SKIP_UPGRADE to true).
+	// This image should be pre-built with `make docker-build-debug` either in CI or locally.
+	LocalOsmoRepository = "osmosis"
+	LocalOsmoTag        = "debug"


Can you explain why this value gets exported but the others dont?

Because we need to reuse it outside of the package for the upgrade:

osmosis/tests/e2e/e2e_setup_test.go

Line 553 in 3ddd10a

Repository: dockerconfig.LocalOsmoRepository,

czarcas7ic · 2022-05-24T14:36:13Z

tests/e2e/e2e_setup_test.go

+	// Run a relayer between every possible pair of chains.
+	for i := 0; i < len(s.chainConfigs); i++ {
+		for j := i + 1; j < len(s.chainConfigs); j++ {
+			s.runIBCRelayer(s.chainConfigs[i].chain, s.chainConfigs[j].chain)


Do you think this abstraction is worth it? There might be some benefit to reducing the scope of end to end tests to a max of two chains. With that being said, totally open to expanding the scope

I was thinking of still having the 2 chains but aiming to design for extensibility. So that if we want to add a third chain for any reason, like testing some ibc functionality in the future, it works out of the box.

The primary reason for this refactor though is to just encapsulate more "chain-specific" data under the same abstraction. That allows me to add more methods on that abstraction related to state-sync. This particular change is secondary and doesn't add more value to the end goal

czarcas7ic · 2022-05-24T14:39:45Z

tests/e2e/e2e_setup_test.go

-	s.createPool(s.chains[0], "pool1A.json")
-	s.createPool(s.chains[1], "pool1B.json")
+	chainA := s.chainConfigs[0].chain
+	chainB := s.chainConfigs[1].chain


Love this, makes it way easier to think of instead of using chain[0] as chain A

p0mvn · 2022-05-24T14:47:47Z

Thanks for the speedy review @czarcas7ic . Let me know if I can help rebasing your change against this one

Closes: #XXX This PR refactors e2e test setup to have an abstraction `chainConfig` that has information about which validators should not be run during initialization. This is a first step toward testing state-sync as we want to postpone running some nodes so that we can test nodes "catching up". Additionally, this PR extracts a separate package `docker` for managing and storing all information related to docker images. - create a `chainConfig` struct to encapsulate all configurations related to chains in a single abstraction * remove global variables `propHeightA`, `propHeightB`, `votingPeriodA`, `votingPeriodB`. Instead, these are part of the `chainConfig` struct * `skipRunValidatorIndexes` - this is needed to skip certain validators from being run during setup so that we can test state-sync post-initialization and upgrade - remove code duplication for every chain. Instead, always loop over `chainConfig`s so that if we add more chains later, the setup would work out of the box - tested running both with and without upgrade - 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`? no - How is the feature or change documented? not applicable

* change to osmo no stake * currently working * superfluid vote overwrite test * change docker file * refactor: e2e setup to be more extensible for state sync (#1565) Closes: #XXX This PR refactors e2e test setup to have an abstraction `chainConfig` that has information about which validators should not be run during initialization. This is a first step toward testing state-sync as we want to postpone running some nodes so that we can test nodes "catching up". Additionally, this PR extracts a separate package `docker` for managing and storing all information related to docker images. - create a `chainConfig` struct to encapsulate all configurations related to chains in a single abstraction * remove global variables `propHeightA`, `propHeightB`, `votingPeriodA`, `votingPeriodB`. Instead, these are part of the `chainConfig` struct * `skipRunValidatorIndexes` - this is needed to skip certain validators from being run during setup so that we can test state-sync post-initialization and upgrade - remove code duplication for every chain. Instead, always loop over `chainConfig`s so that if we add more chains later, the setup would work out of the box - tested running both with and without upgrade - 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`? no - How is the feature or change documented? not applicable * add extract oper address * extract val addr * new line * delete no longer needed script * Delete .bash_history * add helper functions to declutter * remove majority boilerplate * move docker file back to distroless * no longer use bash to add account * remove debugging print statements * Delete .bash_history * remove extra line in docker file * Apply suggestions from code review Co-authored-by: Roman <[email protected]> * code review suggestions * further validator abstractions Co-authored-by: Roman <[email protected]>

refactor: e2e setup to be more extensible for state sync

10ed742

p0mvn added 8 commits May 23, 2022 17:37

add validator that should be skipped in initialization

3332efe

remove TestStateSync

74bffb1

skipping log

a3ee7f9

skip validators from voting on proposals

e5ee0b9

skip validator in upgrade

7ff6f61

skip validators in upgrade

b3a8aaf

skip in upgrade

c3f2385

initialize less resources

3ddd10a

p0mvn marked this pull request as ready for review May 23, 2022 23:16

p0mvn requested a review from a team May 23, 2022 23:16

czarcas7ic approved these changes May 24, 2022

View reviewed changes

p0mvn added the A:automerge label May 24, 2022

mergify bot merged commit fc2c105 into main May 24, 2022

mergify bot deleted the roman/e2e-setup-refactor branch May 24, 2022 15:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: e2e setup to be more extensible for state sync #1565

refactor: e2e setup to be more extensible for state sync #1565

p0mvn commented May 23, 2022 •

edited

Loading

codecov-commenter commented May 23, 2022 •

edited

Loading

czarcas7ic left a comment

czarcas7ic May 24, 2022

p0mvn May 24, 2022

czarcas7ic May 24, 2022

p0mvn May 24, 2022

czarcas7ic May 24, 2022

p0mvn commented May 24, 2022

refactor: e2e setup to be more extensible for state sync #1565

refactor: e2e setup to be more extensible for state sync #1565

Conversation

p0mvn commented May 23, 2022 • edited Loading

What is the purpose of the change

Brief Changelog

Testing and Verifying

Documentation and Release Note

codecov-commenter commented May 23, 2022 • edited Loading

Codecov Report

czarcas7ic left a comment

Choose a reason for hiding this comment

czarcas7ic May 24, 2022

Choose a reason for hiding this comment

p0mvn May 24, 2022

Choose a reason for hiding this comment

czarcas7ic May 24, 2022

Choose a reason for hiding this comment

p0mvn May 24, 2022

Choose a reason for hiding this comment

czarcas7ic May 24, 2022

Choose a reason for hiding this comment

p0mvn commented May 24, 2022

p0mvn commented May 23, 2022 •

edited

Loading

codecov-commenter commented May 23, 2022 •

edited

Loading