How to review a pull request?

Question

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.

Possible duplicate of How should my team distribute checking pull requests? — gnat, Aug 10 '16 at 14:50
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 '16 at 14:53
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 '16 at 14:54
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 '16 at 14:54
how long should the average PR take? -- Long enough to answer those questions. — Robert Harvey, Aug 10 '16 at 14:56
I feel that the team as a whole should discuss this and maybe keep a checklist document on a Wiki page or so. — RemcoGerlich, Aug 10 '16 at 15:23

Apalala · Accepted Answer · 2016-08-10T15:18:05.027

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.

How to review a pull request?

1 Answers1