e2e with IBC tx and test #1216

czarcas7ic · 2022-04-07T16:45:47Z

Closes: #1201

Description

Please note, a majority of this is the original work of @alexanderbez and this is a simple port over from his work on gaia to osmosis.

Added IBC functionality through hermes and added a single IBC test to ensure the tx was received on chainB from chainA
Changed from name to id as requested by Dev
Commented out TestQueryBalance. This is because we are currently relying on the order of tests for this test to pass. As we develop further understanding of how we want these tests to be ran, we can re-enable this and/or use it elsewhere.

For contributor use:

Targeted PR against correct branch (see CONTRIBUTING.md)
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer

p0mvn

Great work on this. Left some comments, please take a look

Also, noticed that fmt is still complaining. We can run it from the tests/e2e folder to make sure that it's applied

Makefile

tests/e2e/docker/hermes.Dockerfile

p0mvn · 2022-04-07T17:36:15Z

tests/e2e/e2e_setup_test.go

+	s.hermesResource, err = s.dkrPool.RunWithOptions(
+		&dockertest.RunOptions{
+			Name:       fmt.Sprintf("%s-%s-relayer", s.chainA.id, s.chainB.id),
+			Repository: "ghcr.io/cosmos/hermes-e2e",


It looks like we are depending on an image that is not under our control. Let's push the image to our own repository and switch to using that

@nikever do you know how to add this as an image to our repo? I'm not positive of the process

To avoid blocking this PR, let's create an issue to address this separately?

It should be fine to use the cosmos image temporarily, The main concern about depending on external images, especially with the latest tag, is that whenever they make a change, our tests might get broken

The credentials for the osmobot Docker account are saved as GitHub Secrets. You can use them to build the Image via CI as in https://github.com/osmosis-labs/osmosis/blob/main/.github/workflows/docker.yml#L24-L29

I will also create personal accounts for both of you on our Docker account.

Lets use a new repository osmolabs/osmosis-e2e for all these new images

p0mvn · 2022-04-07T17:37:19Z

Makefile

@@ -238,6 +238,11 @@ benchmark:
 docker-build-debug:
 	@docker build -t osmolabs/osmosisd-e2e --build-arg IMG_TAG=debug -f e2e.Dockerfile .

+# TODO: Push this to the Cosmos Dockerhub so we don't have to keep building it


It looks like we are already not building this image in ci. Instead, we grab it from here: https://github.com/orgs/cosmos/packages/container/package/hermes-e2e

I think we should push the image to our own docker hub or github packages and use that instead

@nikever can we link on getting this on our docker hub as well?

I would also like to learn about how to get access to our docker hub. Please let me know if it's possible to get permissions for that

Yes, good idea. It's very easy to push to our org. All you need is a github PAT (personal access token) and then you login with that and just push it. Note, just make sure the package set to public.

tests/e2e/e2e_util_test.go

tests/e2e/scripts/hermes_bootstrap.sh

tests/e2e/e2e_test.go

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

tests/e2e/e2e_test.go

p0mvn

Please restore commented-out lines in TestQueryBalances. Other than that LGTM!

alexanderbez

🎉 🎉 🎉

alexanderbez · 2022-04-07T22:37:18Z

Makefile

@@ -238,6 +238,11 @@ benchmark:
 docker-build-debug:
 	@docker build -t osmolabs/osmosisd-e2e --build-arg IMG_TAG=debug -f e2e.Dockerfile .

+# TODO: Push this to the Cosmos Dockerhub so we don't have to keep building it


Yes, good idea. It's very easy to push to our org. All you need is a github PAT (personal access token) and then you login with that and just push it. Note, just make sure the package set to public.

alexanderbez · 2022-04-07T22:38:45Z

tests/e2e/e2e_setup_test.go

+	s.Require().NoError(err)
+
+	endpoint := fmt.Sprintf("http://%s/state", s.hermesResource.GetHostPort("3031/tcp"))
+	s.Require().Eventually(


Validators? You mean the node containers themselves? I believe we already have a health check for that. Are you suggesting that we implement a test that queries for validators?

tests/e2e/e2e_test.go

codecov-commenter · 2022-04-08T00:57:54Z

Codecov Report

Merging #1216 (826bad4) into main (eed3294) will decrease coverage by 0.06%.
The diff coverage is 25.80%.

@@            Coverage Diff             @@
##             main    #1216      +/-   ##
==========================================
- Coverage   21.03%   20.96%   -0.07%     
==========================================
  Files         196      196              
  Lines       25349    25337      -12     
==========================================
- Hits         5332     5312      -20     
- Misses      19054    19066      +12     
+ Partials      963      959       -4

Impacted Files	Coverage Δ
x/gamm/keeper/pool.go	`64.91% <0.00%> (ø)`
x/gamm/pool-models/balancer/balancer_pool.go	`62.06% <ø> (-1.82%)`	⬇️
x/gamm/types/pool.go	`0.00% <ø> (ø)`
x/gamm/keeper/pool_service.go	`56.61% <23.07%> (-0.61%)`	⬇️
x/gamm/keeper/invariants.go	`29.41% <29.41%> (+29.41%)`	⬆️
x/gamm/client/cli/query.go	`37.09% <0.00%> (-1.59%)`	⬇️
x/gamm/types/tx.pb.go	`0.66% <0.00%> (ø)`
x/gamm/types/query.pb.go	`0.85% <0.00%> (ø)`
x/gamm/keeper/grpc_query.go	`42.20% <0.00%> (ø)`

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 e9cf9f6...826bad4. Read the comment docs.

czarcas7ic · 2022-04-08T15:45:52Z

Had a call with @nikever, he was able to get our own hosted hermes image and integrated it into the e2e test. He will now attempt a small modification on the gh action and after he pushes this we can merge this pr

Makefile

tests/e2e/docker/hermes.Dockerfile

tests/e2e/e2e_setup_test.go

niccoloraspa · 2022-04-08T17:40:07Z

e2e running correctly with the last changes: https://github.com/osmosis-labs/osmosis/runs/5889286653?check_suite_focus=true

🚀🚀

p0mvn

🚢

adizere · 2022-04-15T12:36:27Z

tests/e2e/e2e_util_test.go

+		User:         "root",
+		Cmd: []string{
+			"hermes",
+			"create",


Note that we plan to change the create channel invocation in Hermes. From

create channel <CHAIN_A> <CHAIN_B> --port-a=transfer --port-b=transfert

to

create channel <CHAIN-A-ID> <CONNECTION-A-ID> --port-a <PORT-ID> --port-b <PORT-ID>.

In other words, you need to (1) create connection and then (2) pass the connection identifier when you invoke create channel. This mean you would need to adapt the CI to add an additional CLI command for creating a new connection, as follows:

create connection s.chainA.id s.chainB.id.

Assuming the chains are newly-spawned, the connection identifier will be connection-0 on both chains. Then you would need to invoke create channel as follows:

create channel s.chainA.id connection-0 --port-a=transfer --port-b=transfer

PS. This change was motivated by relayer operators abusing the command (we got complaints that people create many channels in production with varying client security parameters, ref: informalsystems/hermes#1421).

Would be glad to take your requirements into account and do stuff differently if you have suggestions to change this.

This seems like a great change to me! Thanks for letting us know / good design decision!

Great to hear it won't affect your setup adversely.

The change will land in Hermes v0.14, slated for end of April.

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

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

Adam Tucker added 4 commits April 6, 2022 02:46

second chain with tests

13027cc

add hermes, comment out balance query for now

e1d35e9

Merge branch 'main' into adam/e2e-test-ibc

a0007c5

eventually

625d894

czarcas7ic requested review from alexanderbez and p0mvn April 7, 2022 16:45

p0mvn requested changes Apr 7, 2022

View reviewed changes

czarcas7ic and others added 2 commits April 7, 2022 14:42

Update Makefile

6311466

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

remove gas fees and unused functions

bf33585

p0mvn reviewed Apr 7, 2022

View reviewed changes

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

p0mvn reviewed Apr 7, 2022

View reviewed changes

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

p0mvn approved these changes Apr 7, 2022

View reviewed changes

alexanderbez approved these changes Apr 7, 2022

View reviewed changes

alexanderbez added the T:tests label Apr 7, 2022

readded check

bf51d99

Nicco changes to own hermes image

4b91fd4

p0mvn requested changes Apr 8, 2022

View reviewed changes

Makefile Outdated Show resolved Hide resolved

tests/e2e/docker/hermes.Dockerfile Outdated Show resolved Hide resolved

tests/e2e/e2e_setup_test.go Show resolved Hide resolved

Niccolo Raspa and others added 5 commits April 8, 2022 18:16

Remove unused hermes.Dockerfile

9442265

Use a single Dockerfile for both debug and official image

af9ed4a

readd build hermes in makefile

c38c1c6

remove hermes

d0b3705

Set correct golang image and use correct debug image tag

826bad4

p0mvn approved these changes Apr 8, 2022

View reviewed changes

czarcas7ic merged commit 20cfa05 into main Apr 8, 2022

czarcas7ic deleted the adam/e2e-test-ibc branch April 8, 2022 17:43

ValarDragon mentioned this pull request Apr 14, 2022

End-to-end IBC tests #672

Closed

adizere reviewed Apr 15, 2022

View reviewed changes

p0mvn mentioned this pull request Apr 24, 2022

backport: e2e tests (#1193) #1338

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

e2e with IBC tx and test #1216

e2e with IBC tx and test #1216

czarcas7ic commented Apr 7, 2022

p0mvn left a comment •

edited

Loading

p0mvn Apr 7, 2022

czarcas7ic Apr 7, 2022

p0mvn Apr 7, 2022

niccoloraspa Apr 7, 2022

p0mvn Apr 7, 2022

czarcas7ic Apr 7, 2022

p0mvn Apr 7, 2022

alexanderbez Apr 7, 2022

p0mvn left a comment

alexanderbez left a comment

alexanderbez Apr 7, 2022

alexanderbez Apr 7, 2022

codecov-commenter commented Apr 8, 2022 •

edited

Loading

czarcas7ic commented Apr 8, 2022

niccoloraspa commented Apr 8, 2022

p0mvn left a comment

adizere Apr 15, 2022

adizere Apr 15, 2022

ValarDragon Apr 15, 2022

adizere Apr 15, 2022

e2e with IBC tx and test #1216

e2e with IBC tx and test #1216

Conversation

czarcas7ic commented Apr 7, 2022

Description

p0mvn 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

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

p0mvn left a comment

Choose a reason for hiding this comment

alexanderbez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Apr 8, 2022 • edited Loading

Codecov Report

czarcas7ic commented Apr 8, 2022

niccoloraspa commented Apr 8, 2022

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

p0mvn left a comment •

edited

Loading

codecov-commenter commented Apr 8, 2022 •

edited

Loading