Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: begin modularizing e2e tests in preparation for upgrade #1293

Merged
merged 11 commits into from
Apr 23, 2022

Conversation

p0mvn
Copy link
Member

@p0mvn p0mvn commented Apr 19, 2022

Closes: #XXX

What is the purpose of the change

This is a spike related to #1235 to determine a test plan needed for implementing the upgrade.

Currently, there is no way to create genesis or configs such as app.toml for an older version of Osmosis because we directly depend on the branch that executes e2e tests. This is required to implement the logic for testing the upgrade.

This change decouples the functionality that directly depends on the Osmosis codebase from the e2e test runner by creating a chain package that is independent of the e2e package.

By decoupling the logic for configuring the chain (creating genesis + config files), we can use an older Docker image of the Osmosis codebase to configure the chains for the desired version. Then, we may use the created genesis and configs for the old version to execute the upgrade and perform e2e tests from the main branch.

Please note that this PR does not introduce any functional changes and only modularizes and decouples the code to make it possible to add the new upgrade functionality.

Brief change log

  • create the chain package
  • move some utility and io logic that is unrelated to the chain to util package
  • move all remaining logic that is directly dependent on Osmosis codebase to chain package
  • refactor chain package so that:
    • main.go - contains a single Init() function that initializes a chain with 2 validators
      • This function is called from the test setup in e2e package
    • chain.go and validator.go are mostly unchanged and only contain the implementations for Chain and Validator structs respectively
    • util.go is moved to chain/chain_util.go
    • config.go has constants and configuration logic
  • refactor e2e_setup.go to have the following flow in setup:
    • create 2 chains by calling the chain.Init(<id>) method for each
      • the plan is to do this in a Docker container for a specific version of Osmosis in the future PR
    • configure Docker resources
    • run the 2 chains
    • run relayer

Testing and Verifying

This change is a trivial rework / code cleanup without any test coverage.

  • run the e2e tests several times without issues

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 - will be documented when the upgrade implementation is finalized

@codecov-commenter
Copy link

codecov-commenter commented Apr 19, 2022

Codecov Report

Merging #1293 (e3622c2) into main (b6d342a) will decrease coverage by 0.75%.
The diff coverage is 9.32%.

@@            Coverage Diff             @@
##             main    #1293      +/-   ##
==========================================
- Coverage   20.90%   20.14%   -0.76%     
==========================================
  Files         196      203       +7     
  Lines       25425    26824    +1399     
==========================================
+ Hits         5316     5405      +89     
- Misses      19118    20412    +1294     
- Partials      991     1007      +16     
Impacted Files Coverage Δ
x/claim/abci.go 0.00% <ø> (ø)
x/claim/client/cli/query.go 34.45% <ø> (ø)
x/claim/client/cli/tx.go 0.00% <ø> (ø)
x/claim/handler.go 0.00% <ø> (ø)
x/claim/keeper/claim.go 64.33% <ø> (ø)
x/claim/keeper/grpc_query.go 2.50% <ø> (ø)
x/claim/keeper/hooks.go 28.00% <ø> (ø)
x/claim/keeper/keeper.go 87.50% <ø> (ø)
x/claim/keeper/params.go 81.81% <ø> (ø)
x/claim/module.go 58.06% <ø> (ø)
... and 106 more

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 2029415...e3622c2. Read the comment docs.

@p0mvn p0mvn force-pushed the roman/upgrade-genesis branch from a1e8fa7 to ea07f4e Compare April 19, 2022 18:18
@p0mvn p0mvn force-pushed the roman/upgrade-genesis branch from ef744bb to 97211cc Compare April 21, 2022 20:24
@p0mvn p0mvn marked this pull request as ready for review April 21, 2022 20:52
@p0mvn p0mvn requested a review from ValarDragon April 21, 2022 20:53
Copy link
Member

@czarcas7ic czarcas7ic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested this locally as well and all prior tests still pass. Totally agree with pushing chain to a separate library. Great work!

I think the next step is figuring out what changes we need to make to the current osmosis docker image in order to facilitate these upgrade tests. After that, I think the next steps should be more straight forward.

@p0mvn p0mvn merged commit 280479a into main Apr 23, 2022
@p0mvn p0mvn deleted the roman/upgrade-genesis branch April 23, 2022 01:24
mergify bot pushed a commit that referenced this pull request Apr 23, 2022
Closes: #XXX

## What is the purpose of the change

This is a follow-up PR to #1293 and builds upon its work. It is part of the e2e test chain upgrade epic #1235 

This PR introduces the ability to run chain initialization in a Docker container by running the following:
```
docker run -v < path >:/tmp/osmo-test osmosis-e2e-chain-init:debug --data-dir=/tmp/osmo-test
```
All chain data is placed at the given `< path >` that is mounted as a volume on the container. In addition, this PR introduces documentation about the current state of the e2e tests. 

## Brief change log

- [pull chain temp folder creation our of chain to e2e package](c175289)
- [create image and makefile steps to initialize chain state](de89119)
- [allow for running the upgrade initialization in Docker by providing a data dir](dabb683)
- [improve abstractions for chain initialization and add README](cf0d8a3)
- improve README

## Testing and Verifying

- ran e2e tests locally a few times
- `make build-e2e-chain-init`
- `make docker-build-e2e-chain-init`
- `docker run -v /home/roman/cosmos/osmosis/tmp:/tmp/osmo-test osmosis-e2e-chain-init:debug --data-dir=/tmp/osmo-test`

All steps worked as desired

## 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? improved `tests/e2e/README.md`

## Next Steps

The next step is to switch our chain initialization logic in `func (s *IntegrationTestSuite) configureChain(chainId string)` to use Dockertest and the newly introduced container. Then, mount the genesis and configs on the Osmosis containers, against which the e2e tests are executed
@p0mvn p0mvn mentioned this pull request Apr 24, 2022
8 tasks
p0mvn added a commit that referenced this pull request Apr 24, 2022
)

* begin modularizing e2e test in preparation for upgrade

* rename common package to util

* move chain related constants from util to chain package

* fix genesis.go

* exctract initNodes into the genesis package

* remove genesis package, move all logic to chain

* continue cleaning up chain package and refactoring e2e

* store chains in a slice

* reuse common cdc from util package

* lexicographical reorder of functions in config.go of chain package

* clean up names
p0mvn added a commit that referenced this pull request Apr 24, 2022
Closes: #XXX

## What is the purpose of the change

This is a follow-up PR to #1293 and builds upon its work. It is part of the e2e test chain upgrade epic #1235 

This PR introduces the ability to run chain initialization in a Docker container by running the following:
```
docker run -v < path >:/tmp/osmo-test osmosis-e2e-chain-init:debug --data-dir=/tmp/osmo-test
```
All chain data is placed at the given `< path >` that is mounted as a volume on the container. In addition, this PR introduces documentation about the current state of the e2e tests. 

## Brief change log

- [pull chain temp folder creation our of chain to e2e package](c175289)
- [create image and makefile steps to initialize chain state](de89119)
- [allow for running the upgrade initialization in Docker by providing a data dir](dabb683)
- [improve abstractions for chain initialization and add README](cf0d8a3)
- improve README

## Testing and Verifying

- ran e2e tests locally a few times
- `make build-e2e-chain-init`
- `make docker-build-e2e-chain-init`
- `docker run -v /home/roman/cosmos/osmosis/tmp:/tmp/osmo-test osmosis-e2e-chain-init:debug --data-dir=/tmp/osmo-test`

All steps worked as desired

## 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? improved `tests/e2e/README.md`

## Next Steps

The next step is to switch our chain initialization logic in `func (s *IntegrationTestSuite) configureChain(chainId string)` to use Dockertest and the newly introduced container. Then, mount the genesis and configs on the Osmosis containers, against which the e2e tests are executed
@alexanderbez
Copy link
Contributor

Looks like this just moved a few things around to isolate logic in packages with a few tweaks. LGTM.

