...
How does this work? In summary, we will look at the following:
Table of Contents |
---|
Is this your first time submitting a PR? Check Your first CockroachDB PR!
Presence of unit tests
Every CockroachDB change that fixes a bug or changes functionality must be accompanied by testing code.
A unit test is good when:
it would fail if ran on CockroachDB before your change,
it succeeds when ran after the change,
it does not test too many other things besides the area you're changing,
it is fully automated.
If your change affects multiple areas, there should probably be a different unit test for each area (principle 3).
Currently in the CockroachDB project we use the following types of test:
Go unit tests, via functions named
TestXXX
placed in the same Go package as the change. This is the most common.Use this when a change is local to a few functions and these functions can be exercised in isolation.
Take inspiration from the other tests in the same package.
Data-driven tests, ran via
datadriven.RunTest
, with test files in atestdata
subdirectory.If a package uses data-driven tests already, it's possible that you should use that too for your change.
SQL logic tests, ran via
logictest.RunLogicTest
, with test files in atestdata
subdirectory.If your change affects how CockroachDB runs SQL statements, you should probably include a SQL logic test.
Tests for CLI commands meant to be ran interactively in a terminal, using TCL, with test files in
pkg/cli/interactive_tests
.If your change affects the
cockroach
command-line interface and only affects the behavior of interactive use, you may need this.We also use non-interactive
Example_xxxx
tests inpkg/cli/cli_test.go
.
Client language/framework tests in
pkg/acceptance/adapter_test.go
.If your change adds feature to the client/server protocol, you probably need to try it out via this test infrastructure.
Vertical tests using
roachtest
inpkg/cmd/roachtest
.If testing your change require special flags in
cockroach start
, or if it needs to run for a longer time, you may want to useroachtest
.
Adherence to the coding style
During a review, some attention is given to the coding style.
CockroachDB uses a couple extra formatting rules in addition to those used by
gofmt
. We provide the commandcrlfmt
to enforce those automaticaly: Use:crlfmt -tab 2
We advise to integrate this in your text editor.We value good naming for variables, functions for new concepts, etc. If your change extends an area where there is already an established naming pattern, you can reuse the existing pattern. If you are introducing new concepts, be creative! But during the review we might kindly request you to rename some of the entities.
Adequate in-line comments
We value in-line comments that clarify what is going on in the Go code.
Good comments:
are written in grammatical English, with full sentences (capital at beginning, period at end).
do not repeat what the code does (i.e. do not write
// This loop iterates from 1 to 10
in front of afor
-loop), but instead clarifies the goal and/or the motivation (e.g.// This loop aggregates a result over all the instances
).
We value adequate (but not excessive) single empty lines to separate blocks of code that correspond to units of reasoning.
In some cases we even value adding comments on code that you have not changed, but that was unclear to you (when the comments would add a future reader).
Explanatory commit message and release note annotation
...
As much as possible, put all the information about your change primarily in the git commit message, and merely maintain the PR description as a copy or a summary of the git commit message.
GitHub shows the PR description message, however, when developing, we get context and navigate code using the Git commit message.
The PR description message is auto-populated by GitHub from the Git commit message, so it "magically" works if the information is there in the first place.
If the discussion on a PR enables a new understanding, be careful to update the git commit message accordingly.
Place your release note(s) in git commit messages.
See the separate page on Git Commit Messages.
...
If there is potential UX impact, we kindly request that you approach us to discuss this impact prior to starting to program the change. Depending on that discussion, we might express additional requirement that can influence the approach you'll take.
...
This happens mainly in two situations:
A larger project may be decomposed in sub-parts / steps. We recommend one commit per logical change in the PR, up to and including when the PR is merged. See below for details.
During code review, to answer reviewer comments. We accept seeing fix-up commits during the review, but we request they are merged together (“squashed”) before the PR is merged. See below for more nuance.
Generally, we do not wish to review PRs that contain more than a dozen commits. A large number of commits is a symptom that the change is disorganized and will put undue burden on the reviewer team. If you think you need to make a large change, please approach our team on Slack first to discuss how to organize the work.
See Organizing PRs and Commits for details.