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

Add some helpers for testing to its own package in app #1008

Merged
merged 2 commits into from
Mar 3, 2022

Conversation

ValarDragon
Copy link
Member

Description

Adds some helpers for testing various functions in a re-usable way in app.go

I think this should probably be paired with more custom functions / integrations in modules, to get a more composable testing framework. This at least does a first step with setting up validators, and epochs across modules.

I am torn on whether it should be suiteI, or an embeddable struct though. I'm leaning towards making it an embeddable struct.


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

@codecov-commenter
Copy link

codecov-commenter commented Mar 1, 2022

Codecov Report

Merging #1008 (bd67d42) into main (08b3bd6) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1008   +/-   ##
=======================================
  Coverage   20.60%   20.60%           
=======================================
  Files         193      193           
  Lines       25237    25237           
=======================================
  Hits         5199     5199           
  Misses      19084    19084           
  Partials      954      954           

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 08b3bd6...bd67d42. Read the comment docs.

Copy link
Collaborator

@mconcat mconcat left a comment

Choose a reason for hiding this comment

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

I think this is overall great PR that encapsulates common logics to SuiteI!

)

// TODO: Consider an embedded struct here rather than an interface
type SuiteI interface {
Copy link
Collaborator

Choose a reason for hiding this comment

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

awesome

return valAddr
}

func BeginNewBlock(suite SuiteI, executeNextEpoch bool) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

These consensus-related functions might be better to be methods? Especially if SuiteI becomes a struct

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah agreed. Would need to make suite a struct to make it a method (AFAICT, you can't do methods on an interface sadly)

Copy link
Member

@mattverse mattverse left a comment

Choose a reason for hiding this comment

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

This is awesome! We should also aim to write more integration tests using these, as imo they provide a "flow test".

Also, I'm planning to write up a follow up issue on doing the same thing for commonly used methods such as LockTokens if you agree?

Also, can we get lint fixed before merge?

@ValarDragon
Copy link
Member Author

Any thoughts on struct vs interface? I think interface gives us better composability / more re-usability across codebases, but struct will simplify the code a lot.

I'm leaning towards struct, since these whole keeper test suites are kinda odd in that they depend on the OsmosisApp already. I'm generally not a big fan of how they depend on the entirety of the app, but its alright

@ValarDragon
Copy link
Member Author

@mattverse to try a second PR on top of this with the struct approach w/ superfluid, so we can see how we think it compares.

@mattverse
Copy link
Member

@ValarDragon can I go ahead and merge this?

@ValarDragon ValarDragon merged commit 41a0a0b into main Mar 3, 2022
@ValarDragon ValarDragon deleted the dev/apptest9ing branch March 3, 2022 02:57
@ValarDragon ValarDragon added the A:backport/v7.x Do not use. backport patches to v7.x branch label Mar 7, 2022
mergify bot pushed a commit that referenced this pull request Mar 7, 2022
* Add some helpers for testing to its own package in app

* Fix lint

(cherry picked from commit 41a0a0b)
ValarDragon added a commit that referenced this pull request Mar 7, 2022
* Add some helpers for testing to its own package in app

* Fix lint

(cherry picked from commit 41a0a0b)

Co-authored-by: Dev Ojha <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:backport/v7.x Do not use. backport patches to v7.x branch
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants