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.

Sign up for GitHub

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

contribution guide: Add guidelines for testing #1696

Merged
merged 3 commits into from
Jul 18, 2018
Merged

contribution guide: Add guidelines for testing #1696

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4b68899
Merge pull request #1691: Table driven test indexes
ValarDragon Jul 16, 2018
1edfb89
contribution guide: Add guidelines for testing
ValarDragon Jul 16, 2018
2b72e33
Clarify when to use table driven tests
ValarDragon Jul 17, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,29 @@ tested by circle using `go test -v -race ./...`. If not, they will need a
`circle.yml`. Ideally, every repo has a `Makefile` that defines `make test` and
includes its continuous integration status using a badge in the `README.md`.

We expect tests to use `require` or `assert` rather than `t.Skip` or `t.Fail`,
unless there is a reason to do otherwise.
We prefer to use [table driven tests](https://github.com/golang/go/wiki/TableDrivenTests)
where applicable.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd specify where this is applicable.. I'd hate for somebody to take some nice simple non-table driven tests and attempt to fit them into a table because of this comment. Maybe something along the lines of:
If a test describes a reasonably complex situation it should be isolated in it's own test function (aka. TestXxx. Where testing is fairly simple we prefer the tests to grouped through the use of [table driven tests](https://github.com/golang/go/wiki/TableDrivenTests)
+where applicable

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure thats the right boundary for table driven / not table driven. I think the better boundary is whether you're testing a function under multiple scenarios or just a single scenario.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense

Error messages should follow the following format
`<desc>, tc #<index>, i #<index>`.
`<desc>` is an optional short description of whats failing, `tc` is the
index within the table of the testcase that is failing, and `i` is when there
is a loop, exactly which iteration of the loop failed.
The idea is you should be able to see the
error message and figure out exactly what failed.
Here is an example check:

```
<some table>
for tcIndex, tc := range cases {
<some code>
for i := 0; i < tc.numTxsToTest; i++ {
<some code>
require.Equal(t, expectedTx[:32], calculatedTx[:32],
"First 32 bytes of the txs differed. tc #%d, i #%d", tcIndex, i)
```

## Branching Model and Release

User-facing repos should adhere to the branching model: http://nvie.com/posts/a-successful-git-branching-model/.
Expand Down
44 changes: 22 additions & 22 deletions types/coin_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,9 @@ func TestIsPositiveCoin(t *testing.T) {
{NewCoin("a", -1), false},
}

for _, tc := range cases {
for tcIndex, tc := range cases {
res := tc.inputOne.IsPositive()
require.Equal(t, tc.expected, res)
require.Equal(t, tc.expected, res, "%s positivity is incorrect, tc #%d", tc.inputOne.String(), tcIndex)
}
}

Expand All @@ -33,9 +33,9 @@ func TestIsNotNegativeCoin(t *testing.T) {
{NewCoin("a", -1), false},
}

for _, tc := range cases {
for tcIndex, tc := range cases {
res := tc.inputOne.IsNotNegative()
require.Equal(t, tc.expected, res)
require.Equal(t, tc.expected, res, "%s not-negativity is incorrect, tc #%d", tc.inputOne.String(), tcIndex)
}
}

Expand All @@ -52,9 +52,9 @@ func TestSameDenomAsCoin(t *testing.T) {
{NewCoin("steak", -11), NewCoin("steak", 10), true},
}

for _, tc := range cases {
for tcIndex, tc := range cases {
res := tc.inputOne.SameDenomAs(tc.inputTwo)
require.Equal(t, tc.expected, res)
require.Equal(t, tc.expected, res, "coin denominations didn't match, tc #%d", tcIndex)
}
}

Expand All @@ -70,9 +70,9 @@ func TestIsGTECoin(t *testing.T) {
{NewCoin("a", 1), NewCoin("b", 1), false},
}

for _, tc := range cases {
for tcIndex, tc := range cases {
res := tc.inputOne.IsGTE(tc.inputTwo)
require.Equal(t, tc.expected, res)
require.Equal(t, tc.expected, res, "coin GTE relation is incorrect, tc #%d", tcIndex)
}
}

Expand All @@ -89,9 +89,9 @@ func TestIsEqualCoin(t *testing.T) {
{NewCoin("steak", -11), NewCoin("steak", 10), false},
}

for _, tc := range cases {
for tcIndex, tc := range cases {
res := tc.inputOne.IsEqual(tc.inputTwo)
require.Equal(t, tc.expected, res)
require.Equal(t, tc.expected, res, "coin equality relation is incorrect, tc #%d", tcIndex)
}
}

Expand All @@ -106,9 +106,9 @@ func TestPlusCoin(t *testing.T) {
{NewCoin("asdf", -4), NewCoin("asdf", 5), NewCoin("asdf", 1)},
}

for _, tc := range cases {
for tcIndex, tc := range cases {
res := tc.inputOne.Plus(tc.inputTwo)
require.Equal(t, tc.expected, res)
require.Equal(t, tc.expected, res, "sum of coins is incorrect, tc #%d", tcIndex)
}

tc := struct {
Expand All @@ -132,9 +132,9 @@ func TestMinusCoin(t *testing.T) {
{NewCoin("asdf", 10), NewCoin("asdf", 1), NewCoin("asdf", 9)},
}

for _, tc := range cases {
for tcIndex, tc := range cases {
res := tc.inputOne.Minus(tc.inputTwo)
require.Equal(t, tc.expected, res)
require.Equal(t, tc.expected, res, "difference of coins is incorrect, tc #%d", tcIndex)
}

tc := struct {
Expand Down Expand Up @@ -212,10 +212,10 @@ func TestPlusCoins(t *testing.T) {
{Coins{{"A", negone}, {"B", zero}}, Coins{{"A", zero}, {"B", zero}}, Coins{{"A", negone}}},
}

for _, tc := range cases {
for tcIndex, tc := range cases {
res := tc.inputOne.Plus(tc.inputTwo)
assert.True(t, res.IsValid())
require.Equal(t, tc.expected, res)
require.Equal(t, tc.expected, res, "sum of coins is incorrect, tc #%d", tcIndex)
}
}

Expand All @@ -242,12 +242,12 @@ func TestParse(t *testing.T) {
{"5foo-bar", false, nil}, // once more, only letters in coin name
}

for _, tc := range cases {
for tcIndex, tc := range cases {
res, err := ParseCoins(tc.input)
if !tc.valid {
require.NotNil(t, err, "%s: %#v", tc.input, res)
require.NotNil(t, err, "%s: %#v. tc #%d", tc.input, res, tcIndex)
} else if assert.Nil(t, err, "%s: %+v", tc.input, err) {
require.Equal(t, tc.expected, res)
require.Equal(t, tc.expected, res, "coin parsing was incorrect, tc #%d", tcIndex)
}
}

Expand Down Expand Up @@ -296,10 +296,10 @@ func TestSortCoins(t *testing.T) {
{dup, false, false},
}

for _, tc := range cases {
require.Equal(t, tc.before, tc.coins.IsValid())
for tcIndex, tc := range cases {
require.Equal(t, tc.before, tc.coins.IsValid(), "coin validity is incorrect before sorting, tc #%d", tcIndex)
tc.coins.Sort()
require.Equal(t, tc.after, tc.coins.IsValid())
require.Equal(t, tc.after, tc.coins.IsValid(), "coin validity is incorrect after sorting, tc #%d", tcIndex)
}
}

Expand Down
2 changes: 1 addition & 1 deletion types/rational.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,7 @@ func (r *Rat) UnmarshalAmino(text string) (err error) {
//___________________________________________________________________________________
// helpers

// test if two rat arrays are the equal
// test if two rat arrays are equal
func RatsEqual(r1s, r2s []Rat) bool {
if len(r1s) != len(r2s) {
return false
Expand Down
56 changes: 29 additions & 27 deletions types/rational_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,22 +48,22 @@ func TestNewFromDecimal(t *testing.T) {
{"0.foobar.", true, Rat{}},
}

for _, tc := range tests {
for tcIndex, tc := range tests {
res, err := NewRatFromDecimal(tc.decimalStr, 4)
if tc.expErr {
require.NotNil(t, err, tc.decimalStr)
require.NotNil(t, err, tc.decimalStr, "error expected, tc #%d", tcIndex)
} else {
require.Nil(t, err, tc.decimalStr)
require.True(t, res.Equal(tc.exp), tc.decimalStr)
require.Nil(t, err, tc.decimalStr, "unexpected error, tc #%d", tcIndex)
require.True(t, res.Equal(tc.exp), tc.decimalStr, "equality was incorrect, tc #%d", tcIndex)
}

// negative tc
res, err = NewRatFromDecimal("-"+tc.decimalStr, 4)
if tc.expErr {
require.NotNil(t, err, tc.decimalStr)
require.NotNil(t, err, tc.decimalStr, "error expected (negative case), tc #%d", tcIndex)
} else {
require.Nil(t, err, tc.decimalStr)
require.True(t, res.Equal(tc.exp.Mul(NewRat(-1))), tc.decimalStr)
require.Nil(t, err, tc.decimalStr, "unexpected error (negative case), tc #%d", tcIndex)
require.True(t, res.Equal(tc.exp.Mul(NewRat(-1))), tc.decimalStr, "equality was incorrect (negative case), tc #%d", tcIndex)
}
}
}
Expand Down Expand Up @@ -99,10 +99,10 @@ func TestEqualities(t *testing.T) {
{NewRat(-1, 7), NewRat(-3, 7), true, false, false},
}

for _, tc := range tests {
require.Equal(t, tc.gt, tc.r1.GT(tc.r2))
require.Equal(t, tc.lt, tc.r1.LT(tc.r2))
require.Equal(t, tc.eq, tc.r1.Equal(tc.r2))
for tcIndex, tc := range tests {
require.Equal(t, tc.gt, tc.r1.GT(tc.r2), "GT result is incorrect, tc #%d", tcIndex)
require.Equal(t, tc.lt, tc.r1.LT(tc.r2), "LT result is incorrect, tc #%d", tcIndex)
require.Equal(t, tc.eq, tc.r1.Equal(tc.r2), "equality result is incorrect, tc #%d", tcIndex)
}

}
Expand Down Expand Up @@ -135,15 +135,15 @@ func TestArithmetic(t *testing.T) {
{NewRat(100), NewRat(1, 7), NewRat(100, 7), NewRat(700), NewRat(701, 7), NewRat(699, 7)},
}

for _, tc := range tests {
require.True(t, tc.resMul.Equal(tc.r1.Mul(tc.r2)), "r1 %v, r2 %v", tc.r1.Rat, tc.r2.Rat)
require.True(t, tc.resAdd.Equal(tc.r1.Add(tc.r2)), "r1 %v, r2 %v", tc.r1.Rat, tc.r2.Rat)
require.True(t, tc.resSub.Equal(tc.r1.Sub(tc.r2)), "r1 %v, r2 %v", tc.r1.Rat, tc.r2.Rat)
for tcIndex, tc := range tests {
require.True(t, tc.resMul.Equal(tc.r1.Mul(tc.r2)), "r1 %v, r2 %v. tc #%d", tc.r1.Rat, tc.r2.Rat, tcIndex)
require.True(t, tc.resAdd.Equal(tc.r1.Add(tc.r2)), "r1 %v, r2 %v. tc #%d", tc.r1.Rat, tc.r2.Rat, tcIndex)
require.True(t, tc.resSub.Equal(tc.r1.Sub(tc.r2)), "r1 %v, r2 %v. tc #%d", tc.r1.Rat, tc.r2.Rat, tcIndex)

if tc.r2.Num().IsZero() { // panic for divide by zero
require.Panics(t, func() { tc.r1.Quo(tc.r2) })
} else {
require.True(t, tc.resDiv.Equal(tc.r1.Quo(tc.r2)), "r1 %v, r2 %v", tc.r1.Rat, tc.r2.Rat)
require.True(t, tc.resDiv.Equal(tc.r1.Quo(tc.r2)), "r1 %v, r2 %v. tc #%d", tc.r1.Rat, tc.r2.Rat, tcIndex)
}
}
}
Expand All @@ -168,9 +168,9 @@ func TestEvaluate(t *testing.T) {
{NewRat(113, 12), 9},
}

for _, tc := range tests {
require.Equal(t, tc.res, tc.r1.RoundInt64(), "%v", tc.r1)
require.Equal(t, tc.res*-1, tc.r1.Mul(NewRat(-1)).RoundInt64(), "%v", tc.r1.Mul(NewRat(-1)))
for tcIndex, tc := range tests {
require.Equal(t, tc.res, tc.r1.RoundInt64(), "%v. tc #%d", tc.r1, tcIndex)
require.Equal(t, tc.res*-1, tc.r1.Mul(NewRat(-1)).RoundInt64(), "%v. tc #%d", tc.r1.Mul(NewRat(-1)), tcIndex)
}
}

Expand All @@ -192,10 +192,10 @@ func TestRound(t *testing.T) {
{NewRat(1, 2), NewRat(1, 2), 1000},
}

for _, tc := range tests {
require.Equal(t, tc.res, tc.r.Round(tc.precFactor), "%v", tc.r)
for tcIndex, tc := range tests {
require.Equal(t, tc.res, tc.r.Round(tc.precFactor), "%v", tc.r, "incorrect rounding, tc #%d", tcIndex)
negR1, negRes := tc.r.Mul(NewRat(-1)), tc.res.Mul(NewRat(-1))
require.Equal(t, negRes, negR1.Round(tc.precFactor), "%v", negR1)
require.Equal(t, negRes, negR1.Round(tc.precFactor), "%v", negR1, "incorrect rounding (negative case), tc #%d", tcIndex)
}
}

Expand All @@ -211,8 +211,8 @@ func TestToLeftPadded(t *testing.T) {
{NewRat(1000, 3), 8, "00000333"},
{NewRat(1000, 3), 12, "000000000333"},
}
for _, tc := range tests {
require.Equal(t, tc.res, tc.rat.ToLeftPadded(tc.digits))
for tcIndex, tc := range tests {
require.Equal(t, tc.res, tc.rat.ToLeftPadded(tc.digits), "incorrect left padding, tc #%d", tcIndex)
}
}

Expand Down Expand Up @@ -296,11 +296,13 @@ func TestRatsEqual(t *testing.T) {
{[]Rat{NewRat(1), NewRat(0)}, []Rat{NewRat(1), NewRat(0)}, true},
{[]Rat{NewRat(1), NewRat(0)}, []Rat{NewRat(0), NewRat(1)}, false},
{[]Rat{NewRat(1), NewRat(0)}, []Rat{NewRat(1)}, false},
{[]Rat{NewRat(1), NewRat(2)}, []Rat{NewRat(2), NewRat(4)}, false},
{[]Rat{NewRat(3), NewRat(18)}, []Rat{NewRat(1), NewRat(6)}, false},
}

for _, tc := range tests {
require.Equal(t, tc.eq, RatsEqual(tc.r1s, tc.r2s))
require.Equal(t, tc.eq, RatsEqual(tc.r2s, tc.r1s))
for tcIndex, tc := range tests {
require.Equal(t, tc.eq, RatsEqual(tc.r1s, tc.r2s), "equality of rational arrays is incorrect, tc #%d", tcIndex)
require.Equal(t, tc.eq, RatsEqual(tc.r2s, tc.r1s), "equality of rational arrays is incorrect (converse), tc #%d", tcIndex)
}

}
Expand Down
4 changes: 2 additions & 2 deletions types/utils_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,10 @@ func TestSortJSON(t *testing.T) {
wantErr: false},
}

for _, tc := range cases {
for tcIndex, tc := range cases {
got, err := SortJSON([]byte(tc.unsortedJSON))
if tc.wantErr != (err != nil) {
t.Fatalf("got %t, want: %t, err=%s", err != nil, tc.wantErr, err)
t.Fatalf("got %t, want: %t, tc #%d, err=%s", err != nil, tc.wantErr, tcIndex, err)
}
require.Equal(t, string(got), tc.want)
}
Expand Down