docs/contributing/code-reviewing.md
Code review is a key part of how Zulip does development. It's an essential aspect of our process to build a high-quality product with a maintainable codebase. See the pull request review process guide for a detailed overview of Zulip's PR review process.
Zulip has an active contributor community, and just a small handful of maintainers who can do the final rounds of code review. As such, we would love for contributors to help each other with making pull requests that are not only correct, but easy to review. Doing so ensures that PRs can be finalized and merged more quickly, and accelerates the pace of progress for the entire project.
If you're new to open source, this may be the first time you do a code review of anyone's changes! We have therefore written this step-by-step guide to be accessible to all Zulip contributors.
One of the best ways to improve the quality of your own work as a software engineer is to do a code review of your own work before submitting it to others for review. We thus strongly encourage you to get into the habit of reviewing you own code. You can often find things you missed by taking a step back to look over your work before asking others to do so, and this guide will walk you through the process. Catching mistakes yourself will help your PRs be merged faster, and folks will appreciate the quality and professionalism of your work.
Doing code reviews is a valuable contribution to the Zulip project. It's also an important skill to develop for participating in open-source projects and working in the industry in general. If you're contributing to Zulip and have been working in our code for a little while, we would love for you to start doing code reviews!
Anyone can do a code review -- you don't have to have a ton of experience, and you don't have to have the power to ultimately merge the PR. The sections below offer accessible, step-by-step guidance for how to go about reviewing Zulip PRs.
For students participating in Google Summer of Code or a similar program, we expect you to spend a chunk of your time each week (after the first couple of weeks as you're getting going) doing code reviews.
Whether you are reviewing your own code or somebody else's, this section describes how to go about it.
If you are reviewing somebody else's code, you will likely need to first fetch
it so that you can play around with the new functionality. If you're working in
the Zulip server codebase, use our Git tool
tools/fetch-rebase-pull-request to check out a pull request locally and rebase
it onto main.
The following review steps apply to the majority of PRs.
Think about the issue:
Start by rereading the issue the PR is intended to solve. Are you confident that you understand everything the issue is asking for? If not, try exploring the relevant parts of the Zulip app and reading any linked discussions on the development community server to see if the additional context helps. If any part is still confusing, post a GitHub comment or a message on the development community server explaining precisely what points you find confusing.
Now that you're confident that you understand the issue, does the PR address all the points described in the issue? If not, is it easy to tell without reading the code which points are not addressed and why? Here is a handful of good ways for the author to communicate why the issue as written might not be fully solved by the PR:
Think about the code:
Make sure the PR uses clear function, argument, variable, and test names.
Every new piece of Zulip code will be read many times by other developers, and
future developers will grep for relevant terms when researching a problem, so
it's important that variable names communicate clearly the purpose of each
piece of the codebase.
Make sure the PR avoids duplicated code. Code duplication is a huge source of bugs in large projects and makes the codebase difficult to understand, so we avoid significant code duplication wherever possible. Sometimes avoiding code duplication involves some refactoring of existing code; if so, that should usually be done as its own series of commits (not squashed into other changes or left as a thing to do later). That series of commits can be in the same pull request as the feature that they support, and we recommend ordering the history of commits so that the refactoring comes before the feature. That way, it's easy to merge the refactoring (and minimize risk of merge conflicts) if there are still user experience issues under discussion for the feature itself.
Good comments help. It's often worth thinking about whether explanation
in a commit message or pull request discussion should be included in
a comment, /docs, or other documentation. But it's better yet if
verbose explanation isn't needed. We prefer writing code that is
readable without explanation over a heavily commented codebase using
lots of clever tricks.
Make sure the PR follows Zulip's coding style. See the Zulip coding style documentation for details. Our goal is to have as much of this as possible verified via the linters and tests, but there will always be unusual forms of Python/JavaScript style that our tools don't check for.
If you can, step back and think about the technical design. There are a lot of considerations here: security, migration paths/backwards compatibility, cost of new dependencies, interactions with features, speed of performance, API changes. Security is especially important and worth thinking about carefully with any changes to security-sensitive code like views.
Think about testing:
The CI build tests need to pass. One can investigate
any failures and figure out what to fix by clicking on a red X next
to the commit hash or the Detail links on a pull request. (Example:
in #17584,
click the red X before 49b10a3 to see the build jobs
for that commit. You can see that there are 7 build jobs in total.
All the 7 jobs run in GitHub Actions. You can see what caused
the job to fail by clicking on the failed job. This will open
up a page in the CI that has more details on why the job failed.
For example this
is the page of an "Ubuntu 22.04 (Python 3.10, backend + frontend)" job.
See our docs on continuous integration
to learn more.
Make sure the code is well-tested; see below for details. The PR should summarize any manual testing that was done to validate that the feature is working as expected.
Think about the commits:
Does the PR follow the principle that “Each commit is a minimal coherent idea”? See the commit discipline guide to learn more about commit structure in Zulip.
Does each commit have a clear commit message? Check for content, format, spelling and grammar. See the Zulip commit discipline documentation for details on what we look for.
You should also go through any of the following checks that are applicable:
Error handling. The code should always check for invalid user input. User-facing error messages should be clear and when possible be actionable (it should be obvious to the user what they need to do in order to correct the problem).
Translation. Make sure that the strings are marked for translation.
Completeness of refactoring. When reviewing a refactor, verify that the changes are
complete. Usually, one can check that efficiently using git grep,
and it's worth it, as we very frequently find issues by doing so.
Documentation updates. If this changes how something works, does it update the documentation in a corresponding way? If it's a new feature, is it documented, and documented in the right place?
mypy annotations in Python code. New functions should be annotated using
mypy and existing annotations should be updated. Use of Any, ignore, and
unparameterized containers should be limited to cases where a more precise
type cannot be specified.
The tests should validate that the feature works correctly, and
specifically test for common error conditions, bad user input, and potential
bugs that are likely for the type of change being made. Tests that exclude
whole classes of potential bugs are preferred when possible (e.g., the common
test suite test_markdown.py between the Zulip server's frontend and backend
Markdown processors, or the GetEventsTest test
for buggy race condition handling). See the test writing
documentation to learn more.
We are trying to maintain ~100% test coverage on the backend, so backend changes should have negative tests for the various error conditions. See backend testing documentation for details.
If the feature involves frontend changes, there should be frontend tests. See frontend testing documentation for details.
If the PR makes any frontend changes, you should make sure to play with the part of the app being changed to validate that things look and work as expected. While not all of the situations below will apply, here are some ideas for things that should be tested if they are applicable. Use the development environment to test any web app changes.
This might seem like a long process, but you can go through it quite quickly once you get the hang of it. Trust us, it will save time and review round-trips down the line!
Visual appearance:
git grep to see if the code you modified is being used
elsewhere.Responsiveness and internationalization:
Strings (text): If the PR adds or modifies strings, check the following:
N = 1 and N > 1 cases both handled properly?Tooltips:
Functionality: We're finally getting to the part where you actually use the new/updated feature. :) Test to see if it works as expected, trying a variety of scenarios. If it works as described in the issue but seems awkward in some way, note this on the PR.
If relevant, be sure to check that:
Some scenarios to consider:
@-mention or a direct message?The pull request review process guide provides a detailed overview of Zulip's PR review process. Your reviewers and Zulip's maintainers will help shepherd your PR through the process. There are also some additional ways to ask for a code review:
Are there folks who have been working on similar things, or a loosely related
area? If so, they might be a good person to review your PR. @-mention them
with something like "@person, would you be up for reviewing this?" If
you're not sure whether they are familiar with how Zulip code reviews work, you
can also include a link to this guide.
If you're not sure who to ask, you can post a message in #code-review on the Zulip development community server to reach out to a wider group of potential reviewers.
Please be patient and mindful of the fact that it isn't always possible to provide a quick reply. Going though the review process described above for your own PR will make your code easier and faster to review, which makes it much more likely that it will be reviewed quickly and require fewer review cycles.
For the author of a PR, getting feedback quickly is really important for making progress quickly and staying productive. That means that if you get @-mentioned on a PR with a request for you to review it, it helps the author a lot if you reply promptly.
A reply doesn't even have to be a full review; if a PR is big or if you're pressed for time, then just getting some kind of reply in quickly -- initial thoughts, feedback on the general direction, or just saying you're busy and when you'll have time to look harder -- is still really valuable for the author and for anyone else who might review the PR.
People in the Zulip project live and work in many time zones, and code reviewers also need focused chunks of time to write code and do other things, so an immediate reply isn't always possible. But a good benchmark is to try to always reply within one workday, at least with a short initial reply, if you're working regularly on Zulip. And sooner is better.
Any time you leave a code review, be sure to treat the author with respect. Remember that they are generously spending their time on an effort to improve the Zulip project. Thank them for their work, and express your appreciation for anything the author did especially well, whether it's a nice commit message, a prompt response to earlier feedback, or a well-written test.
Be as clear and direct as you can when describing requested changes. There is no need to apologize when asking for a change; you're collaborating with the author to make the PR as good as you can together.
Be sure to explain the motivation for the changes you're requesting if it's not obvious, so that the author can learn for next time. It may be helpful to point to developer documentation, such as the commit discipline guide.
If a pull request just needs a little fixing to make it mergeable, feel free to do that in a new commit, then push your branch to GitHub and mention the branch in a comment on the pull request. That'll save the maintainer time and get the PR merged quicker.
Once you've received a review and resolved any feedback, it's critical to update the GitHub thread to reflect that. Best practices are to:
main.If you resolve the feedback, but the PR has merge conflicts, CI failures, or the most recent comment is the reviewer asking you to fix something, it's very likely that a potential reviewer skimming your PR will assume it isn't ready for review and move on to other work.
If you need help or think an open discussion topic requires more feedback or a more complex discussion, move the discussion to a topic in the Zulip development community server. Be sure to provide links from the GitHub PR to the conversation (and vice versa) so that it's convenient to read both conversations together.
We also recommend the following resources on code reviews.