Guidelines for Code Managers and Reviewers

Important Note

For all reviewers - Reviewers should only approve a pull request if they are content with the proposed code changes as-is. If changes are desired or required, please do not toggle the Approve option in the review.

The Joint Effort for Data assimilation Integration (JEDI) provides an overview of code reviews, including their purpose, benefits, and what to look for when reviewing code. SRW-specific code review standards are described below.

General

• Is the developer's branch updated to HEAD of develop?
• Is the branch appropriately named using a bugfix/, feature/, or text/ prefix? (If not, remind user.)
• Is there an open issue that this PR addresses? (Code manager discretion: Ask user to create issue or remind user to do so next time.)
• Have appropriate labels been added to the PR? (This is most important if the contributor does not have permissions to add labels.)
• Is required documentation included in the PR?

Code

• Does the code contribution follow the Code and Configuration Standards laid out in the Contributor's Guide? For example:
  • Externals point to appropriate hash of authoritative repositories.
  • Platform-specific settings are handled only through configuration and modulefiles, not in code or scripts.
  • Components build using the common modules located in the modulefiles/srw_common.lua file.
  • All bash scripts must explicitly be #!/bin/bash scripts (and not login enabled).
• Has functionality from another repository been added to the PR (i.e., maintaining manage_externals in the SRW App rather than cloning the manage_externals repository via Git submodules)? If so, then the PR should be rejected, as this is bad code management practice.
• Does the contribution follow NCO Guidelines for the scripts, jobs, and ush directories?
• Does the code break supported capabilities on any supported platforms?

Testing

• Has the PR been sufficiently tested?
  • Was at least one workflow end-to-end (WE2E) test run on at least one supported platform?
  • Has all new functionality been tested explicitly?
  • Which platform(s) did the user test on?/Which platforms still require testing?
  • Did the user run the fundamental test suite on at least one supported platform? (Code manager discretion: in minor PRs, running a subset of tests may be acceptable.)
  • Which (if any) comprehensive tests were run? (Code manager discretion whether to require some/all comprehensive tests.)

Testing Requirements Before Merging a PR into develop

• Following two approvals, please ensure that the Jenkins automated test label, run_we2e_coverage_tests, has been added to the PR. Please note that the Jet tests will likely fail due to HPSS access. The Jet coverage WE2E tests will need to be run manually.
• If the Jenkins tests need to be aborted due to failures, please reach out to Kris Booker, Bruce Kropp, or Mark Potts to assist. Also, Jenkins tests can only be resubmitted through the Jenkins API (removing and adding the label won't resubmit the Jenkins tests). Please reach out to them to resubmit Jenkins tests for a previously failed and updated PR.

Documentation

• Did the user update the .rst documentation files with information related to their PR?
• If not, and documentation is required, did the user open an issue to add documentation?

Merging

• Is at least one of the code reviewers a code manager? (If not, request that a code manager review the PR before merging.)
• If code has changed since the PR was opened, ensure that appropriate tests were re-run.
• Did the code owner conducting the merge check with the PR author to ensure that it is ready for merging (e.g., no last-minute minor fixes coming in)?
• Are there any unresolved conversations in the PR? Please reach out to reviewers who left comments to ensure that they are happy with the changes before merging.