From d90907b0063ea19aa618b48c1357f15a6e950362 Mon Sep 17 00:00:00 2001 From: Alya Abbott Date: Fri, 25 Feb 2022 22:50:51 -0800 Subject: [PATCH] docs: Rewrite code review documentation. --- docs/contributing/code-reviewing.md | 511 ++++++++++++++++++---------- 1 file changed, 328 insertions(+), 183 deletions(-) diff --git a/docs/contributing/code-reviewing.md b/docs/contributing/code-reviewing.md index 0c47a9c14a..dce24793ba 100644 --- a/docs/contributing/code-reviewing.md +++ b/docs/contributing/code-reviewing.md @@ -1,89 +1,315 @@ # Reviewing Zulip code -Code review is a key part of how Zulip does development! If you've -been contributing to Zulip's code, we'd love for you to do reviews. -This is a guide to how. (With some thoughts for writing code too.) - -## Protocol for authors - -When you send a PR, try to think of a good person to review it -- -outside of the handful of people who do a ton of reviews -- and -`@`-mention them with something like "`@person`, would you review -this?". Good choices include - -- someone based in your time zone or a nearby time zone -- people working on similar things, or in a loosely related area - -Alternatively, posting a message in -[#code-review](https://chat.zulip.org/#narrow/stream/91-code-review) on [the Zulip -development community server](https://zulip.com/development-community/), would -help in reaching out to a wider group of reviewers. Either way, please be -patient and mindful of the fact that it isn't possible to provide a -quick reply always, but that the reviewer would get to it sooner or later. -Lastly, ensuring that your PR passes CI and is organized into coherent -commits would help save reviewers time, which could otherwise be used -to dive right into reviewing the PR's core functionality. - -### Responding to a review feedback - -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: - -- Make sure that CI passes and the PR is rebased onto recent `main`. -- Post comments on each feedback thread explaining at least how you - resolved the feedback, as well as any other useful information - (problems encountered, reasoning for why you picked one of several - options, a test you added to make sure the bug won't recur, etc.). -- Mark any resolved threads as "resolved" in the GitHub UI, if - appropriate. -- Post a summary comment in the main feed for the PR, explaining that - this is ready for another review, and summarizing any changes from - the previous version, details on how you tested the changes, new - screenshots/etc. More detail is better than less, as long as you - take the time to write clearly. - -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. +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 code base. ## Principles of code review -### Anyone can review +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. -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. If you +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. -- read the code, see if you understand what the change is - doing and why, and ask questions if you don't; or +### Reviewing your own code -- fetch the code (for Zulip server code, - [tools/fetch-rebase-pull-request][git tool] is super handy), play around - with it in your dev environment, and say what you think about how - the feature works +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. -those are really helpful contributions. +### Reviewing other contributors' code -### Please do reviews - -Doing code reviews is an important part of making the project grow. +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 some of your time contributing to come -in the form of doing code reviews! +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. +## How to review code + +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][git-tool] +`tools/fetch-rebase-pull-request` to check out a pull request locally and rebase +it onto `main`. + +### Code review checklist + +The following review steps apply to the majority of PRs. + +**Think about the issue:** + +1. 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][czo] 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][czo] explaining + precisely what points you find confusing. + +2. 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: + + - The issue explicitly notes that it's fine for some parts to be completed + separately, and the PR clearly indicates which parts are solved. + - After discussion of initial prototypes (in GitHub comments or on the + [development community server][czo]), it was decided to change some part of + the specification, and the PR notes this. + - The author explains why the PR is a better way to solve the issue than what + was described. + - The solution changed because of changes in the project or application since + the issue was written, and the author explains the adjustments that were + made. + +**Think about the code:** + +1. 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. + +1. 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. + +1. **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. + +1. Make sure the PR follows Zulip's **coding style**. See the Zulip [coding + style documentation][code-style] 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. + +1. 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:** + +1. **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](https://github.com/zulip/zulip/pull/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](https://github.com/zulip/zulip/runs/2092955762) + is the page of the "Debian 10 Buster (Python 3.7, backend + frontend)" job. + See our docs on [continuous integration](../testing/continuous-integration.md) + to learn more. + +2. Make sure **the code is well-tested**; see below for details. The PR should + summarize any [manual testing](#manual-testing) that was done to validate + that the feature is working as expected. + +**Think about the commits:** + +1. Does the PR follow the principle that “**Each commit is a minimal coherent + idea**”? See the [commit discipline guide][commit-discipline] to learn more + about commit structure in Zulip. + +2. Does each commit have a **clear commit message**? Check for content, format, + spelling and grammar. See the [Zulip version control][commit-messages] + 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. + +### Automated testing + +- 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](../subsystems/markdown.md), or the `GetEventsTest` test + for buggy race condition handling). See the [test writing][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][backend-testing] for details. + +- If the feature involves frontend changes, there should be frontend tests. See + [frontend testing documentation][frontend-testing] for details. + +### Manual testing + +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][development-environment] to test any webapp 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:** + +- Open up the parts of the UI that were changed, and make sure they look as + you were expecting. +- Is the new UI consistent with similar UI elements? Think about fonts, colors, + sizes, etc. If a new or modified element has multiple states (e.g. "on" and + "off"), consider all of them. +- Is the new UI aligned correctly with the elements around it, both vertically and + horizontally? +- If the PR adds or modifies a clickable element, does it have a hover behavior + that's consistent with similar UI elements? +- If the PR adds or modifies an element (e.g. a button or checkbox) that is + sometimes disabled, is the disabled version of the UI consistent with similar + UI elements? +- Did the PR accidentally affect any other parts of the UI? E.g., if the PR + modifies some CSS, look for other elements that may have been altered + unintentionally. Use `git grep` to see if the code you modified is being used + elsewhere. +- Now check all of the above in the other theme (light/dark). + +**Responsiveness and internationalization:** + +- Check the new UI at different window sizes, including mobile sizes (you can + use Chrome DevTools if you like). Does it look good in both wide and narrow + windows? +- To simulate what will happen when the UI is translated to different languages, + try changing any new strings, or ones that are now displayed differently, to + make them 1.5x longer, and check if anything breaks. What would happen if the + strings were half as long as in English? + +**Strings (text):** +If the PR adds or modifies strings, check the following: + +- Does the wording seem consistent with similar features (similar style, level + of detail, etc.)? +- If there is a number, are the `N = 1` and `N > 1` cases both handled properly? + +**Tooltips:** + +- Do elements that require tooltips have them? Check similar elements to see + whether a tooltip is needed, and what information it should contain. + +**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: + +- Live updates are working as expected. +- Keyboard navigation, including tabbing to the interactive elements, is working + as expected. + +Some scenarios to consider: + +- Try clicking on any interactive elements, multiple times, in a variety of orders. +- If the feature affects the **message view**, try it out in different types of + narrows: topic, stream, All messages, PMs. +- If the feature affects the **compose box** in the webapp, try both ways of + [resizing the compose box](https://zulip.com/help/resize-the-compose-box). + Test both stream messages and PMs. +- If the feature might require **elevated permissions**, check it out as a user who has + permissions to use it and one who does not. +- Think about how the feature might **interact with other features**, and try out + such scenarios. For example: + - If the PR adds a banner, is it possible that it would be shown at the same + time as other banners? Does something reasonable happen? + - If the feature has to do with topic editing, do you need to think + about what happens when a topic is resolved/unresolved? + - If it's a message view feature, would anything go wrong if the message was + collapsed or muted? If it was colored like an `@`-mention or a PM? + +## Review process and communication + +### Asking for a code review + +There are a few good 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 the code review process, 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](https://chat.zulip.org/#narrow/stream/91-code-review) on [the Zulip + development community server](https://zulip.com/development-community/) to reach + out to a wider group of potential reviewers. + +- If you would like feedback on user-facing changes, you can `@`-mention `@alya` + on your PR. She can also help find someone to review the code once the PR is + ready from a product perspective. + +- Finally, if you are not sure who should review the PR, just indicate clearly + that it is ready for review, and the project maintainers will take a look and + follow up with next steps. + +With any of these approaches, please be patient and mindful of the fact that it +isn't always possible to provide a quick reply. Going though the [review +process](#how-to-review-code) 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. + ### Fast replies are key For the author of a PR, getting feedback quickly is really important @@ -105,127 +331,41 @@ 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. -## Things to look for - -- _The CI build._ The 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](https://github.com/zulip/zulip/pull/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](https://github.com/zulip/zulip/runs/2092955762) - is the page of the "Debian 10 Buster (Python 3.7, backend + frontend)" job. - See our docs on [continuous integration](../testing/continuous-integration.md) - to learn more. - -- _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. - -- _User interface and visual design._ If frontend changes are - involved, the reviewer will check out the code, play with the new - UI, and verify it for both quality and consistency with the rest of - the Zulip UI. We highly encourage posting screenshots to save - reviewers time in getting a feel for what the feature looks like -- - you'll get a quicker response that way. - -- _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). - -- _Testing._ 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](../subsystems/markdown.md), or the `GetEventsTest` test for - buggy race condition handling). - -- _Translation._ Make sure that the strings are marked for - [translation]. - -- _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. - -- _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. - -- _Completeness._ 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? - -- _Good comments._ 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. - -- _Coding style._ See the Zulip [code-style] documentation for - details. Our goal is to have as much of this as possible verified - via the linters and tests, but there's always going to be unusual - forms of Python/JavaScript style that our tools don't check for. - -- _Clear commit messages._ See the [Zulip version - control][commit-messages] documentation for details on what we look - for. - -### Zulip server - -Some points specific to the Zulip server codebase: - -- _Testing -- Backend._ We are trying to maintain ~100% test coverage - on the backend, so backend changes should have negative tests for - the various error conditions. - -- _Testing -- Frontend._ If the feature involves frontend changes, - there should be frontend tests. See the [test - writing][test-writing] documentation for more details. - -- _mypy annotations._ 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. - -## Tooling - -To make it easier to review pull requests, 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`. - 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. +### Responding to review feedback + +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: + +- Make sure that CI passes and the PR is rebased onto recent `main`. +- Post comments on each feedback thread explaining at least how you + resolved the feedback, as well as any other useful information + (problems encountered, reasoning for why you picked one of several + options, a test you added to make sure the bug won't recur, etc.). +- Post a summary comment in the main feed for the PR, explaining that + this is ready for another review, and summarizing any changes from + the previous version, details on how you tested the changes, new + screenshots/etc. More detail is better than less, as long as you + take the time to write clearly. + +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][czo]. 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. + ## Additional resources -We also strongly recommend reviewers to go through the following resources. +We also recommend the following resources on code reviews. - [The Gentle Art of Patch Review](https://sage.thesharps.us/2014/09/01/the-gentle-art-of-patch-review/) article by Sarah Sharp @@ -237,8 +377,13 @@ We also strongly recommend reviewers to go through the following resources. - [Zulip code of conduct](../code-of-conduct.md) [code-style]: code-style.md +[commit-discipline]: version-control.md#commit-discipline [commit-messages]: version-control.md#commit-messages [test-writing]: ../testing/testing.md +[backend-testing]: ../testing/testing-with-django.md +[frontend-testing]: ../testing/testing-with-node.md [mypy]: ../testing/mypy.md -[git tool]: ../git/zulip-tools.md#fetch-a-pull-request-and-rebase +[git-tool]: ../git/zulip-tools.md#fetch-a-pull-request-and-rebase [translation]: ../translating/translating.md +[czo]: https://zulip.com/development-community/ +[development-environment]: ../development/overview.md