How to review a pull request

Code review could be time-consuming, but it is extremely important. All pull requests (PR) to the CEA should be reviewed by at least one contributor with maintenance right. The reviewer needs to ensure the changes in the code are aligned with the authors’ description and do not compromise the existing functionalities in the CEA.

1. Read the PR description and follow the test

The author of the PR should provide an instruction on how to test the implementation of the new changes. As the reviewer, you should be able to follow the instruction provided by the PR’s author, or provide feedback if the instruction is unclear. Once the test provided by the author has passed, the reviewer may proceed to the next step.

2. Go through the file changes

It is always a good idea to go through all the changes at least once. Please follow this guide to review the file changes on GitHub.

During this process, the reviewer should check for:

  • Conflicts with master. Make sure the branch is updated with the latest master, and all conflicts are resolved.

  • Sufficient documentation. Check if the documentation is sufficient for the next person to understand the code.

  • Hard-coded values. All hard-coded values should be avoided if possible.

  • Unit tests to implement. The reviewer should decide whether a unit test should be implemented, and request the PR author to implement one accordingly.

  • Changes that might affect other existing functions. In this case, the reviewer should come up with a test to ensure the existing functions are still working as intended.

Once all the points are checked out, the reviewer may proceed to the next step.

3. Run tests

All PRs are automatically sent to test by Jenkins, it executes cea test --workflow quick on a remote computer. The test result is directly shown in the PR page on GitHub.

Additionally, it is always a good idea to run a complete test (cea test --workflow slow) on your local computer. If Jenkins encounters any errors, you can also reproduce those errors by running cea test --workflow quick locally. See here for more information on cea test.

Once cea test is passed, the reviewer may proceed to the last step!

4. Merge the Pull Request

Now you have made sure the PR is going to improve the CEA, thank you for your time! You may go ahead and merge the PR. If the new changes would affect many users, you might want to consider publishing it on the #_critical_updates channel on Slack.