Skip to content

CodeReview

Jump to bottom
Andy Porter edited this page Nov 27, 2024 · 40 revisions

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. (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 needs to respond to each of 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 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. If a test using the disable_declaration_check fixture is modified then the test should also be modified to remove the need for this fixture. If this results in this fixture no longer being used in any tests then the fixture should also be removed and issue #754 closed.
  7. 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.
  8. If code changes suggested in 7. are not minor then return to developer to address.
  9. Check that the docstring in the containing method/function for any new/modified code describes its arguments (using Sphinx markup notation). See also step 17 which effectively checks that the Sphinx markup is correct.
  10. 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 GitHub Actions 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%. (Occasionally CodeCov fails to work properly. In that case, you can see the raw coverage reports at the end of the detailed output for the GitHub Action.)
  11. 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). This check is also performed by the integration tests.
  12. Check that the copyright/author details are correct in any modified files.
  13. Check that any new/modified code passes flake8(flake8 . or just flake8 <python file>)
  14. Check that any new/modified code passes pylint, i.e. is free from errors (but see note below) and has a score > 9/10.
  15. 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.
  16. Repeat for the Developers' Guide (PSyclone/doc/developer_guide) and check that the implementation details for any new functionality have been adequately described.
  17. Build the Reference Guide (cd PSyclone/doc/reference_guide; make html) and check that no warnings are produced by any of the modified code. (You may need to do rm source/autogenerated/*.rst in order to ensure that the latest version of the code is being processed.)
  18. Check that any new functionality has been reflected in the mindmap 'big picture' document.
  19. Check that the examples work when compilation is enabled (make compile in the examples directory or run the integration tests). Consider whether the ticket includes significant new functionality that would benefit from being demonstrated in an example (and if so, whether it is).
  20. Integration tests are now supported (instructions) which test functionality and performance of PSyclone with LFRic and NEMO. These integration tests should be run near, or at the of a review before approving, unless you suspect something will break or cause performance issues, in which case which they can be run earlier, and any problems pointed out in a comment in a review with actions.
  21. If the integration tests completed successfully, performance numbers for NEMO and LFRic will be uploaded into a Github Gist. The reviewer can check that there hasn't been any performance degradation, but note that currently the runner does not have exclusive access to the testing system and the performance may sometimes be impacted by other users in the system.

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: 
    cd PSyclone/doc/user_guide/; make latexpdf; cp _build/latex/psyclone.pdf ../../psyclone.pdf`
  4. Update the changelog in the top-level directory (using the text describing the Issue) 
    cd PSyclone; vi changelog
  5. Commit these changes and push branch to GitHub: 
    git add changelog psyclone.pdf
git commit -m "#<issue-number> update changelog and UG"
git push
  6. On the pull-request page on GitHub, request the branch be merged to master
  7. Delete the original branch
  8. Close the associated issue (if appropriate and if GitHub hasn't already done so)

Launching the Integration Tests

Note that only users with the Admin role are able to launch integration tests and see the results.

If you are a reviewer, go to the PSyclone Github Actions tab, then click on the "Push to private" workflow in the list on the left hand side. This will display the "This workflow has a workflow_dispatch event trigger." message. Click on the corresponding "Run workflow" button, select the appropriate branch and Run. After a few seconds a new "Push to private" action will appear with a yellow icon saying "pending"; click this, go to "Review" and press "Approve". This will push the selected branch to a private, mirror repository which will then run the integration tests. After at least an hour, go to the private mirror repository to see the results.

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.

Clone this wiki locally