cosmos / interchain-security

Interchain Security is an open sourced IBC application which allows cosmos blockchains to lease their proof-of-stake security to one another.
https://cosmos.github.io/interchain-security/
Other
154 stars 115 forks source link

Refactor UTs to use consistent test strategy #625

Closed shaspitz closed 2 weeks ago

shaspitz commented 1 year ago

Problem

This issue is only in regard to what we refer to as unit tests in this repo.

Some UTs in main construct expected return values from properties encoded as local functions, sorting helpers, etc. This is similar to property based testing, but is inconsistent with how most UTs are written in this repo, as black-box functional tests with hardcoded expected values. We have the option of fully embracing property based testing for UTs, with useful syntax, fuzzing tools, etc. (bigger refactor to make everything consistent). Or we can refactor a smaller number of tests to use functional testing strategies.

Closing criteria

Either

  1. fully adopt a golang property based testing framework with succint syntax and fuzzing like rapid (CC @danwt, who recently hosted a session using this repo)
  2. Or standardize the unit tests in this repo to consistently use black-box, functional testing with hardcoded expectations, example here. This would involve refactoring the recently added tests from keeper.go, particularly ones that use sorting.

Problem details

If we do want to adopt property based testing for UTs, I'd just hope we could fully commit to that route and get all the included niceties of such a strategy. Mainly fuzzed inputs. It's a tradeoff because black box functional UTs are significantly more readable than property based testing, in that you don't have to spend much effort to understand the test logic.

See this comment for a concrete example regarding this issue.

mpoke commented 1 year ago

We should go with the second option:

Or standardize the unit tests in this repo to consistently use black-box, functional testing with hardcoded expectations, example here. This would involve refactoring the recently added tests from keeper.go, particularly ones that use sorting.

mpoke commented 2 weeks ago

Closing as it's low priority.