The shoe was on the other foot a few years back and I was contracted on part of a large/legacy enterprise app product. I could 'fix' a bug, but couldn't always write a test, as I just didn't understand the system well enough to grok what was there. Policy was "you have to have a test". Fair enough. I remember hitting up 5-6 other devs on the team - some of whom had been there for 10 years - to pair for an hour or so to help me write a test. The same people who were blocking the PR requiring a test would not help me write a test. Some were honest enough to say "I don't know how you'd test that". But... it was frustrating. I had a 20 minute code patch (which demonstrably fixed the bug to the PM's satisfaction) take 8 calendar days to figure out how to test. Then a reviewer didn't like how the test setup was done. When they learned it was done with code from a colleague (and not of my own doing) they suddenly liked it and said it was 'clever'.
Happy happy days...
Want a list of catastrophes that occurred because someone did this?
Edit: Also, Fowler didn't write this article, it's just on his site.
I could write a long, long list of examples that were, in essence if not literally, typos that were missed by code review but bombed when in prod, too.
Both had code reviews. Both catastrophically failed.
No I am not trying to draw causation that code reviewed software ends in tragedy :)
It's a subtle difference, but I think an important one, especially when considering the complexities for program proofs.
I'm sort of (not eagerly) awaiting the day when something written in Rust is implicated in a catastrophic failure. All the platitudes about safety and guarantees will smack hard against a wall like Wile E. Coyote in a Road Runner flick.
> lost count the number of times ... "I need to bypass this policy because shit's on fire and I've got a fix"
The problem is elsewhere. Asking the "Five Why's" might help locate it.
In such code reviews it can also help to point out
> this is a hotfix for issue XYZ. A better fix / more tests / etc will follow"
to make others aware why a change might not meet all best practices. And obviously if that is the case, the follow-up later on should really happen in order to avoid losing trust from reviewers.
From the OP:
> Changes are categorized as either Ship (merge into mainline without review), Show (open a pull request for review, but merge into mainline immediately), or Ask (open a pull request for discussion before merging).
Aside from the "small changes, fast fixes" type of mantra, and "pairing is better than reviewing" that I suspect Martin Fowler is leaning on (and that I both support strongly):
There is a difference between having reviews, and enforcing reviews.
In my multi-decade career I'm yet to have a signle instance of the thought "thank god we prohibit changes that haven't been reviewed by two other people"
But I have lost count the number of times I've had the thought "I need to bypass this policy because shit's on fire and I've got a fix"
Then comes the question of quality of review when they are mandated.
I'll interject with: Code reviews are no inherently bad. Feedback is good. Collaboration is good. There are better methods of providing feedback than code reviews (I strongly object to the post-facto nature of code reviews)
Mandated peer reviews less so. Feedback is hurried, if not entirely absent, as it becomes another task that people _have_ to do. The time spent reviewing is rarely accounted for. There's a lot of knowledge transfer required for any non-trivial review to be effective, further increasing the demand on the participants.
Code reviews are another tool in the box, but they should not be used for every job.