-
Notifications
You must be signed in to change notification settings - Fork 607
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
test: switch e2e test setup to create genesis and configs via Dockertest #1363
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work on this @czarcas7ic ! Left some comments, please take a look
Co-authored-by: Roman <[email protected]>
Co-authored-by: Roman <[email protected]>
Co-authored-by: Roman <[email protected]>
Co-authored-by: Roman <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #1363 +/- ##
=======================================
Coverage 19.53% 19.53%
=======================================
Files 200 200
Lines 27532 27532
=======================================
Hits 5377 5377
Misses 21153 21153
Partials 1002 1002 Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
// generate genesis txs | ||
genTxs := make([]json.RawMessage, len(c.Validators)) | ||
for i, val := range c.Validators { | ||
genTxs := make([]json.RawMessage, len(c.validators)) | ||
for i, val := range c.validators { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not relevant to this PR, but I think we should extract this into a separate function, and add some more docs on whats going on with stakeAmountCoin, its non-obvious to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I believe this is just adding the initial self-bond amount to each validator IIRC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent work @czarcas7ic
// generate genesis txs | ||
genTxs := make([]json.RawMessage, len(c.Validators)) | ||
for i, val := range c.Validators { | ||
genTxs := make([]json.RawMessage, len(c.validators)) | ||
for i, val := range c.validators { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I believe this is just adding the initial self-bond amount to each validator IIRC.
@@ -0,0 +1,25 @@ | |||
package chain |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a doc.go
on this package, export
? What is it's purpose/intended use-case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We marshal the data into a json within the docker container in order to later retrieve this data by the e2e test suite. When I initually tried to unmarshal this data however, I ran into an issue where the marshaler did not know how to handle some sdk structs that dont get exported. @p0mvn created this export package in order to export only the required structs needed to initialize the genesis and configs in order to get around this issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a README explaining what this package is for:
https://github.com/osmosis-labs/osmosis/blob/main/tests/e2e/README.md
@czarcas7ic could you check if README needs to be updated after this PR please
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can probably add to it by saying that the e2e test runner now does chain initialization in the container by default.
Other updates I noticed:
- there is no
upgrade
package anymore, it was renamed earlier - state marshaling and unmarshaling on the volume
Co-authored-by: Dev Ojha <[email protected]>
Co-authored-by: Aleksandr Bezobchuk <[email protected]>
Closes: #1332
What is the purpose of the change
This pull request utilizes the modularized e2e framework created by @p0mvn and makes it now possible to create a genesis and configuration based off a prior osmosis version. This is a required step in order to automate upgrade testing. Additionally, this PR was done in conjunction with @p0mvn.
Brief change log
ChainMeta
andinternalChain
structsexport.go
to assist in json unmarshallingmake docker-build-e2e-chain-init
Testing and Verifying
This change is covered by the e2e test already implemented in this repository. This was also tested locally and continues to pass.
Documentation and Release Note
Unreleased
section inCHANGELOG.md
? yes