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

EPIC: Unit Testing of Modules via Mocks #12398

Closed
23 of 27 tasks
tac0turtle opened this issue Jun 30, 2022 · 3 comments
Closed
23 of 27 tasks

EPIC: Unit Testing of Modules via Mocks #12398

tac0turtle opened this issue Jun 30, 2022 · 3 comments

Comments

@tac0turtle
Copy link
Member

tac0turtle commented Jun 30, 2022

Summary

Currently we have two main ways to test modules, the first are unit tests and the second are integration tests. The integration tests are closer to e2e tests because we do spin up tendermint validators in process. These integration tests rely on simapp and with the app wiring work rely on other modules. We should rewrite integration tests that can be written as unit tests as unit tests with mocks. The integration/e2e tests that test various things like cross module communication may need to be moved to the simapp level.

This has been a long standing problem with how we do test-race and test in ci. By rewriting to use mocks and away from integration tests in modules we should be able to remove tat-race ci jobs and the new coupling of modules introduced by app wiring.

Problem Definition

Current integration tests are closer to e2e tests and are testing things that can be tested in unit tests with mocks.

Proposal

  • Identify which modules integration tests can be written with unit tests and mocks and rewrite them
  • remove app.yaml and/or simapp dependency in modules
  • Remove test-race ci jobs

TODOs


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@julienrbrt
Copy link
Member

When mocking modules, please always re-enable the disabled tests that required of gov to app wiring (They won't be re-enabled in #12274). See #12274 (comment)

mergify bot pushed a commit that referenced this issue Jul 5, 2022
## Description

Ref: #12398



---

### Author Checklist

*All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.*

I have...

- [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#pr-targeting))
- [ ] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/main/docs/building-modules)
- [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] updated the relevant documentation or specification
- [ ] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

### Reviewers Checklist

*All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.*

I have...

- [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed 
- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)
mergify bot pushed a commit that referenced this issue Jul 8, 2022
## Description

Ref: #12398



---

### Author Checklist

*All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.*

I have...

- [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [x] added `!` to the type prefix if API or client breaking change
- [x] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#pr-targeting))
- [x] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/main/docs/building-modules)
- [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] updated the relevant documentation or specification
- [ ] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

### Reviewers Checklist

*All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.*

I have...

- [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed 
- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)
@tac0turtle
Copy link
Member Author

part of this issue should also be removing the build flag for testing with the race detector.

There is still a larger open todo of mocking tendermint. @aaronc mentioned having something already, could we reuse this?

@tac0turtle
Copy link
Member Author

In the last team call we discussed moving the current integration tests into simapp to not lose them but still decouple simapp from the repo.

The current tests, cli tests, that rely on testutil/network would be rewritten to be cli unit tests of the cli interface. This will allow modules to not depend on each other for testing as well. We will plan on doing more in-depth integration tests as well

@tac0turtle tac0turtle moved this from 📝 Todo to 💪 In Progress in Cosmos-SDK Aug 8, 2022
Repository owner moved this from 💪 In Progress to 👏 Done in Cosmos-SDK Sep 2, 2022
@tac0turtle tac0turtle removed this from Cosmos-SDK Oct 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants