Hacker News Reader

Comment by jaredsohn | Hacker News Reader

For many developers, people merge their branches directly to production so the pull request review is where that checking happens.

View on HN

Sure, I mostly meant automated processes. They won't always save you from yourself but doing blue-green, having an extensive test suite, etc, are all things that will help reduce the risk of a bad deployment.

Yeah exactly, code review isn't a good place to catch actual bugs. Humans are terrible at catching bugs consistently... Humans probably introduced those bugs in the first place. It's also an exceedingly poor use of human time, which costs a heck of a lot more than machine time.

We really should be relying on automated systems to catch bugs for the most part, and leave code review for what it's good for: making sure a different human can understand and maintain the code.

In fact, if we manage to catch a bug in a code review that didn't also trigger our automated systems to alert us, that should probably prompt us to look into how we can expand the coverage of that automated system, similar to what we'd do in a postmortem if the bug wasn't caught in code review and reached production, because we can't rely on that happening the next time.

How do you systematically (and proactively) ensure your testing is thorough? With testing you often run into unknown unknown cases.

I’ve always code reviewed/gotten code reviewed on both the implementation and the tests, with completeness as the main quality you’re reviewing test code for. If that’s how we do it, we’re right back at code reviews.

That's definitely an unsolved (unsolvable?) problem. The best we can really do is start with a common sense set of initial tests based on the user observable behaviors we want from the system and then add more tests organically as we learn more about the system's edge cases through incidents, bugs and regressions.

I'm not saying code review can't find any bugs (in practice bugs are found in code review all the time), I'm just saying that if we do find a bug through code review, it means we got lucky, and we shouldn't count on getting lucky again, so better add a test/linter rule for the next time when we might not get so lucky. That's just one more way to grow your automated systems organically.

> code review isn't a good place to catch actual bugs.

Disagree, but it's also a good place to be ensuring that the system is evolving in a way that is reasonable and consistent — or that it is unreasonable and inconsistent for good reasons.

If there's one thing I've learned over many years writing lots and lots of automated tests for everything is that no matter how far you go with your tests, barring formal proofs or equivalent, the tests cannot save you from, at some point, breaking production badly. One small, tiny thing that slipped through your tests and boom - huge fuck up (even small things can easily cause your whole new feature to fail miserably on the big launch).

> to fail miserably on the big launch

What big launch? :) Is useful to feature flag new functionality so that you can release gradually and spot the bugs - often via error monitoring and user feedback - before they affect everyone. Yes, realize it can happen but there are techniques to minimize the chance of it happening.

Ok, but many companies don't use such ridiculous process.

iirc Google does it, it's not ridiculous at all. You need a lot of automated tests and canary deployment to pull it off though.

Not really. Googler merges to HEAD, but systems aren't running directly from head.

It's not as ridiculous as it sounds as long as the testing and verification happens as part of the PR.

Sure, but then you are just begging the question: if you have a better process for release verification you might not be need any per-commit code review at all.

If you want to learn more about this, read up on 'continuous deployment'. I remember when I was first exposed to it it seemed dangerous/scary but with the right culture changes it can work really well; lots of companies do it.

Reply on news.ycombinator.com