Reorganize information in CONTRIBUTING.rst, link to ReadTheDocs
This commit is contained in:
@@ -18,6 +18,7 @@ Contents:
|
||||
overview.rst
|
||||
extending_platform/index
|
||||
process/index
|
||||
testing/index
|
||||
|
||||
APIs
|
||||
-----
|
||||
|
||||
@@ -18,7 +18,7 @@ and effort making a pull request.
|
||||
It’s also sometimes useful to submit a pull request even before the code is
|
||||
working properly, to make it easier to collect early feedback. To indicate to
|
||||
others that your pull request is not yet in a functional state, just prefix the
|
||||
pull request title with "WIP:" (which stands for Work In Progress).
|
||||
pull request title with "(WIP)" (which stands for Work In Progress).
|
||||
|
||||
Once you’re ready to submit your changes in a pull request, check the following
|
||||
list of requirements to be sure that your pull request is ready to be reviewed:
|
||||
@@ -95,5 +95,17 @@ if we do reject your pull request, we will explain why we aren’t taking it, an
|
||||
try to suggest other ways that you can accomplish the same result in a way that
|
||||
we will accept.
|
||||
|
||||
Further Information
|
||||
-------------------
|
||||
For futher information on the pull request requirements, please see the following
|
||||
links:
|
||||
|
||||
* :doc:`../testing`
|
||||
* :doc:`../testing/jenkins`
|
||||
* :doc:`../testing/code-coverage`
|
||||
* :doc:`../testing/code-quality`
|
||||
* `Python Guidelines <https://github.com/edx/edx-platform/wiki/Python-Guidelines>`_
|
||||
* `Javascript Guidelines <https://github.com/edx/edx-platform/wiki/Javascript-Guidelines>`_
|
||||
|
||||
.. _contributor's agreement with edX: http://code.edx.org/individual-contributor-agreement.pdf
|
||||
.. _compatible licenses: https://github.com/edx/edx-platform/wiki/Licensing
|
||||
|
||||
26
docs/en_us/developers/source/testing/code-coverage.rst
Normal file
26
docs/en_us/developers/source/testing/code-coverage.rst
Normal file
@@ -0,0 +1,26 @@
|
||||
*************
|
||||
Code Coverage
|
||||
*************
|
||||
|
||||
We measure which lines of our codebase are covered by unit tests using
|
||||
`coverage.py`_ for Python and `JSCover`_ for Javascript.
|
||||
|
||||
Our codebase is far from perfect, but the goal is to slowly improve our coverage
|
||||
over time. To do this, we wrote a tool called `diff-cover`_ that will
|
||||
report which lines in your branch are not covered by tests, while ignoring
|
||||
other lines in the project that may not be covered. Using this tool,
|
||||
we can ensure that pull requests have a very high percentage of test coverage
|
||||
-- and ideally, they increase the test coverage of existing code, as well.
|
||||
|
||||
To check the coverage of your pull request, just go to the top level of the
|
||||
edx-platform codebase and run::
|
||||
|
||||
$ rake coverage
|
||||
|
||||
This will print a coverage report for your branch. We aim for
|
||||
a coverage report score of 95% or higher. We also encourage you to write
|
||||
acceptance tests as your changes require.
|
||||
|
||||
.. _coverage.py: https://pypi.python.org/pypi/coverage
|
||||
.. _JSCover: http://tntim96.github.io/JSCover/
|
||||
.. _diff-cover: https://github.com/edx/diff-cover
|
||||
41
docs/en_us/developers/source/testing/code-quality.rst
Normal file
41
docs/en_us/developers/source/testing/code-quality.rst
Normal file
@@ -0,0 +1,41 @@
|
||||
************
|
||||
Code Quality
|
||||
************
|
||||
|
||||
In order to keep our codebase as clear and readable as possible, we use various
|
||||
tools to assess the quality of pull requests:
|
||||
|
||||
* We use the `pep8`_ tool to follow `PEP-8`_ guidelines
|
||||
* We use `pylint`_ for static analysis and uncovering trouble spots in our code
|
||||
|
||||
Our codebase is far from perfect, but the goal is to slowly improve our quality
|
||||
over time. To do this, we wrote a tool called `diff-quality`_ that will
|
||||
only report on the quality violations on lines that have changed in a
|
||||
pull request. Using this tool, we can ensure that pull requests do not introduce
|
||||
any new quality violations -- and ideally, they clean up existing violations
|
||||
in the process of introducing other changes.
|
||||
|
||||
To check the quality of your pull request, just go to the top level of the
|
||||
edx-platform codebase and run::
|
||||
|
||||
$ rake quality
|
||||
|
||||
This will print a report of the quality violations that your branch has made.
|
||||
|
||||
Although we try to be vigilant and resolve all quality violations, some Pylint
|
||||
violations are just too challenging to resolve, so we opt to ignore them via
|
||||
use of a pragma. A pragma tells Pylint to ignore the violation in the given
|
||||
line. An example is::
|
||||
|
||||
self.assertEquals(msg, form._errors['course_id'][0]) # pylint: disable=protected-access
|
||||
|
||||
The pragma starts with a ``#`` two spaces after the end of the line. We prefer
|
||||
that you use the full name of the error (``pylint: disable=unused-argument`` as
|
||||
opposed to ``pylint: disable=W0613``), so it's more clear what you're disabling
|
||||
in the line.
|
||||
|
||||
.. _PEP-8: http://legacy.python.org/dev/peps/pep-0008/
|
||||
.. _pep8: https://pypi.python.org/pypi/pep8
|
||||
.. _coverage.py: https://pypi.python.org/pypi/coverage
|
||||
.. _pylint: http://pylint.org/
|
||||
.. _diff-quality: https://github.com/edx/diff-cover
|
||||
19
docs/en_us/developers/source/testing/index.rst
Normal file
19
docs/en_us/developers/source/testing/index.rst
Normal file
@@ -0,0 +1,19 @@
|
||||
*******
|
||||
Testing
|
||||
*******
|
||||
|
||||
Testing is something that we take very seriously at edX: we even have a
|
||||
"test engineering" team at edX devoted purely to making our testing
|
||||
infrastructure even more awesome.
|
||||
|
||||
This file is currently a stub: to find out more about our testing infrastructure,
|
||||
check out the `testing.md`_ file on Github.
|
||||
|
||||
.. toctree::
|
||||
:maxdepth: 2
|
||||
|
||||
jenkins
|
||||
code-coverage
|
||||
code-quality
|
||||
|
||||
.. _testing.md: https://github.com/edx/edx-platform/blob/master/docs/en_us/internal/testing.md
|
||||
68
docs/en_us/developers/source/testing/jenkins.rst
Normal file
68
docs/en_us/developers/source/testing/jenkins.rst
Normal file
@@ -0,0 +1,68 @@
|
||||
*******
|
||||
Jenkins
|
||||
*******
|
||||
|
||||
`Jenkins`_ is an open source continuous integration server. edX has a Jenkins
|
||||
installation specifically for testing pull requests to our open source software
|
||||
project, including edx-platform. Before a pull request can be merged, Jenkins
|
||||
must run all the tests for that pull request: this is known as a "build".
|
||||
If even one test in the build fails, then the entire build is considered a
|
||||
failure. Pull requests cannot be merged until they have a passing build.
|
||||
|
||||
Kicking Off Builds
|
||||
==================
|
||||
|
||||
Jenkins has the ability to automatically detect new pull requests and changed
|
||||
pull requests on Github, and it can automatically run builds in response to
|
||||
these events. We have Jenkins configured to automatically run builds for all
|
||||
pull requests from core committers; however, Jenkins will *not* automatically
|
||||
run builds for new contributors, so a community manager will need to manually
|
||||
kick off a build for a pull request from a new contributor.
|
||||
|
||||
The reason for this distinction is a matter of trust. Running a build means that
|
||||
Jenkins will execute all the code in the pull request. A pull request can
|
||||
contain any code whatsoever: if we allowed Jenkins to automatically build every
|
||||
pull request, then a malicious developer could make our Jenkins server do whatever
|
||||
he or she wanted. Before kicking off a build, community managers look at the
|
||||
code changes to verify that they are not malicious; this protects us from nasty
|
||||
people.
|
||||
|
||||
Once a contributor has submitted a few pull requests, they can request to be
|
||||
added to the Jenkins whitelist: this is a special list of people that Jenkins
|
||||
*will* kick off builds for automatically. If the community managers feel that
|
||||
the contributor is trustworthy, then they will grant the request, which will
|
||||
make future development faster and easier for both the contributor and edX. If
|
||||
a contibutor shows that they can not be trusted for some reason, they will be
|
||||
removed from this whitelist.
|
||||
|
||||
Failed Builds
|
||||
=============
|
||||
|
||||
Click on the build to be brought to the build page. You'll see a matrix of blue
|
||||
and red dots; the red dots indicate what section failing tests were present in.
|
||||
You can click on the test name to be brought to an error trace that explains
|
||||
why the tests fail. Please address the failing tests before requesting a new
|
||||
build on your branch. If the failures appear to not have anything to do with
|
||||
your code, it may be the case that the master branch is failing. You can ask
|
||||
your reviewers for advice in this scenario.
|
||||
|
||||
If the build says "Unstable" but passes all tests, you have introduced too many
|
||||
pep8 and pylint violations. Please refer to the documentation for :doc:`code-quality`
|
||||
and clean up the code.
|
||||
|
||||
Successful Builds
|
||||
=================
|
||||
|
||||
If all the tests pass, the "Diff Coverage" and "Diff Quality" reports are
|
||||
generated. Click on the "View Reports" link on your pull request to be brought
|
||||
to the Jenkins report page. In a column on the left side of the page are a few
|
||||
links, including "Diff Coverage Report" and "Diff Quality Report". View each of
|
||||
these reports (making note that the Diff Quality report has two tabs - one for
|
||||
pep8, and one for Pylint).
|
||||
|
||||
Make sure your quality coverage is 100% and your test coverage is at least 95%.
|
||||
Adjust your code appropriately if these metrics are not high enough. Be sure to
|
||||
ask your reviewers for advice if you need it.
|
||||
|
||||
|
||||
.. _Jenkins: http://jenkins-ci.org/
|
||||
Reference in New Issue
Block a user