The workflow change that helped us deliver more, deliver faster

by Matthew Andrews

What is a software peer review?
In the same way someone peer reviewing an article to be published in a newspaper will be looking for more than just spelling and grammatical errors, an engineer peer reviewing a change to software will be checking the work hasn’t made any false assumptions; has fulfilled its requirements; won’t cause problems or extra work for anyone else later on; and, of course, it’s a good idea to check for spelling mistakes too.

Peer reviews 1.0
At FT Labs, our previous process for peer reviews was very ad-hoc. Sometimes developers sat down together to talk through the changes made in person but this was very disruptive and proved impractical when working with developers in different offices or time zones.

Alternatively, developers might invite their colleagues to check over a few changes they’d made via email. The problem with this approach was that almost all peer reviews happened after the code had been already been committed.

Committing is a difficult to reverse process that submits the feature or bug fix being worked on into a shared area where it can be downloaded as an update by other developers and testers.

Once you’ve ‘committed’ to that shared area if another developer wants to submit their new feature they must first download your update and combine it with their change before the system will let them submit their new code.

If someone submits a particularly bad update to that shared area then, until it is fixed, productivity across the whole team can be affected. A big mistake might completely break the project, preventing testers from testing and developers from developing. This is called breaking the build.

Breaking the build wastes time, slows down development and costs the business money.

As our code reviews were generally done after the work had been committed, or sometimes not at all, we frequently broke the build.

What is a Pull Request?
Over the past year we migrated to GitHub Enterprise for our software development, which has enabled us to adopt a new workflow for peer reviews called Pull Requests. (We have since moved to Stash, which enables the same workflow)

Rather than committing straight into that shared area and hoping for the best, pull requests are proposals for changes a developer would like to make that must be accepted before they become part of the software.

Once a pull request is sent other developers can review the set of changes, comment on the lines of code that have been changed — and they or the original developer can tweak those changes before finally clicking the button that merges their proposed changes into the project.

Simultaneously a pull request can initiate the automated tests for that project determining automatically whether the changes are likely to cause problems.

We no longer break the build
As we run automated tests on pull request, we know when a change is going to break the build before it is able to affect anyone else.

Although the occasional bad change does still make it through, it happens much less frequently — and each time it happens we add more automated tests to guarantee that the same mistake can never be made twice.

One of the first lessons we learnt was to make sure we always tested what the resulting code would be after the pull request was accepted. The problem we discovered with tools that didn’t do this and just tested the pull request in isolation was that sometimes a pull request itself would pass all the tests but it could still break when combined with other recent changes to the project.

Everyone on the same page
A mistake we made early on was to always assign pull requests to the most senior developer. That person quickly became a bottleneck, slowing development velocity. If that person falls more than a day behind, work grinds to a halt.

Code reviews are more than about getting work checked, they’re a tool for team communication. It’s often just as valuable to assign a pull request to a developer that is less experienced with the code that it touches as it helps spread knowledge about different areas of a product and help prevent situations where only certain individuals can work on certain parts of products.

Working in this way helps keep everyone on the team aware of changes across the entire project. Reviewing pull requests has become a tool to help new developers get up to speed.

Continuous delivery
Because unfinished or broken code is not allowed to become part of the project, our application is always ready to go live. This means that as soon as a feature is complete and signed off by the test team, it could be deployed. That code needn’t sit waiting for other, unrelated features or bug fixes to be finished before going live.

This means finished code could get out and in front of customers and start delivering business value as soon as it is ready. We hope to move closer to this type of release cycle as our automated tests become comprehensive.

It’s too easy not to do
The additional overhead of creating a pull request compared to just pushing code directly into the project is just a few extra clicks. After 6 months we’ve found it’s not worth the risk not to do it and now all changes to all projects are made through pull requests.

Happier, more productive
Pull requests have revolutionised the way we work. They make the team more productive; encourage more communication; and result in better products built.

Rules for effective pull requests:

• Always create a pull request, even for small changes.
• Don’t assign all pull requests to the same person.
• Run automated tests when pull requests are created or updated. Don’t merge if they fail.
• Run automated tests against what the code will be once its merged (the merged product)