8000 test: cosmosclient (part2) by tbruyelle · Pull Request #2791 · ignite/cli · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

test: cosmosclient (part2) #2791

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Sep 8, 2022
Merged

test: cosmosclient (part2) #2791

merged 4 commits into from
Sep 8, 2022

Conversation

tbruyelle
Copy link
Contributor
@tbruyelle tbruyelle commented Aug 29, 2022 •
edited
Loading

Please see #2789 before reading this PR.

This PR adds a unit-test on cosmosclient.CreateTX. Because this requires a lot of new mocks, I wanted to make a separate PR, in order to gather your opinion specifically on this test. In particular, I would like to know if you're fine with this test pattern, and how it is written.

The PR introduces 4 new mocks, that are now part of the cosmosclient.Client struct fields :

  • accountRetriever of type client.AccountRetriever (interface from cosmos-sdk)
  • bankQueryClient of type x/bank/types.QueryClient
  • faucetClient of type FaucetClient, a newly created interface defined in cosmosclient package. This allows to mock the interaction with the pkg/cosmosfaucet.
  • gasomoter of type Gasometer, an other newly created interface defined in cosmosclient package. This allows to mock the tx.CalculateGas function from the cosmos-sdk. Because this is a function, this time I needed to create a specific struct that exposes CalculateGas method that itself call tx.CalculateGas. I can understand if some of you find this pattern cumbersome, but afaik this is the only way to mock a function call.

Now the cool part of this is ofc the tests: I can test all the different cases we have in CreateTx; like when fauchet is enabled with enough or insufficient balances, when fees and gasPrices are both provided, how gas amount is computed etc...

For me the benefit is worth the small amount of complexity added in the Client implementation. But maybe you're not agree, then let's discuss :)

@tbruyelle tbruyelle added the skip-changelog Don't check changelog for new entries label Aug 29, 2022
@tbruyelle tbruyelle force-pushed the test/cosmosclient_tx branch from d2c7360 to 6fa0f6c Compare September 1, 2022 12:04
Base automatically changed from test/cosmosclient to develop September 6, 2022 16:35
aljo242
aljo242 previously approved these changes Sep 7, 2022
View reviewed changes
@aljo242 aljo242 self-requested a review September 7, 2022 15:23
@aljo242 aljo242 merged commit 9b03f61 into develop Sep 8, 2022
@aljo242 aljo242 deleted the test/cosmosclient_tx branch September 8, 2022 14:35
Jchicode pushed a commit to Jchicode/cli that referenced this pull request Aug 9, 2023
@tbruyelle
test: cosmosclient (part2) (ignite#2791)
9cc39c4 
* test: cosmosclient.CreateTx

* test: cosmosclient.BankBalances

* style: import order

Co-authored-by: Alex Johnson <alex@shmeeload.xyz>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aljo242 aljo242 aljo242 approved these changes

@ilgooz ilgooz Awaiting requested review from ilgooz

@lumtis lumtis Awaiting requested review from lumtis

@jeronimoalbi jeronimoalbi Awaiting requested review from jeronimoalbi

@fadeev fadeev Awaiting requested review from fadeev

Assignees
No one assigned
Labels
skip-changelog Don't check changelog for new entries type:testing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tbruyelle @aljo242
0