12691: Improve the PR checklist. #7140
Conversation
Resolves brave/brave-browser#12691 The idea is to clean up the checklist and leave only relevant and important entries. Also populate the reviewers section with frequently overlooked issues.
| - [ ] Used Github [auto-closing keywords](https://help.github.com/articles/closing-issues-via-commit-messages/) in the commit message. | ||
| - [ ] Added/updated tests for this change (for new code or code which already has tests). | ||
| - Verified that these changes build without errors on | ||
| - [ ] Android |
There was a problem hiding this comment.
might still be useful to have this when mixing local test runs + CI
| - [ ] Windows | ||
| - [ ] Verified that all lint errors/warnings are resolved (`npm run lint`, `npm run gn_check`) | ||
| - [ ] Wrote a good [PR/commit description](https://google.github.io/eng-practices/review/developer/cl-descriptions.html) | ||
| - [ ] Checked the PR locally: `npm run test -- brave_browser_tests`, `npm run test -- brave_unit_tests`, `npm run lint`, `npm run gn_check` | ||
| - [ ] Ran `git rebase master` (if needed). |
There was a problem hiding this comment.
Can we do something similar to the first checkbox - describing the state of the PR rather than an "if needed" action? I'm thinking something along the lines of:
- The PR is rebased/up to date with the most recent master branch
There was a problem hiding this comment.
I believe this state is nearly impossible - master is frequently updated, so in a minute after rebase you are not up-to-date anymore. So the current sentence just reminds that it's useful to rebase once in a while.
And if your RP is seriously outdated, CI wouldn't merge it anyway
There was a problem hiding this comment.
btw this also needs to be in https://github.com/brave/brave-browser/blob/master/.github/PULL_REQUEST_TEMPLATE.md
.github/PULL_REQUEST_TEMPLATE.md
Outdated
| - [ ] Windows | ||
| - [ ] Verified that all lint errors/warnings are resolved (`npm run lint`, `npm run gn_check`) | ||
| - [ ] Wrote a good [PR/commit description](https://google.github.io/eng-practices/review/developer/cl-descriptions.html) | ||
| - [ ] Checked the PR locally: `npm run test -- brave_browser_tests`, `npm run test -- brave_unit_tests`, `npm run lint`, `npm run gn_check` |
There was a problem hiding this comment.
One problem I commonly have is that I almost never have the browser tests working locally and it's never because of my changes. Maybe we should split that one into a separate item (or just leave it for CI)?
There was a problem hiding this comment.
same here! But I reckon it is still useful to run tests locally just in case you catch an error and save some CI cycles
|
|
||
| - [ ] The associated issue milestone is set to the smallest version that the | ||
| changes has landed on. | ||
| - [ ] All relevant documentation has been updated, for instance: |
There was a problem hiding this comment.
💯 This is never appropriate to do until after the merge :)
|
|
||
| ## Reviewer Checklist: | ||
|
|
||
| - [ ] New files have MPL-2.0 license header. |
There was a problem hiding this comment.
This is a frequently forgotten one. Might be worth keeping until we have it automated in the linter.
There was a problem hiding this comment.
linter checks it, at least for C++ files
There was a problem hiding this comment.
That explains why I haven't seen it missing from C++ files lately, though I have seen it missing from other file types.
There was a problem hiding this comment.
We can have ts files checked with tslint brave/brave-browser#12724
There was a problem hiding this comment.
ok, let's keep it until we can lint more file types
Fixes #12691 PR to brave-core: brave/brave-core#7140
Resolves brave/brave-browser#12691
The idea is to clean up the checklist and leave only relevant
and important entries. Also populate the reviewers section with
frequently overlooked issues.
Resolves
Submitter Checklist:
npm run lint,npm run gn_check)git rebase master(if needed).git rebase -ito squash commits (if needed).QA/YesorQA/No) to the associated issuerelease-notes/includeorrelease-notes/exclude) to the associated issueTest Plan:
Reviewer Checklist:
After-merge Checklist:
changes has landed on.