czarcas7ic added a commit that referenced this pull request Apr 29, 2022
* Setup e2e tests on a single chain; add balances query test (#1193)

* create e2e image and a makefile step to build

* progress

* e2e tests in ci

* use root distroless image and correct volume path

* remove chain b references

* implement query balances

* implement TestQueryBalances

* trigger worflow

* trigger

* test-e2e Makefile step

* fmt and sleep if service unavailable

* README

* restore branches

* add changelog entry

* exclude e2e from regular tests

* -E flag for grep exclusion

* grep

* go mod tidy --compat=1.17

* manually tidy go.mod

* second e2e chain with expanded test (#1206)

* e2e with IBC tx and test (#1216)

* second chain with tests

* add hermes, comment out balance query for now

* eventually

* Update Makefile

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

* remove gas fees and unused functions

* readded check

* Nicco changes to own hermes image

* Remove unused hermes.Dockerfile

* Use a single Dockerfile for both debug and official image

* readd build hermes in makefile

* remove hermes

* Set correct golang image and use correct debug image tag

Co-authored-by: Adam Tucker <[email protected]>
Co-authored-by: Roman <[email protected]>
Co-authored-by: Niccolo Raspa <[email protected]>

* missing e2e ci job

* cleanup Makefile and ci workflows (#1203)

* cleanup makefile and ci workflows

* update changelog

* fix sim test

* fix makefile and rename docker repo

* fix make test-cover (#1219)

* refactor: begin modularizing e2e tests in preparation for upgrade (#1293)

* begin modularizing e2e test in preparation for upgrade

* rename common package to util

* move chain related constants from util to chain package

* fix genesis.go

* exctract initNodes into the genesis package

* remove genesis package, move all logic to chain

* continue cleaning up chain package and refactoring e2e

* store chains in a slice

* reuse common cdc from util package

* lexicographical reorder of functions in config.go of chain package

* clean up names

* refactor: implement Dockerized chain initialization in e2e tests (#1330)

Closes: #XXX

## What is the purpose of the change

This is a follow-up PR to #1293 and builds upon its work. It is part of the e2e test chain upgrade epic #1235 

This PR introduces the ability to run chain initialization in a Docker container by running the following:
```
docker run -v < path >:/tmp/osmo-test osmosis-e2e-chain-init:debug --data-dir=/tmp/osmo-test
```
All chain data is placed at the given `< path >` that is mounted as a volume on the container. In addition, this PR introduces documentation about the current state of the e2e tests. 

## Brief change log

- [pull chain temp folder creation our of chain to e2e package](c175289)
- [create image and makefile steps to initialize chain state](de89119)
- [allow for running the upgrade initialization in Docker by providing a data dir](dabb683)
- [improve abstractions for chain initialization and add README](cf0d8a3)
- improve README

## Testing and Verifying

- ran e2e tests locally a few times
- `make build-e2e-chain-init`
- `make docker-build-e2e-chain-init`
- `docker run -v /home/roman/cosmos/osmosis/tmp:/tmp/osmo-test osmosis-e2e-chain-init:debug --data-dir=/tmp/osmo-test`

All steps worked as desired

## 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? improved `tests/e2e/README.md`

## Next Steps

The next step is to switch our chain initialization logic in `func (s *IntegrationTestSuite) configureChain(chainId string)` to use Dockertest and the newly introduced container. Then, mount the genesis and configs on the Osmosis containers, against which the e2e tests are executed

* Setup e2e tests on a single chain; add balances query test (#1193)

* create e2e image and a makefile step to build

* progress

* e2e tests in ci

* use root distroless image and correct volume path

* remove chain b references

* implement query balances

* implement TestQueryBalances

* trigger worflow

* trigger

* test-e2e Makefile step

* fmt and sleep if service unavailable

* README

* restore branches

* add changelog entry

* exclude e2e from regular tests

* -E flag for grep exclusion

* grep

* go mod tidy --compat=1.17

* manually tidy go.mod

Co-authored-by: Adam Tucker <[email protected]>
Co-authored-by: Adam Tucker <[email protected]>
Co-authored-by: Niccolo Raspa <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants