RFR: 8332313: Update code review guidelines

Wed May 15 19:36:18 UTC 2024

On Wed, 15 May 2024 17:45:56 GMT, Kevin Rushforth <kcr 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).

"but **It** is" -> it

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.

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 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.

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: 

I would add that it's a good idea to search for JBS issues similar/linked to the one being targeted. There are many unfound duplicates in JBS that arise only when searching, or at least closely linked ones (problems in `TableTreeView` often have "pseudo-duplicates" for `TreeView` and `TableView`, for example). 

I would explain (or link to where it is explained) when to close an issue a a duplicated vs. when to use the /add Skara command to bundle issues into the same fix.

We might also find that the targeted issue is a regression of another issue. This is somewhat mentioned in the bullet point below "Consider the risk of regression".

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?

README-code-reviews.md line 66:

> 64: * Focus first on substantive comments rather than stylistic comments
> 65: * Consider the risk of regression
> 66: * Consider any compatibility concerns

This might be included in this point already, but one thing I sometimes miss is the inadvertent introduction of new API (that will have to be deprecated if missed). This can happen with `protected` methods, default `public` constructors (esp. if a non-API constructor is removed), or if a class is moved from a non-exported to an exported package (something that you can't see by looking at the area you're reviewing, you need to check the `module-info`, or more practically, look at the package names).

README-code-reviews.md line 68:

> 66: * Consider any compatibility concerns
> 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

These tests sometimes fail and we integrate anyway. I noticed that sometimes they need a few iterations to succeed. Are we really dependent on them?

Edit: currently the Linux test is failing, and this PR can't be the reason.

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.

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

PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602118351
PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602122027
PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602145807
PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602134914
PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602135321
PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602142181
PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602125749
PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602129587