Fix epochs modules tests #1893
Merged
Fix epochs modules tests #1893
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
ValarDragon
commented
Jun 28, 2022
Comment on lines
-216
to
-217
| // This test ensures legacy EpochInfo messages will not throw errors via InitGenesis and BeginBlocker | ||
| func TestLegacyEpochSerialization(t *testing.T) { |
There was a problem hiding this comment.
This is deadcode / for an obsoleted concern. (it was for the v4 upgrade, when we added some more fields to this struct, and deleted a dead one)
ValarDragon
commented
Jun 28, 2022
Comment on lines
+81
to
+83
| heights := maps.Keys(test.blockHeightTimePairs) | ||
| osmoutils.SortSlice(heights) | ||
| for _, h := range heights { |
There was a problem hiding this comment.
This go 1.18 deterministic iteration over maps API 😍
alexanderbez
approved these changes
Jun 28, 2022
Comment on lines
+81
to
+83
| heights := maps.Keys(test.blockHeightTimePairs) | ||
| osmoutils.SortSlice(heights) | ||
| for _, h := range heights { |
Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>
p0mvn
reviewed
Jun 29, 2022
| expectedEpochInfo := epochInfo | ||
| expectedEpochInfo.StartTime = suite.Ctx.BlockTime() | ||
| expectedEpochInfo.CurrentEpochStartHeight = suite.Ctx.BlockHeight() | ||
| suite.Require().Equal(expectedEpochInfo, epochInfoSaved) | ||
|
|
||
| allEpochs := suite.App.EpochsKeeper.AllEpochInfos(suite.Ctx) |
There was a problem hiding this comment.
Does it make sense splitting up the AllEpochInfos test into its own test fixture? It seems that we are testing 2 separate methods in one fixture
There was a problem hiding this comment.
Not really sure theres much to add as a general test here that'd be of value, versus us making this test have documentation for what exactly its doing. I guess we could provide a list of []types.EpochInfo and then call AllEpochInfos on it
Co-authored-by: Roman <roman@osmosis.team>
|
All comments resolved I believe! |
p0mvn
approved these changes
Jun 29, 2022
| block1Time := time.Unix(1656907200, 0).UTC() | ||
| const defaultIdentifier = "hourly" | ||
| const defaultDuration = time.Hour | ||
| const eps = time.Nanosecond |
There was a problem hiding this comment.
Consider adding a comment with the explanation from our earlier discussion please:
#1893 (comment)
mergify bot
pushed a commit
that referenced
this pull request
Aug 4, 2022
* Fix epochs modules tests * Remove debug code * Fix altered API usage within superfluid * Add changelog entry * Add more AddEpochInfo checks * Fix superfluid tests * Update osmoutils/slice_helper.go Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com> * Changelog nit * Apply Roman's suggestions! Co-authored-by: Roman <roman@osmosis.team> * add const * Apply roman's suggestion of tests w/ non-default values * Apply roman's comment update suggestions * AddEpochInfo comment * fix gomod order Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com> Co-authored-by: Roman <roman@osmosis.team> (cherry picked from commit 21f7f98) # Conflicts: # CHANGELOG.md # go.mod # x/epochs/abci_test.go # x/epochs/keeper/epoch.go # x/epochs/keeper/genesis.go # x/superfluid/keeper/keeper.go # x/superfluid/keeper/twap_price_test.go
ValarDragon
pushed a commit
that referenced
this pull request
Aug 4, 2022
* Fix epochs modules tests (#1893) * Fix epochs modules tests * Remove debug code * Fix altered API usage within superfluid * Add changelog entry * Add more AddEpochInfo checks * Fix superfluid tests * Update osmoutils/slice_helper.go Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com> * Changelog nit * Apply Roman's suggestions! Co-authored-by: Roman <roman@osmosis.team> * add const * Apply roman's suggestion of tests w/ non-default values * Apply roman's comment update suggestions * AddEpochInfo comment * fix gomod order
Closed
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.
Part of: #1834
What is the purpose of the change
Update the epochs modules tests to be readable, make sense & cover more relevant cases.
As part of this, we remove the prior
SetEpochInfoAPI, and solely use a saferAddEpochInfoapi, that includes relevant entries if they're left blank, and errors if its an invalid configurationBrief Changelog
Testing and Verifying
This change added tests and can be verified as follows:
Documentation and Release Note
Unreleasedsection inCHANGELOG.md? yes