Update PULL_REQUEST_TEMPLATE.md #21728
Conversation
|
|
||
| <!-- REQUIRED | ||
|
|
||
| Short description of the changes you made and how they address the linked issue. | ||
| Further description of the changes you made if the pull request title is insufficient, and how they address the linked issue. |
There was a problem hiding this comment.
Maybe we should have a separate paragraph about making good titles
There was a problem hiding this comment.
as one of the checklist items?
There was a problem hiding this comment.
I don't have a strong opinion on this. @KevinMind any thoughts ?
There was a problem hiding this comment.
I think it makes sense especially given how we are implicitly expecting the title to be the longest living and most prominent part of the PR in terms of future references.
Regarding the changes in this PR especially to the headers I've experimented with a simpler syntax in a few PRs, wdyt?
- Description
- Context
- Testing
All optional (begrudgingly) wdyt? I'd like to keep the titles short, even shorter than I originally conceived them.
Ex:
- FIx: add missing task to sync_cinder_policies command #21787
- Add management command for sync suppressed emails #21784
- Add CinderPolicy model and sync_cinder_policies management command #21773
- Add localization job to CircleCI config #21734
@diox could you add a suggestion in the original PR for a section on titles. I think that would be good to explain in the PR template that when you merge the title of the PR will be the commit and how it should be formatted. All great context for a contributor.
IF we agree on the simplified section names then I would add a commit on the original PR and then @eviljeff could you add your changes as suggestions? Then we will have a finallized version on the original PR and can squash and merge it.
| ## Check list | ||
|
|
||
| - [ ] This PR relates to an existing open issue and there are no existing PRs open for the same issue. | ||
| - [ ] The change has been successfully run locally. | ||
| - [ ] Add tests to cover the changes added in this PR. | ||
| - [ ] Add a comment to the issue that describes how to manually test the changes you have made in this PR. |
There was a problem hiding this comment.
I would push back on this. If this information is needed, it should just be in the description. Adding as a comment is just splitting information in two places and I'd rather avoid that.
If the info is not needed, it just isn't added, if it is needed it's added in the description. Wdyt?
There was a problem hiding this comment.
The thinking here is that ultimately, testing should be done by QA to verify an issue has been fixed. QA should never need to look at PRs, so if you have manual testing steps (*)... they really should be in the issue.
(*) Which should be the exception and not the rule, for PRs. We want everything to be covered by tests ran by CI anyway.
There was a problem hiding this comment.
yeah, this was about the instructions for QA to test the patch, which should be in the issue. Agree that if there are instructions for the reviewer they should be in the description.
There was a problem hiding this comment.
Maybe reword this with something like "Make sure the issue has steps to reproduce or a description of how to test it" ? Or do we want this out of this template and word something here differently for any (optional) supplemental manual testing that would be required for the PR itself ?
There was a problem hiding this comment.
The problem that I have with this approach is that we currently only QA on master. So a reviewer is basically reading the code, and potentially looking at the automated tests.
This seems like a total misordering of the steps. I might be resisting the overall dev -> review -> merge -> QA flow and that might be why we are not aligned on where the QA / Tests steps should go.
In principle, I think it is a good idea to have the high level QA test scenarios (navigate to this page, click this button, expect "foo") on the issue. That is fine.
But ... should the reviewer need to run those steps theirself? How can we be happy merging a PR that we have not played around with and verified, at least to a minimum degree. And the question is, are the steps you give to QA for "full" testing the same as you give to a reviewer for "minimal" testing? I don't have an answer for that but it seems relevant.
I would be.... somewhat okay.. adding this as a check that "The issue linked in fixes has verifiable steps to QA this PR " Assuming we have a place where we document how to write test cases
The trick here is, sometimes merging a PR is not completely finishing a feature. Sometimes it's just one part. How do we handle those cases? That is why I had originally in mind a test section which is "how to test this chunk of the feature that maybe be anywhere from <0-1 / 1 of the total feature scope.
Thoughts?
for #21711 - doesn't needing reviewing separately.