test: upgrade testing automation #1389
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
czarcas7ic
requested review from
alexanderbez,
niccoloraspa,
ValarDragon and
p0mvn
alexanderbez
reviewed
May 3, 2022
p0mvn
requested changes
May 3, 2022
Codecov Report
@@ Coverage Diff @@
## main #1389 +/- ##
==========================================
- Coverage 19.82% 19.53% -0.29%
==========================================
Files 202 226 +24
Lines 27685 30953 +3268
==========================================
+ Hits 5489 6048 +559
- Misses 21175 23800 +2625
- Partials 1021 1105 +84
Continue to review full report at Codecov.
|
Co-authored-by: Roman <roman@osmosis.team>
czarcas7ic
commented
May 4, 2022
czarcas7ic
commented
May 4, 2022
p0mvn
reviewed
May 5, 2022
tests/e2e/chain_init/main.go
Outdated
Comment on lines
22
to
24
| telemetryEnabled bool | ||
| telemetryRetentionTime int64 | ||
| prometheusEnabled bool |
There was a problem hiding this comment.
Why would we ever need these in a CI job?
p0mvn
reviewed
May 5, 2022
tests/e2e/chain/config.go
Outdated
| @@ -249,7 +266,7 @@ func initNodes(c *internalChain) error { | |||
| return nil | |||
| } | |||
|
|
|||
| func initValidatorConfigs(c *internalChain) error { | |||
| func initValidatorConfigs(c *internalChain, pruning string, pruningKeepRecent string, pruningInterval string, snapshotInterval uint64, snapshotKeepRecent uint32, telemetryEnabled bool, telemetryRetentionTime int64, prometheusEnabled bool) error { | |||
There was a problem hiding this comment.
I think these configs should instead be per-validator. Can we do this in a separate PR?
I think we should limit the scope of this one to just the upgrade and keep adding on top of it after. Otherwise, we might have to keep iterating on this large PR for some time. Let me know your thoughts
p0mvn
requested changes
May 5, 2022
Closes: #1277 ## What is the purpose of the change This PR adds scaling factors to current stable swap implementation. For context on why scaling factors are needed: Suppose 1000 stablecoin1 equals 1 dollars, 100 stablecoin2 equals 2 dollars due to precision difference. Currently we compare each pool assets without considering these differences in precisions in `calcOutAmtGivenIn`, `calcInAmtGivenOut`, `SpotPrice`. This PR adds a field of scaling factors for each pool asset so that when internally calculating in `amm.go`, we compare two different assets upon same precision points. We do this by feeding the internal functions assets / scaling factors. ## Brief change log - Adds scaling factor to stableswap *(for example:)* - *The metadata is stored in the blob store on job creation time as a persistent artifact* - *Deployments RPC transmits only the blob storage reference* - *Daemons retrieve the RPC data from the blob cache* ## Testing and Verifying This change is a trivial rework / code cleanup without any test coverage. ## Documentation and Release Note - Does this pull request introduce a new feature or user-facing behavior changes? yes - Is a relevant changelog entry added to the `Unreleased` section in `CHANGELOG.md`? no - How is the feature or change documented? not applicable
czarcas7ic
force-pushed
the
adam/e2e-upgrade
branch
from
f0f61b0 to
5e9feb5
Compare
github-actions
bot
added
the
C:x/gamm
alexanderbez
approved these changes
May 5, 2022
github-actions
bot
removed
the
C:x/gamm
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes: #1333 , #1235
What is the purpose of the change
This pull request follows the methodology laid out in #1333 to complete the minimum viable product needed for #1235
Process
Testing and Verifying
This change is already covered by existing e2e test suite. This was tested locally and through github CI
Here is an example of the test output
Documentation and Release Note
Unreleasedsection inCHANGELOG.md? noNotes / Next Steps
From here, we can create an environment variable that allows us to skip upgrade testing and instead only compile locally and go though the chain tests. This will save us time instead of having to go through the upgrade process for every commit.
Another important note, I removed the locally compile osmosis-init-image step here since it is not needed at the moment. When we introduce the environment variable addressed above, we will need to re-add this to compile the v8 init image instead of compiling with the v7 init image from docker hub. We should split these into two separate make tests so that way we are only compiling the docker images we absolutely need. The full upgrade testing takes only 8 minutes though.
Another note, I need to backport the config changes to v7.x that changes the gov voting period
Last note, we need to change the test.yml because if you can make a commit that does not pass the e2e-test at first, but if you do a second commit that does not trigger this
enc.GIT_DIFF, it will skip the test despite actually not passing it. cc @daniel-farina if you know how to go about fixing this. I know it will run on a PR which is all that really matters, but in my mind I feel if it fails once instead of just skipping it the second time it should show "still fails" if that makes sense.