-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Add recommendation on using table tests for single case tests. #167
base: master
Are you sure you want to change the base?
Conversation
The exact wording is TBD - this is to start a discussion. My personal preference is to use table-test all the time, and would obviously would like it to be recommended. However, the primary goal is to avoid arguing in the code reviews. I am okay with the other version too (no table tests for single case tests), as long as we call something out here.
I would advise the opposite: don't table test until you need it. |
@mway - you had other comments, I believe? @abhinav what annoys me mostly is when we have a table test, we remove one of the 2 cases, then we have a well-formatted test, but then converting test to non-table test means 20+ line change. I see how the premature branching might a problem ... but also it's nice to read code like (paraphrasing):
makes it easier to see what actually matters in the tests, rather parsing input/output here in the body of the test. |
@rabbbit I'm completely on-board with table tests when there are multiple test cases. I agree I'm advising against premature tables because the existence of the table test for a single test case will push the next test case to be an entry in the table, even if it would be better expressed as an independent test or subtest. Adding a table test too early is akin to creating an abstraction to reduce duplication before we know what the duplication looks like. Part of the reason for this line of thinking is social: the next person who changes that code may not be confident in what and how much they're allowed to change, so they may want to keep their footprint small. (Anecdotes suggest that this is not uncommon.) They'll pattern match on existing tests, assume there's a good reason for the table, that all tests for Foo must be in the TestFoo table, and shoehorn their new test case in there even if it doesn't fit properly.
I feel like that has to be very rare--to have a table with only two test cases, one of which you are deleting. (Normally the test case count goes in the other direction.) But, if you're really running into this, I'm sure an exception can be made: When modifying existing test cases, if the test is already using table tests, don't change its nature. That said, for simple enough tables, you can sidestep the issue: just add a second test case. It can't be that hard to add a second test case for something that merits a table test. Separately, on the benefit of input and output declared at the top followed by 100 lines of code. You can still do that without a table test: have := // some input
want := // some output
// some 100 lines of code here In fact, that prepares that test nicely to become a table test in the future, should the need arise. |
I think this is superseded by the recent changes to the table test section. |
The exact wording is TBD - this is to start a discussion.
My personal preference is to use table-test all the time, and would obviously would like it to be recommended.
However, the primary goal is to avoid arguing in the code reviews. I am okay with the other version too (no table tests for single case tests), as long as we call something out here.