Pull Request Process

Before Opening Your PR

Pull requests are automatically tested in our Continuous Integration environment. You can avoid automated failures by running the following checks and addressing any issues they reveal:

make format
make check-lint
make check-censored
make check-clang-tidy
make unittest

Use debug build type, checkin warning level, and AddressSanitizer enabled. For more detail on these flags, see the Development Builds page.

In addition, you should verify that Doxygen builds the documentation correctly. In the apidoc directory, ensure that doxygen Doxyfile.in runs without errors.

Continuous Integration

We currently use Jenkins and Codecov for our Continuous Integration. We require all pull requests to pass Jenkins and Codecov prior to merging.

Details

The checks run for each CI system are:

• Jenkins
  • make
  • make unittest
  • make coveralls
  • make runbenchmark
• Codecov
  • The overall coverage should not drop below 85%.
  • Your PR should not drop coverage by more than 2%.

To ensure that you pass the CI checks, run these commands before you commit. Note that you may need to git add any files that were modified.

• make check-clang-tidy
  • This runs clang-tidy on your code. It checks your code for common errors and/or bugs.
  • Fix all of the issues that are reported manually.
• make format
  • This runs clang-format on your code. It will rearrange lines, comments, make sure you're within the line length limit, etc.
  • This automatically formats your code.
  • You should pass make check-format after running this.
• make lint
  • This runs cpplint, which checks that your code is written in (approximately) Google C++ style.
  • Fix all of the issues that are reported manually.

Note that these checks are not comprehensive, e.g. the above tools struggle to enforce good whitespace habits. We rely on you and on code review to keep the codebase clean.

Labels

When filing issues or opening PRs, you should always tag them appropriately to make it easier for everyone to keep track of what's going on. The common tags are:

• best-practice: style fixes or refactors
• bug: fixes incorrect behavior
• feature: adds a new feature
• in-progress: not ready to be reviewed or merged
• infrastructure: CMake, third party dependencies, and Continuous Integration changes
• performance: optimizes Terrier performance
• ready-for-review: passes CI, and is ready for code review
• ready-to-merge: passes CI and code review, and is ready for merge
• tests: test infrastructure (Google Test, Google Benchmark, JUnit) changes

Code Review

Once a pull request completes Continuous Integration and has been tagged ready-to-review, it should be reviewed by developers familiar with the system component(s) impacted by the changes. Reviewers should follow the following guidelines:

• Don't just view the diff on Github as these views lack context and make it difficult to spot larger code design issues. View the modified files in their totality, and clone large PRs locally.
• Do the changes adhere to developer guidelines with respect to file structure, program behavior, and code style? Basically, look for code style issues that our automated tools can't catch.
• Check the documentation. Just because a PR passes the Doxygen check doesn't mean that the documentation is comprehensive or accurate. Could a new developer still follow the behavior of the changes?
• Check the test cases. Do they exercise common scenarios as well as edge cases? Is the tests' behavior obvious or documented enough that a developer could easily use them to debug issues that arise in the future?
• Check the Coveralls results in detail. If the changes are not being exercised then it usually means that either the code is not correct or the tests are not thorough enough — sometimes both.

Merge conflicts

The submitter is generally responsible for keeping their branch up to date and free of merge conflicts. Old branches without merge conflicts may be updated by a repository admin, but the submitted should be proactive about their PRs. Any PRs with merge conflicts will be tagged in-progress and need to be reviewed again after conflicts are resolved.

After you merge

(TODO: describe nightly tests and the process of putting out fire if nightly build fails)