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

Fix epochs modules tests #1893

Merged
merged 15 commits into from
Jun 29, 2022
Merged

Fix epochs modules tests #1893

merged 15 commits into from
Jun 29, 2022

Conversation

ValarDragon
Copy link
Member

@ValarDragon ValarDragon commented Jun 28, 2022

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 SetEpochInfo API, and solely use a safer AddEpochInfo api, that includes relevant entries if they're left blank, and errors if its an invalid configuration

Brief Changelog

  • Remove epochskeeper.SetEpochInfo API, in favor of epochskeeper.AddEpochInfo

Testing and Verifying

This change added tests and can be verified as follows:

  • Added unit test that validates expected epoch behavior on begin block
    • Open to any other test cases we can think of for edge cases / etc.

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? yes
  • How is the feature or change documented? not applicable

@ValarDragon ValarDragon requested a review from a team June 28, 2022 21:39
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) {
Copy link
Member Author

Choose a reason for hiding this comment

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

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)

@github-actions github-actions bot added the C:docs Improvements or additions to documentation label Jun 28, 2022
Comment on lines +81 to +83
heights := maps.Keys(test.blockHeightTimePairs)
osmoutils.SortSlice(heights)
for _, h := range heights {
Copy link
Member Author

Choose a reason for hiding this comment

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

This go 1.18 deterministic iteration over maps API 😍

Copy link
Contributor

Choose a reason for hiding this comment

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

love it

@ValarDragon ValarDragon requested a review from p0mvn June 28, 2022 22:28
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

🧀

CHANGELOG.md Outdated Show resolved Hide resolved
osmoutils/slice_helper.go Show resolved Hide resolved
x/epochs/keeper/abci_test.go Show resolved Hide resolved
Comment on lines +81 to +83
heights := maps.Keys(test.blockHeightTimePairs)
osmoutils.SortSlice(heights)
for _, h := range heights {
Copy link
Contributor

Choose a reason for hiding this comment

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

love it

ValarDragon and others added 2 commits June 28, 2022 17:55
Copy link
Member

@p0mvn p0mvn left a comment

Choose a reason for hiding this comment

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

I feel like reviewing this has really improved my understanding of epochs implementation details. Great work on the refactor and tests!

go.mod Outdated Show resolved Hide resolved
x/epochs/keeper/genesis.go Show resolved Hide resolved
expectedEpochInfo := epochInfo
expectedEpochInfo.StartTime = suite.Ctx.BlockTime()
expectedEpochInfo.CurrentEpochStartHeight = suite.Ctx.BlockHeight()
suite.Require().Equal(expectedEpochInfo, epochInfoSaved)

allEpochs := suite.App.EpochsKeeper.AllEpochInfos(suite.Ctx)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

x/epochs/keeper/epoch.go Outdated Show resolved Hide resolved
x/epochs/keeper/epoch.go Show resolved Hide resolved
x/epochs/keeper/abci_test.go Outdated Show resolved Hide resolved
x/epochs/keeper/abci_test.go Outdated Show resolved Hide resolved
x/epochs/keeper/abci_test.go Outdated Show resolved Hide resolved
x/epochs/keeper/abci_test.go Outdated Show resolved Hide resolved
x/epochs/keeper/abci_test.go Outdated Show resolved Hide resolved
@ValarDragon
Copy link
Member Author

ValarDragon commented Jun 29, 2022

All comments resolved I believe!
Thanks for the detailed review =)

Copy link
Member

@p0mvn p0mvn left a comment

Choose a reason for hiding this comment

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

🚀

block1Time := time.Unix(1656907200, 0).UTC()
const defaultIdentifier = "hourly"
const defaultDuration = time.Hour
const eps = time.Nanosecond
Copy link
Member

Choose a reason for hiding this comment

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

Consider adding a comment with the explanation from our earlier discussion please:
#1893 (comment)

x/epochs/keeper/abci_test.go Outdated Show resolved Hide resolved
@ValarDragon ValarDragon merged commit 21f7f98 into main Jun 29, 2022
@ValarDragon ValarDragon deleted the dev/epochs_test branch June 29, 2022 13:08
@osmo-bot osmo-bot added the A:backport/v11.x backport patches to v11.x branch label Aug 4, 2022
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 <[email protected]>

* Changelog nit

* Apply Roman's suggestions!

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

* 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 <[email protected]>
Co-authored-by: Roman <[email protected]>
(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 <[email protected]>

* Changelog nit

* Apply Roman's suggestions!

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

* add const

* Apply roman's suggestion of tests w/ non-default values

* Apply roman's comment update suggestions

* AddEpochInfo comment

* fix gomod order
@github-actions github-actions bot mentioned this pull request May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:backport/v11.x backport patches to v11.x branch C:docs Improvements or additions to documentation C:x/epochs C:x/superfluid
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants