Submitting Pull Requests

Before submitting a Pull Request, please review each of the following items.

Getting started

First, you need to determine where to branch from before writing your code. This will depend on several things (whether it's a bug fix or a larger feature, where we are in the current release cycle, etc). See the strategy document for branching and merging for guidance, but always ask if you're not sure.

Once you've decided what branch to work off of -- let's say it's develop -- do the following:

git checkout upstream/develop
git checkout -b name_of_your_feature

And after making and committing changes, push them to a branch on your fork using:

git push origin name_of_your_feature

Then, go to your fork and click the "New pull request" button. Someone will review your code and you can push new commits to your branch to have the updates added to the open PR (pull request). See GitHub's notes for how to create a Pull Request.

Coding Guidelines and Conventions

We expect submitted code to follow our code conventions and styling. We also have careful guidelines about app structure and inter-app dependencies, including expectations about how commits are structured.

Documentation

For all new features and any major changes (including class or function additions) for bugfixes, we require documentation to be submitted along with code changes.

For comments, we follow Google's Python Style Guide, which contain docstring formatting instructions.

We use docstrings & comments for each of the following

• New modules / apps: docstring in the module's __init__.py, explaining the high-level need, design, and dependencies.
• New files: docstring at the top of the file defining what lives inside, and any overall design.
• New functions/classes: docstring for each
• Inline: as needed

Here's an example of the standard docstring for a public function:

def public_fn_with_googley_docstring(name, state=None):
    """This function does something.

    Args:
       name (str):  The name to use.

    Kwargs:
       state (bool): Current state to be in.

    Returns:
       int.  The return code::

          0 -- Success!
          1 -- No good.
          2 -- Try again.

    Raises:
       AttributeError, KeyError

    A really great idea.  A way you might use me is

    >>> print public_fn_with_googley_docstring(name='foo', state=None)
    0

    BTW, this always returns 0.  **NEVER** use with :class:`MyPublicClass`.

    """
    return 0

Testing

Test Types

We have four types of tests in our repository.

• Unit tests - These tests are direct tests of function inputs and outputs. Unit tests are meant to test python functions that will be called by the Django framework. They're meant to do test most of the business logic inside our app.

• API tests - These tests use a programmable HTTP client to test API endpoints. In general, API tests have to make sure that they either

  1. have the correct input and output
  2. throw the right exceptions when given bad input
  3. call into the right function, which is sufficiently tested by unit tests.

  In general, both API and unit tests work in tandem. You don't want to duplicate testing across these two types of tests. For example, if you've tested a certain utility function, you just want to make sure that a view just calls that function with the right arguments. Re-testing the logic of the view function is just duplication of testing and should be avoided as much as possible.

• Browser tests - Also known as integration tests, wherein you test that the entire feature works, from frontend to backend. These tests use a browser to click links, submit forms, examine elements, and test end-user experience.

• Ecosystem tests - Another type of integration test, specifically for testing interactions between different server types. These tests must be run from our central server repository. These tests install multiple versions of KA Lite (including the central server) to test data sharing across installations.

Note that these 4 tests all work in tandem to fully test the code.

We use Django's extension of the unittest framework for unit tests, Django's LiveServerTest for any tests requiring KA Lite to be running (API, Browser, and Ecosystem tests), and selenium to launch and run Browser tests.

Test Requirements

All new features must have unit tests, and should use other types of tests as applicable (see below for examples). Regression tests (tests for bugs that have been found) can be in whatever test type is applicable as well.

Test Structure and Tools

Files should be contained within the app in a tests directory.

• If no such directory exists, create one! Copy an __init__.py from an existing test repository--it contains code needed for loading all tests into the `kalite.{app}.

Test Classes and Examples

Unit Tests

API Tests

Browser Tests

Code review guidelines

After submitting your pull request, someone will review your code and provide feedback. Here are the guidelines you and the code reviewers should follow:

• Be thorough. We should try to get all bugs, ugliness, and confusion out of the code before it is merged into the main repository, so don’t let anything slide. This is the primary filter point.
• Don’t make anything personal, and don’t take anything personally. This is about leveraging collective intelligence to best improve the code. We all have lots to learn from each other.
• It’s OK to have points in the code that could use further development (as long as they’re not making the user’s experience worse or introducing bugs/security vulnerabilities). However, any incomplete pieces in the code should be clearly noted as a TODO, with a username, e.g.: # TODO(jamalex): this could be made more efficient by doing the requests in bulk
• If a piece of code is confusing, don’t hesitate to ask for it to be made clearer, or at least better-commented. Readable code is critical to the long-term sustainability of the codebase. If a piece of code requires an explanation during the review process, this explanation should be condensed into comments in the code for the benefit of those who weren’t on the code review.