When I worked for Apple, we had a strict "all code needs to be reviewed" policy, which I had no problem with. Then, after being there for about 1.5 years, I make a PR, ask two different people to approve it, which they do, and it gets merged. A week later, the code is released, it looks like my code caused a fairly major bug.
My manager's manager and my manager had a meeting with me, and told me that this was unacceptable. I ask "what exactly was I supposed to do differently? Clearly this bug was sneaky enough to where two other people also didn't see it." Their response was "the code needs to work, you wrote the code, the code was sloppy. This is your fault." Apparently they took the bug pretty seriously, since it was actually mentioned in my yearly review.
This is fine, I probably should have tested the code a bit better, but it always annoyed me that my "sloppy" code managed to pass code review [1], and yet I'm the only one to get in trouble over it. If the PRs aren't catching bugs and I still am going to get in trouble, then I'm failing to see why I should bother waiting an hour for someone to look through it. Instead, why not make it so we can merge quickly, test it quickly, and revert it if something breaks?
[1] Obviously I'm biased here, but I honestly think that in this particular case I was pretty easily the least-liked person on the team, and it was easier to throw me under the bus than anything else.
Yes, in your scenario the lack of testing stands out as the major opportunity for improvement. If a bug is so important that it's mentioned in a performance review, then it's important enough that there should be tests that would have caught it. Automated preferably, or manual if necessary.
And everyone involved with the software, from you on up and sideways, should be calling for this testing. It's clearly important / worthwhile!
Sure. Seems like a classic "seven whys" kind of situation... coming down hard on the person who wrote the bug is counterproductive in a way that the whole tech industry understands, except for your former bosses at Apple.
Did you purposefully introduce this bug? Then it was your fault. Otherwise, from all the evidence we have, you couldn't predict it from where you were standing at the time. You can't be at fault for that.
Any other conclusion is a cultural problem that hints at a "shoot the messenger" philosophy ruling.
If we want a Westrum generative culture (and we do!) we can't go around assigning fault to ourselves or others when we end up in bad situations.
Focus on whether the process is good or bad, not where individuals ended up due to random variance around the mean.
End rant. Sorry, this is one of the major things that upset me about how other people run their organisations.
Edge cases and cascading failures are things that code reviews should be focusing on, as that’s a place where more brains is better than one, rather than the usual “does the code match the formatting and naming conventions”.
I don't know what your point is. You would have gotten in trouble whether or not code reviews were present. If you're saying that code reviews should not be default, then the entire blame would rest on your shoulders anyway. Are you saying you wish you got in less trouble at Apple and the code reviewers should have gotten in more trouble for missing YOUR bug?
> Are you saying you wish you got in less trouble at Apple and the code reviewers should have gotten in more trouble for missing YOUR bug?
If “my” bug was a result of sloppy code, my PR should have been rejected.
I don’t really think anyone should get in trouble for a code bug (at least in a majority of cases), but yes, if this bug was obvious enough for me to be in trouble for not catching it, then the people approving the PR should share responsibility for it.
If code reviewers share responsibility for the code they review, why on earth would anyone review code? Everyone would stop reviewing other people's code. Then you're in the same situation where no one reviews code, and then you yourself would be responsible for your own code by yourself.
It sounds like you think you shouldn't have gotten in trouble for having bugs in your code. I have no comment on that situation because there's not enough back story. Generally "punishing" coders for bugs isn't good policy unless it's particularly egregious. If you were told that your code was sloppy, and if your performance review was affected by it, maybe it was sloppy code? Like I said, there's not enough back story.
If it was an innocuous bug or if it was a hard-to-determine bug, then I don't think you should have been written up, but again, there's not enough backstory to determine that.
But to say that your responsibility is lessened because it was blessed by code reviewers and they should share in your punishment, is, frankly, immature. It does sounds like they were ineffective code reviewers. If your code was sloppy, they should have picked it up, and if it was a hard-to-detect bug, neither you nor the code reviewers should be written up for it.
> If code reviewers share responsibility for the code they review, why on earth would anyone review code?
For the same reason that people write code they'll be responsible for: it's their job. Code review works when it's seen as equal priority and equal importance to writing code.
> If code reviewers share responsibility for the code they review, why on earth would anyone review code?
Ok, and if there are no shared responsibility, then what’s to stop the reviewers from just mindlessly clicking “approve” to shut the person up? I know for a fact that that happens all the time.
> If you were told that your code was sloppy, and if your performance review was affected by it, maybe it was sloppy code?
Sure, but again, isn’t “sloppy code” the lowest hanging fruit for a code reviewer? If the PR process doesn’t spot that, what exactly does it offer?
> But to say that your responsibility is lessened because it was blessed by code reviewers and they should share in your punishment, is, frankly, immature
I think we are just going to have to agree to disagree on this. If you are signing off on something, you are attaching your name and credibility towards it. I didn’t really want any of the reviewers to get into “trouble”, but i do think they should share a percentage of the responsibility on breakage if I am going to get in trouble if they signed off on something that was “sloppy”.
When I worked for Apple, we had a strict "all code needs to be reviewed" policy, which I had no problem with. Then, after being there for about 1.5 years, I make a PR, ask two different people to approve it, which they do, and it gets merged. A week later, the code is released, it looks like my code caused a fairly major bug.
My manager's manager and my manager had a meeting with me, and told me that this was unacceptable. I ask "what exactly was I supposed to do differently? Clearly this bug was sneaky enough to where two other people also didn't see it." Their response was "the code needs to work, you wrote the code, the code was sloppy. This is your fault." Apparently they took the bug pretty seriously, since it was actually mentioned in my yearly review.
This is fine, I probably should have tested the code a bit better, but it always annoyed me that my "sloppy" code managed to pass code review [1], and yet I'm the only one to get in trouble over it. If the PRs aren't catching bugs and I still am going to get in trouble, then I'm failing to see why I should bother waiting an hour for someone to look through it. Instead, why not make it so we can merge quickly, test it quickly, and revert it if something breaks?
[1] Obviously I'm biased here, but I honestly think that in this particular case I was pretty easily the least-liked person on the team, and it was easier to throw me under the bus than anything else.