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

application interfaces for simulation #5378

Merged
merged 20 commits into from
Dec 17, 2019
Merged

application interfaces for simulation #5378

merged 20 commits into from
Dec 17, 2019

Conversation

fedekunze
Copy link
Collaborator

@fedekunze fedekunze commented Dec 9, 2019

Description

This PR introduces app interfaces in order to simplify the creation of the simulation tests (like the ones defined in /simapp/sim_test.go) for SDK based chains.

TODO:


For contributor use:

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

For admin use:

  • Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
  • Reviewers assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@fedekunze fedekunze mentioned this pull request Dec 9, 2019
13 tasks
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.

We should discuss this strategy and goals first. I would rather not have legitimate applications to expose their underlying baseapp and store key references. If this means apps need to duplicate a bit of code, so be it.

@fedekunze
Copy link
Collaborator Author

I would rather not have legitimate applications to expose their underlying baseapp and store key references.

👍 I will pass app.baseapp on the test and remove the GetBaseapp() function. I'll also remove the other store key getters.

@fedekunze fedekunze marked this pull request as ready for review December 9, 2019 20:03
@fedekunze fedekunze added R4R and removed WIP labels Dec 9, 2019
@fedekunze fedekunze added the Type: Code Hygiene General cleanup and restructuring of code to provide clarity, flexibility, and modularity. label Dec 10, 2019
@codecov
Copy link

codecov bot commented Dec 10, 2019

Codecov Report

Merging #5378 into master will decrease coverage by 0.04%.
The diff coverage is 25.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5378      +/-   ##
==========================================
- Coverage   54.79%   54.75%   -0.05%     
==========================================
  Files         311      312       +1     
  Lines       18761    18789      +28     
==========================================
+ Hits        10280    10287       +7     
- Misses       7703     7724      +21     
  Partials      778      778
Impacted Files Coverage Δ
simapp/state.go 0% <0%> (ø) ⬆️
simapp/test_helpers.go 0% <0%> (ø) ⬆️
simapp/app.go 89.63% <0%> (-0.43%) ⬇️
simapp/config.go 54.28% <54.28%> (ø)
simapp/utils.go 22.03% <7.14%> (-20%) ⬇️
x/mock/app.go 64.18% <0%> (+1.35%) ⬆️

@alexanderbez alexanderbez added this to the SDK Roadmap milestone Dec 11, 2019
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.

Thanks for cleaning this up @fedekunze! Left some pretty minor comments.

simapp/sim_bench_test.go Outdated Show resolved Hide resolved
simapp/sim_bench_test.go Outdated Show resolved Hide resolved
simapp/sim_bench_test.go Outdated Show resolved Hide resolved
simapp/sim_bench_test.go Outdated Show resolved Hide resolved
simapp/sim_bench_test.go Outdated Show resolved Hide resolved
simapp/sim_test.go Show resolved Hide resolved
simapp/sim_test.go Show resolved Hide resolved
simapp/sim_test.go Show resolved Hide resolved
simapp/types/types.go Outdated Show resolved Hide resolved
simapp/types/types.go Outdated Show resolved Hide resolved
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.

ACK -- I see calls to SetupSimulation where a logger is created after. Why not use the logger from the SetupSimulation call?

simapp/sim_bench_test.go Outdated Show resolved Hide resolved
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.

ACK

simapp/sim_bench_test.go Outdated Show resolved Hide resolved
@alexanderbez alexanderbez merged commit 3471256 into master Dec 17, 2019
@alexanderbez alexanderbez deleted the app-interfaces branch December 17, 2019 18:28
xiangjianmeng pushed a commit to xiangjianmeng/cosmos-sdk that referenced this pull request Dec 18, 2019
larry0x pushed a commit to larry0x/cosmos-sdk that referenced this pull request May 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:Simulations Type: Code Hygiene General cleanup and restructuring of code to provide clarity, flexibility, and modularity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants