3 ms·

Purpose of review is to ask questions to help you write more tests to catch more bugs.

• pizlonator 3y ago
  I’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 ago
    Test 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 ago
      Coverage 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 ago
        I 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.