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.

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

tests: Make all table driven tests tell you where they failed #1664

Closed
ValarDragon opened this issue Jul 13, 2018 · 5 comments
Closed

tests: Make all table driven tests tell you where they failed #1664

ValarDragon opened this issue Jul 13, 2018 · 5 comments

Comments

@ValarDragon
Copy link
Contributor

ValarDragon commented Jul 13, 2018

Currently the majority of our table driven tests don't tell you where exactly they failed. (This is true of most tests in types, random example: https://github.com/cosmos/cosmos-sdk/blob/master/types/rational_test.go#L57) This makes debugging extremely hard. We definitely need to do something in the spirit of telling you which test case faiiled, however this doesn't need to be prelaunch, could easily be pre1.0.

Here is an example of a test case in something I created:

require.Equal(t, expectedTx[:32], updatedTx[:32],
       "First 32 bytes of the txs differed. testcase #%d, tx test #%d", tcIndex, i))

This makes it easy to figure out what you're looking at (as opposed to just a pile of different bytes in a file with many tests), and exactly which test case failed.

The above is actually not ideal. Here I propose is a more ideal way which we standardize throughout the sdk.

require.Equal(t, obj1, obj2, 
  "<desc>. tc #%d, i #%d", test case index, other variables necessary to specify at what step this occured

(Newline doesn't matter, was just including for ease of reading in the PR)

where <desc> is a short description which indicates what the objs are (since you don't have variable names), and what is failing. The indices that specify where it occured can be as simple as "i" and "j" in the for loop, or the variable name.

A sample log output of the above would be:

--- FAIL: TestGenerateTxUpdateTxConsistentency (0.53s)
	require.go:157: 
			Error Trace:	transacter_test.go:44
			Error:      	Not equal: 
			            	expected: []byte{0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0xcb, 0x6, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0xde, 0xec, 0xab, 0x16, 0x49, 0xd, 0x38, 0xc0, 0x91, 0x97, 0x6b, 0x93, 0x1c, 0x41, 0x6d, 0xd, 0xa5, 0xe9, 0xa0, 0xda, 0x5, 0x0, 0x0, 0x0}
			            	actual  : []byte{0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0xcb, 0x6, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0xde, 0xec, 0xab, 0x16, 0x49, 0xd, 0x38, 0xc0, 0x91, 0x97, 0x6b, 0x93, 0x1c, 0x41, 0x6d, 0xd, 0xa4, 0xe9, 0xa0, 0xda, 0x5, 0x0, 0x0, 0x0}
			            	
			            	Diff:
			            	--- Expected
			            	+++ Actual
			            	@@ -3,3 +3,3 @@
			            	  00000010  de ec ab 16 49 0d 38 c0  91 97 6b 93 1c 41 6d 0d  |....I.8...k..Am.|
			            	- 00000020  a5 e9 a0 da 05 00 00 00                           |........|
			            	+ 00000020  a4 e9 a0 da 05 00 00 00                           |........|
			            	 }
			Test:       	TestGenerateTxUpdateTxConsistentency
			Messages:   	First 32 bytes of the txs differed. tc #2, i #793

The idea is that it should be easy to go from failed test case to where exactly the error occured, as line number doesn't tell you what test case, or what part of the test case. (In many cases, you run a for loop some amount of iterations) This would save debugging time significantly, when you're breaking an already existing thing.

@rigelrozanski
Copy link
Contributor

Big ++ to this - super frustrating

@alexanderbez
Copy link
Contributor

Good idea, let's start doing this!

ValarDragon added a commit that referenced this issue Jul 16, 2018
Make table driven tests in /types follow the format described in #1664
@ValarDragon
Copy link
Contributor Author

Lets document this somewhere in contributing.md, and require this of all new tests that get written in future PR's.

rigelrozanski pushed a commit that referenced this issue Jul 16, 2018
* types: Switch table driven test error messages to new format

Make table driven tests in /types follow the format described in #1664

* typos / lower case errors

* lower case, not sentences

* lower case, not sentences
@rigelrozanski
Copy link
Contributor

@ValarDragon want to add to contributing.md and then close this issue?

@ValarDragon
Copy link
Contributor Author

Sure!

@ValarDragon ValarDragon self-assigned this Jul 16, 2018
ValarDragon added a commit that referenced this issue Jul 16, 2018
Indicates to use require's and asserts, and to use table driven
tests, with error messages as described in #1664.

Closes #1664
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants