Gerrit: A Comprehensive Guide to Code Review and Testing

George Woodard

Home > Blog > Gerrit: A Comprehensive Guide to Code Review and Testing

Gerrit: A Comprehensive Guide to Code Review and Testing

Overview

Asterisk has been growing at a rapid pace since its creation, and a large part of this is due to the community. As an open source project, contributors from around the world have taken the time to write up some code, put it through a code review process, and then have it merged into the Asterisk code base itself. It sounds like a lot, but it’s easier than you think to get started writing your own patches for Asterisk and putting them up for review. Here’s a step by step guide on how to get started!

Setting Up Your Workspace

The Gerrit Usage wiki page[1] is a great start. You can follow the instructions here to create an account on Gerrit[2], add your SSH key to your account, and clone the repositories you wish to make changes to. It also covers the CLA agreement and git review, which you’ll need to upload your patch to Gerrit.

Once this is done, you can start working on your patch. Here are a few tips for keeping things organized:

Keep your mainline branches clean (18, 19, master, etc.). These should be updated to mirror the upstream repositories whenever you wish to make changes. Then, you can create a new branch from the mainline branch.
Name your branches something meaningful. Your change should have an Asterisk issue ID from JIRA[3]. It’s good practice to name the branch after the Asterisk issue ID to explicitly show what you’re working on. There might be a rare occasion where this is not the case, in which case you will want to come up with a descriptive branch name that explains what its purpose is.
Check out individual branches. If you prefer having a directory per branch rather than the full repository of all branches, you can do so. The Gerrit Usage wiki page covers how to do this. Sometimes this can be easier to manage, but it all depends on your personal preference.

I’ve Got My Patch, Now What?

Great! It’s a serious time investment to get involved with a project as large as Asterisk, so you’ve already crossed the biggest hurdle. First, let’s talk about how to format the patch. As an example, let’s say that your patch modifies some code in res_pjsip.c. Your commit message might look something like this:

res_pjsip.c: Fixed a deadlock when this condition happens.

Whenever this condition happens, a deadlock occurs. The solution was to change the locking order to match the locking order used by the rest of the code.

ASTERISK-12345

