Pull request review guidelines

[epixa]

I started this in a gist, but folks were making some good suggestions and I wanted to track the conversation in a more discoverable place. Ultimately my intention is to open a pull request that adds all of this to our docs, but I didn't wanted to focus on content before dealing with integration.

---

Pull request review guidelines

Every change made to Kibana must be held to a high standard, and while the responsibility for quality in a pull request is ultimately on the author, Kibana team members have the responsibility as reviewers to verify during their review process.

Minimum requirements for pull requests

• CLA check passes
• Jenkins job runs and passes
• Has thorough unit test coverage
• Adheres to the spirit of our various styleguides
• Automated tests provide high confidence the change works without running Kibana or there are selenium tests providing that confidence
• Appropriate product documentation is included (asciidocs)
• Any new UI changes are accessible to differently abled persons, including but not limited to sufficient contrasts in colors, keyboard navigation, and aria tags
• Includes APIs for new or changed functionality, either programmatically for plugins or as REST endpoints
• PR title concisely summarizes the change for other humans to read (no "fixes bug number 123")
• PR description includes:
  • A detailed summary of what changed
  • The motivation for the change
  • Screenshot(s) if the UI is changing
  • A link to each issue that is closed by the PR (e.g. Closes #000)

[epixa]

From @w33ble

If selenium tests are included, they have successfully ran in CI a couple of times without a failure

Why more than once? And how would you ensure that they ran successfully multiple times?

[epixa]

Why more than once? And how would you ensure that they ran successfully multiple times?

To help identify flaky tests up front before we merge them. Good point on how we ensure they were successful each time. I think we can just rely on people's word on this one, but it's interesting how this set of requirements is going to read differently depending on whether you are a Kibana team member or a contributor. I would expect a PR author to deal with confirming the new selenium tests run successfully a couple of times if that person is a Kibana team member, but I would expect a reviewer to handle that if the author was an external contributor. I'll have to give some thought to how best to document that, and I'm open to suggestions!

[epixa]

From @weltenwort

If we are looking for a technical solution it could be something like a script that determines the set of added selenium tests (easier with junit output 😉) and executes these three (🎲) additional times during the PR jenkins job run.

[epixa]

I think we should definitely consider technical solutions wherever practical, and that certainly sounds like a good one to me.

[epixa]

I'm going to pull the note about re-running tests entirely from this particular issue. I think we should do it, but I don't want to block having any PR guidelines on any specific new requirement.

[kobelb]

We have this now per https://www.elastic.co/guide/en/kibana/current/pr-review.html, closing.