Permalink
Browse files

Added a new documentation page about our review process.

  • Loading branch information...
1 parent eb8071a commit 55171be23bd99f98e7211850aa9fc89a728bebb4 @adngdb adngdb committed Jan 25, 2012
Showing with 105 additions and 7 deletions.
  1. +1 −0 docs/newdeveloperguide.rst
  2. +95 −0 docs/reviewprocess.rst
  3. +9 −7 docs/setupdevenv.rst
@@ -13,3 +13,4 @@ If you are new to Socorro, you will find here good resources to start hacking:
generalarchitecture
setupdevenv
addaservice
+ reviewprocess
View
@@ -0,0 +1,95 @@
+.. index:: reviewprocess
+
+.. _reviewprocess-chapter:
+
+How to Review a Pull Request
+============================
+
+Part of our job as developers is to review and provide feedback on what
+our colleagues do. The goal of this process is to:
+
+ * test that a new feature works as expected
+ * make sure the code is clean
+ * make sure the code doesn't break anything
+
+Here are several steps you can follow when reviewing a pull request. Depending
+on the size of that pull request, you might want to skip some phases.
+
+Read the code
+-------------
+
+The first task when reviewing is to read the code and verify that it is
+coherent and clean. Try to understand the algorithm and its goal, make sure
+that it is what was asked in the related bug. When there is something that
+you find non-trivial and that is not documented, ask for a doc-string or
+an inline comment so it becomes easier for others to understand the code.
+
+Pull the code into your local environment
+-----------------------------------------
+
+To go on testing, you will need to have the code in your local environment.
+Let's say you want to test the branch ``my-dev-branch`` of rhelmer's git
+repository. Here is one method to get the content of that remote branch into
+your repo::
+
+ git remote add rhelmer https://github.com/rhelmer/socorro.git # the first time only
+ git fetch rhelmer my-dev-branch:my-dev-branch
+ git checkout my-dev-branch
+
+Once you are in that branch, you can actually test the code or run tools on it.
+
+Use a code quality tool
+-----------------------
+
+Running a code quality tool is a good and easy way to find coding and styling
+problems. For Python, we use ``check.py`` (`check by jbalogh on github
+<https://github.com/jbalogh/check>`_). This tool will run `pyflakes
+<http://pypi.python.org/pypi/pyflakes>`_ on a file or a folder, and will then
+check that PEP 8 is respected.
+
+To install ``check.py``, run the following command::
+
+ pip install -e git://github.com/jbalogh/check.git#egg=check
+
+For JavaScript, we suggest that you use `JSHint <http://www.jshint.com/>`_.
+There are also a lot of tools for PHP, you can choose one you like.
+
+For HTML and CSS files, please use the tools from the W3C: `CSS Validator
+<http://jigsaw.w3.org/css-validator/>`_ and `HTML Validator
+<http://validator.w3.org/>`_.
+
+Run the unit tests
+------------------
+
+Socorro has a growing number of unit tests that are very helpful at verifying
+nothing breaks. Before approving and merging a pull request, you should run
+all unit tests to make sure they still pass.
+
+Note that those unit tests will be run when the pull request is merged, but
+it is easier to fix something before it lands on master than after.
+
+To run the unit tests in a Vagrant VM, do the following::
+
+ make test
+
+This installs all the dependencies needed and run all the tests. You need to
+have a running PostgreSQL instance for this to work, with a specific config
+file for the tests in ``socorro/unittest/config/commonconfig.py``.
+
+For further documentation on unit tests, please read :ref:`unittesting-chapter`.
+
+Test manually
+-------------
+
+This is not always possible in a local environment, but when it is you
+should make sure the new code behave as expected. Read :ref:`applychanges-label`.
+
+Test before
+-----------
+
+This is a process to verify that one's work is good and can go into master
+with little risk of breaking something. However, the developer is responsible
+for his or her bug and that review process doesn't mean he or she shouldn't
+go through all these steps. The reviewer is here to make sure the developer
+didn't miss something, but it's easier to fix something before a review
+process than after. Please test your code before opening a pull request!
View
@@ -25,6 +25,8 @@ the VM, execute the following commands::
edit .git/config # change `url = git@github.com:mozilla/socorro.git` with your repo's URL
git fetch origin
+.. _applychanges-label:
+
Apply your changes
------------------
@@ -53,19 +55,19 @@ website that explains it all: http://vagrantup.com/docs/nfs.html
Setting up a new database
----------------
-If you do not have an existing production database to import, or wish to
-create a new standalone database for testing, see :ref:`populatepostgres-chapter`
+If you do not have an existing production database to import, or wish to
+create a new standalone database for testing, see :ref:`populatepostgres-chapter`
or :ref:`populateelasticsearch-chapter`.
Pulling crash reports from production
----------------
-The Socorro PostgreSQL database only contains a small subset of the information
+The Socorro PostgreSQL database only contains a small subset of the information
about individual crashes (enough to run aggregate reports). For instance the
full stack is only available in long-term storage (such as HBase).
If you have imported a database from a production instance, you may want
-to configure the web UI to pull individual crash reports from production via
-the web service (so URLs such as https://crash-stats.mozilla.com/report/index/0f3f3360-40a6-4188-8659-b2a5c2110808 will work).
+to configure the web UI to pull individual crash reports from production via
+the web service (so URLs such as https://crash-stats.mozilla.com/report/index/0f3f3360-40a6-4188-8659-b2a5c2110808 will work).
The /report/index page actually pulls it's data from a URL such as:
https://crash-stats.mozilla.com/dumps/0f3f3360-40a6-4188-8659-b2a5c2110808.jsonz
@@ -83,7 +85,7 @@ Change the URL in this config value to point to your desired production instance
$config['crash_dump_local_url_fallback'] = 'https://crash-stats.mozilla.com/dumps/%1$s.jsonz';
?>
-Note that the crash ID must be in both your local database and the remote
+Note that the crash ID must be in both your local database and the remote
(production) HBase instance for this to work.
-See https://github.com/mozilla/socorro/blob/master/webapp-php/application/config/application.php-dist
+See https://github.com/mozilla/socorro/blob/master/webapp-php/application/config/application.php-dist

0 comments on commit 55171be

Please sign in to comment.