-
Notifications
You must be signed in to change notification settings - Fork 302
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
Update PR template with task list for testing and documentation #762
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
.github/PULL_REQUEST_TEMPLATE.md
Outdated
@@ -2,5 +2,16 @@ | |||
|
|||
*Description of changes:* | |||
|
|||
---- | |||
*Testing* | |||
- [ ] Did you run our unit tests? `make test` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: our is relative, might be better to phrase it as the
.github/PULL_REQUEST_TEMPLATE.md
Outdated
- [ ] Did you run our unit tests? `make test` | ||
- [ ] Did you run our integration tests? `make integ-test` | ||
- [ ] Did you add new unit test(s)? | ||
- [ ] Did you add new integration test(s)? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about we also add a section for listing manual test cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applicable cases could change depending on what functionality as added, but minimally we could have section like, "List manual checks done and their output (include default case, embellished case(s), new/old functionality, etc.)".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added:
- Listed manual checks and their outputs in the comments (example)
.github/PULL_REQUEST_TEMPLATE.md
Outdated
---- | ||
*Testing* | ||
- [ ] Did you run our unit tests? `make test` | ||
- [ ] Did you run our integration tests? `make integ-test` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to list checks as affirmative statements rather than questions. Ex:
make test
passesmake integ-test
passes- Unit tests added for new functionality
- etc...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good this seems more Teutonic, I had taken my template from AppMesh. Updated.
.github/PULL_REQUEST_TEMPLATE.md
Outdated
- [ ] Did you add new unit test(s)? | ||
- [ ] Did you add new integration test(s)? | ||
---- | ||
*Documentation* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add check for "Updated README if applicable"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.