Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add code review checklist #12037

Merged
merged 2 commits into from
Feb 5, 2021
Merged

add code review checklist #12037

merged 2 commits into from
Feb 5, 2021

Conversation

feerrenrut
Copy link
Contributor

Link to issue number:

None

Summary of the issue:

Authors of changes for NVDA and reviewers alike often forget several "small" things which accumulate causing issues for the project.

Description of how this pull request fixes the issue:

Add a checklist to the PR template for authors and reviewers to run through to ensure nothing is missed.

In addition, the following will be added to the end of https://github.com/nvaccess/nvda/wiki/Github-pull-request-template-explanation-and-examples

## Code Review

Code must be reviewed (via a Pull Request on GitHub) before it can be accepted into the project.
The Pull Request template (``.github/PULL_REQUEST_TEMPLATE.md``) asks authors (and reviewers) to consider several aspects of the change.

### Pull Request description is up to date.
Often the approach taken can change during the review process.
However, in the future, developers may need to come to the PR in search of an explanation for the approach.
Double check that the PR description is accurate.

### Unit tests
Can the changed code be covered by automated unit tests?
Any comments on this can be put under the "testing strategy" heading.

### System tests
Can the changed code be covered by automated system tests
System tests are end to end tests? of NVDA.
Any comments on this can be put under the "testing strategy" heading.

### Manual tests
Is the described manual testing appropriate for the change?
This should describe in detail the cases checked by the author.
In some cases, a change will need to be tested by alpha testers and should be described under the "testing strategy" heading of the Pull Request template.

### User Documentation
Has the user documentation been updated?

### Change log entry
Has an appropriate change log entry been supplied.
As a reviewer please review it.

### Context sensitive help for GUI changes.
If the change adds a new GUI option, ensure that the context sensitive help assignment has been added.

Testing strategy:

N/A

Known issues with pull request:

None

Change log entry:

None

Code Review Checklist:

This checklist is a reminder of things commonly forgotten in a new PR.
Please do a self-review to check these items.
Reviewers will not approve the PR until this are met.

  • Pull Request description is up to date.
  • Unit tests.
  • System (end to end) tests.
  • Manual tests.
  • User Documentation.
  • Change log entry.
  • Context sensitive help for GUI changes.

LeonarddeR
LeonarddeR previously approved these changes Feb 5, 2021
.github/PULL_REQUEST_TEMPLATE.md Outdated Show resolved Hide resolved
@feerrenrut feerrenrut merged commit 1392aa3 into master Feb 5, 2021
@feerrenrut feerrenrut deleted the addCodeReveiwChecklist branch February 5, 2021 07:50
@nvaccessAuto nvaccessAuto added this to the 2021.1 milestone Feb 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants