CodeReview

Requesting a Code Review

In order to request a code review you must create a pull request (PR) on GitHub. Once you are happy that your PR is ready for review please give it the "ready for review" label and request a review from one or more people. (Rupert [rupertford], Andy [arporter], Sergi [sergisiso], Iva [teranivy] or Joerg [hiker] are your current options.) Although you can request a review from multiple people, only one review actually needs to be performed.

A review is performed in GitHub by creating comments on points in the code that need attention. The PR author may respond to these comments and the result is a 'conversation'. It is up to the reviewer to decide when a conversation can be marked as 'resolved' (i.e. the point has been dealt with to their satisfaction).

Guidelines for Performing a PSyclone Code Review

1. Replace the "ready for review" label on the PR with the "under review" label. This helps to prevent multiple people attempting to review the same thing and also makes it clear at what stage in the work-flow the PR is.
2. Check that branch is up-to-date with master (the pull request should report that the branch can be "merged automatically"). If not, return to developer for them to bring it up-to-date.
3. py.test must report 0 failures.
4. Check that any x-failing tests are unaffected by the current Issue. (i.e. should they now pass?)
5. Check that there are no TODOs in the code that refer to the current Issue/PR.
6. Use the "Files changed" tab on the pull request to review all code changes. Check that all code modifications are as pythonic as possible, well commented, easy to understand and that the code is correct. Comments and requests for changes may be made in-line on the "Files changed" tab. This makes it easier for the developer to see which part of the code is being discussed.
7. If code changes suggested in 5. are not minor then return to developer to address.
8. Check that the docstring in the containing method/function for any new/modified code describes its arguments (using sphinx markup notation).
9. Check that any new/modified code is covered by the test suite e.g. py.test --cov-report term-missing --cov psyclone.dynamo0p3 or see the report produced by CodeCov after Travis has run the test suite. To see this report, look for the CodeCov comment on the PR (in the conversation) and follow the "Continue to review full report at Codecov" link. Once there, go to the "Diff" tab and look at any files for which the diff coverage is not 100%.
10. If the Fortran being generated by PSyclone has changed then check that the test suite includes an option to compile it. Use py.test --compile --compileopencl ... to check compilation of the new code (this must be run from the tests directory in order to pick-up the compilation-related test fixtures).
11. Check that the copyright/author details are correct in any modified files.
12. Check that any new/modified code passes pycodestyle (py.test --pep8 or just pycodestyle <python file>)
13. Check that any new/modified code passes pylint, i.e. is free from errors (but see note below) and has a score > 9/10.
14. Generate User Guide (cd PSyclone/doc/user_guide; make latexpdf) and check that it is up-to-date if new functionality has been added in the ticket.
15. Repeat for the Developers' Guide (PSyclone/doc/developer_guide) and check that the implementation details for any new functionality have been adequately described.
16. Check that any new functionality has been reflected in the mindmap 'big picture' document.
17. Check that the examples work (this is now done by Travis). Consider whether the ticket includes significant new functionality that would benefit from being demonstrated in an example (and if so, whether it is).
18. If applicable, the reviewer should attempt to build the LFRic model using the version of PSyclone on the development branch. Bear in mind that the LFRic code/build-system might need to be modified in order to actually exercise the new PSyclone features introduced in the branch.

Note that pylint doesn't understand pytest and therefore reports spurious errors of the form:

"Module 'pytest' has no 'raises' member (no-member)"

these can safely be ignored.

Once the review is complete there are two options:

1. If the reviewer is happy with the pull request then they can proceed to merge the branch onto master (see below)
2. If the reviewer is requesting changes, then the "under review" label on the PR should be replaced by "reviewed with actions" and it is then up to the original developer to address the reviewer's concerns.

Merging a branch to master

Once a pull-request has passed code review, the code reviewer should merge the associated branch onto master:

1. Check-out the most recent version of the branch
2. Check that all tests pass in this version (sanity check)
3. Generate the User Guide (psyclone.pdf) and update the copy in the top-level directory
4. Update the changelog in the top-level directory (using the text describing the Issue)
5. Commit these changes and push branch to github
6. On the pull-request page on github, request the branch be merged to master
7. Checkout the latest master branch and check that all tests still pass!
8. Check that the examples all run OK (cd PSyclone/examples; ./check_examples)
9. Delete the original branch
10. Close the associated issue

Verifying that test kernels/algorithms are actually used in tests

To count the number of times each testkern is used by each of the test algorithms (assuming the latter are all named something like 1*.f90):

cd test_files/dynamo0p3
for i in testkern_*.f90; do echo $i; j=`echo $i | sed -e 's/.f90//'`; grep -c $j 1*.f90 | awk 'BEGIN{FS=":"};
{count+=$2};END{print count}'; done

To count the number of times each test algorithm is actually used in a test:

cd test_files/dynamo0p3
for i in 1*.f90; do echo $i; grep -c $i ../../*.py | awk 'BEGIN{FS=":"};{count+=$2};END{print count}'; done

Be aware that, particularly for long filenames, this grep may not work because the name might be broken over more than one line in the python file.