4

Does a pull request review consist of testing, code review or both and how long should the average PR take?

I don't know whether the testing just involves testing the functionality that's been altered, or thoroughly testing the whole app and what purpose this has if the author has already tested this.

If it involves a code review should it be a brief glance over within github or a thorough review including a parse through the code in action within your chosen IDE.

I'm part of a paid development team where 100% of our PRs come from our team.

Improve this question
7
  • Possible duplicate of How should my team distribute checking pull requests?
    – gnat
    Aug 10, 2016 at 14:50
  • 1
    That doesn't seem like a suitable duplicate.
    – Robert Harvey
    Aug 10, 2016 at 14:52
  • That question is how to distribute the PRs this is how to actually do it. I'm ashamed to say I've never reviewed a pull request.
    – Declan McKenna
    Aug 10, 2016 at 14:53
  • 1
    You should review and test a pull-request from somebody outside your organization as if its author is a malicious party who wants to introduce a security hole in your program.
    – CodesInChaos
    Aug 10, 2016 at 14:54
  • 2
    Does a pull request review consist of testing, code review or both -- A review consists of whatever the reviewer needs to do to determine that the change is suitable to incorporate into the main branch. Questions they might as are: Is this change useful? Is it consistent with the rest of the project? Does it work? Does it fulfill our design principles? Your set of questions might be different.
    – Robert Harvey
    Aug 10, 2016 at 14:54

1 Answer 1

Reset to default
7

There are procedural tasks to be performed in a code review:

  • That the code abides to the coding standards agreed upon by the team.
  • That it is complete regarding unit tests, documentation, etc.
  • Others in the same vein.

But the most important tasks in any code review are to verify that:

  • You can understand the code (all of it) because it is written clearly.
  • The code is correct in the sense of absence of errors (bugs).
  • The code is correct in the handling of expected border cases.
  • The code is correct in that it performs the intended task.

Additionally, if you're polite, you can suggest better (clearer, more efficient, library-based) ways of implementing the functionality.

Improve this answer

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.