We like to have some sort of identifier first, followed by a very brief description of the patch. After that, be as descriptive as you want (the more, the better)! The only thing to watch out for is keeping line length under 72 characters. This is for release formatting purposes. You’ll notice too that there’s an Asterisk issue ID at the bottom. If you follow the exact format (ASTERISK-#####), Gerrit will automatically handle the status of the issue on JIRA, which we will talk about next. If there is an Asterisk issue that matches the review, someone will ask you to add it anyways if it’s not there, so save yourself some time and knock it out beforehand.

Speaking of JIRA, let’s talk about the CLA agreement. Before a patch can be submitted, an agreement must be signed and reviewed. This is a pretty short process overall, and you only have to do it once. Instructions on how to do this can be found on the Gerrit Usage wiki page, which should already be done if you followed the steps in the previous section.

Whenever you are planning on submitting a patch for review, there should always be a JIRA issue created for it. This will not only allow Gerrit to link back to it, but helps keep track of issues actively being worked on. Mark the issue as a patch / code contribution, and you should be set. Someone will acknowledge the review, assign it to you, and the rest is automated.

Creating the review for the patch is easy. Make sure your changes are committed to the branch. You should be able to issue the command git review to submit changes. If it’s not an available command, install the package for your system. The Gerrit Usage wiki page has instructions on how to set all this up. Once that’s installed, you can run the command git review upload your patch. Gerrit will use the branch name as the topic, which is why the branch name should reflect the Asterisk issue ID. It saves a little time on top of its organizational value. And just like that, you’ve submitted your first patch to Gerrit!

git review

There are some other neat things git review can do for you. Here’s some of the more commonly used options:

-D: Set the review to be in draft mode. This should make it a Work In Progress.

-r: Set the remote for Gerrit. Useful if working on a repository other than the default.
-t: Set the topic for the review. This can be changed in Gerrit as well.

Note that git review takes a branch parameter at the end. This is not necessary to provide. If you look inside of your Asterisk repository, a .gitreview file should be inside. There are different settings here that serve as defaults when cloning the repository. The branch should always match the version in this file.

What Happens Now?

Your patch is up on Gerrrit, waiting for other people to review it. Usually this will be done by someone on the Asterisk team. When they do, they can respond with a -2, -1, 0, +1, or +2. Here’s what they mean:

A -2 means that the review should not be merged in. This could be for a number of reasons, but the reviewer will let you know what their reasoning is.
A -1 means that there are things that can be improved or fixed in the review, ranging anywhere from a memory leak to a typo. This means that the change itself is fine, but needs a little more work before being merged in.
A 0 is not common, but is usually used when the reviewer has a question for the submitter.
A +1 means that the review looks good, although someone can still -1 or even -2 the review. However, a +1 does not mean that the review will be merged.
A +2 means that the review has been submitted. The change will go through test builds to verify there are no failures. If that succeeds, the change can be merged in. This is the final step of the review process.

If at any point you feel that more work needs to be done on your patch, you can set it to Work In Progress, which will disable comments and signal to others that it isn’t ready yet. If you have any questions about your patch or the comments someone left on the review, you can leave a comment or ask in an asterisk-dev channel, either the mailing list[4] or IRC[5]. There are plenty of developers in the community who know a great deal about Asterisk and are happy to answer any questions you have.

If you do need to make changes to your patch, the process is very simple. All you have to do is go back to your local branch with the commit, make the changes, git commit –amend them, and run git review again. Viola! Review updated.

Feedback and Cherry-picking

Typically, a review will spend quite a bit of time in the feedback stage. The easiest way to maintain your patch is to start with the lowest applicable branch and create your patch there. This is because reviewers usually leave comments in the lowest branch of a review to maintain consistency with discussion. If you get a -1 and address the feedback, you now only have to upload those changes to one review. Don’t worry about cherry-picking your change until it has passed a round of approval. This is for multiple reasons. It’s much easier to maintain this way, but sometimes you might need to submit patches in a series. Maybe you’ve made a change that depends on another change, but it makes more sense to have them separate rather than in one patch. This should be avoided where possible, but sometimes it’s necessary.

Once your patch has a +1, you’ll most likely be asked to cherry-pick your change to applicable branches. At the time of writing this, a change in 16 would be cherry-picked to 18, 19, and master. This can all be done within Gerrit itself. There might be merge conflicts, and if that’s the case, you can manually cherry-pick from your local branch, resolve the conflicts there, and then create another review for that branch from the command line just like you did to create the original review.

If you are working on a regression (e.g., 16.18.0-rc1 contained a change that introduced a crash that you are trying to fix), then that change would need to be cherry-picked into release branches as well. In this case, that change would be cherry-picked to 16.18 and the equivalents for the other mainline branches. This should never be done by anyone other than an Asterisk team member. Once a review is ready to be merged, someone will take care of it for you.

The Testsuite

With any change to Asterisk, it’s nice to have a test to go with it. You can also clone the testsuite from Gerrit in the same way you cloned Asterisk. There’s additional setup that needs to be done to get it up and running, which can be found on this wiki page[6]. If you’d like more information on the testsuite itself, this blog post[7] is a good read.

Unrelated to the testsuite but related to testing in general, we also have unit tests in Asterisk itself. Here’s another blog post[8] that covers the basics. These are generally easier to get started with than the testsuite, but different scenarios call for different tactics.

Sometimes a change actually requires a test.This is strictly for changes or additions that affect or add core functionality. Depending on the part of the code it touches, it may not be required, but tests are still strongly encouraged no matter what. Of course, this doesn’t prevent all bugs, but tests are a great way to catch ones that are easy to miss as well as prevent them from being introduced in the future.

Just like changes can be pushed up to Gerrit for Asterisk, the same can be done for the testsuite. The process is exactly the same, and the testsuite has branches now as well. You should always be thinking of ways to write tests for your code!