Merge "adding review guidelines"
This commit is contained in:
Jenkins
Gerrit Code Review
commit
44d1c227de
387
REVIEW_GUIDELINES.rst
Normal file
387
REVIEW_GUIDELINES.rst
Normal file
@ -0,0 +1,387 @@
|
||||
Review Guidelines
|
||||
=================
|
||||
|
||||
Effective code review is a skill like any other professional skill you
|
||||
develop with experience. Effective code review requires trust. No
|
||||
one is perfect. Everyone makes mistakes. Trust builds over time.
|
||||
|
||||
This document will enumerate behaviors commonly observed and
|
||||
associated with competent reviews of changes purposed to the Swift
|
||||
code base. No one is expected to "follow these steps". Guidelines
|
||||
are not *rules*, not all behaviors will be relevant in all situations.
|
||||
|
||||
|
||||
Checkout the Change
|
||||
-------------------
|
||||
|
||||
You will need to have a copy of the change in an environment where you
|
||||
can freely edit and experiment with the code in order to provide a
|
||||
non-superficial review. Superficial reviews are not terribly helpful.
|
||||
Always try to be helpful. ;)
|
||||
|
||||
Check out the change so that you may begin.
|
||||
|
||||
Commonly, ``git review -d <change-id>``
|
||||
|
||||
Run it
|
||||
------
|
||||
|
||||
Imagine that you submit a patch to Swift, and a reviewer starts to
|
||||
take a look at it. Your commit message on the patch claims that it
|
||||
fixes a bug or adds a feature, but as soon as the reviewer downloads
|
||||
it locally and tries to test it, a severe and obvious error shows up.
|
||||
Something like a syntax error or a missing dependency.
|
||||
|
||||
"Did you even run this?" is the review comment all contributors dread.
|
||||
|
||||
Reviewers in particular need to be fearful merging changes that just
|
||||
don't work - or at least fail in frequently common enough scenarios to
|
||||
be considered "horribly broken". A comment in our review that says
|
||||
roughly "I ran this on my machine and observed ``description of
|
||||
behavior change is supposed to achieve``" is the most powerful defense
|
||||
we have against the terrible terrible scorn from our fellow Swift
|
||||
developers and operators when we accidentally merge bad code.
|
||||
|
||||
If you're doing a fair amount of reviews - you will participate in
|
||||
merging a change that will break my clusters - it's cool - I'll do it
|
||||
to you at some point too (sorry about that). But when either of us go
|
||||
look at the reviews to understand the process gap that allowed this to
|
||||
happen - it better not be just because we were too lazy to check it out
|
||||
and run it before it got merged.
|
||||
|
||||
Or be warned, you may receive, the dreaded...
|
||||
|
||||
"Did you even *run* this?"
|
||||
|
||||
I'm sorry, I know it's rough. ;)
|
||||
|
||||
Consider edge cases very seriously
|
||||
----------------------------------
|
||||
|
||||
Saying "that should rarely happen" is the same as saying "that
|
||||
*will* happen"
|
||||
|
||||
-- Douglas Crockford
|
||||
|
||||
Scale is an *amazingly* abusive partner. If you contribute changes to
|
||||
Swift your code is running - in production - at scale - and your bugs
|
||||
can not hide. I wish on all of us that our bugs may be exceptionally
|
||||
rare - meaning they only happen in extremely unlikely edge cases. For
|
||||
example, bad things that happen only 1 out of every 10K times an op is
|
||||
performed will be discovered in minutes. Bad things that happen only
|
||||
1 out of every one billion times something happens will be observed -
|
||||
by multiple deployments - over the course of a release. Bad things
|
||||
that happen 1/100 times some op is performed are considered "horribly
|
||||
broken". Tests must exhaustively exercise possible scenarios. Every
|
||||
system call and network connection will raise an error and timeout -
|
||||
where will that Exception be caught?
|
||||
|
||||
Run the tests
|
||||
-------------
|
||||
|
||||
Yes, I know Gerrit does this already. You can do it *too*. You might
|
||||
not need to re-run *all* the tests on your machine - it depends on the
|
||||
change. But, if you're not sure which will be most useful - running
|
||||
all of them best - unit - functional - probe. If you can't reliably
|
||||
get all tests passing in your development environment you will not be
|
||||
able to do effective reviews. Whatever tests/suites you are able to
|
||||
exercise/validate on your machine against your config you should
|
||||
mention in your review comments so that other reviewers might choose
|
||||
to do *other* testing locally when they have the change checked out.
|
||||
|
||||
e.g.
|
||||
|
||||
I went ahead and ran probe/test_object_metadata_replication.py on
|
||||
my machine with both sync_method = rsync and sync_method = ssync -
|
||||
that works for me - but I didn't try it with object_post_as_copy =
|
||||
false
|
||||
|
||||
Maintainable Code is Obvious
|
||||
----------------------------
|
||||
|
||||
Style is an important component to review. The goal is maintainability.
|
||||
|
||||
However, keep in mind that generally style, readability and
|
||||
maintainability are orthogonal to the suitability of a change for
|
||||
merge. A critical bug fix may be a well written pythonic masterpiece
|
||||
of style - or it may be a hack-y ugly mess that will absolutely need
|
||||
to be cleaned up at some point - but it absolutely should merge
|
||||
because: CRITICAL. BUG. FIX.
|
||||
|
||||
You should comment inline to praise code that is "obvious". You should
|
||||
comment inline to highlight code that that you found to be "obfuscated".
|
||||
|
||||
Unfortunately "readability" is often subjective. We should remember
|
||||
that it's probably just our own personal preference. Rather than a
|
||||
comment that says "You should use a list comprehension here" - rewrite
|
||||
the code as a list comprehension, run the specific tests that hit the
|
||||
relevant section to validate your code is correct, then leave a
|
||||
comment that says:
|
||||
|
||||
I find this more readable:
|
||||
|
||||
``diff with working tested code``
|
||||
|
||||
If the author (or another reviewer) agrees - it's possible the change will get
|
||||
updated to include that improvement before it is merged; or it may happen in a
|
||||
follow up.
|
||||
|
||||
However, remember that style is non-material - it is useful to provide (via
|
||||
diff) suggestions to improve maintainability as part of your review - but if
|
||||
the suggestion is functionally equivalent - it is by definition optional.
|
||||
|
||||
Commit Messages
|
||||
---------------
|
||||
|
||||
Read the commit message thoroughly before you begin the review.
|
||||
|
||||
Commit messages must answer the "why" and the "what for" - more so
|
||||
than the "how" or "what it does". Commonly this will take the form of
|
||||
a short description:
|
||||
|
||||
- What is broken - without this change
|
||||
- What is impossible to do with Swift - without this change
|
||||
- What is slower/worse/harder - without this change
|
||||
|
||||
If you're not able to discern why a change is being made or how it
|
||||
would be used - you may have to ask for more details before you can
|
||||
successfully review it.
|
||||
|
||||
Commit messages need to have a high consistent quality. While many
|
||||
things under source control can be fixed and improved in a follow up
|
||||
change - commit messages are forever. Luckily it's easy to fix minor
|
||||
mistakes using the in-line edit feature in Gerrit! If you can avoid
|
||||
ever having to *ask* someone to change a commit message you will find
|
||||
yourself an amazingly happier and more productive reviewer.
|
||||
|
||||
Also commit messages should follow the OpenStack Commit Message
|
||||
guidelines, including references to relevant impact tags or bug
|
||||
numbers. You should hand out links to the OpenStack Commit Message
|
||||
guidelines *liberally* via comments when fixing commit messages during
|
||||
review.
|
||||
|
||||
Here you go: `GitCommitMessages <https://wiki.openstack.org/wiki/GitCommitMessages#Summary_of_Git_commit_message_structure>`_
|
||||
|
||||
New Tests
|
||||
---------
|
||||
|
||||
New tests should be added for all code changes. Historically you
|
||||
should expect good changes to have a diff line count ratio of at least
|
||||
2:1 tests to code. Even if a change has to "fix" a lot of *existing*
|
||||
tests, if a change does not include any *new* tests it probably should
|
||||
not merge.
|
||||
|
||||
If a change includes a good ratio of test changes and adds new tests -
|
||||
you should say so in your review comments.
|
||||
|
||||
If it does not - you should write some!
|
||||
|
||||
... and offer them to the patch author as a diff indicating to them that
|
||||
"something" like these tests I'm providing as an example will *need* to be
|
||||
included in this change before it is suitable to merge. Bonus points if you
|
||||
include suggestions for the author as to how they might improve or expanded on
|
||||
the tests stubs you provide.
|
||||
|
||||
Be *very* careful about asking an author to add a test for a "small change"
|
||||
before attempting to do so yourself. It's quite possible there is a lack of
|
||||
existing test infrastructure needed to develop a concise and clear test - the
|
||||
author of a small change may not be the best person to introduce a large
|
||||
amount of new test infrastructure. Also, most of the time remember it's
|
||||
*harder* to write the test than the change - if the author is unable to
|
||||
develop a test for their change on their own you may prevent a useful change
|
||||
from being merged. At a minimum you should suggest a specific unit test that
|
||||
you think they should be able to copy and modify to exercise the behavior in
|
||||
their change. If you're not sure if such a test exists - replace their change
|
||||
with an Exception and run tests until you find one that blows up.
|
||||
|
||||
Documentation
|
||||
-------------
|
||||
|
||||
Most changes should include documentation. New functions and code
|
||||
should have Docstrings. Tests should obviate new or changed behaviors
|
||||
with descriptive and meaningful phrases. New features should include
|
||||
changes to the documentation tree. New config options should be
|
||||
documented in example configs. The commit message should document the
|
||||
change for the change log.
|
||||
|
||||
Always point out typos or grammar mistakes when you see them in
|
||||
review, but also consider that if you were able to recognize the
|
||||
intent of the statement - documentation with tpyos may be easier to
|
||||
iterate and improve on than nothing.
|
||||
|
||||
If a change does not have adequate documentation it may not be suitable to
|
||||
merge. If a change includes incorrect or misleading documentation or is
|
||||
contrary to *existing* documentation is probably is not suitable to merge.
|
||||
|
||||
Every change could have better documentation.
|
||||
|
||||
Like with tests, a patch isn't done until it has docs. Any patch that
|
||||
adds a new feature, changes behavior, updates configs, or in any other
|
||||
way is different than previous behavior requires docs. manpages,
|
||||
sample configs, docstrings, descriptive prose in the source tree
|
||||
|
||||
Reviewers Write Code
|
||||
--------------------
|
||||
|
||||
Reviews have been shown to to provide many benefits - one of which is shared
|
||||
ownership. After providing a positive review you should understand how the
|
||||
change works. Doing this will probably require you to "play with" the change.
|
||||
|
||||
You might functionally test the change in various scenarios. You may need to
|
||||
write a new unittest to validate the change will degrade gracefully under
|
||||
failure. You might have to write a script to exercise the change under some
|
||||
superficial load. You might have to break the change and validate the new
|
||||
tests fail and provide useful errors. You might have to step through some
|
||||
critical section of the code in a debugger to understand when all the possible
|
||||
branches are exercised in tests.
|
||||
|
||||
When you're done with your review an artifact of your effort will be
|
||||
observable in the piles of code and scripts and diffs you wrote while
|
||||
reviewing. You should make sure to capture those artifacts in a paste
|
||||
or gist and include them in your review comments so that others may
|
||||
reference them.
|
||||
|
||||
e.g.
|
||||
|
||||
When I broke the change like this:
|
||||
|
||||
``diff``
|
||||
|
||||
it blew up like this:
|
||||
|
||||
``unittest failure``
|
||||
|
||||
|
||||
It's not uncommon that a review takes more time than writing a change -
|
||||
hopefully the author also spent as much time as you did *validating* their
|
||||
change but that's not really in your control. When you provide a positive
|
||||
review you should be sure you understand the change - even seemingly trivial
|
||||
changes will take time to consider the ramifications.
|
||||
|
||||
Leave Comments
|
||||
--------------
|
||||
|
||||
Leave. Lots. Of. Comments.
|
||||
|
||||
A popular web comic has stated that
|
||||
`WTFs/Minute <http://www.osnews.com/images/comics/wtfm.jpg>`_ is the
|
||||
*only* valid measurement of code quality.
|
||||
|
||||
If something initially strikes you as questionable - you should jot
|
||||
down a note so you can loop back around to it.
|
||||
|
||||
However, because of the distributed nature of authors and reviewers
|
||||
it's *imperative* that you try your best to answer your own questions
|
||||
as part of your review.
|
||||
|
||||
Do not say "Does this blow up if it gets called when xyz" - rather try
|
||||
and find a test that specifically covers that condition and mention it
|
||||
in the comment so others can find it more quickly. Of if you can find
|
||||
no such test, add one to demonstrate the failure, and include a diff
|
||||
in a comment. Hopefully you can say "I *thought* this would blow up,
|
||||
so I wrote this test, but it seems fine."
|
||||
|
||||
But if your initial reaction is "I don't understand this" or "How does
|
||||
this even work?" you should notate it and explain whatever you *were*
|
||||
able to figure out in order to help subsequent reviewers more quickly
|
||||
identify and grok the subtle or complex issues.
|
||||
|
||||
Because you will be leaving lots of comments - many of which are
|
||||
potentially not highlighting anything specific - it is VERY important
|
||||
to leave a good summary. Your summary should include details of how
|
||||
you reviewed the change. You may include what you liked most, or
|
||||
least.
|
||||
|
||||
If you are leaving a negative score ideally you should provide clear
|
||||
instructions on how the change could be modified such that it would be
|
||||
suitable for merge - again diffs work best.
|
||||
|
||||
Scoring
|
||||
-------
|
||||
|
||||
Scoring is subjective. Try to realize you're making a judgment call.
|
||||
|
||||
A positive score means you believe Swift would be undeniably better
|
||||
off with this code merged than it would be going one more second
|
||||
without this change running in production immediately. It is indeed
|
||||
high praise - you should be sure.
|
||||
|
||||
A negative score means that to the best of your abilities you have not
|
||||
been able to your satisfaction, to justify the value of a change
|
||||
against the cost of it's deficiencies and risks. It is a surprisingly
|
||||
difficult chore to be confident about the value of unproven code or a
|
||||
not well understood use-case in an uncertain world, and unfortunately
|
||||
all too easy with a **thorough** review to uncover our defects, and be
|
||||
reminded of the risk of... regression.
|
||||
|
||||
Reviewers must try *very* hard first and foremost to keep master stable.
|
||||
|
||||
If you can demonstrate a change has an incorrect *behavior* it's
|
||||
almost without exception that the change must be revised to fix the
|
||||
defect *before* merging rather than letting it in and having to also
|
||||
file a bug.
|
||||
|
||||
Every commit must be deployable to production.
|
||||
|
||||
Beyond that - almost any change might be merge-able depending on
|
||||
it's merits! Here's some tips you might be able to use to find more
|
||||
changes that should merge!
|
||||
|
||||
#. Fixing bugs is HUGELY valuable - the *only* thing which has a
|
||||
higher cost than the value of fixing a bug - is adding a new
|
||||
bug - if it's broken and this change makes it fixed (with out
|
||||
breaking anything else) you have a winner!
|
||||
|
||||
#. Features are INCREDIBLY difficult to justify their value against
|
||||
the cost of increased complexity, lowered maintainability, risk
|
||||
of regression, or new defects. Try to focus on what is
|
||||
*impossible* without the feature - when you make the impossible
|
||||
possible things are better. Make things better.
|
||||
|
||||
#. Purely test/doc changes, complex refactoring, or mechanical
|
||||
cleanups are quite nuanced because there's less concrete
|
||||
objective value. I've seen lots of these kind of changes
|
||||
get lost to the backlog. I've also seen some success where
|
||||
multiple authors have collaborated to "push-over" a change
|
||||
rather than provide a "review" ultimately resulting in a
|
||||
quorum of three or more "authors" who all agree there is a lot
|
||||
of value in the change - however subjective.
|
||||
|
||||
Because the bar is high - most reviews will end with a negative score.
|
||||
|
||||
However, for non-material grievances (nits) - you should feel
|
||||
confident in a positive review if the change is otherwise complete
|
||||
correct and undeniably makes Swift better (not perfect, *better*). If
|
||||
you see something worth fixing you should point it out in review
|
||||
comments, but when applying a score consider if it *need* be fixed
|
||||
before the change is suitable to merge vs. fixing it in a follow up
|
||||
change? Consider if the change makes Swift so undeniably *better*
|
||||
and it was deployed in production without making any additional
|
||||
changes would it still be correct and complete? Would releasing the
|
||||
change to production without any additional follow up make it more
|
||||
difficult to maintain and continue to improve Swift?
|
||||
|
||||
Endeavor to leave a positive or negative score on every change you review.
|
||||
|
||||
Use your best judgment.
|
||||
|
||||
A note on Swift Core Maintainers
|
||||
================================
|
||||
|
||||
Swift Core maintainers may provide positive reviews scores that *look*
|
||||
different from your reviews - a "+2" instead of a "+1"
|
||||
|
||||
But it's *exactly the same* as your "+1"
|
||||
|
||||
It means the change has been thoroughly and positively reviewed. The
|
||||
only reason it's different is to help identify changes which have
|
||||
received multiple competent and positive reviews. If you consistently
|
||||
provide competent reviews you run a *VERY* high risk of being
|
||||
approached to have your future positive review scores changed from a
|
||||
"+1" to "+2" in order to make it easier to identify changes which need
|
||||
to get merged.
|
||||
|
||||
Ideally a review from a core maintainer should provide a clear path
|
||||
forward for the patch author. If you don't know how to proceed
|
||||
respond to the reviewers comments on the change and ask for help.
|
||||
We'd love to try and help.
|
||||
Loading…
Reference in New Issue
Block a user