Table-Driven Bank Module Unit Tests #1764

alexanderbez · 2018-07-19T18:37:31Z

Changes

Update x/bank app unit tests to be (mostly) table-driven
Cleaned up existing unit tests

closes: #1546

For Admin Use:

Added appropriate labels to PR (ex. wip, ready-for-review, docs)
Reviewers Assigned
Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

ValarDragon · 2018-07-19T19:21:33Z

x/bank/app_test.go

+			privKeys: []crypto.PrivKey{priv1},
+		},
+		{
+			addr:  addr1,


Any particular reason you have a switch case to check the expected state transition, instead of having a different field in the appTestCase struct for expected outcome? Putting it in the appTestCase struct seems more optimal to me.

You mean why do I leverage the mock#* calls? No particular reason. This was the original code and figured I'd leave it this way to leverage existing functionality. I can take a look at those function calls and put the require statements directly in here if that's what you'd prefer?

No, I mean that the struct has a "simBlock" flag. When that is true, it runs a block, when its false, it tests that the expected action happened.

I think it would be easier if we had a list of expected address / coin amounts after running the tx, and we put those directly in the struct. this would mean adding a new struct type:
expectedAddressCoinAmount {Address, Coins} (better name probably exists)

and then adding to the appTestCase struct, an []expectedAddressCoinAmount and removing simBlock

I see, yeah we can do that for sure 👍

ebuchman · 2018-07-19T21:58:31Z

Can we do this without mock ?

codecov · 2018-07-19T22:06:37Z

Codecov Report

❗ No coverage uploaded for pull request base (develop@f3a1290). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             develop    #1764   +/-   ##
==========================================
  Coverage           ?   62.77%           
==========================================
  Files              ?      122           
  Lines              ?     7125           
  Branches           ?        0           
==========================================
  Hits               ?     4473           
  Misses             ?     2390           
  Partials           ?      262

alexanderbez · 2018-07-20T00:59:22Z

Yeah, I'll see if we can do this without mock @ebuchman 👍

alexanderbez · 2018-07-20T13:32:46Z

@ValarDragon updated to address your concern.

@ebuchman I can remove the dependency on mock by simply copying that code. But I imagine we'd do that for the other modules as well. Would you like to move the test helpers/utils file with this logic out of mock?

Thoughts on moving those helpers to test/utils.go?

alexanderbez · 2018-07-20T19:19:48Z

I think the mock issue is left up to another PR. I think this is R4R 👍

ValarDragon

LGTM, thanks for doing this!

One minor comment re order of expected balance

ValarDragon · 2018-07-20T19:22:53Z

x/bank/app_test.go

+			privKeys: []crypto.PrivKey{priv1, priv4},
+			expectedBalances: []expectedBalance{
+				expectedBalance{addr1, sdk.Coins{sdk.NewCoin("foocoin", 32)}},
+				expectedBalance{addr4, sdk.Coins{sdk.NewCoin("foocoin", 32)}},


minor nitpick, can the addresses be sorted? i.e. addr1, addr2, addr3 then 4

Sure! I had them originally as they were implemented, but I can certainly order them by address.

ValarDragon · 2018-07-20T19:24:05Z

x/bank/app_test.go

-	mock.CheckBalance(t, mapp, addr2, sdk.Coins{sdk.NewCoin("foocoin", 52)})
-	mock.CheckBalance(t, mapp, addr3, sdk.Coins{sdk.NewCoin("foocoin", 10)})
+		for _, eb := range tc.expectedBalances {
+			mock.CheckBalance(t, mapp, eb.addr, eb.coins)


This should be left for a seperate PR, but perhaps mock.CheckBalance should take in a message parameter, so that we can have error messages as described in https://github.com/cosmos/cosmos-sdk/blob/develop/CONTRIBUTING.md#testing

Definitely worthy for another PR :-)

cwgoes

utACK, agreed on removing mock, needs to be discussed.

alexanderbez requested review from ValarDragon and cwgoes July 19, 2018 18:37

alexanderbez requested review from ebuchman and rigelrozanski as code owners July 19, 2018 18:37

alexanderbez added T: Tests non-breaking labels Jul 19, 2018

alexanderbez changed the title ~~Update bank app unit tests to be table-driven~~ Table-Driven Bank Module Unit Tests Jul 19, 2018

alexanderbez force-pushed the bez/1546-bank-table-driven-tests branch from 81c86df to 49b7a4a Compare July 19, 2018 18:39

alexanderbez pushed a commit that referenced this pull request Jul 19, 2018

Merge pull request #1764: Table-Driven Bank Module Unit Tests

49b7a4a

ValarDragon reviewed Jul 19, 2018

View reviewed changes

alexanderbez added wip and removed ready-for-review labels Jul 20, 2018

alexanderbez force-pushed the bez/1546-bank-table-driven-tests branch from c6d187f to a323a68 Compare July 20, 2018 13:30

Merge pull request #1764: Table-Driven Bank Module Unit Tests

63ccb2c

alexanderbez force-pushed the bez/1546-bank-table-driven-tests branch from a323a68 to 63ccb2c Compare July 20, 2018 13:31

alexanderbez removed the wip label Jul 20, 2018

alexanderbez added the ready-for-review label Jul 20, 2018

ValarDragon approved these changes Jul 20, 2018

View reviewed changes

cwgoes approved these changes Jul 20, 2018

View reviewed changes

Update PENDING.md

cee13ef

cwgoes merged commit f437f36 into develop Jul 20, 2018

cwgoes deleted the bez/1546-bank-table-driven-tests branch July 20, 2018 22:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Table-Driven Bank Module Unit Tests #1764

Table-Driven Bank Module Unit Tests #1764

alexanderbez commented Jul 19, 2018 •

edited by cwgoes

Loading

ValarDragon Jul 19, 2018

alexanderbez Jul 19, 2018 •

edited

Loading

ValarDragon Jul 19, 2018 •

edited

Loading

alexanderbez Jul 20, 2018

ebuchman commented Jul 19, 2018

codecov bot commented Jul 19, 2018 •

edited

Loading

alexanderbez commented Jul 20, 2018

alexanderbez commented Jul 20, 2018 •

edited

Loading

alexanderbez commented Jul 20, 2018

ValarDragon left a comment

ValarDragon Jul 20, 2018

alexanderbez Jul 20, 2018

ValarDragon Jul 20, 2018

alexanderbez Jul 20, 2018

cwgoes left a comment •

edited

Loading

Table-Driven Bank Module Unit Tests #1764

Table-Driven Bank Module Unit Tests #1764

Conversation

alexanderbez commented Jul 19, 2018 • edited by cwgoes Loading

Changes

ValarDragon Jul 19, 2018

Choose a reason for hiding this comment

alexanderbez Jul 19, 2018 • edited Loading

Choose a reason for hiding this comment

ValarDragon Jul 19, 2018 • edited Loading

Choose a reason for hiding this comment

alexanderbez Jul 20, 2018

Choose a reason for hiding this comment

ebuchman commented Jul 19, 2018

codecov bot commented Jul 19, 2018 • edited Loading

Codecov Report

alexanderbez commented Jul 20, 2018

alexanderbez commented Jul 20, 2018 • edited Loading

alexanderbez commented Jul 20, 2018

ValarDragon left a comment

Choose a reason for hiding this comment

ValarDragon Jul 20, 2018

Choose a reason for hiding this comment

alexanderbez Jul 20, 2018

Choose a reason for hiding this comment

ValarDragon Jul 20, 2018

Choose a reason for hiding this comment

alexanderbez Jul 20, 2018

Choose a reason for hiding this comment

cwgoes left a comment • edited Loading

Choose a reason for hiding this comment

alexanderbez commented Jul 19, 2018 •

edited by cwgoes

Loading

alexanderbez Jul 19, 2018 •

edited

Loading

ValarDragon Jul 19, 2018 •

edited

Loading

codecov bot commented Jul 19, 2018 •

edited

Loading

alexanderbez commented Jul 20, 2018 •

edited

Loading

cwgoes left a comment •

edited

Loading