Re: [MirageOS-devel] [PATCH v2 4/6] Add Code Review Guide

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 26 Sep 2019, Lars Kurth wrote:
> From: Lars Kurth <lars.kurth@xxxxxxxxxx>
> 
> This document highlights what reviewers such as maintainers and committers 
> look
> for when reviewing code. It sets expectations for code authors and provides
> a framework for code reviewers.

I think the document is missing a couple of things:

- a simple one line statement that possibly the most important thing in
  a code review is to indentify any bugs in the code

- an explanation that requests for major changes to the series should be
  made early on (i.e. let's not change the architecture of a feature at
  v9 if possible) I also made this comment in reply to patch #5. I'll
  let you decide where is the best place for it.


> Signed-off-by: Lars Kurth <lars.kurth@xxxxxxxxxx>
> ---
> Cc: minios-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: xen-api@xxxxxxxxxxxxxxxxxxxx
> Cc: win-pv-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: mirageos-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: committers@xxxxxxxxxxxxxx
> ---
>  code-review-guide.md | 125 
> +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 125 insertions(+)
>  create mode 100644 code-review-guide.md
> 
> diff --git a/code-review-guide.md b/code-review-guide.md
> new file mode 100644
> index 0000000..8639431
> --- /dev/null
> +++ b/code-review-guide.md
> @@ -0,0 +1,125 @@
> +# Code Review Guide
> +
> +This document highlights what reviewers such as maintainers and committers 
> look
> +for when reviewing your code. It sets expectations for code authors and 
> provides
> +a framework for code reviewers.
> +
> +This document does **not cover** the following topics:
> +* [Communication Best Practice](communication-practice.md)
> +* [Resolving Disagreement](resolving-disagreement.md)
> +* [Patch Submission 
> Workflow](https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches)
> +* [Managing Patch Submission with 
> Git](https://wiki.xenproject.org/wiki/Managing_Xen_Patches_with_Git)
> +
> +## What we look for in Code Reviews
> +When performing a code review, reviewers typically look for the following 
> things
> +
> +### Is the change necessary to accomplish the goals?
> +* Is it clear what the goals are?
> +* Do we need to make a change, or can the goals be met with existing
> +  functionality?
> +
> +### Architecture / Interface
> +* Is this the best way to solve the problem?
> +* Is this the right part of the code to modify?
> +* Is this the right level of abstraction?
> +* Is the interface general enough? Too general? Forward compatible?
> +
> +### Functionality
> +* Does it do what it’s trying to do?
> +* Is it doing it in the most efficient way?
> +* Does it handle all the corner / error cases correctly?
> +
> +### Maintainability / Robustness
> +* Is the code clear? Appropriately commented?
> +* Does it duplicate another piece of code?
> +* Does the code make hidden assumptions?
> +* Does it introduce sections which need to be kept **in sync** with other 
> sections?
> +* Are there other **traps** someone modifying this code might fall into?
> +
> +**Note:** Sometimes you will work in areas which have identified 
> maintainability
> +and/or robustness issues. In such cases, maintainers may ask you to make 
> additional
> +changes, such that your submitted code does not make things worse or point 
> you
> +to other patches are already being worked on.
> +
> +### System properties
> +In some areas of the code, system properties such as
> +* Code size
> +* Performance
> +* Scalability
> +* Latency
> +* Complexity
> +* &c
> +are also important during code reviews.
> +
> +### Style
> +* Comments, carriage returns, **snuggly braces**, &c
> +* See 
> [CODING_STYLE](https://xenbits.xenproject.org/gitweb/?p=xen.git;a=blob;f=CODING_STYLE)
> +  and 
> [tools/libxl/CODING_STYLE](https://xenbits.xenproject.org/gitweb/?p=xen.git;a=blob;f=tools/libxl/CODING_STYLE)
> +* No extraneous whitespace changes
> +
> +### Documentation and testing
> +* If there is pre-existing documentation in the tree, such as man pages, 
> design
> +  documents, etc. a contributor may be asked to update the documentation 
> alongside
> +  the change. Documentation is typically present in the
> +  [docs](https://xenbits.xen.org/gitweb/?p=xen.git;a=tree;f=docs) folder.
> +* When adding new features that have an impact on the end-user,
> +  a contributor should include an update to the
> +  [SUPPORT.md](https://xenbits.xen.org/gitweb/?p=xen.git;a=tree;f=docs) file.
> +  Typically, more complex features require several patch series before it is 
> ready to be
> +  advertised in SUPPORT.md
> +* When adding new features, a contributor may be asked to provide tests or
> +  ensure that existing tests pass
> +
> +#### Testing for the Xen Project Hypervisor
> +Tests are typically located in one of the following directories
> +* **Unit tests**: 
> [tools/tests](https://xenbits.xenproject.org/gitweb/?p=xen.git;a=tree;f=tools/tests)
> +or 
> [xen/test](https://xenbits.xenproject.org/gitweb/?p=xen.git;a=tree;f=xen/test)<br>
> +  Unit testing is hard for a system like Xen and typically requires building 
> a subsystem of
> +  your tree. If your change can be easily unit tested, you should consider 
> submitting tests
> +  with your patch.
> +* **Build and smoke test**: see [Xen GitLab 
> CI](https://gitlab.com/xen-project/xen/pipelines)<br>
> +  Runs build tests for a combination of various distros and compilers 
> against changes
> +  committed to staging. Developers can join as members and test their 
> development
> +  branches **before** submitting a patch.
> +* **XTF tests** (microkernel-based tests): see 
> [XTF](https://xenbits.xenproject.org/docs/xtf/)<br>
> +  XTF has been designed to test interactions between your software and 
> hardware.
> +  It is a very useful tool for testing low level functionality and is 
> executed as part of the
> +  project's CI system. XTF can be easily executed locally on xen.git trees.
> +* **osstest**: see 
> [README](https://xenbits.xenproject.org/gitweb/?p=osstest.git;a=blob;f=README)<br>
> +  Osstest is the Xen Projects automated test system, which tests basic Xen 
> use cases on
> +  a variety of different hardware. Before changes are committed, but 
> **after** they have
> +  been reviewed. A contributor’s changes **cannot be applied to master** 
> unless the
> +  tests pass this test suite. Note that XTF and other tests are also 
> executed as part of
> +  osstest.
> +
> +### Patch / Patch series information
> +* Informative one-line changelog
> +* Full changelog
> +* Motivation described
> +* All important technical changes mentioned
> +* Changes since previous revision listed
> +* Reviewed-by’s and Acked-by’s dropped if appropriate
> +
> +More information related to these items can be found in our
> +[Patch submission 
> Guide](https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches).
> +
> +## Reviewing for Patch Authors
> +
> +The following presentation by George Dunlap, provides an excellent overview 
> on how
> +we do code reviews, specifically targeting non-maintainers.
> +
> +As a community, we would love to have more help reviewing, including from 
> **new
> +community members**. But many people
> +* do not know where to start, or
> +* believe that their review would not contribute much, or
> +* may feel intimidated reviewing the code of more established community 
> members
> +
> +The presentation demonstrates that you do not need to worry about any of 
> these
> +concerns. In addition, reviewing other people's patches helps you
> +* write better patches and experience the code review process from the other 
> side
> +* and build more influence within the community over time
> +
> +Thus, we recommend strongly that **patch authors** read the watch the 
> recording or
> +read the slides:
> +* [Patch Review for Non-Maintainers 
> slides](https://www.slideshare.net/xen_com_mgr/xpdds19-keynote-patch-review-for-nonmaintainers-george-dunlap-citrix-systems-uk-ltd)
> +* [Patch Review for Non-Maintainers recording - 
> 20"](https://www.youtube.com/watch?v=ehZvBmrLRwg)
> -- 
> 2.13.0
>

_______________________________________________
MirageOS-devel mailing list
MirageOS-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/mirageos-devel