What is your team code review process?
#codequality#productivity#discuss#question
José Jesús Ochoa Torres

José Jesús Ochoa Torres @josejesusochoatorres

About: Software engineer @ Wizeline

Location:
México
Joined:
Oct 13, 2020

What is your team code review process?

Publish Date: Oct 25 '20
7 4

In my last five years, I have been working in different dev teams, and each one has a different code review process. I am interested in know-how is the code review process of your team.

A couple of questions could provide more information:

  • Do you use an integrated tool in Slack to notify a new pull request? Witch one?
  • What is the minimum of approvals needed in a pull request?
  • How the reviewers are assigned?
  • Do you use a pull request template?
  • Are you evaluating unit test cases automatically in your PRs?
  • What other automated tools are you using?
  • After approving the PR what happens?
  • Is your current process working fine?
  • How is this process?
  • How do you manage your development environments?

Comments 4 total

  • Charanjit Chana
    Charanjit ChanaOct 25, 2020

    I’d be interested to hear some more on this topic.

    In my previous role it was all manual although we had notifications of every commit and verified code quality and that tests passed with every commit (SVN). The notifications were very useful. We gamified it and the added accountability made everyone more aware of their responsibilities. No one wanted to be at the top of the chat for most breakages!

  • Periklis Gkolias
    Periklis GkoliasOct 25, 2020

    For all the companies I have worked for it is pretty much identical. A dev branches out from master/main/sprint/develop branch.

    They do the work. They open a pull request against the origin branch.

    The reviewer checks the diffs (for styling, readability, approach, architecture etc), does a smoke test, maybe uses tools like linters or sonar to point out problems in disguise.

    If they exist, checks the unit tests as well (do they succeed, do they need to be amended, do we need more).

  • José Jesús Ochoa Torres
    José Jesús Ochoa TorresOct 25, 2020

    @Periklis and @charanjit; thanks for contributing to this post.

    How the reviewers are assigned to the pull request?

    For example in my last 3 teams we followed these approaches:

    • We have something like a table where we can see who will review our code, so when I do a PR I have to assign my ticket to this person and that is a review Q1 then when this person finish the review needs to assign to another specific teammate to review the PR one more time that should be the review Q1, just if both teammates approve it the code is merged to dev where a testing team will review it.

    • In another team, we don't have to assign it to a specific teammate, but we have a simple rule, after taking a new ticket from the backlog you have to review a Pull request thicket. In that way, we avoid having a lot of open pull requests. Teammates can review wherever thicket they want or understand.

    • In my current team we add from 2 to 5 code reviewers but we need just 2 approvals (to merge the PR). How we select our reviewers? Well, we select people who have been working with us, teammates with high skills in the main pull request topic, teammates who worked in the same than us, I mean, teammates who worked on the same files or code in the past.

    In the first two approaches, it works better for me. The problem with the last one is, if you assign 5 teammates usually they will say "mm another teammate will review it"

    What do you think? Is pretty similar to you?

    • Periklis Gkolias
      Periklis GkoliasOct 25, 2020

      Pretty much. I have seen the following three approaches (and also a "screw code review" one), all of them can work if the team is committed and knows the drawbacks.

      • Internal knowledge. I have refactored the environmental variables...who's the "guru" or the accountable person here? I will give it to them.
      • Team meeting. The dev presents their work to the team or a subset of the team. An approval or decline, should happen asap, ideally at the meeting's end.
      • Leave it hanging. Let the team know there is a pending review. Anyone can pick it up, sometimes someone not senior enough to handle it.

      Any number of people can approve, they carry an equal level of responsibility though. This can turn out very complex like: "I will not approve because John has not reviewed yet" or "Screw it, lets move to it production without review".

Add comment