Avoiding LGTM PR Cultures

Introduction

Making a code change when using a distributed version control system (DVCS) like Git is usually done by packaging a change on a branch as a “pull request” (PR) which indicates the author would like the project to “pull” the change into it.

This was, and is, a key part of open source projects as it allows outside contributors to contribute to a project in a controlled way, however many internal software development teams also work in this fashion as there are many benefits of this approach over committing directly to a shared branch or trunk.

I’ve seen the pull request approach have a positive impact on software quality since pull requests facilitate discussion through peer reviews and allow running of automated tests against every commit and change that is proposed to be merged into the default branch.

taken lgtm.jpg

What is a LGTM PR culture?

I’ve also seen some negative behaviours emerge when moving to pull request based development which I’ll call a LGTM PR culture.

LGTM is a common acronym found in peer reviews of pull requests which means “Looks Good To Me”, and I’ve seen teams let unsuitable changes through with LGTM comments without doing solid peer reviews and testing.

How do you know if you have a LGTM PR culture?

One way to “test” your peer review process is by creating PRs and leaving a subtle bug or something not quite right that you know about in the PR. When it gets reviewed do you get a LGTM? I did this recently and whilst the PR didn’t even do what it was meant to do I received a LGTM 😕

2dsag2

How can you move away from a LGTM PR culture?

It’s tempting to just tell everyone to do better peer reviews but it’s not that easy!

I’ve found there’s some steps that an author of a pull request can do to facilitate better pull request reviews and lead towards a better culture.

1. Make pull requests as small as possible:

The smaller the pull request the more likely you’ll get specific and broad feedback on it – and you can then iterate on that feedback. A 500 line change is daunting to review and will lead to more LGTMs. For larger refactorings where there’ll be lots of lines changed, you can start with a small focussed change and get lots of review and discussion, and once the refactoring is established with a smaller example you can easily apply that feedback to a broader impact PR that won’t need as much feedback as you’ve established the new pattern previously.

2. Review your own pull request

Review your own work. This works best if you do something else then come back to it with a fresh mind. Anything you’re unsure about or doesn’t look right leave it as a comment on your own PR to encourage other reviewers to look closely at those areas also.

3. Include clear instructions for reviewing and testing your pull request

A list of test steps is good as well as asking for what type of feedback you’d like – you can explicitly ask reviewers something like “please leave a comment after your review listing what you tested and what areas of the code you reviewed.” This discourages shortcuts and LGTMs.

4. Test your peer review process – see above.

Conclusion

Developing software using pull requests can mean much higher quality code and less technical debt due to the feedback on peer reviews that accompany pull requests. As an author you can take steps to ensuring pull requests are easy to review and encourage a culture of effective peer reviews.

→ How Canaries Help Us Merge Good Pull Requests

I recently published an article on the WordPress.com Developer’s Blog about how we run automated canary tests on pull requests to give us confidence to release frequent changes without breaking things. Feel free to check it out.