4 ms·

I am vehemently against making reviewers checkout and test code. You should have tests that demonstrate correctness with very high confidence, and a release pro

I am vehemently against making reviewers checkout and test code. You should have tests that demonstrate correctness with very high confidence, and a release process that can find bugs early. Expecting reviewers to checkout and run/test things is such a time sink, and over the course of a year of doing it at a new work place, I've never seen it be the thing that caught a bug. If anything I feel like it caused more bugs because people end up getting laxed with tests.

JKCalhoun 6y ago
It's as though code reviews are a waste of time. I have to say, now over 30 years as a developer, I've only been introduced to code reviews in the last few years. Somehow, I have no idea how, we shipped code for nearly 30 years without code reviews or unit tests. Thank god we now no longer ship with bugs.
- doctor_eval 6y ago
  Although I think you’re being sarcastic, I’m sort of in the same camp. We started doing code reviews a few years ago after years of informal releases, and I found they acted like a sieve; I reckon we halved our deployed bug count, at reasonably low cost, but also code style and knowledge improved across the board. Code reviews are not a silver bullet, and we still released some whoppers, but halving the deployed bug count and improving the code base is a worthwhile outcome. We’ve recently started doing CD with E2E tests and I reckon E2E has halved the deployed bug count again. It’s definitely stopped bugs from entering production. There always seems to be a trade off between the cost of action (in resources, time to release, flexibility) and quality, there is no right answer (despite what people will tell you), and it’s up to you to set the acceptable bar for released bugs, and to deal with the consequences. For me, code reviews improved bug counts less than I’d expected but had other quality benefits that I felt more than made up for the modest cost of the additional ceremony.
  - JKCalhoun 6y ago
    I think you have a point about spreading knowledge. Rather than code review though, I think having engineers pair up on a feature is another (better?)way to knowledge share that probably is more efficient time-wise?
    - lmilcin 6y ago
      I prefer pairing up. Giving feedback on code that has already been written, and by more junior member of the team, is fraught with problems. People get defensive thinking they have already completed the task and now I am delaying the closure. It is much easier to share friendly hints and observations while writing the code. When the code is written it is difficult to show how it could have looked or demonstrate the process of getting there. And so even though my org requires us to conduct code reviews (and I find them helpful, at a hefty cost), I will still ask people to pair up with me regularly. This to get to know the person better, to see what the problem is (and sometimes I am the problem), to help work out better design, etc. All of which is difficult with regular review.
      - chihuahua 6y ago
        For a while I worked on a team where we wrote all code in pairs. The team room had 4 computers, each of which had 2 pairs of 2 monitors and 2 mice and 2 keyboards. During the day, every hour or so, each dev would switch between driver and passenger roles. It was a great way to work, because junior people could get up to speed quickly, and it was much less likely that would would get stuck on any problem. As a bonus, we didn't need code reviews because every line of code had already been seen (and if necessary, critiqued) by another person. One drawback was that you could not start working until your partner for the day arrived, and you'd have to stop working when the first person of a pair left for the day. I'm not sure how this way of working pays off during the annual performance reviews, I didn't stick around long enough to find out.
      - BobbyJo 6y ago
        I am all for pairing up and interacting with people directly and in a human manner. It's good for building rapport and trust so that things run smoother in every facet. That being said, I think impersonal reviews are still needed. You don't always have time for the soft human touch, especially at a smaller startup. The defensiveness some people have should really only ever be a short term problem. I've worked with people like that and found that being persistent, objective (follow standards that you make everyone aware of) and consistent with reviews fixed that over time. Rapport building is good , but you can't require it every time, so you need the other end of the spectrum too.
- keeganpoppen 6y ago
  Rich Hickey nails this hilarious in his (amazing) "simple made easy" talk from 2011 where he calls this mentality guard rail driven development: we don't drive down the highway careening back and forth off the guard rails as we drive down the road, thinking "thank goodness these rails are here or i wouldn't be able to drive down this highway"-- why do we think this about our unit tests? im hardly doing the bit justice because it's truly an all-timer-- highly recommend the whole thing, beyond just that bit of it (which i think comes early on iirc): https://www.youtube.com/watch?v=kGlVcSMgtV4 https://www.youtube.com/watch?v=kGlVcSMgtV4
elwell 6y ago
This boggles my mind. I checkout and manually test every PR that I review, and the vast majority of the time (> 80%) I do find a bug. In fact, I'm very suspicious whenever my PR's get the green checkmark without any other feedback.
- BobbyJo 6y ago
  If 80% of the code you're sent to review has bugs, your upstream process has to be horribly broken.
  - lixtra 6y ago
    Or the PRs are just reasonable large with 1-25 bugs per 1000 loc. https://stackoverflow.com/questions/2898571/basis-for-claim-that-the-number-of-bugs-per-line-of-code-is-constant-regardless https://stackoverflow.com/questions/2898571/basis-for-claim-...
bigbillheck 6y ago
> Expecting reviewers to checkout and run/test things is such a time sink I agree. Is everybody writing and running their own bespoke testing on each PR, with no cross-person commonality, in which case why is your code so complicated that they have to do this? Or is everybody doing the same kind of thing every time, in which case why don't you automate that and run it in a CI process?
ufmace 6y ago
To be clear, there definitely should be unit tests run via a CI system. That's a minimum bar instead of a cure-all though. What checks that there are enough unit tests to cover appropriate cases, but not so many as to be noisy or excessively time-consuming? What checks that the tests actually assert the correct behavior? Code review does. What do you do when your test suite takes multiple hours to run, but still doesn't catch all of the bugs? IMO, you can only really apply one ideological principle to your practices. I think that should be that your code works correctly while being no more complex than needed. Ideally many things shouldn't need to be done, but in the end, you do what you need to do to make sure that's true, including reviewers checking out code and working with it. If your ideological principle is instead that reviewers must never checkout code and try it, then you will eventually sacrifice the other in favor of it. I don't think that's a good result.

JKCalhoun 6y ago

It's as though code reviews are a waste of time. I have to say, now over 30 years as a developer, I've only been introduced to code reviews in the last few years. Somehow, I have no idea how, we shipped code for nearly 30 years without code reviews or unit tests. Thank god we now no longer ship with bugs.

doctor_eval 6y ago
Although I think you’re being sarcastic, I’m sort of in the same camp. We started doing code reviews a few years ago after years of informal releases, and I found they acted like a sieve; I reckon we halved our deployed bug count, at reasonably low cost, but also code style and knowledge improved across the board. Code reviews are not a silver bullet, and we still released some whoppers, but halving the deployed bug count and improving the code base is a worthwhile outcome. We’ve recently started doing CD with E2E tests and I reckon E2E has halved the deployed bug count again. It’s definitely stopped bugs from entering production. There always seems to be a trade off between the cost of action (in resources, time to release, flexibility) and quality, there is no right answer (despite what people will tell you), and it’s up to you to set the acceptable bar for released bugs, and to deal with the consequences. For me, code reviews improved bug counts less than I’d expected but had other quality benefits that I felt more than made up for the modest cost of the additional ceremony.
- JKCalhoun 6y ago
  I think you have a point about spreading knowledge. Rather than code review though, I think having engineers pair up on a feature is another (better?)way to knowledge share that probably is more efficient time-wise?
  - lmilcin 6y ago
    I prefer pairing up. Giving feedback on code that has already been written, and by more junior member of the team, is fraught with problems. People get defensive thinking they have already completed the task and now I am delaying the closure. It is much easier to share friendly hints and observations while writing the code. When the code is written it is difficult to show how it could have looked or demonstrate the process of getting there. And so even though my org requires us to conduct code reviews (and I find them helpful, at a hefty cost), I will still ask people to pair up with me regularly. This to get to know the person better, to see what the problem is (and sometimes I am the problem), to help work out better design, etc. All of which is difficult with regular review.
    - chihuahua 6y ago
      For a while I worked on a team where we wrote all code in pairs. The team room had 4 computers, each of which had 2 pairs of 2 monitors and 2 mice and 2 keyboards. During the day, every hour or so, each dev would switch between driver and passenger roles. It was a great way to work, because junior people could get up to speed quickly, and it was much less likely that would would get stuck on any problem. As a bonus, we didn't need code reviews because every line of code had already been seen (and if necessary, critiqued) by another person. One drawback was that you could not start working until your partner for the day arrived, and you'd have to stop working when the first person of a pair left for the day. I'm not sure how this way of working pays off during the annual performance reviews, I didn't stick around long enough to find out.
    - BobbyJo 6y ago
      I am all for pairing up and interacting with people directly and in a human manner. It's good for building rapport and trust so that things run smoother in every facet. That being said, I think impersonal reviews are still needed. You don't always have time for the soft human touch, especially at a smaller startup. The defensiveness some people have should really only ever be a short term problem. I've worked with people like that and found that being persistent, objective (follow standards that you make everyone aware of) and consistent with reviews fixed that over time. Rapport building is good , but you can't require it every time, so you need the other end of the spectrum too.
keeganpoppen 6y ago
Rich Hickey nails this hilarious in his (amazing) "simple made easy" talk from 2011 where he calls this mentality guard rail driven development: we don't drive down the highway careening back and forth off the guard rails as we drive down the road, thinking "thank goodness these rails are here or i wouldn't be able to drive down this highway"-- why do we think this about our unit tests? im hardly doing the bit justice because it's truly an all-timer-- highly recommend the whole thing, beyond just that bit of it (which i think comes early on iirc): https://www.youtube.com/watch?v=kGlVcSMgtV4 https://www.youtube.com/watch?v=kGlVcSMgtV4

elwell 6y ago

This boggles my mind. I checkout and manually test every PR that I review, and the vast majority of the time (> 80%) I do find a bug. In fact, I'm very suspicious whenever my PR's get the green checkmark without any other feedback.

BobbyJo 6y ago
If 80% of the code you're sent to review has bugs, your upstream process has to be horribly broken.
- lixtra 6y ago
  Or the PRs are just reasonable large with 1-25 bugs per 1000 loc. https://stackoverflow.com/questions/2898571/basis-for-claim-that-the-number-of-bugs-per-line-of-code-is-constant-regardless https://stackoverflow.com/questions/2898571/basis-for-claim-...

bigbillheck 6y ago

> Expecting reviewers to checkout and run/test things is such a time sink I agree. Is everybody writing and running their own bespoke testing on each PR, with no cross-person commonality, in which case why is your code so complicated that they have to do this? Or is everybody doing the same kind of thing every time, in which case why don't you automate that and run it in a CI process?

ufmace 6y ago

To be clear, there definitely should be unit tests run via a CI system. That's a minimum bar instead of a cure-all though. What checks that there are enough unit tests to cover appropriate cases, but not so many as to be noisy or excessively time-consuming? What checks that the tests actually assert the correct behavior? Code review does. What do you do when your test suite takes multiple hours to run, but still doesn't catch all of the bugs? IMO, you can only really apply one ideological principle to your practices. I think that should be that your code works correctly while being no more complex than needed. Ideally many things shouldn't need to be done, but in the end, you do what you need to do to make sure that's true, including reviewers checking out code and working with it. If your ideological principle is instead that reviewers must never checkout code and try it, then you will eventually sacrifice the other in favor of it. I don't think that's a good result.