RFR: 8332313: Update code review guidelines

Wed May 15 22:24:11 UTC 2024

On Wed, 15 May 2024 19:04:19 GMT, Nir Lisker <nlisker at openjdk.org> wrote:

>> Update the code review guidelines for JavaFX.
>> 
>> The JavaFX [CONTRIBUTING](https://github.com/kevinrushforth/jfx/blob/8332313-contributing/CONTRIBUTING.md) guidelines includes guidance for creating, reviewing, and integrating changes to JavaFX, along with a pointer to a [Code Review Policies](https://wiki.openjdk.org/display/OpenJFX/Code+Reviews) Wiki page.
>> 
>> This PR updates these guidelines to improve the quality of reviews, with a goal of improving JavaFX and decreasing the chance of introducing a serious regression or other critical bug.
>> 
>> The source branch has three commits:
>> 1. Converts the Code Review Policies Wiki page to a [README-code-reviews.md](https://github.com/kevinrushforth/jfx/blob/8332313-contributing/README-code-reviews.md) file in the repo and updates hyperlinks to the new location.
>> 2. Update `README-code-reviews.md` with new guidelines
>> 3. Update `CONTRIBUTING.md` to highlight important requirements  (and minor changes to `README-code-reviews.md`)
>> 
>> Commit 1 is content neutral, so it might be helpful for reviewers to look at the changes starting from the second commit.
>> 
>> The updates are:
>> 
>> * In the Overview section, add a list of items for Reviewers, PR authors, and sponsoring Committers to verify prior to integration
>> * Create a "Guidelines for reviewing a PR" subsection of the Code review policies section
>> * Create a "Before you integrate or sponsor a PR" subsection of the Code review policies section
>> * Update the `CONTRIBUTING.md` page to highlight important requirements
>
> README-code-reviews.md line 48:
> 
>> 46: All code reviews must be done via a pull request submitted against this GitHub repo, [openjdk/jfx](https://github.com/openjdk/jfx). A JBS bug ID must exist before the pull request will be reviewed. See [CONTRIBUTING.md](CONTRIBUTING.md) for information on how to submit a pull request.
>> 47: 
>> 48: All fixes must be reviewed by at least one reviewer with the "Reviewer" role (aka a "R"eviewer). We have a different code review threshold for different types of changes. If there is disagreement as to whether a fix is low-impact or high-impact, then it is considered high-impact. In other words we will always err on the side of quality by "rounding up" to the next higher category. The contributor can say whether they think something is low-impact or high-impact, but It is up to a Reviewer to confirm this. A Reviewer either adds a comment indicating that they think a single review is sufficient, or else issues the Skara `/reviewers 2` command requesting a second reviewer (a Reviewer can request more than 2 reviewers in some cases where a fix might be especially risky or cut across multiple functional areas).
> 
> About requesting reviews. I think that only some people can request reviews through GitHub, I never managed to do it on this repo, probably a matter of permissions. Might worth clarifying how this works.

It's better to just add comments `@` mentioning who you want to do the review, since there isn't a way for most people to request a review from others.

> README-code-reviews.md line 58:
> 
>> 56: * Determine whether this needs 2 reviewers and whether it needs a CSR; issue the `/reviewers 2` or `/csr` command as needed
>> 57: * If you want to indicate your approval, but still feel additional reviewers are needed, you may increase the number of reviewers (e.g., from 2 to 3)
>> 58: * If you want an area expert to review a PR, indicate this in a comment of the form: `Reviewers: @PERSON1 @PERSON2`; the requested reviewers can indicate whether or not they plan to review it
> 
> Should a list of experts per area be made available somewhere? The Wiki has an old "code ownership" table that is out of date. Usually you get to know the experts only after they have reviewed your code a couple of times.

That might be something to consider as a follow-on.

> README-code-reviews.md line 60:
> 
>> 58: * If you want an area expert to review a PR, indicate this in a comment of the form: `Reviewers: @PERSON1 @PERSON2`; the requested reviewers can indicate whether or not they plan to review it
>> 59: * If you want to ensure that you have the opportunity to review this PR yourself, add a comment of the form: `@PRAUTHOR Wait for me to review this PR`, optionally add any concerns you might have
>> 60: 
> 
> Another thing to look out for is the targeting branch during rampdown. Should they all target master and then be backported as needed, or can some target the rampdown branch?

Good idea. I'll add the bit about checking the target branch -- if the PR isn't a backport it should almost always target the master branch (there might be _very_ rare exception where a bug is specific to the stabilization branch).

> README-code-reviews.md line 69:
> 
>> 67: * Check whether there is an automated test; if not, ask for one, if it is feasible
>> 68: * Make sure that the PR has executed the GHA tests and that they all pass; if they aren't being run, ask the PR author to enable GHA workflows
>> 69: * If the PR source branch hasn't synced up from master in a long time, or if there is an upstream commit not in the source branch that might interfere with the PR, ask the PR author to merge the latest upstream master.
> 
> This is the only bullet point with a period at the end.

I'll fix it.

-------------

PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602301589
PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602301960
PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602303525
PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602310146