[Summary view] [Print] [Text view]

   1  @title User Guide: Review vs Audit
   2  @group userguide
   3  
   4  Discusses the differences between Review and Audit workflows.
   5  
   6  = Overview =
   7  
   8  Phabricator supports two similar but separate code review workflows:
   9  
  10    - **Differential** is used for pre-push code review, called "reviews"
  11      elsewhere in the documentation. You can learn more in
  12      @{article:Differential User Guide}.
  13    - **Audit** is used for post-push code reviews, called "audits" elsewhere in
  14      the documentation. You can learn more in @{article:Audit User Guide}.
  15  
  16  (By "pre-push", this document means review which blocks deployment of changes,
  17  while "post-push" means review which happens after changes are deployed or
  18  en route to deployment.)
  19  
  20  Both are lightweight, asynchronous web-based workflows where reviewers/auditors
  21  inspect code independently, from their own machines -- not synchronous review
  22  sessions where authors and reviewers meet in person to discuss changes.
  23  
  24  = Advantages of Review =
  25  
  26  Pre-push review is significantly more powerful than post-push auditing. You
  27  gain these advantages by requiring review //before// changes may be pushed:
  28  
  29    - Authors have a strong incentive to craft small, well-formed changes that
  30      will be readily understood, to explain them adequately, and to provide
  31      appropriate test plans, test coverage and context.
  32    - Reviewers have a real opportunity to make significant suggestions about
  33      architecture or approach in review. These suggestions are less attractive
  34      to adopt from audit, and may be much more difficult to adopt if significant
  35      time has passed between push and audit.
  36    - Authors have a strong incentive to fix problems and respond to feedback
  37      received during review, because it blocks them. Authors have a much weaker
  38      incentive to address problems raised during audit.
  39    - Authors can ask reviewers to apply and verify fixes before they are pushed.
  40    - Authors can easily pursue feedback early, and get course corrections on
  41      approach or direction.
  42    - Reviewers are better prepared to support a given change once it is in
  43      production, having already had a chance to become familiar with and reason
  44      through the code.
  45    - Reviewers are able to catch problems which automated tests may have
  46      difficulty detecting. For example, human reviewers are able to reason about
  47      performance problems that tests can easily miss because they run on
  48      small datasets and stub out service calls.
  49    - Communicating about changes //before// they happen generally leads to better
  50      preparation for their effects.
  51  
  52  The theoretical cost of review is that it slows down development by introducing
  53  a blocking step into the process and generally wastes developer time that could
  54  be better spent developing. This is less true than it appears, because the costs
  55  are low and pay for themselves in other ways:
  56  
  57    - Differential is fast and provides a very lightweight process for submitting
  58      code for review and for performing review.
  59    - Authors are free to pursue other changes while code is being reviewed. With
  60      appropriate change management (like local branching in Git) they can even
  61      pursue dependent changes easily. Authors should rarely if ever be blocked on
  62      review, even though an individual change is blocked until it is approved.
  63    - The workflow as a whole is lightweight and, with skillful reviewers,
  64      effective at identifying bugs. It is generally faster to fix bugs in review
  65      than in production.
  66    - More importantly, it is effective at identifying problems with architecture
  67      and approach. These are free to fix in review ("don't do this, it is a bad
  68      idea") and may be very time consuming to fix in production. No matter how
  69      good your test suite is, it can't identify solutions which are poor because
  70      of missing context, or miscommunication, or which are simply bad ideas.
  71    - Changes which are too large or too complicated to be reviewed quickly are
  72      often //too large and too complicated, period//. Nearly all large changes
  73      can be split apart into small, independent pieces which are easier to
  74      understand and test. Review tends to encourage smaller and better-factored
  75      changes.
  76    - Review can be integrated with static analysis which can detect (and,
  77      in many cases, correct) mechanical problems with code like syntax,
  78      formatting, naming conventions, style problems, misspellings, and some
  79      program errors. This reduces the amount of time it takes to review code,
  80      and means reviewers can focus on actual problems with the code rather than
  81      minor stylistic issues.
  82    - Review creates a permanent record of context and intent which explains why
  83      a change was made, generally with much more information than commit messages
  84      alone (authors have an incentive to properly explain a change when sending
  85      it for review). This makes it easier to understand code later, and to
  86      respond appropriately when it breaks.
  87    - With `arc patch`, it is roughly as easy to pull a change out of Differential
  88      as it is to pull it out of the remote.
  89  
  90  = Advantages of Audit =
  91  
  92  Post-push review is significantly better than nothing. If you are unpersuaded
  93  by the arguments above (or work on a team that is unswayed), audits provide
  94  some of the benefits of review with less friction:
  95  
  96    - Audits are driven entirely by Phabricator, users do not need to install
  97      `arc`.
  98    - Audits require little adjustment to existing workflows and little training.
  99    - Audits are completely nonblocking, and send fewer notifications than review.
 100    - Even if you have review, audits can be useful as a supplement to keep tabs
 101      on lower-importance changes or raise issues that are discovered after
 102      review.
 103  
 104  = Recommendations =
 105  
 106  Here are super biased recommendations from developers of code review software:
 107  
 108    - If you can do review, do it. Supplement it with audits for less important
 109      changes as your organization scales.
 110    - If you can't do review immediately, set up audits and try to transition
 111      toward review. Some types of changes (like tentative changes or requests
 112      for feedback about code) are a naturally good fit for review and can serve
 113      as a stepping stone toward broader acceptance. Greater familiarity with the
 114      toolset may also foster more acceptance toward review, and the value of
 115      review may become more obvious as the organization scales (e.g., once you
 116      get interns).
 117    - If you aren't interested in review, just do audits. You can always
 118      change your mind later. But consider review! It's really good, we promise!
 119  
 120  = Next Steps =
 121  
 122    - Learn more about reviews in @{article:Differential User Guide}; or
 123    - learn more about audits in @{article:Audit User Guide}.