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

Deprecate x/mock, transition integration tests to use simapp #4875

Closed
17 of 20 tasks
colin-axner opened this issue Aug 8, 2019 · 1 comment · Fixed by #5725, #5738 or #5744
Closed
17 of 20 tasks

Deprecate x/mock, transition integration tests to use simapp #4875

colin-axner opened this issue Aug 8, 2019 · 1 comment · Fixed by #5725, #5738 or #5744
Assignees
Labels

Comments

@colin-axner
Copy link
Contributor

colin-axner commented Aug 8, 2019

Summary

Deprecate (eventually remove) usage/reliance on x/mock in favor of using SimApp for integration tests.

Problem Definition

Integration tests have been a pain point for import cycles. To test a modules integration we need access to many of the other modules. There are a few approaches for this.

One is to mock the usage of the other modules. After a lot of deliberation, I don't think this is an optimal approach. It adds the possibility of more bugs when trying to mock a module, and requires maintenance as changes are made. In fact the current mock module is quite out of date and doesn't even have a module manager.

Furthermore, x/mock is only used by about half of the modules and the rest of the modules have redundant code in common_test/test_common files.

Proposal

Instead, I think we should draw a clear line between unit and integration tests such that they are approached in two different standardized manners.

Unit tests will continue to be how they are currently done now, i.e. they don't need to import outside modules, have the same package name as non-testing code, and ideally are readable (table tests).

Integration tests however will have two main changes.

  • usage of SimApp (thanks @fedekunze for working to refactor simulation!!)
  • a package name of pkgname_test (ex: keeper_test as opposed to just keeper)

Justification:
Our integration is very tangled up and trying to work around this will probably cause even more complexity. Instead, lets make the process much simpler for testing. The go std lib also runs into this issue as well; for example, the testing pkg uses the strings pkg and the strings pkg needs to use testing. So the test file has a different package name so this dependency on testing is not exported with strings during testing compilation. You can see that here

Since no package should be importing a package ending in _test we can use simapp which imports everything we may need.

We can add a guide that makes the two routes of testing very clear.
Unit tests -> don't import, make readable
Integration tests -> change package name, import simapp, make keeper/handler calls

References:

Modules to modify:

  • auth (keeper)
  • bank (top level and keeper)
  • crisis (top level and keeper)
  • distribution (keeper)
  • gov
    • top level
    • keeper
  • mint (keeper)
  • params (top level)
  • slashing
    • top level
    • keeper
  • staking
    • top level
    • keeper
  • supply (keeper)

For Admin Use

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

alexanderbez commented Feb 14, 2020

Update: x/mock has completely been removed (finally)!!!

However, we still have to update and fix the ad-hoc logic in the keeper unit tests to also use SimApp. I've tried giving this a go with staking but ran into test failures. I would like for someone to help out here if possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment