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

Remove testing related functions from simapp #8597

Closed
2 of 4 tasks
robert-zaremba opened this issue Feb 16, 2021 · 5 comments
Closed
2 of 4 tasks

Remove testing related functions from simapp #8597

robert-zaremba opened this issue Feb 16, 2021 · 5 comments
Labels
dependencies Pull requests that update a dependency file T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine). Type: Code Hygiene General cleanup and restructuring of code to provide clarity, flexibility, and modularity.

Comments

@robert-zaremba
Copy link
Collaborator

robert-zaremba commented Feb 16, 2021

Summary

The simapp is polluted with lot of not production related code.

Problem Definition

simapp is becoming next /types - lot of loosely related code imported in maaaany places. The technical dept there is already big.

Proposal

  • create new package for test related code
  • change parameters - functions should receive only what they need. Eg simapp.FundAccount doesn't need simapp as a parameter - it only needs BankKeeper
  • break down the simapp package and move code to other packages.
  • reduce coupling of simapp package with other packages.

Ref: #8473 (comment)


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@robert-zaremba robert-zaremba added T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine). Type: Code Hygiene General cleanup and restructuring of code to provide clarity, flexibility, and modularity. dependencies Pull requests that update a dependency file labels Feb 16, 2021
@fdymylja
Copy link
Contributor

fdymylja commented Feb 16, 2021

I'd propose to: split simapp from all the test related utilities, let simapp package be a normal cosmos-sdk application.

And create another package for tests which has maybe a builder pattern to allow you to initialize simapp with different setups, example:

  • init genesis or not
  • define validators
  • define addresses
  • different permissions
  • genesis manipulation
  • etc..

@amaury1093
Copy link
Contributor

I believe simapp plays 2 diffrent roles that are not always compatible:

  • a reference app for other chains
  • a test app we use in SDK integration tests

I agree with @fdymylja's proposal: separate (via packages) the 2 concerns of simapp. The "reference app" part of simapp should be really clean (and not contain stuff like testdata, MakeTestEncodingConfig...)

@robert-zaremba
Copy link
Collaborator Author

Exactly - this is what were discussing in #8473. I already wanted to stop messing up with simapp in that PR. Moreover, it's not only about moving the functions but also changing arguments - some are taking more then they need.

@fdymylja
Copy link
Contributor

Maybe we could create a specific x/ test suite (if I'm not mistaken x/packages are the ones which always import simapp for testing, I haven't checked the other pieces laying outside of x). This could also be a good tool for developers to test their own modules.

WDYT @robert-zaremba @AmauryM

@robert-zaremba
Copy link
Collaborator Author

I think /x/... should be reserved for modules. We can have /testing/... for test related stuff. Of course things should be in few sub-packages.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine). Type: Code Hygiene General cleanup and restructuring of code to provide clarity, flexibility, and modularity.
Projects
None yet
Development

No branches or pull requests

3 participants