4 ms·

Agree with this: pull requests are code reviews. Web UIs with visual diffs make this even more efficient.

eloisant 13y ago
I don't know, code review without running the code is a very very light variant, too light to call it code review I believe.
- martinvanaken 13y ago
  Hi, I think both are actually useful. The CI is supposed to run the tests (even if I like to run some myself when reviewing), but I agree that starting the application is a part of the review - you should not stop at just looking at the code (but just startint the application does not cut it either for me). Martin (OP)
- alphadevx 13y ago
  In my team each feature branch is run by our build server (unit test), even before merge.
- benihana 13y ago
  Code reviews are about psychology. When you know your teammates are going to be reviewing your code, you write it differently than you do when you know no one but you will ever look at it. Static analysis and regression tests are tools to make sure the code isn't broken.
  - masklinn 13y ago
    > Code reviews are about psychology. Code reviews are also about distance from the code. When it's your production it's easy to miss the forest for the trees. That's why books are generally better when beta readers or reviewers are involved. Code reviews are also about spreading knowledge (about the subsystems and about choices made in implementation and the reason for them) and increasing the code's bus factor.
- warp 13y ago
  And yet most shops don't even do that :(
- masklinn 13y ago
  Code review is about reviewing the code. That's literally what the name says. CI runs the tests, QA test the final behavior, code review checks the code.
  - potatolicious 13y ago
    That's nice in theory, but in practice another dev really should be running the code and doing some manual testing. Having full knowledge of the innards allows a competent dev to foresee potential trouble spots in the architecture and hit some edge cases to make sure they don't break. CI may run the tests, but that assumes your tests have captured every possible edge case. It may be the intention of tests to be comprehensive and cover everything under the sun, but that is rarely the case in reality. Ditto QA - they should be able to black-box test the software and all of its possible states, but in reality something is going to pass through the net. Having a dev run the code themselves and poke around in it is just a plain good idea.
    - masklinn 13y ago
      It might be a somewhat good idea (in my experience, the scenario you outline is significantly less likely than domain knowledge by QA hitting patterns the developer with limited domain knowledge had not anticipated), but it's not a necessity for code review to be a good idea.
    - dragonwriter 13y ago
      > That's nice in theory, but in practice another dev really should be running the code and doing some manual testing. Why? If they are reviewing the code including the unit tests, shouldn't they instead be suggesting any missing unit tests, which then become permanent and reusable rather than "doing some manual testing"? > CI may run the tests, but that assumes your tests have captured every possible edge case. It may be the intention of tests to be comprehensive and cover everything under the sun, but that is rarely the case in reality. Insofar as the other dev doing code review can address this with their own testing, isn't it better for this to be done-once and preserved by adding automated tests rather than done-and-lost by doing manual tests?
- courtewing 13y ago
  Using pull requests as your code review doesn't force you to not run the code. My team uses forks of upstream repos and all merges upstream happen via a PR that is peer reviewed. It isn't uncommon for us to pull down the changes of a PR locally to "play around" with a new feature or change to existing functionality. We have tests in place to help stop regressions, but sometimes seeing a change in action can really help to put the corresponding code in context. Most of my team uses hub, but you can check out a pull request easily enough with git: git fetch <remote> +refs/pull/<pull-request-number>/head git checkout FETCH_HEAD

I don't know, code review without running the code is a very very light variant, too light to call it code review I believe.

martinvanaken 13y ago
Hi, I think both are actually useful. The CI is supposed to run the tests (even if I like to run some myself when reviewing), but I agree that starting the application is a part of the review - you should not stop at just looking at the code (but just startint the application does not cut it either for me). Martin (OP)
alphadevx 13y ago
In my team each feature branch is run by our build server (unit test), even before merge.
benihana 13y ago
Code reviews are about psychology. When you know your teammates are going to be reviewing your code, you write it differently than you do when you know no one but you will ever look at it. Static analysis and regression tests are tools to make sure the code isn't broken.
- masklinn 13y ago
  > Code reviews are about psychology. Code reviews are also about distance from the code. When it's your production it's easy to miss the forest for the trees. That's why books are generally better when beta readers or reviewers are involved. Code reviews are also about spreading knowledge (about the subsystems and about choices made in implementation and the reason for them) and increasing the code's bus factor.
warp 13y ago
And yet most shops don't even do that :(
masklinn 13y ago
Code review is about reviewing the code. That's literally what the name says. CI runs the tests, QA test the final behavior, code review checks the code.
- potatolicious 13y ago
  That's nice in theory, but in practice another dev really should be running the code and doing some manual testing. Having full knowledge of the innards allows a competent dev to foresee potential trouble spots in the architecture and hit some edge cases to make sure they don't break. CI may run the tests, but that assumes your tests have captured every possible edge case. It may be the intention of tests to be comprehensive and cover everything under the sun, but that is rarely the case in reality. Ditto QA - they should be able to black-box test the software and all of its possible states, but in reality something is going to pass through the net. Having a dev run the code themselves and poke around in it is just a plain good idea.
  - masklinn 13y ago
    It might be a somewhat good idea (in my experience, the scenario you outline is significantly less likely than domain knowledge by QA hitting patterns the developer with limited domain knowledge had not anticipated), but it's not a necessity for code review to be a good idea.
  - dragonwriter 13y ago
    > That's nice in theory, but in practice another dev really should be running the code and doing some manual testing. Why? If they are reviewing the code including the unit tests, shouldn't they instead be suggesting any missing unit tests, which then become permanent and reusable rather than "doing some manual testing"? > CI may run the tests, but that assumes your tests have captured every possible edge case. It may be the intention of tests to be comprehensive and cover everything under the sun, but that is rarely the case in reality. Insofar as the other dev doing code review can address this with their own testing, isn't it better for this to be done-once and preserved by adding automated tests rather than done-and-lost by doing manual tests?
courtewing 13y ago
Using pull requests as your code review doesn't force you to not run the code. My team uses forks of upstream repos and all merges upstream happen via a PR that is peer reviewed. It isn't uncommon for us to pull down the changes of a PR locally to "play around" with a new feature or change to existing functionality. We have tests in place to help stop regressions, but sometimes seeing a change in action can really help to put the corresponding code in context. Most of my team uses hub, but you can check out a pull request easily enough with git: git fetch <remote> +refs/pull/<pull-request-number>/head git checkout FETCH_HEAD