The role of the reviewer

One thing I value at Mozilla and within the Release Engineering team is, from my perspective, how mindful and respectful we are when reviewing each other’s work. Sure, we are not perfect but the review process is core to our DNA and something all code contributors practice on a daily basis.

Related to this, I’d like to highlight what I feel is the responsibility of the reviewer and what it means when you are asked to look at something. We often talk about what’s expected of the author and the guidelines around the work we are contributing but I’d like to post today about the responsibility of the reviewer. The r? and f? (request flags to review and provide feedback) are asking you to be involved. And that involvement is usually associated with an OKR or an individual deliverable. Below are ways in which we can stay productive and healthy while not letting the integrity and quality of our code drop.

• An r- blocks: I recently reached out to Dustin Mitchell (dustin), someone who is no stranger to code reviews and maintaining code bases, to discuss what makes a great review. He said one of the things he has struggled with a lot lately is how to decide when “making a point matters”. If your issue does not involve documentation, test coverage, standardization, or reliability, be critical with yourself on whether you want to block on it.
• Authors wait on you: While I don’t think we need to get down to minutes in response time, it’s always good to have a regular reminder to communicate more with authors when we can’t review within 24h since the request was made. Reaching out means either giving feedback of a first pass or by letting the person know when you will be able to look at the patch.
• Consider finding a compromise rather than defending your opinion: consensus is great but it won’t always happen. Instead try “if you do X, Y, and Z, I won’t block.” Examples include, ask for a follow up patch, provide more tests, give more documentation, or do some small tweaks.
• Include others in the review: when you get to a stalemate, perhaps suggests others to help review. Consider whether you can tolerate someone else giving an r+ and letting it land as is.
• Offer to help with code samples: for hard blockers or if the author does not understand what you are looking for, it can be helpful if you write some pseudo code or actual snippets to help get the patch landed.
• Encourage different approaches or ideas: This is about being inclusive and open to new styles. If it makes you uncomfortable, ask them to walk you through their approach, thought process, or for external references.
• Upgrade your communication channels: Sometimes walking through the proposed changes on a video call can resolve sticky points of confusion in a few minutes instead of days of email or bug comment exchanges.

From my experience, we largely do this well. That said, it’s good to discuss these, come to a common alliance, and set expectations from reviewers. I’d love to have this or something like it standardized across teams. For now, these are expectations within Releng and we will tweak it as a team together.

If you have any thoughts on any of the above, please feel free to reach out to me, at jlund@mozilla.com or on slack/irc @jlund. I’d love to discuss this more and clarify anything that I may have missed.