4 ms·
No. In any actually complex piece of code even adding significant amounts testing over new code is not going to cover every possible code path. It's also immen
by olliej 3y ago
No. In any actually complex piece of code even adding significant amounts testing over new code is not going to cover every possible code path.
It's also immensely difficult to write tests to find errors in code you're writing - most patches I see with "extensive tests" test all the correct code paths, and _maybe_ a few error cases. It's a very easy trap to get yourself into.
The purpose of review is not to catch obvious issues - though it hopefully will - but the subtle ones.
- hgsgm 3y agoPurpose of review is to ask questions to help you write more tests to catch more bugs.
- pizlonator 3y agoI’ve had both thorough and cursory reviews of changes like this. It makes no difference for finding actual bugs. Reviews are great for spreading knowledge about the change and making sure the style is dialed in. Reviews aren’t great for ensuring test coverage or finding security bugs.
- eru 3y agoTest coverage should be ensured by your coverage tools in the CI. (At least basic test coverage.) You are right that reviews aren't necessarily there to catch bugs. Reviews are there to tell you to write code that's simple and understandable. If your coworkers can't understand what's going on, how could you expect anyone else to do so in a year or so?
- olliej 3y agoCoverage tools only tell you have tests that hit each line, but most serious bugs aren't the "you didn't hit one line", they're the result of specific interactions between multiple paths. e.g. (note this is a super simple and trivial example) int size = 0; char *buffer; if (a) { buffer = new char[1]; size = 1; } else if (b) { buffer = new char[2]; size = 2; } ... if (b) { buffer[1]++; } ... Coverage tests will happily show that every line is hit, without tests necessarily hitting the buffer overflow if an and b are both true.
- eru 3y agoI agree. Hence my parenthetical remark about '(At least basic test coverage.)' Code review can help you prod you to find ways to simplify your code, so that you have fewer paths to consider.
- troupo 3y ago> and _maybe_ a few error cases. Also it's often unit tests only, with no hint of any integration tests. I'd settle for a tests that check integration of at least two units, but alas.