diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 9c73f4537d0..8be1c8f2290 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -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