Reviewing - reanahub/reana GitHub Wiki

This page summarises our principles and practices when reviewing pull requests (PRs).

Contents

  1. Every line of code should be seen by another pair of eyes
  2. Reviewing is an opportunity to share knowledge
  3. Make it clear who is reviewing issue or PR
  4. Every PR should preserve or increase code coverage
  5. Check it as a black box
  6. Check it as a white box
  7. Next steps

Every line of code should be seen by another pair of eyes

We don't push code directly into any of the REANA repositories on GitHub. Every line of code should be seen by another person than the author. Therefore, we require code reviews. These can be sometimes light, based upon reading the code differences, but more often they require full testing and attention to details.

Reviewing is an opportunity to share knowledge

In addition to checking the quality of the code, a review is an excellent way of sharing knowledge. Found some issue or possible optimization? Add a few more details to your PR comments to educate others.

Make it clear who is reviewing issue or PR

When you review an issue or PR, assign yourself as an assignee or self-request review.

Example:

  • when reviewing an issue with one or more PRs, assign yourself to the issue, when reviewing PRs related to the issue self-request review;

  • when reviewing a PR without issue, self-request review.

It allows other team members to see who is responsible for the work.

Every PR should preserve or increase code coverage

If the pull request is not green, it isn't finished. Please always make the Continuous Integration (CI) pass first before asking for review.

Note, there are exceptions where a pull request set may be red. For example, if implementing some feature requires to touch several REANA cluster components, such as reana-job-controller and reana-workflow-controller, as well as some changes to the shared reana-commons component. Here it is possible that a PR to reana-job-controller wouldn't be "green" until a new version of reana-commons is released on PyPI. What we do in these cases is the following:

  1. We review the pull requests locally, making sure it is working;
  2. We release the approved reana-commons on PyPI;
  3. We upgrade PRs for each other component of the PR set (i.e. reana-job-controller or reana-workflow-controller) to depend on this newly released reana-commons version.

This should make the two pull requests to pass CI tests. Moreover, the commits will be nicely atomic, declaring that this new functionality in reana-job-controller is inherently dependent on some reana-commons version.

Check it as a black box

As a reviewer, you should first check whether the pull request is understandable from the "outside". That is, the pull request surface should make sense without reading the implementation. It is helpful to ask yourself the following questions:

  • Is the goal of the pull request understandable from the commit message?
  • Is it working as it should? Does it pass the integration tests?
  • Is the bug fix or the new feature announced in the CHANGES.rst file?

Check it as a white box

As a reviewer, if the pull request works overall, and it is understandable on the "outside", it is time to look on the "inside".

  • Does the implementation look good? Is the code idiomatic?
  • Does the code follow the coding standards?
  • Can the implementation fail for some corner cases?
  • Is the runtime performance acceptable?
  • Are all configuration variables centralised in one place and easy to alter?

Next steps

Now that the pull request is reviewed, let's proceed with Integrating it.