Skip to content

Commit

Permalink
Add test standards and test templates to CONTRIBUTING.md (#1977)
Browse files Browse the repository at this point in the history 
* formatting and nit

* add writing tests section with examples and templates

* markdown linter

* add verbosity rule of thumb

Co-authored-by: Roman <roman@osmosis.team>

* add example for previous commit

Co-authored-by: Roman <roman@osmosis.team>
  • Loading branch information
@AlpinYukseloglu @p0mvn
AlpinYukseloglu and p0mvn authored Jul 13, 2022
1 parent 50d9348 commit bc85410
Showing 1 changed file with 259 additions and 0 deletions.
259 changes: 259 additions & 0 deletions CONTRIBUTING.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,265 @@ To contribute a change proposal, use the following workflow:
5. After you receive feedback from a reviewer, make the requested changes, commit them to your branch, and push them to your remote fork again.
6. Once approval is given, feel free to squash & merge!
## Writing tests
We use table-driven tests because they allow us to test similar logic on many different cases in a way that is easy to both implement and understand. [This article](https://dave.cheney.net/2019/05/07/prefer-table-driven-tests) does a fantastic job explaining the motivation and structure of table-driven testing.
Making table-driven tests in an environment built on the Cosmos SDK has some quirks to it, but overall the structure should be quite similar to what is laid out in the article linked above.
We'll lay out three examples below (one that uses our format for messages, one that applies to keeper methods, and one that applies to our GAMM module), each of which will hopefully be simple enough to copy-paste into a test file and use as a starting point for your test-writing in the Osmosis Core repo.

### Rules of thumb for table-driven tests

1. Each test case should test one thing
2. Each test case should be independent from one another (i.e. ideally, reordering the tests shouldn't cause them to fail)
3. Functions should not be set as fields for a test case (this usually means that the table-driven approach is being sidestepped and that the logic in the function should probably be factored out to cover multiple/all test cases)
4. Avoid verbosity by creating local variables instead of constantly referring to struct field (e.g. doing `lockupKeeper := suite.App.LockupKeeper` instead of using `suite.App.LockupKeeper` every time).
### Example #1: [Message-Related Test](https://github.com/osmosis-labs/osmosis/blob/f0270d04bd77cc5e1c23f7913118b3c2ba737e97/x/tokenfactory/keeper/admins_test.go#L122-L181)
This type of test is mainly for functions that would be triggered by incoming messages (we interact directly with the message server since all other metadata is stripped from a message by the time it hits the msg_server):
```go
func (suite *KeeperTestSuite) TestBurnDenom() {
var addr0bal int64
// Create a denom.
suite.CreateDefaultDenom()
// mint 10 default token for testAcc[0]
suite.msgServer.Mint(sdk.WrapSDKContext(suite.Ctx), types.NewMsgMint(suite.TestAccs[0].String(), sdk.NewInt64Coin(suite.defaultDenom, 10)))
addr0bal += 10
for _, tc := range []struct {
desc string
amount int64
burnDenom string
admin string
valid bool
}{
{
desc: "denom does not exist",
amount: 10,
burnDenom: "factory/osmo1t7egva48prqmzl59x5ngv4zx0dtrwewc9m7z44/evmos",
admin: suite.TestAccs[0].String(),
valid: false,
},
{
desc: "burn is not by the admin",
amount: 10,
burnDenom: suite.defaultDenom,
admin: suite.TestAccs[1].String(),
valid: false,
},
{
desc: "burn amount is bigger than minted amount",
amount: 1000,
burnDenom: suite.defaultDenom,
admin: suite.TestAccs[1].String(),
valid: false,
},
{
desc: "success case",
amount: 10,
burnDenom: suite.defaultDenom,
admin: suite.TestAccs[0].String(),
valid: true,
},
} {
suite.Run(fmt.Sprintf("Case %s", tc.desc), func() {
// Test minting to admins own account
_, err := suite.msgServer.Burn(sdk.WrapSDKContext(suite.Ctx), types.NewMsgBurn(tc.admin, sdk.NewInt64Coin(tc.burnDenom, 10)))
if tc.valid {
addr0bal -= 10
suite.Require().NoError(err)
suite.Require().True(suite.App.BankKeeper.GetBalance(suite.Ctx, suite.TestAccs[0], suite.defaultDenom).Amount.Int64() == addr0bal, suite.App.BankKeeper.GetBalance(suite.Ctx, suite.TestAccs[0], suite.defaultDenom))
} else {
suite.Require().Error(err)
suite.Require().True(suite.App.BankKeeper.GetBalance(suite.Ctx, suite.TestAccs[0], suite.defaultDenom).Amount.Int64() == addr0bal, suite.App.BankKeeper.GetBalance(suite.Ctx, suite.TestAccs[0], suite.defaultDenom))
}
})
}
}
```
### Example #2: [Keeper-Related Test](https://github.com/osmosis-labs/osmosis/blob/f0270d04bd77cc5e1c23f7913118b3c2ba737e97/x/epochs/keeper/abci_test.go#L20-L105)
This type of test is mainly for functions that would be triggered by other modules calling public keeper methods (or just to unit-test keeper methods in general):
```go
func (suite KeeperTestSuite) TestEpochInfoBeginBlockChanges() {
block1Time := time.Unix(1656907200, 0).UTC()
const defaultIdentifier = "hourly"
const defaultDuration = time.Hour
// eps is short for epsilon - in this case a negligible amount of time.
const eps = time.Nanosecond
tests := map[string]struct {
// if identifier, duration is not set, we make it defaultIdentifier and defaultDuration.
// EpochCountingStarted, if unspecified, is inferred by CurrentEpoch == 0
// StartTime is inferred to be block1Time if left blank.
initialEpochInfo types.EpochInfo
blockHeightTimePairs map[int]time.Time
expEpochInfo types.EpochInfo
}{
"First block running at exactly start time sets epoch tick": {
initialEpochInfo: types.EpochInfo{StartTime: block1Time, CurrentEpoch: 0, CurrentEpochStartTime: time.Time{}},
expEpochInfo: types.EpochInfo{StartTime: block1Time, CurrentEpoch: 1, CurrentEpochStartTime: block1Time, CurrentEpochStartHeight: 1},
},
"First block run sets start time, subsequent blocks within timer interval do not cause timer tick": {
initialEpochInfo: types.EpochInfo{StartTime: block1Time, CurrentEpoch: 0, CurrentEpochStartTime: time.Time{}},
blockHeightTimePairs: map[int]time.Time{2: block1Time.Add(time.Second), 3: block1Time.Add(time.Minute), 4: block1Time.Add(30 * time.Minute)},
expEpochInfo: types.EpochInfo{StartTime: block1Time, CurrentEpoch: 1, CurrentEpochStartTime: block1Time, CurrentEpochStartHeight: 1},
},
"Second block at exactly timer interval later does not tick": {
initialEpochInfo: types.EpochInfo{StartTime: block1Time, CurrentEpoch: 0, CurrentEpochStartTime: time.Time{}},
blockHeightTimePairs: map[int]time.Time{2: block1Time.Add(defaultDuration)},
expEpochInfo: types.EpochInfo{StartTime: block1Time, CurrentEpoch: 1, CurrentEpochStartTime: block1Time, CurrentEpochStartHeight: 1},
},
"Second block at timer interval + epsilon later does tick": {
initialEpochInfo: types.EpochInfo{StartTime: block1Time, CurrentEpoch: 0, CurrentEpochStartTime: time.Time{}},
blockHeightTimePairs: map[int]time.Time{2: block1Time.Add(defaultDuration).Add(eps)},
expEpochInfo: types.EpochInfo{StartTime: block1Time, CurrentEpoch: 2, CurrentEpochStartTime: block1Time.Add(time.Hour), CurrentEpochStartHeight: 2},
},
"Downtime recovery (many intervals), first block causes 1 tick and sets current start time 1 interval ahead": {
initialEpochInfo: types.EpochInfo{StartTime: block1Time, CurrentEpoch: 0, CurrentEpochStartTime: time.Time{}},
blockHeightTimePairs: map[int]time.Time{2: block1Time.Add(24 * time.Hour)},
expEpochInfo: types.EpochInfo{StartTime: block1Time, CurrentEpoch: 2, CurrentEpochStartTime: block1Time.Add(time.Hour), CurrentEpochStartHeight: 2},
},
"Downtime recovery (many intervals), second block is at tick 2, w/ start time 2 intervals ahead": {
initialEpochInfo: types.EpochInfo{StartTime: block1Time, CurrentEpoch: 0, CurrentEpochStartTime: time.Time{}},
blockHeightTimePairs: map[int]time.Time{2: block1Time.Add(24 * time.Hour), 3: block1Time.Add(24 * time.Hour).Add(eps)},
expEpochInfo: types.EpochInfo{StartTime: block1Time, CurrentEpoch: 3, CurrentEpochStartTime: block1Time.Add(2 * time.Hour), CurrentEpochStartHeight: 3},
},
"Many blocks between first and second tick": {
initialEpochInfo: types.EpochInfo{StartTime: block1Time, CurrentEpoch: 1, CurrentEpochStartTime: block1Time},
blockHeightTimePairs: map[int]time.Time{2: block1Time.Add(time.Second), 3: block1Time.Add(2 * time.Second), 4: block1Time.Add(time.Hour).Add(eps)},
expEpochInfo: types.EpochInfo{StartTime: block1Time, CurrentEpoch: 2, CurrentEpochStartTime: block1Time.Add(time.Hour), CurrentEpochStartHeight: 4},
},
"Distinct identifier and duration still works": {
initialEpochInfo: types.EpochInfo{Identifier: "hello", Duration: time.Minute, StartTime: block1Time, CurrentEpoch: 0, CurrentEpochStartTime: time.Time{}},
blockHeightTimePairs: map[int]time.Time{2: block1Time.Add(time.Second), 3: block1Time.Add(time.Minute).Add(eps)},
expEpochInfo: types.EpochInfo{Identifier: "hello", Duration: time.Minute, StartTime: block1Time, CurrentEpoch: 2, CurrentEpochStartTime: block1Time.Add(time.Minute), CurrentEpochStartHeight: 3},
},
"StartTime in future won't get ticked on first block": {
initialEpochInfo: types.EpochInfo{StartTime: block1Time.Add(time.Second), CurrentEpoch: 0, CurrentEpochStartTime: time.Time{}},
// currentEpochStartHeight is 1 because thats when the timer was created on-chain
expEpochInfo: types.EpochInfo{StartTime: block1Time.Add(time.Second), CurrentEpoch: 0, CurrentEpochStartTime: time.Time{}, CurrentEpochStartHeight: 1},
},
"StartTime in past will get ticked on first block": {
initialEpochInfo: types.EpochInfo{StartTime: block1Time.Add(-time.Second), CurrentEpoch: 0, CurrentEpochStartTime: time.Time{}},
expEpochInfo: types.EpochInfo{StartTime: block1Time.Add(-time.Second), CurrentEpoch: 1, CurrentEpochStartTime: block1Time.Add(-time.Second), CurrentEpochStartHeight: 1},
},
}
for name, test := range tests {
suite.Run(name, func() {
suite.SetupTest()
suite.Ctx = suite.Ctx.WithBlockHeight(1).WithBlockTime(block1Time)
initialEpoch := initializeBlankEpochInfoFields(test.initialEpochInfo, defaultIdentifier, defaultDuration)
suite.App.EpochsKeeper.AddEpochInfo(suite.Ctx, initialEpoch)
suite.App.EpochsKeeper.BeginBlocker(suite.Ctx)
// get sorted heights
heights := maps.Keys(test.blockHeightTimePairs)
osmoutils.SortSlice(heights)
for _, h := range heights {
// for each height in order, run begin block
suite.Ctx = suite.Ctx.WithBlockHeight(int64(h)).WithBlockTime(test.blockHeightTimePairs[h])
suite.App.EpochsKeeper.BeginBlocker(suite.Ctx)
}
expEpoch := initializeBlankEpochInfoFields(test.expEpochInfo, initialEpoch.Identifier, initialEpoch.Duration)
actEpoch := suite.App.EpochsKeeper.GetEpochInfo(suite.Ctx, initialEpoch.Identifier)
suite.Require().Equal(expEpoch, actEpoch)
})
}
}
```
### Example #3: [Gamm-Related Test](https://github.com/osmosis-labs/osmosis/blob/5be82920fa67a97c2c2dff1c06edd10e0ab66319/x/gamm/pool-models/balancer/pool_test.go#L423-L503)
Since the GAMM module is core to the Osmosis repo, it might be useful to have a good example of a well-structured GAMM-specific test. This example covers a simple getter function and validates the specific error messages around the function (as opposed to merely the presence of an error):
```go
func TestGetPoolAssetsByDenom(t *testing.T) {
testCases := []struct {
name string
poolAssets []balancer.PoolAsset
expectedPoolAssetsByDenom map[string]balancer.PoolAsset
err error
}{
{
name: "zero pool assets",
poolAssets: []balancer.PoolAsset{},
expectedPoolAssetsByDenom: make(map[string]balancer.PoolAsset),
},
{
name: "one pool asset",
poolAssets: []balancer.PoolAsset{
{
Token: sdk.NewInt64Coin("uosmo", 1e12),
Weight: sdk.NewInt(100),
},
},
expectedPoolAssetsByDenom: map[string]balancer.PoolAsset{
"uosmo": {
Token: sdk.NewInt64Coin("uosmo", 1e12),
Weight: sdk.NewInt(100),
},
},
},
{
name: "two pool assets",
poolAssets: []balancer.PoolAsset{
{
Token: sdk.NewInt64Coin("uosmo", 1e12),
Weight: sdk.NewInt(100),
},
{
Token: sdk.NewInt64Coin("atom", 123),
Weight: sdk.NewInt(400),
},
},
expectedPoolAssetsByDenom: map[string]balancer.PoolAsset{
"uosmo": {
Token: sdk.NewInt64Coin("uosmo", 1e12),
Weight: sdk.NewInt(100),
},
"atom": {
Token: sdk.NewInt64Coin("atom", 123),
Weight: sdk.NewInt(400),
},
},
},
{
name: "duplicate pool assets",
poolAssets: []balancer.PoolAsset{
{
Token: sdk.NewInt64Coin("uosmo", 1e12),
Weight: sdk.NewInt(100),
},
{
Token: sdk.NewInt64Coin("uosmo", 123),
Weight: sdk.NewInt(400),
},
},
err: fmt.Errorf(balancer.ErrMsgFormatRepeatingPoolAssetsNotAllowed, "uosmo"),
},
}
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
actualPoolAssetsByDenom, err := balancer.GetPoolAssetsByDenom(tc.poolAssets)
require.Equal(t, tc.err, err)
if tc.err != nil {
return
}
require.Equal(t, tc.expectedPoolAssetsByDenom, actualPoolAssetsByDenom)
})
}
}
```
## Working with the SDK
### Updating dependencies for builds
Expand Down

0 comments on commit bc85410

Please sign in to comment.