Pul Requests

When working with different branches it is easy to do pull requests before merging changes into the master branch. The most common usage of this is to be able to do code review prior to the merge. There is however a lot more possible to assist your daily development process and achieve higher quality software.

In general you should do as much as you can before merging because up until that point it is straightforward as to what caused any problem, as you are only dealing with those changes. Another approach that needs to be taken is to automate as much as possible.

While the list of things you could do on a pull request is infinite, I explain the most common to do and what the benefits are.

A code review is a process where one or more other software developers go through the code. While many believe that this is a process to find bugs, I disagree as automated tests are much better for this. What you should do during a code review is verify that the code makes sense semantically, does it actually do what we want it to do. This can normally be validated based on the written unit tests as they must guarantee that everything functions as expected. By checking the unit tests and verify that the unit tests actually check what should be checked you verify the correctness of the test itself. During code review it is also important to try and understand the code, this will give you some insight in how clean and easy to understand the code is, if this is too complicated then changes should be made to make later maintenance easier.

Running unit tests on your pull requests verify that your changes didn’t break anything else. This reliefs all developers of the dreadful task to think about all possible cases and whether something has changed. For this to work you have to make sure your unit tests run as fast as possible and are reliable. A failing test due to some race condition is easily ignored as an ‘unreliable’ test and dismissed even when it fails.

Code coverage is a tool I always use to make sure I didn’t forget to test anything. Others often use it to see whether they have written enough tests by simply checking the coverage with a certain required goal but I strongly disagree with this practice. Code coverage does not tell you how well your tests are, or if you written enough. If you inverse the code coverage and see what lines have not been covered then you know that you did not test these lines and therefor not enough tests where written.

Mutation testing takes it one step further by manipulating the code and verifying whether the change was detected. Because the change in code resulted in a different outcome, at least one test should have failed. This often takes a lot of time due to the complexity and the size of the search field, so doing it on every pull request is only possible if you can minimise the code that is being mutated.

Code analysis can be used for different reasons, to check whether the code applies to the coding standard or to check for some possible bugs. Both of them are valid points and are a good idea to do. It is however important to know that often these tools report false negatives regarding to the problems they detect. How to deal with this is up to you, but trying to minimise the amount of detected problems should always be the first thing on your mind.

A good procedure is to make sure that if any of these steps fail, or does not meet a certain requirement the pull request is automatically denied and merging bad code is prevented. Setting up such a system may cause a lot of friction with people who just want to merge code and fix later and the argument of the process slowing things down will often be used to reject to it. But just as with proper testing the opposite is true and doing such a procedure will prevent you to spend a lot of time on maintenance.


Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out /  Change )

Google photo

You are commenting using your Google account. Log Out /  Change )

Twitter picture

You are commenting using your Twitter account. Log Out /  Change )

Facebook photo

You are commenting using your Facebook account. Log Out /  Change )

Connecting to %s

This site uses Akismet to reduce spam. Learn how your comment data is processed.