Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 PULL_REQUEST_TEMPLATE.md #21728
base: pr-template
Are you sure you want to change the base?
Update PULL_REQUEST_TEMPLATE.md #21728
Changes from 1 commit
5e696a4
f02c6af
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Maybe we should have a separate paragraph about making good titles
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.
as one of the checklist items?
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 don't have a strong opinion on this. @KevinMind any thoughts ?
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 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?
All optional (begrudgingly) wdyt? I'd like to keep the titles short, even shorter than I originally conceived them.
Ex:
@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.
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 casesThe 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?